Add client view initial search content #158 #160

projectyang · 2018-10-12T17:40:24Z

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 · 2018-10-12T18:42:59Z

src/views/Clients/ViewClients/index.jsx

  state = {
-    clientSearch: '',
+    clientSearch: this.props.user.credentials.clientId,


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 · 2018-10-12T19:44:42Z

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 · 2018-10-12T19:46:38Z

Good catch. Sanitization would be great to have!

helfi92

Looks good, just a small change.

src/views/Clients/ViewClients/index.jsx

projectyang · 2018-10-12T21:22:47Z

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

helfi92

Hopefully the last change before merging :)

helfi92 · 2018-10-13T16:33:09Z

src/views/Clients/ViewClients/index.jsx

@@ -30,8 +32,11 @@ import clientsQuery from './clients.graphql';
  },
 }))
 export default class ViewWorker extends PureComponent {
+  componentDidMount() {
+    this.handleClientSearchSubmit();
+  }


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 · 2018-10-13T16:34:12Z

src/views/Clients/ViewClients/index.jsx


 @hot(module)
+@withAuth
 @graphql(clientsQuery, {
  options: () => ({


options can have access to props:

options: (props) => ({

src/views/Clients/ViewClients/index.jsx

helfi92 · 2018-10-13T16:36:58Z

src/views/Clients/ViewClients/index.jsx

-    e.preventDefault();
+    if (e) {
+      e.preventDefault();
+    }


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

projectyang · 2018-10-15T15:38:58Z

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 · 2018-10-15T19:05:13Z

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();
   };

…ients view

projectyang · 2018-10-15T20:31:17Z

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.

src/views/Clients/ViewClients/index.jsx

helfi92 · 2018-10-16T21:00:22Z

Let's make Travis happy :)

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

projectyang · 2018-10-16T21:30:09Z

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 · 2018-10-16T22:24:28Z

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

projectyang · 2018-10-17T00:07:43Z

@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 · 2018-10-17T11:44:03Z

src/views/Clients/ViewClients/index.jsx

  state = {
    clientSearch: '',
+    previousClientId: '',


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 🎉

Add client view initial search content taskcluster#158

e4df3c3

projectyang requested a review from helfi92 as a code owner October 12, 2018 17:40

helfi92 suggested changes Oct 12, 2018

View reviewed changes

add full list and fix log out error

367df6b

projectyang added 2 commits October 12, 2018 16:17

remove any spaces from search query

cac3476

remove console.log

80c5a74

helfi92 reviewed Oct 12, 2018

View reviewed changes

src/views/Clients/ViewClients/index.jsx Outdated Show resolved Hide resolved

src/views/Clients/ViewClients/index.jsx Outdated Show resolved Hide resolved

src/views/Clients/ViewClients/index.jsx Outdated Show resolved Hide resolved

replace regex with trim

58e7c60

helfi92 suggested changes Oct 13, 2018

View reviewed changes

remove componentDidMount

b6e9875

remove didComponentUpdate

db90b15

add async/await to sign in/out and add getDerivedStatefromProps to cl…

4b34045

…ients view

helfi92 suggested changes Oct 16, 2018

View reviewed changes

src/views/Clients/ViewClients/index.jsx Outdated Show resolved Hide resolved

src/views/Clients/ViewClients/index.jsx Outdated Show resolved Hide resolved

src/views/Clients/ViewClients/index.jsx Outdated Show resolved Hide resolved

helfi92 mentioned this pull request Oct 16, 2018

Migrate tc-web to neutrino v9 #157

Merged

edit to previousClientId

ea3a732

helfi92 reviewed Oct 17, 2018

View reviewed changes

disable no unsused state for previousClientId

74b52d9

helfi92 approved these changes Oct 17, 2018

View reviewed changes

helfi92 merged commit 2c2fa70 into taskcluster:master Oct 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add client view initial search content #158 #160

Add client view initial search content #158 #160

projectyang commented Oct 12, 2018 •

edited

Loading

helfi92 left a comment

helfi92 Oct 12, 2018

projectyang commented Oct 12, 2018 •

edited

Loading

helfi92 commented Oct 12, 2018

helfi92 left a comment

projectyang commented Oct 12, 2018

helfi92 left a comment

helfi92 Oct 13, 2018

helfi92 Oct 13, 2018

helfi92 Oct 13, 2018

projectyang commented Oct 15, 2018 •

edited

Loading

helfi92 commented Oct 15, 2018

projectyang commented Oct 15, 2018

helfi92 left a comment

helfi92 commented Oct 16, 2018

projectyang commented Oct 16, 2018

eliperelman commented Oct 16, 2018

projectyang commented Oct 17, 2018 •

edited

Loading

helfi92 Oct 17, 2018

helfi92 left a comment •

edited

Loading

Add client view initial search content #158 #160

Add client view initial search content #158 #160

Conversation

projectyang commented Oct 12, 2018 • edited Loading

helfi92 left a comment

Choose a reason for hiding this comment

helfi92 Oct 12, 2018

Choose a reason for hiding this comment

projectyang commented Oct 12, 2018 • edited Loading

helfi92 commented Oct 12, 2018

helfi92 left a comment

Choose a reason for hiding this comment

projectyang commented Oct 12, 2018

helfi92 left a comment

Choose a reason for hiding this comment

helfi92 Oct 13, 2018

Choose a reason for hiding this comment

helfi92 Oct 13, 2018

Choose a reason for hiding this comment

helfi92 Oct 13, 2018

Choose a reason for hiding this comment

projectyang commented Oct 15, 2018 • edited Loading

helfi92 commented Oct 15, 2018

projectyang commented Oct 15, 2018

helfi92 left a comment

Choose a reason for hiding this comment

helfi92 commented Oct 16, 2018

projectyang commented Oct 16, 2018

eliperelman commented Oct 16, 2018

projectyang commented Oct 17, 2018 • edited Loading

helfi92 Oct 17, 2018

Choose a reason for hiding this comment

helfi92 left a comment • edited Loading

Choose a reason for hiding this comment

projectyang commented Oct 12, 2018 •

edited

Loading

projectyang commented Oct 12, 2018 •

edited

Loading

projectyang commented Oct 15, 2018 •

edited

Loading

projectyang commented Oct 17, 2018 •

edited

Loading

helfi92 left a comment •

edited

Loading