Fixes #25957 - Subscription page UI loading fixes #7954

johnpmitsch · 2019-01-31T20:47:10Z

Parts of the subscription page are not loading during certain
workflows and it was making duplicate API requests on load

There are multiple semi-related changes here, I'll detail them in PR
comments

Parts of the subscription page are not loading during certain workflows and it is making duplicate API requests on load

theforeman-bot · 2019-01-31T20:47:16Z

Issues: #25957

johnpmitsch · 2019-01-31T21:21:57Z

Updates in this PR:

Pass just the id into `changeCurrentOrganization`

https://github.com/Katello/katello/pull/7954/files#diff-fac55bdc3e1308653222134902ab60d2R42

This is affecting the actual API call, since the arg for this function is passed into the URL making the request, the API call would fail because it was using the id+label, not just the id.

Check for org being switched in the `withOrg` component and load org if it has not been loaded yet

https://github.com/Katello/katello/pull/7954/files#diff-daaf4d03843ed16bfd9996e98f32dc43R49

We wrap all of our components that need organizations selected with a withOrg component. There is logic here that is triggered when the organization is switched.

The issue is, we load the organization in the Application container, which is one of the highest-level components. This works great when switching pages and on page refreshes, but when you use the org switcher, this higher component isn't updated, meaning no organization is loaded after picking the organization.

In order to load the organization using withOrg, I had to connect the component to redux to access the organization from the redux store (to check if it doesn't exists and its not loading) and use the loadOrganization method. This ensures the WrappedComponent has an organization loaded if the user has switched orgs using the org switcher page. I'll post before and after workflow so we can see the real-world impact.

A side note, I haven't figured out how to test withOrg now that checkOrg is connected to redux and is using the global store, any suggestions would be appreciated.

Update the Foreman Table to use different attributes as row keys

https://github.com/Katello/katello/pull/7954/files#diff-4a4f462c54c21140681143c0a4705c87

There was an error in the manage manifest modal where the manifest history doesn't have id's, so it couldn't use the id as a unique key. This is was showing up as an error in the console, so I modified it to use a default of 'id', but take a different rowKey if needed.

Pass in `undefined` for `subscriptionTableSettings` rather than `{}`

https://github.com/Katello/katello/pull/7954/files#diff-0d6063953b58c2306f62fd8d8c9b508aR28

We were passing in {} when no subscription table settings were loaded yet. The problem is {} is not equal to {} in JavaScript (yay javascript equalities!)

> {} === {}
> false

So this means React was registering this as a props change and re-rendering the component multiple times, even though nothing actually changed.

I updated to pass in undefined and then use a default prop of {}, this made the component update less times.

Side note - I found this out using this 'how did my props change?' wrapper function https://gist.github.com/sqren/780ae8ca1e2cf59050b0695c901b5aa3

Default to `disconnected=false` for subs

https://github.com/Katello/katello/pull/7954/files#diff-61cdabbb07b4a33567fae345a15f8ebcR34

This was another trigger for needless re-render. the disconnected attribute would start out as undefined, but the change to false when the API call was completed. The component would trigger an update from this change, but none of the logic changed since disconnected is still a falsey value.

Call the subscriptions API once instead of 3 times on page load

https://github.com/Katello/katello/pull/7954/files#diff-79ce8e491145b55ec7798ca245f3854f

When loading the subscription page, you would see a call for the subscriptions 3 times. The three times were triggered for these reasons:

componentDidMount calls this.props.loadSubscriptions()
componentDidUpdate calls this.pollTasks() (which calls loadSubscriptions) when the previous organization diffs from the current one because of the loading attribute, the org isn't actually loaded yet, its just signifying it is loading
componentDidUpdate calls this.pollTasks() because the actual org has now loaded, so the current and previous orgs from props differ.

I changed this so just the 3rd action happens now. This creates only one API request to subscriptions on page refresh, navigation change, org change, etc.. I haven't seen any issues with using pollTasks to call loadSubscriptions, but I'm open to other suggestions.

This also changes the subscrpition page from 'loading twice' in the UI. On the master branch currently, you will see the spinner, the table load, and the spinner goes again.

I'll provide some screenshots and screencaptures to help show the actual UI impact of these changes

johnpmitsch · 2019-01-31T21:33:33Z

Regarding the subs API being called three times:

Here you see SUBSCRIPTIONS_REQUEST three times on the Redux dev tools on the master branch on a page refresh:

And only once for this PR:

We can check the network tab too:
master branch:

This PR:

johnpmitsch · 2019-01-31T21:35:55Z

Here is the subs loading twice issue:
master branch

This PR:

johnpmitsch · 2019-01-31T22:09:29Z

Here is what I believe to be the cause of the original issue linked to this PR, the subs and manifest aren't visible on page load. I was able to recreate this by logging in as a user without a default organization, navigating to subscriptions, and choosing an org from the dropdown

Master branch:

PR:

johnpmitsch · 2019-01-31T22:11:29Z

There is a lot in this PR (even if its not a huge diff), please ask questions about any of the confusing parts, I'm also open to alternate solutions. 👂

johnpmitsch · 2019-01-31T22:17:26Z

I have to update tests, the one thing I'm stuck on is the withOrg tests. Since CheckOrg is now connected to redux, it doesn't have a store, and returns this error:

./node_modules/.bin/jest webpack/components/WithOrganization/withOrganization.test.js
 FAIL  webpack/components/WithOrganization/withOrganization.test.js
  subscriptions page
    ✕ should render the wrapped component (53ms)
    ✕ should render select org page (17ms)

  ● subscriptions page › should render the wrapped component

    Error: Uncaught [Invariant Violation: Could not find "store" in either the context or props of "Connect(CheckOrg)". Either wrap the root component in a <Provider>, or explicitly pass
 "store" as a prop to "Connect(CheckOrg)".]

I think I'll have to break out CheckOrg and test that as a disconnected component (which is what we do for every other component), but then I don't know how to pass the WrappedComponent to it. Any suggestions would be appreciated

johnpmitsch · 2019-01-31T22:18:29Z

ping @waldenraines and @sharvit for review please 🙏

jturel · 2019-02-01T18:51:56Z

webpack/components/SelectOrg/SetOrganization.js


-    changeCurrentOrgaziation(encodeURIComponent(`${id}-${item}`)).then(() =>
+    changeCurrentOrgaziation(`${id}`).then(() =>


Maybe it's a good time to correct the method name to 'changeCurrentOrganization' !

I agree! I meant to do this and forgot. I'll update!

jturel · 2019-02-01T18:53:59Z

webpack/scenes/Subscriptions/SubscriptionsPage.js

@@ -69,7 +68,10 @@ class SubscriptionsPage extends Component {
      }
    }

-    if (!isEqual(organization, prevProps.organization)) {
+    // remove the loading attribute so the action isn't called when org starts loading
+    const { loading: _cloading, ...currentOrgInfo } = organization;


what are _cloading and _ploading ?

I am removing the loading attribute from the object. to do this, I'm assigning it to a variable and pulling the rest of the object to another variable currentOrgInfo. If I just use loading, I can't use loading again in the next line since its a const, so I added 'c' and 'p' and then '_' to signify its not being used.

Definitely open to suggestions here, I don't love these names either 🤢

Got it. Had to play in the repl but it makes sense. Very interesting syntax!

what about:

const getOrgInfo = (org) => { const { loading, ...info } = org; return info; }; const currentOrgInfo = getOrgInfo(organization); const prevOrgInfo = getOrgInfo(prevProps.organization);

jturel

Changes look great. Left a few questions and I'll take another look once we can get a green build. I ran tests locally and see there are plenty of failures from your changes so that's reassuring of our test coverage. Unfortunately I don't have any advice on testing the withOrg code, but breaking the part out that you mentioned seems like a good start!

jturel · 2019-02-01T20:10:48Z

Side note: i found an issue that was present on the page with and without this change - so I opened an issue https://projects.theforeman.org/issues/25969

johnpmitsch · 2019-02-01T20:40:58Z

I also found an issue (that exists in master) with the org switcher, which I wasn't able to solve in this PR https://projects.theforeman.org/issues/25967

johnpmitsch · 2019-02-04T19:47:18Z

@jturel updated the method name and the tests

jturel

Super nice changes. ACK

sharvit · 2019-02-06T11:28:51Z

webpack/scenes/Subscriptions/index.js

@@ -25,7 +24,8 @@ import SubscriptionsPage from './SubscriptionsPage';
 // map state to props
 const mapStateToProps = (state) => {
  const subscriptions = selectSubscriptionsState(state);
-  const subscriptionTableSettings = state.katello.settings.tables[SUBSCRIPTION_TABLE_NAME] || {};
+  const subscriptionTableSettings =


Could you use a selector here to make it a bit more readable?

I mean something like:

const subscriptionTableSettings = selectTableSettings(SUBSCRIPTION_TABLE_NAME);

waldenraines

Minor comments and a question.

waldenraines · 2019-02-06T17:35:19Z

webpack/scenes/Subscriptions/Manifest/ManageManifestModal.js

@@ -268,6 +268,7 @@ class ManageManifestModal extends Component {
                  rows={manifestHistory.results}
                  columns={columns}
                  emptyState={emptyStateData()}
+                  rowKey="created"


Is this guaranteed to be unique?

good question, I can double check how granular the timestamp is

The timestamp looks like its down to the second, here is the an example of the actual key react uses 2019-01-29T16:28:59+0000-row

The object we are getting looks like this:

{ "statusMessage":"test_manifest_roles file imported successfully."," status":"SUCCESS", "created":"2019-01-29T16:28:59+0000" }

I added id to the manifest history API, it looks like we can return a uuid too

{ "id":"4028f92a689a347f01689a7140c002b7", "statusMessage":"test_manifest_roles file imported successfully.", "status":"SUCCESS", "created":"2019-01-29T16:28:59+0000"}

I can just use the id from the API after adding it there, I don't see a reason to not add it in the API. I'll revert the table changes too since I think we want to continue to use id until there is a reason not too

waldenraines · 2019-02-06T17:37:29Z

webpack/components/WithOrganization/withOrganization.js

+});
+
+const actions = { ...organizationActions };
+const mapDispatchToProps = dispatch => bindActionCreators(actions, dispatch);


Is there any real need for this instead of just using bindActionCreators({ ...organizationActions }, dispatch);?

either way works for me, I can update to one line 👍

johnpmitsch · 2019-02-06T20:57:07Z

@sharvit @waldenraines updated (in separate commits) according to your suggestions

johnpmitsch · 2019-02-06T21:32:01Z

[test katello]

waldenraines

LGTM, thanks @johnpmitsch

jlsherrill · 2019-02-08T04:14:35Z

[test katello]

jturel · 2019-02-12T05:03:33Z

@johnpmitsch just making sure ... you waiting for anything from me on this one?

johnpmitsch · 2019-02-12T13:28:24Z

@jturel all set, thanks for asking. @sharvit is going to take another look when he gets back from PTO

sharvit · 2019-02-13T15:38:49Z

webpack/components/WithOrganization/__snapshots__/withOrganization.test.js.snap

-<CheckOrg>
-  <Header
-    title="Select Organization"
+<Connect(CheckOrg)


Those snapshots became too big to maintain and understand and the reason is that this component is now connected.

I would like to suggest a solution to it:

Separate the CheckOrg class from the withOrganization.js to a new file (CheckOrg.js) so it will be a standalone component.
The CheckOrg component will recive a children instead using the WrappedComponent.
The withOrganization hoc will render CheckOrg with the WrappedComponent as it's children and will connect it to redux.

Do the current snapshoot testings only for the CheckOrg component so we will have lean snapshoots.

Need to have lean snapshoot testing for actions/reducers/selectors/helpers.

Need to have a lean integration-testing for the HOC that run all the react-redux lifecycle using the IntegrationTestHelper.
Notice the IntegrationTestHelper is in a process to be a standalone npm package, see Fixes #25910 - Move js test-helpers to npm theforeman/foreman#6432 and Fixes #25912 - Move js test-helpers to npm #7940
You can use those docs: https://sharvit.github.io/react-redux-test-utils/manual/integration-testing.html
And some examples:
https://github.com/theforeman/foreman/blob/edc0a1c73eb59b28b266e63a1a028c788837a942/webpack/assets/javascripts/react_app/components/Layout/__tests__/integration.test.js
https://github.com/theforeman/foreman/blob/c18d67464d81943b77feeb0e24ec649574860ab7/webpack/assets/javascripts/react_app/components/BreadcrumbBar/__tests__/integration.test.js
https://github.com/theforeman/foreman/blob/26345fcd991a3d3ca63f917ae1eed161167dd5aa/webpack/assets/javascripts/react_app/components/SearchBar/__tests__/integration.test.js
https://github.com/theforeman/foreman/blob/c18d67464d81943b77feeb0e24ec649574860ab7/webpack/assets/javascripts/react_app/components/PasswordStrength/__tests__/integration.test.js

What do you think?

I get what you are thinking, but I'm having a really hard time putting this into practice.

Here is where I am at johnpmitsch@5b74686 I've had to work past some issues including things like HOCs not playing nice with react-router

The current error that i'm blocked by is:

react-dom.development.js:506 Warning: Functions are not valid as a React child. This may happen if you return a Component instead of <Component /> from render. Or maybe you meant to call this function rather than return it. in Unknown (created by Route) in Route in div in Unknown (created by Application) in Router (created by BrowserRouter) in BrowserRouter (created by Application) in Application (created by Connect(Application)) in Connect(Application) (created by I18nProviderWrapper(Connect(Application))) in IntlProvider (created by I18nProviderWrapper(Connect(Application))) in I18nProviderWrapper(Connect(Application)) (created by StoreProvider(I18nProviderWrapper(Connect(Application)))) in Provider (created by StoreProvider(I18nProviderWrapper(Connect(Application)))) in StoreProvider(I18nProviderWrapper(Connect(Application))) (created by DataProvider(StoreProvider(I18nProviderWrapper(Connect(Application))))) in DataProvider(StoreProvider(I18nProviderWrapper(Connect(Application))))

Let me know if you can see where things are going wrong, I'll keep looking, but i'm pretty stumped to be honest :)

Too bad, code-wise it makes so much sense 😞

johnpmitsch · 2019-02-18T15:53:00Z

Opened up a follow up issue for the HOC discussion https://projects.theforeman.org/issues/26071

sharvit

Thanks @johnpmitsch LGTM 👍

Fixes #25957 - Subscription page UI enhancements

03d9bad

Parts of the subscription page are not loading during certain workflows and it is making duplicate API requests on load

theforeman-bot added Needs testing Not yet reviewed labels Jan 31, 2019

jturel self-assigned this Feb 1, 2019

jturel reviewed Feb 1, 2019

View reviewed changes

John Mitsch added 2 commits February 4, 2019 19:33

Refs #25957 - Update withOrg test

898fdd6

Refs #25957 - update snapshots

84c8556

jturel approved these changes Feb 5, 2019

View reviewed changes

sharvit reviewed Feb 6, 2019

View reviewed changes

waldenraines reviewed Feb 6, 2019

View reviewed changes

John Mitsch added 2 commits February 6, 2019 20:45

Refs #25957 - updated according to suggestions

c3442db

Refs #25957 - Update snapshots

5bf55af

waldenraines approved these changes Feb 7, 2019

View reviewed changes

sharvit suggested changes Feb 13, 2019

View reviewed changes

sharvit approved these changes Feb 18, 2019

View reviewed changes

johnpmitsch merged commit 2890799 into Katello:master Feb 18, 2019

johnpmitsch deleted the manifest_user_issues branch February 18, 2019 16:04


		changeCurrentOrgaziation(encodeURIComponent(`${id}-${item}`)).then(() =>
		changeCurrentOrgaziation(`${id}`).then(() =>

Fixes #25957 - Subscription page UI loading fixes #7954

Fixes #25957 - Subscription page UI loading fixes #7954

Conversation

johnpmitsch commented Jan 31, 2019

theforeman-bot commented Jan 31, 2019

johnpmitsch commented Jan 31, 2019 • edited

Pass just the id into changeCurrentOrganization

Check for org being switched in the withOrg component and load org if it has not been loaded yet

Update the Foreman Table to use different attributes as row keys

Pass in undefined for subscriptionTableSettings rather than {}

Default to disconnected=false for subs

Call the subscriptions API once instead of 3 times on page load

johnpmitsch commented Jan 31, 2019

johnpmitsch commented Jan 31, 2019

johnpmitsch commented Jan 31, 2019

johnpmitsch commented Jan 31, 2019

johnpmitsch commented Jan 31, 2019

johnpmitsch commented Jan 31, 2019

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

jturel left a comment

Choose a reason for hiding this comment

jturel commented Feb 1, 2019

johnpmitsch commented Feb 1, 2019

johnpmitsch commented Feb 4, 2019

jturel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waldenraines 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnpmitsch commented Feb 6, 2019 • edited

johnpmitsch commented Feb 6, 2019

waldenraines left a comment

Choose a reason for hiding this comment

jlsherrill commented Feb 8, 2019

jturel commented Feb 12, 2019

johnpmitsch commented Feb 12, 2019

sharvit Feb 13, 2019 • edited

Choose a reason for hiding this comment

johnpmitsch Feb 14, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnpmitsch commented Feb 18, 2019

sharvit left a comment

Choose a reason for hiding this comment

johnpmitsch commented Jan 31, 2019 •

edited

Pass just the id into `changeCurrentOrganization`

Check for org being switched in the `withOrg` component and load org if it has not been loaded yet

Pass in `undefined` for `subscriptionTableSettings` rather than `{}`

Default to `disconnected=false` for subs

johnpmitsch commented Feb 6, 2019 •

edited

sharvit Feb 13, 2019 •

edited

johnpmitsch Feb 14, 2019 •

edited