Add client view initial search content #158

[projectyang]

Set client id as default search in Clients view

Clients table will now show filtered results by client id as default.

Fixes #158, however users are now unable to retrieve a 'full' default clients view (e.g. expected search query with '' to retrieve full list of clients does not work)

[helfi92]

Awesome work!

Regarding retrieving the full list, give the following a try:

diff --git a/src/views/Clients/ViewClients/index.jsx b/src/views/Clients/ViewClients/index.jsx
index db6aa55..3150ce0 100644
--- a/src/views/Clients/ViewClients/index.jsx
+++ b/src/views/Clients/ViewClients/index.jsx
@@ -93,13 +93,9 @@ export default class ViewWorker extends PureComponent {
     const { clientSearch } = this.state;
 
     refetch({
-      ...(clientSearch
-        ? {
-            clientOptions: {
-              prefix: clientSearch,
-            },
-          }
-        : null),
+      clientOptions: {
+        ...(clientSearch ? { prefix: clientSearch } : null),
+      },
       clientsConnection: {
         limit: VIEW_CLIENTS_PAGE_SIZE,
       },

[helfi92]

If you log out and then refresh the page, the view will crash on this line. This is because this.props.user is undefined.

You can do:

clientSearch: this.props.user ? this.props.user.credentials.clientId || '',

[projectyang]

I also noticed that if I were to query with spaces e.g. "   ", the following error appears:

Should we attempt to sanitize any leading spaces in the search value before querying? E.g. "   exampleId" --> "exampleId"

[helfi92]

Good catch. Sanitization would be great to have!

[helfi92]

Looks good, just a small change.

[projectyang]

Thanks for the help! Really appreciate the time you took to explain everything.

[helfi92]

Hopefully the last change before merging :)

[helfi92]

This is going to fetch the list of clients twice. Let's rely on the initial graphql call to perform search the filtered clients. You can achieve that by adding the same logic for clientsOptions under variables. See other comment.

[helfi92]

options can have access to props:

options: (props) => ({

[helfi92]

Since we won't be calling this method from componentDidMount anymore, we can remove the if statement and just use e.preventDefault().

[projectyang]

I've made the recommended changes but discovered two scenarios that may cause issues for users:

Scenario 1:
If a user logs in or out on the Clients view, the search bar does not update automatically. This can lead to confusion if a user logs in (and triggers the default clientId query) and does not know about it.

My proposed solution is to use ComponentDidUpdate in the clients component to monitor when a log in/out has occurred and update state for the search component.
No need for lifecycle here, just used the props with ternary for the Search component.

Scenario 2:
Related to the above, if a user logins in or out on the Clients view an error is thrown by Apollo, which disappears/resolves shortly with the updated view:

I'm having difficulty solving this one. My assumption is that the graphql query for the Clients component is still in flight when resetStore() is called, thus throwing the error. Is the correct approach to wait for the query to finish before triggering resetStore()?

[helfi92]

I'm having difficulty solving this one. My assumption is that the graphql query for the Clients component is still in flight when resetStore() is called, thus throwing the error.

See if the following fixes it:

diff --git a/src/components/Dashboard/UserMenu.jsx b/src/components/Dashboard/UserMenu.jsx
index 3ff85f3..d573cab 100644
--- a/src/components/Dashboard/UserMenu.jsx
+++ b/src/components/Dashboard/UserMenu.jsx
@@ -59,12 +59,12 @@ export default class UserMenu extends Component {
     this.setState({ signInDialogOpen: false });
   };
 
-  handleClickSignOut = () => {
+  handleClickSignOut = async () => {
     this.handleMenuClose();
-    this.props.onUnauthorize();
     // Since Apollo caches query results, it’s important to get rid of them
     // when the login state changes.
-    this.props.client.resetStore();
+    await this.props.client.resetStore();
+    this.props.onUnauthorize();
   };
 
   render() {
diff --git a/src/components/SignInDialog/index.jsx b/src/components/SignInDialog/index.jsx
index 80983fd..b03c6e9 100644
--- a/src/components/SignInDialog/index.jsx
+++ b/src/components/SignInDialog/index.jsx
@@ -44,11 +44,14 @@ export default class SignInDialog extends Component {
     );
   }
 
-  handleCredentialsSignIn = credentials => {
+  handleCredentialsSignIn = async credentials => {
     const inOneWeek = new Date();
 
     inOneWeek.setDate(inOneWeek.getDate() + 7);
 
+    // Since Apollo caches query results, it’s important to get rid of them
+    // when the login state changes.
+    await this.props.client.resetStore();
     this.props.onAuthorize({
       credentials,
       expires: inOneWeek.toISOString(),
@@ -57,9 +60,6 @@ export default class SignInDialog extends Component {
         displayName: credentials.clientId,
       },
     });
-    // Since Apollo caches query results, it’s important to get rid of them
-    // when the login state changes.
-    this.props.client.resetStore();
     this.props.onClose();
   };

[projectyang]

That fixed it!

I ended up using getDerivedStateFromProps to solve scenario one. Let me know your thoughts on that approach.

The eslint error is appearing since I only reference a state field inside getDerivedStateFromProps. Here's a relevant thread with a PR at the end that addresses this.

[helfi92]

I like the idea of using getDerivedStateFromProps :) Just some nits.

[helfi92]

Let's make Travis happy :)

https://travis-ci.org/taskcluster/taskcluster-web/builds/442345832?utm_source=github_status&utm_medium=notification

[projectyang]

Travis is hard to please...For the error it's failing on:

error: Unused state field: 'previousClientId' (react/no-unused-state) at src/views/Clients/ViewClients/index.jsx:40:5:

I believe it should pass since we are referring to the state previousClientId in getDerivedStateFromProps. This seems to be more of a problem with the eslint rules than the code in my opinion.

See this thread here - which reflects updates made to eslint-plugin-react which allows for this to pass.

The other option could be trying to incorporate previousClientId somewhere else in the component, which seems unnecessary if it's just to satisfy a rule.

[eliperelman]

It's throwing because the previousClientId isn't used within render, which is a smell for something incorrect being done.

[projectyang]

@eliperelman, can you help clarify for me why this is a smell? Apologies for my inexperience with React.

I was under the impression that this pattern was acceptable based on the React docs example.

My belief that this is an issue with the eslint package is based on this thread here for eslint-plugin-react where others have complained about similar situations regarding no-unused-state and getDerivedStateFromProps, which was eventually resolved and merged into master here.

[helfi92]

You can remove the error with:

// eslint-disable-next-line react/no-unused-state
previousClientId: '',

It's also done in

taskcluster-web/src/components/ClientForm/index.jsx

Line 99 in 04d3822

// eslint-disable-next-line react/no-unused-state

.

[helfi92]

Fantastic work 🎉