THREESCALE-2286: Refactor Service Discovery into a React component #763

damianpm · 2019-04-15T10:30:33Z

What this PR does / why we need it:

Refactor Service discovery into a React component
Add flow type checking
Add js tests

Which issue(s) this PR fixes

https://issues.jboss.org/browse/THREESCALE-2286

Verification steps

Configure a Service to be discovered on Openshift without SSO (instructions here).
Go to Dashboard
Click on +New Api
Click on Import from OpenShift

Screenshots:

New service:

Service Discovery:

Error:

Note:
Please test in IE11 and Firefox <61

codecov · 2019-04-17T15:17:29Z

Codecov Report

Merging #763 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master    #763      +/-   ##
=========================================
+ Coverage    92.9%   92.9%   +<.01%     
=========================================
  Files        2414    2414              
  Lines       78366   78366              
=========================================
+ Hits        72806   72809       +3     
+ Misses       5560    5557       -3

Impacted Files	Coverage Δ
lib/tasks/multitenant/tenants.rake	`76.92% <0%> (+23.07%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 99ec8db...53fc8e7. Read the comment docs.

damianpm · 2019-05-02T08:12:22Z

app/javascript/packs/polyfills.js

@@ -0,0 +1,2 @@
+import 'babel-polyfill'
+import 'whatwg-fetch'


This is needed to make fetch and promises work in IE11.
I've found (in this GH Issue) this is the recommended way to do it when using webpacker.
Do you know a better way @didierofrivia @josemigallas ?

You've used it differently in the past: https://github.com/3scale/porta/blob/master/app/javascript/src/Stats/inlinechart/index.jsx#L6. However, if this new one is a preferred way by webpackers, I'd just keep it consistent.

I will keep it consistent with inlinechart component. Later, when we update webpacker gem, we can change it if we decide that.

features/old/services/multiservice.feature

damianpm · 2019-05-02T08:27:04Z

app/javascript/src/NewService/components/ServiceDiscoveryForm.jsx

+
+const ServiceDiscoveryForm = (props: ServiceDiscoveryFormType) => {
+  const {isVisible, providerAdminServiceDiscoveryServicesPath, namespaces, services, selectedNamespace, handleChangeNamespaces} = props
+  return (<form className={`formtastic ${isVisible ? '' : 'is-hidden'}`} id="service_discovery" action={providerAdminServiceDiscoveryServicesPath} acceptCharset="UTF-8" method="post">


At the moment, we don't have a generic form component for React.
But as we are close to implement PF4-React and it includes all kind of form elements, I think we should keep this form (to match the styles of other forms generated by rails), and later replace it with PF4 forms
@josemigallas @didierofrivia @thomasmaas

didierofrivia · 2019-05-03T10:57:24Z

app/javascript/packs/new_service.js

+
+document.addEventListener('DOMContentLoaded', () => {
+  const newServiceWrapper = document.getElementById('new_service_wrapper')
+  const newServiceData = JSON.parse(newServiceWrapper.dataset.newServiceData)


you could use our json_utils here. Maybe decide what to do when the config is wrong...

didierofrivia · 2019-05-03T10:57:35Z

app/javascript/src/NewService/components/ServiceDiscoveryForm.jsx

+import React from 'react'
+// TODO: Get authenticityToken from rails
+// Is this needed, or was just generated by formtastic ?
+const authenticityToken = ''


You could use this: https://github.com/3scale/porta/blob/master/app/javascript/src/Policies/components/CustomPolicy.jsx#L35

didierofrivia · 2019-05-03T10:57:39Z

app/javascript/src/NewService/components/ServiceDiscoveryForm.jsx

+const renderSelectOptions = (options: string[]) => {
+  return options.length > 0
+    ? options.map(option => <option key={option} value={option}>{option}</option>)
+    : undefined


Is there any value in returning explicitly undefined ?

didierofrivia · 2019-05-03T10:57:46Z

app/javascript/src/NewService/components/ServiceNewForm.jsx

+
+import React from 'react'
+// TODO: Get authenticityToken from rails
+const authenticityToken = ''


commented above regarding getting this. You could extract that function to utils

app/javascript/packs/new_service.js

app/javascript/src/utilities/utils.js

didierofrivia · 2019-05-06T14:47:59Z

app/javascript/packs/new_service.js

  }
-  render(<NewServiceWrapper {...props}/>, newServiceWrapper)


there's no component to tell the user there's no service data?

didierofrivia · 2019-05-06T14:53:05Z

app/javascript/src/NewService/index.jsx

+    return response.json()
+  })
+    .then(data => data[dataItem].reduce((acc, current) => acc.concat(current.metadata.name), []))
+    .catch(error => console.error(error))


you are catching the error that you are throwing 5 lines above, and using just the console. It's this meant to be like this? No message to the user?

didierofrivia · 2019-05-06T15:04:38Z

app/javascript/src/NewService/index.jsx

+}
+
+// eslint-disable max-lines-per-function
+const NewServiceWrapper = (props: Props) => {


you could break this huge fn into smaller ones, and use composition and keeping the state separated.

didierofrivia · 2019-05-06T15:06:11Z

app/javascript/src/NewService/components/ServiceDiscoveryForm.jsx

+
+const ServiceDiscoveryForm = (props: ServiceDiscoveryFormType) => {
+  const {isVisible, providerAdminServiceDiscoveryServicesPath, namespaces, services, selectedNamespace, handleChangeNamespaces} = props
+  return (<form className={`formtastic ${isVisible ? '' : 'is-hidden'}`} id="service_discovery" action={providerAdminServiceDiscoveryServicesPath} acceptCharset="UTF-8" method="post">


You are using the same form here and in ServiceNewForm, you might want to extract that one in a separated component

didierofrivia · 2019-05-06T15:08:13Z

spec/javascripts/NewService/ServiceDiscoveryForm.spec.jsx

+    handleChangeNamespaces: jest.fn()
+  }
+
+  it('should render properly ', () => {


it should render properly, I'm expecting some specifics not only the shallow component exists

damianpm · 2019-05-21T07:35:09Z

There is still not enough space between radio button and label text I think.

@thomasmaas using 0.75 rems of space

app/javascript/src/NewService/components/FormElements/FormWrapper.jsx

josemigallas

Apart from the comments, I think there are many meaningless specs. We should start focusing a bit more on that, we are adding a lot of new logic and components that are not tested and makes it less maintainable.

josemigallas · 2019-05-21T07:42:59Z

app/javascript/packs/new_service.js

+
+document.addEventListener('DOMContentLoaded', () => {
+  const newServiceWrapper = document.getElementById('new_service_wrapper')
+  const newServiceData = safeFromJsonString(newServiceWrapper.dataset.newServiceData)


I think it should be called newServiceFormProps to be more explicit.

josemigallas · 2019-05-21T07:46:13Z

app/javascript/src/NewService/components/FormElements/ErrorMessage.jsx

+import React from 'react'
+
+const ErrorMessage = ({error}: {error: string}) => {
+  const classNames = `errorMessage ${error ? '' : 'is-hidden'}`


The proper way to prevent a component from rendering is to return null not using css.

app/javascript/src/NewService/components/FormElements/ErrorMessage.jsx

josemigallas · 2019-05-21T07:48:30Z

app/javascript/src/NewService/components/FormElements/ErrorMessage.jsx

+const ErrorMessage = ({error}: {error: string}) => {
+  const classNames = `errorMessage ${error ? '' : 'is-hidden'}`
+  return <p className={classNames}>
+    {`Sorry, your request has failed with the error: ${error}`}


Why a request? Is it an API error message? Why not simply rendering error?

The error comes from the request response, so yes.
What error do you think is more clear to the user @thomasmaas ?
We have something like this:
Sorry, your request has failed with the error: Cannot read property 'projects' of undefined

The error comes from the request response, so yes.

Ok, in that case it could be renamed FetchErrorMessage or NetworkErrorMessage

@damianpm in order to write something in 'normal' english i first need to know what the error represents.

@thomasmaas It will be a fetch error:
Cannot read property 'projects' of undefined
or
Cannot read property 'services' of undefined
Additionally, the browser shows a full error message like:

app/javascript/src/NewService/components/FormElements/FormWrapper.jsx

josemigallas · 2019-05-21T09:20:34Z

spec/javascripts/NewService/ServiceDiscoveryForm.spec.jsx

+
+Enzyme.configure({adapter: new Adapter()})
+
+const Props = {


PascalCase is reserved for classes, component, types... Use better camel or UPPER.

josemigallas · 2019-05-21T09:21:14Z

spec/javascripts/NewService/ServiceDiscoveryForm.spec.jsx

+    const wrapper = shallow(<ServiceDiscoveryForm {...Props}/>)
+    expect(wrapper.find('#service_source').exists()).toEqual(true)
+  })
+  it('should render error message hidden by default', () => {


What means 'by default'?

josemigallas · 2019-05-21T09:21:21Z

spec/javascripts/NewService/ServiceDiscoveryForm.spec.jsx

+}
+
+describe('Service Discovery Form', () => {
+  it('should render properly', () => {


What's 'properly'?

app/javascript/src/NewService/components/FormElements/ServiceDiscoveryListItems.jsx

josemigallas · 2019-05-21T09:25:29Z

spec/javascripts/NewService/ServiceSourceForm.spec.jsx

+
+Enzyme.configure({adapter: new Adapter()})
+
+describe('Service Source Form', () => {


Since you're including a describe block by default, perhaps it would help to use a more descriptive name for the specs. ServiceSourceForm doesn't mean much itself.

didierofrivia

Keep it up Damian! thanks for the hard work, we are nearly the end now :)
ps: you might want to review the tests also.

app/javascript/src/NewService/components/FormElements/ErrorMessage.jsx

app/javascript/src/NewService/components/FormElements/FormWrapper.jsx

didierofrivia · 2019-05-22T13:25:35Z

app/javascript/src/utilities/utils.js

+    )
+  } catch (error) {
+    console.error(error)
+    return null


Here, as my previous comment, you could instead of returning null, could return the input with the value null. That way you'll be addressing the type signature implicit of the function.
On a different matter, when @josemigallas suggested not to explicitly return the type of a React.Component I was fearing this case... since it would have failed the type check otherwise ;)

didierofrivia · 2019-05-24T13:27:28Z

spec/javascripts/NewService/NewServiceForm.spec.jsx

+  expect(wrapper.find('#service_discovery').exists()).toEqual(true)
+})
+
+it('should render `Import from OpenShift` input not disabled when `isServiceDiscoveryUsable` is true', () => {


There are two things here:

The description of the test should be more "human", since it's meant to communicate what you will assert. This one looks more like pseudo code to me. You could go with something like it('should render "Import from OS" input enabled when service discovery is selected', () =>...

I'd expect to see the text "Import from OpenShift" and that property isServiceDiscoveryUsable asserted since you are mentioning them in the description, but they are not... These apparently are, as I understand, not important and end up being a bit misleading (In this case)

This applies to the rest of the tests below.

didierofrivia

Good! Don't forget to deploy first to OS and confirm it works as expected. Thanks for the hard work! 🎖

damianpm force-pushed the THREESCALE-2286-Refactoring-Service-Discovery branch from 406b073 to a12ea99 Compare April 17, 2019 15:26

guicassolato assigned damianpm Apr 18, 2019

damianpm force-pushed the THREESCALE-2286-Refactoring-Service-Discovery branch 4 times, most recently from b72b5a0 to 7dfa8c3 Compare April 19, 2019 10:05

damianpm force-pushed the THREESCALE-2286-Refactoring-Service-Discovery branch 5 times, most recently from 77b3d2f to 53ba8a0 Compare April 30, 2019 12:47

damianpm changed the title ~~[WIP]THREESCALE-2286: Refactor Service Discovery into a React component~~ THREESCALE-2286: Refactor Service Discovery into a React component Apr 30, 2019

damianpm commented May 2, 2019

View reviewed changes

features/old/services/multiservice.feature Show resolved Hide resolved

damianpm requested review from guicassolato, josemigallas and didierofrivia May 2, 2019 08:22

damianpm commented May 2, 2019

View reviewed changes

didierofrivia reviewed May 3, 2019

View reviewed changes

didierofrivia reviewed May 6, 2019

View reviewed changes

app/javascript/packs/new_service.js Outdated Show resolved Hide resolved

didierofrivia reviewed May 6, 2019

View reviewed changes

app/javascript/src/utilities/utils.js Show resolved Hide resolved

didierofrivia reviewed May 6, 2019

View reviewed changes

damianpm commented May 21, 2019

View reviewed changes

app/javascript/src/NewService/components/FormElements/FormWrapper.jsx Show resolved Hide resolved

damianpm requested a review from didierofrivia May 21, 2019 07:37

3scale deleted a comment from didierofrivia May 21, 2019

josemigallas requested changes May 21, 2019

View reviewed changes

damianpm force-pushed the THREESCALE-2286-Refactoring-Service-Discovery branch from d4eeda8 to bede262 Compare May 21, 2019 14:43

damianpm requested a review from josemigallas May 21, 2019 14:44

damianpm force-pushed the THREESCALE-2286-Refactoring-Service-Discovery branch 4 times, most recently from d74726a to 4298083 Compare May 22, 2019 13:02

didierofrivia requested changes May 22, 2019

View reviewed changes

damianpm requested a review from didierofrivia May 22, 2019 14:44

damianpm force-pushed the THREESCALE-2286-Refactoring-Service-Discovery branch 2 times, most recently from 4dd1aab to 2071204 Compare May 22, 2019 15:17

Damian Peralta added 5 commits May 22, 2019 17:35

Deleting old service discovery files

a13cab7

Adding new service discovery react components with flow types

be69900

Adding js tests

526b0b0

Moving CSRFToken and fetchData to utils

8815ea4

Fixing cucmbers: adding @javascript tag

628b9c5

damianpm force-pushed the THREESCALE-2286-Refactoring-Service-Discovery branch from 2071204 to 628b9c5 Compare May 22, 2019 15:52

Improving test coverage

8801b86

didierofrivia requested changes May 24, 2019

View reviewed changes

Better tests descriptions

53fc8e7

damianpm requested a review from didierofrivia May 24, 2019 14:51

didierofrivia approved these changes May 24, 2019

View reviewed changes

thomasmaas approved these changes May 24, 2019

View reviewed changes

damianpm merged commit 33a6ce8 into master May 29, 2019

hallelujah deleted the THREESCALE-2286-Refactoring-Service-Discovery branch February 28, 2020 11:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

THREESCALE-2286: Refactor Service Discovery into a React component #763

THREESCALE-2286: Refactor Service Discovery into a React component #763

damianpm commented Apr 15, 2019 •

edited

Loading

codecov bot commented Apr 17, 2019 •

edited

Loading

damianpm May 2, 2019

didierofrivia May 3, 2019

damianpm May 6, 2019

damianpm May 2, 2019

didierofrivia May 3, 2019

didierofrivia May 3, 2019

didierofrivia May 3, 2019

didierofrivia May 3, 2019

didierofrivia May 6, 2019

didierofrivia May 6, 2019

didierofrivia May 6, 2019

didierofrivia May 6, 2019

didierofrivia May 6, 2019

damianpm commented May 21, 2019

josemigallas left a comment

josemigallas May 21, 2019

josemigallas May 21, 2019

josemigallas May 21, 2019

damianpm May 21, 2019 •

edited

Loading

josemigallas May 21, 2019 •

edited

Loading

thomasmaas May 21, 2019

damianpm May 21, 2019

josemigallas May 21, 2019

josemigallas May 21, 2019

josemigallas May 21, 2019

josemigallas May 21, 2019

didierofrivia left a comment

didierofrivia May 22, 2019

didierofrivia May 24, 2019

didierofrivia left a comment


		Enzyme.configure({adapter: new Adapter()})

		describe('Service Source Form', () => {

THREESCALE-2286: Refactor Service Discovery into a React component #763

THREESCALE-2286: Refactor Service Discovery into a React component #763

Conversation

damianpm commented Apr 15, 2019 • edited Loading

codecov bot commented Apr 17, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damianpm commented May 21, 2019

josemigallas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damianpm May 21, 2019 • edited Loading

Choose a reason for hiding this comment

josemigallas May 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

didierofrivia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

didierofrivia left a comment

Choose a reason for hiding this comment

damianpm commented Apr 15, 2019 •

edited

Loading

codecov bot commented Apr 17, 2019 •

edited

Loading

damianpm May 21, 2019 •

edited

Loading

josemigallas May 21, 2019 •

edited

Loading