Bug 1520857- add support in tc-web for displaying information from the lastFire table #413

Biboswan · 2019-01-24T19:10:07Z

No description provided.

Biboswan · 2019-01-24T19:20:11Z

The error panel slighty goes out in very small screens as seen in the 2nd pic. When fixing the width there were some other very bad overlapping with other fired hooks info
cc @djmitche

helfi92

Good progress :) Added some requested changes.

helfi92 · 2019-01-24T19:29:06Z

src/views/Hooks/ViewHook/index.jsx

+        hookId: decodeURIComponent(params.hookId),
+      },
+    }),
+    name: 'lastFiresData',
  }),


No need to create hookLastFires.graphql. You should be able to add everything in hook.graphql.

helfi92 · 2019-01-24T19:29:57Z

src/views/Hooks/ViewHook/index.jsx

@@ -128,13 +142,13 @@ export default class ViewHook extends Component {
  };

  render() {
-    const { isNewHook, data } = this.props;
+    const { isNewHook, hookData, lastFiresData } = this.props;


One you merge the last fires data in hook.graphql then everything will be under data.

helfi92 · 2019-01-24T19:30:20Z

src/views/Hooks/ViewHook/hookLastFires.graphql

@@ -0,0 +1,9 @@
+query HookLastFires($hookGroupId: ID!, $hookId: ID!) {


Delete this file. See reason in other comment.

helfi92 · 2019-01-24T19:46:03Z

src/components/HookForm/index.jsx

-            </Fragment>
+                {hookLastFires.map(
+                  ({ taskId, taskCreateTime, firedBy, result, error }) => (
+                    <ListItem key={taskId}>


Let's use the DataTable component in src/components/DataTable to display the result? The table will be similar to something like http://localhost:5080/provisioners/aws-provisioner-v1.

I don't think a DataTable would be a good fit for the data. If result is success then expandable panel has nothing to show. If result is error then there is very large error text to show on opening the panel. In any column it could be trouble to fit the error text especially.

helfi92 · 2019-01-24T19:49:17Z

src/components/HookForm/index.jsx

@@ -131,6 +139,8 @@ export default class HookForm extends Component {
  static propTypes = {
    /** A GraphQL hook response. Not needed when creating a new hook  */
    hook: hook.isRequired,
+    /** Object */


Let's use a different comment here to describe the property.

helfi92 · 2019-01-24T19:52:40Z

src/components/HookForm/index.jsx

+                              </Grid>
+                            </ExpansionPanelSummary>
+                            <ExpansionPanelDetails>
+                              <ErrorPanel error={error} onClose={null} />


We could probably have errors be a separate column in the table and use simple text to display the error instead of having it in a error panel. We use error panels for when the server fails to return data or the user is trying to do something not allowed.

Biboswan · 2019-02-06T19:23:31Z

Biboswan · 2019-02-06T19:24:15Z

have a look @helfi92

helfi92 · 2019-02-06T19:39:02Z

Awesome, I'll have a look soon.

owlishDeveloper · 2019-02-07T17:27:12Z

@helfi92 - I'm a bit out of the loop on the plans for this repository. It's not being archived after having merged to the monorepo?

helfi92 · 2019-02-07T18:09:34Z

@owlishDeveloper I unarchived the repo so @Biboswan and I can continue iterating on this PR. Once we are happy with the PR, @Biboswan will create a PR on on the monorepo and we will re-archive this repo. The discussion happened in Bug 1520857.

helfi92 · 2019-02-07T18:20:52Z

src/components/DataTable/index.jsx

@@ -7,13 +7,15 @@ import {
  oneOf,
  oneOfType,
  object,
+  bool,


Why are we adding pagination? Is the last fire endpoint paginated (i.e., uses continuation token)?

As per discussion with Dustin in irc we are keeping as many as 100 lastfire info rows for each hookId (expiration logic). Hence I think showing all rows at once won't be good.

We also only purge those once per day, so a particularly active hook might have many more rows than that at some time during the day!

helfi92 · 2019-02-07T18:26:28Z

src/components/HookForm/index.jsx

+                      {(hookFire.result === 'SUCCESS' && (
+                        <Link to={`/tasks/${hookFire.taskId}`}>
+                          <Typography>{hookFire.taskId}</Typography>
+                        </Link>


Right now it looks like:

It should look like this:

This will make it look like any other link in a table to the rest of the app. Here's an example: The Worker ID column in the Workers view.

If the lastfire result is success then the taskId links to another route. But if result is error then no task of the mentioned taskId is actually created hence there is no link to the corresponding task. So I'm underlinking taskId for the sucess ones and no underline for error ones to distinguish them. Shall I change it ?

From https://github.com/taskcluster/taskcluster-web/pull/413/files#r254810845, we could remove the link icon when the task has an error.

helfi92 · 2019-02-07T18:27:25Z

src/components/HookForm/index.jsx

+                            />
+                          </ExpansionPanelDetails>
+                        </ExpansionPanel>
+                      )) || <Typography>N/A</Typography>}


Use lower case n/a.

helfi92 · 2019-02-07T18:28:26Z

src/components/HookForm/index.jsx

+                    </TableCell>
+                    <TableCell className={classes.errorTableCell}>
+                      {(hookFire.result === 'ERROR' && (
+                        <ExpansionPanel>


Is there a hook I can see where hookFire.result === 'ERROR'? I want to see how it looks in the UI.

helfi92 · 2019-02-07T18:29:47Z

src/views/Hooks/ViewHook/index.jsx

+                hookLastFires={data.hookLastFires.sort(
+                  (a, b) =>
+                    new Date(b.taskCreateTime) - new Date(a.taskCreateTime)
+                )}


Extract logic from jsx:

const hookLastFires = // ... // ... hookLastFires={hookLastFires}

Biboswan · 2019-02-07T20:24:13Z

helfi92 · 2019-02-11T15:19:49Z

@Biboswan How can I get to see the list of errors in the UI when I pull the code down locally? I want to try to make some tweaks on my local machine.

Biboswan · 2019-02-11T15:38:35Z

@helfi92 generally hooks with hookGroup "project-mobile" have a lot of error and success. Sorry I missed your previous comment on that

helfi92 · 2019-02-11T15:50:30Z

Thanks! I'll take a look.

helfi92

Added requested changes.

helfi92 · 2019-02-11T16:02:24Z

src/components/HookForm/index.jsx

+                    </TableCell>
+                    <TableCell className={classes.errorTableCell}>
+                      {(hookFire.result === 'ERROR' && (
+                        <ExpansionPanel>


Instead of having an expansion panel for each error, we should instead have a button that opens a drawer with the error on click. This is similar to https://taskcluster-web.netlify.com/provisioners/aws-provisioner-v1 when clicking the info icon. The drawer can display the ErrorPanel component.

helfi92 · 2019-02-11T16:06:14Z

src/components/HookForm/index.jsx

+                      {(hookFire.result === 'SUCCESS' && (
+                        <Link to={`/tasks/${hookFire.taskId}`}>
+                          <Typography>{hookFire.taskId}</Typography>
+                        </Link>


From https://github.com/taskcluster/taskcluster-web/pull/413/files#r254810845, we could remove the link icon when the task has an error.

Biboswan · 2019-02-15T23:00:29Z

Only error message i could find at the moment

helfi92

Actual: The error is shown in the table
Expected: The error is displayed outside the table in a Drawer component. To see what I mean:

Navigate to https://taskcluster-web.netlify.com/provisioners/aws-provisioner-v1
Click on the info icon. You will see a drawer that will slide from the right hand side of the screen.

the info button in the worker types view

the drawer in the worker type view after clicking the info button

Coming back to this issue, we want to display the error inside the drawer, not inside the table. You can look at the code of https://taskcluster-web.netlify.com/provisioners/aws-provisioner-v1 to see how the drawer is implemented.

Hope that helps. Let me know if you had any questions/concerns.

Biboswan · 2019-02-21T17:38:18Z

ah i just skipped the drawer :(. making the changes

Biboswan · 2019-02-22T17:55:51Z

helfi92

Looks a lot better! A couple of points:

https://github.com/taskcluster/taskcluster-web/pull/413/files#r254810845 is not done yet.
Let's show "n/a" (without the quotes) instead of the information icon when there's no error.

Biboswan · 2019-02-22T19:57:59Z

https://github.com/taskcluster/taskcluster-web/pull/413/files#r254810845 is not done yet.

Did I misunderstood something. When the task has an error there is no link icon aka underline as you said.When there is no error the task exists so taskId has the link icon as /tasks/${hookFire.taskId} exists. So do you want me to remove link in both the cases ?

helfi92 · 2019-02-22T20:54:13Z

When the task has an error there is no link icon aka underline as you said.When there is no error the task exists so taskId has the link icon as /tasks/${hookFire.taskId} exists. So do you want me to remove link in both the cases ?

Instead of the underline, it should match the following style (look at the link icon beside the id): https://user-images.githubusercontent.com/3766511/52433650-8ccd9b00-2adb-11e9-9df7-a5e272407a82.png. There is also a hover effect that comes from theme.mixins.listItemButton.

helfi92

Nice improvements :) A couple of points:

A task ID that has no link should not be wrapped in a TableCellListItem since it shouldn't be a list item. It should be just regular text.

There seems to be )) beside the information icon

helfi92

A couple of changes:

The "<" and ">" buttons should be to the right of the table, similar to how they are show in https://material-ui.com/demos/tables/#tables.

Set rowsPerPage to 5
Remove the option to change row count

Rename "Last Hook Fire Attempts" to "Last Fired Results"
The FiredBy column should be shown as Fired By. You can use the change-case package to modify the title. See how we do it for the other tables in the app.

Biboswan · 2019-02-26T16:22:05Z

@helfi92 you want to shift just the '< >' buttons rightward and not the page no. (1-20) part ?

helfi92 · 2019-02-26T16:23:47Z

If we can remove the (1-20) part then that would be the best because it will make it consistent with the other tables we use on the site.

Biboswan · 2019-02-26T20:17:20Z

i was asking whether to shift right both arrows and 1-20 or only arrows. If we remove 1-20 then user will not know total count is that ok

helfi92 · 2019-02-26T20:33:08Z

i was asking whether to shift right both arrows and 1-20 or only arrows. If we remove 1-20 then user will not know total count is that ok

Yes that's okay. We want to maintain consistency across our tables. None of our tables that have that (1-20) extra information.

helfi92 · 2019-03-11T19:18:56Z

How is it going @Biboswan. I am back now :)

Biboswan · 2019-03-11T21:05:32Z

Few days ago when I tried to resume the work I got errors on running the web server. I tried both Yarn start and Yarn start devSever

helfi92 · 2019-03-11T21:19:52Z

I pushed taskcluster/taskcluster@8494476 today. Try pulling the recent changes and see if that fixes it :)

Biboswan · 2019-03-12T23:24:32Z

it's done will push today

Biboswan · 2019-03-13T10:57:11Z

helfi92

Good progress!

helfi92 · 2019-03-13T13:02:59Z

src/components/DataTable/index.jsx

+  },
+  tablePaginationRoot: {
+    position: 'relative',
+    left: '207%',


I don't think we need this. You can look at how they do it in https://material-ui.com/demos/tables/.

helfi92 · 2019-03-13T13:08:55Z

src/components/DataTable/index.jsx

+        {isPaginate && (
+          <TableFooter>
+            <TableRow>
+              <TablePagination


We should:

Have the TablePagination outside the table. This would make sure the pagination is always displayed to the user even when the table is being viewed on a small screen where scrolling is required to view the data. There's an example of that in https://material-ui.com/demos/tables/#sorting-amp-selecting. Basically you will need to remove TableFooter and TableRow and then have TablePagination outside the table.

The table should be scrollable similar to how all the tables are in the material site.

helfi92 · 2019-03-13T13:09:41Z

src/components/HookForm/index.jsx

+    whiteSpace: 'normal',
+  },
+  errorPanel: {
+    maxHeight: '300px',


No need to put quotations when the units are px.

maxHeight: 300,

Biboswan · 2019-03-15T07:39:55Z

Biboswan · 2019-03-15T07:42:27Z

Which one is more appealing?

helfi92 · 2019-03-15T12:01:22Z

Second one.

helfi92

@Biboswan I don't seem to be able to open a hook for some reason. Anyways, I think we are in a better shape now to open a PR under taskcluster/taskcluster. Do you want to do that?

Biboswan · 2019-03-18T15:59:28Z

Yes! Will do that.

Biboswan added 2 commits January 24, 2019 03:36

Initial display of lastfire fires

c57174f

Improve responsiveness and overall ui with Grid

7a33df8

Biboswan requested a review from helfi92 as a code owner January 24, 2019 19:10

Remove unwanted class

5b5c97d

helfi92 suggested changes Jan 24, 2019

View reviewed changes

Biboswan added 3 commits January 25, 2019 18:36

Merge two hook graphql queries into one

a154a50

Replace expansion panel with datatable

f55678d

Add pagination

c5f5a16

helfi92 self-requested a review February 6, 2019 19:39

helfi92 suggested changes Feb 7, 2019

View reviewed changes

helfi92 suggested changes Feb 11, 2019

View reviewed changes

Biboswan added 2 commits February 15, 2019 17:29

minor fix

87993e3

Add toggle view for error

f98d385

helfi92 self-requested a review February 16, 2019 13:04

helfi92 suggested changes Feb 19, 2019

View reviewed changes

Add drawer to display error info

4674382

helfi92 suggested changes Feb 22, 2019

View reviewed changes

Minor tweak

0156365

add link icon for taskids

e238f6f

helfi92 suggested changes Feb 25, 2019

View reviewed changes

minor tweak

8bb249f

helfi92 suggested changes Feb 25, 2019

View reviewed changes

Remove 1-20 part and text rename

2b28062

helfi92 suggested changes Mar 13, 2019

View reviewed changes

add scrollbar

aa51ef9

helfi92 reviewed Mar 18, 2019

View reviewed changes

Biboswan mentioned this pull request Mar 18, 2019

ui to display lastfires taskcluster/taskcluster#466

Merged

		@@ -0,0 +1,9 @@
		query HookLastFires($hookGroupId: ID!, $hookId: ID!) {

Bug 1520857- add support in tc-web for displaying information from the lastFire table #413

Are you sure you want to change the base?

Bug 1520857- add support in tc-web for displaying information from the lastFire table #413

Conversation

Biboswan commented Jan 24, 2019

Biboswan commented Jan 24, 2019

helfi92 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Biboswan commented Feb 6, 2019 • edited Loading

Biboswan commented Feb 6, 2019

helfi92 commented Feb 6, 2019

owlishDeveloper commented Feb 7, 2019

helfi92 commented Feb 7, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Biboswan commented Feb 7, 2019

helfi92 commented Feb 11, 2019

Biboswan commented Feb 11, 2019

helfi92 commented Feb 11, 2019

helfi92 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Biboswan commented Feb 15, 2019

helfi92 left a comment • edited Loading

Choose a reason for hiding this comment

Biboswan commented Feb 21, 2019

Biboswan commented Feb 22, 2019

helfi92 left a comment

Choose a reason for hiding this comment

Biboswan commented Feb 22, 2019

helfi92 commented Feb 22, 2019

helfi92 left a comment

Choose a reason for hiding this comment

helfi92 left a comment

Choose a reason for hiding this comment

Biboswan commented Feb 26, 2019

helfi92 commented Feb 26, 2019

Biboswan commented Feb 26, 2019

helfi92 commented Feb 26, 2019

helfi92 commented Mar 11, 2019

Biboswan commented Mar 11, 2019

helfi92 commented Mar 11, 2019

Biboswan commented Mar 12, 2019

Biboswan commented Mar 13, 2019

helfi92 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

Biboswan commented Mar 15, 2019

Biboswan commented Mar 15, 2019 • edited Loading

helfi92 commented Mar 15, 2019

helfi92 left a comment • edited Loading

Choose a reason for hiding this comment

Biboswan commented Mar 18, 2019

Biboswan commented Feb 6, 2019 •

edited

Loading

helfi92 left a comment •

edited

Loading

Biboswan commented Mar 15, 2019 •

edited

Loading

helfi92 left a comment •

edited

Loading