Bug 1609211 - Persist settings in URL by SuyashSalampuria · Pull Request #6656 · mozilla/treeherder

SuyashSalampuria · 2020-07-26T20:29:29Z

Fixes #6226

Order By changes persist in the URL
Group By changes persist in the URL
Metrics changes persist in the URL

codecov-commenter · 2020-07-27T21:53:47Z

Codecov Report

Merging #6656 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #6656   +/-   ##
=======================================
  Coverage   88.32%   88.32%           
=======================================
  Files         280      280           
  Lines       12776    12776           
=======================================
  Hits        11284    11284           
  Misses       1492     1492

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 52ba161...18ed3a3. Read the comment docs.

SuyashSalampuria · 2020-07-28T18:21:46Z

@sarah-clements @camd Could you give your initial reviews if this is the expected way
I am trying to find the best way to persisit metrics too in the URL

sarah-clements · 2020-07-28T20:43:13Z

+  regressionsGroupBy: 'path',
+  knownIssuesOrderBy: 'count',
+  knownIssuesGroupBy: 'path',
+  setRegressionsOrderBy: () => {},


This should be a required prop.

sarah-clements · 2020-07-28T20:54:34Z

+    const queryString = createQueryParams(newParams);
+    updateQueryParams(queryString, history, location);
+    this.setState({ knownIssuesGroupBy });
+  };


You're on the right track here, but since all of this logic is the same you should create one generic function that accepts an argument for the state that should be updated (there is a way to do this without entering a specific state name/key). I suggest naming the generic function something like updateParamsAndState.

I was facing the problem of updating the state variable from a generic function
I mean, how do I come to know which state variable regressionsGroupBy or knownIssuesOrderBy is to be changed
Other things will be surely much better to handle in a generic function

There is a way to solve this problem, you just need to research it :)

It is better to find a DRY solution - here you are repeating the same code multiple times. So there are a few different approaches you can take that would be better.

I am unable to find a good solution to it.
The only way I find is to pass a string parameter from the classification group component to see which state variable has to be changed. Does this method work?Or a still better solution is needed

An easy solution is to pass an object parameter instead of a variable: this.setState(stateObj). Another one would be to call setState inline in the onClick and update the query parameters as a callback function.

You could pass in a string, but then you'd need a value unless you were doing a boolean toogle, so: (stateName, value) => this.setState({ [stateName]: value }). This is possible by using computed property names.

sarah-clements · 2020-07-28T20:58:02Z

@sarah-clements @camd Could you give your initial reviews if this is the expected way
I am trying to find the best way to persisit metrics too in the URL

I would look at what the minimum amount of data that is needed in order to call the push health API for the failure summary for a selected task. You'll also need a query string/param that will tell the task component to be open instead of collapsed.

SuyashSalampuria · 2020-08-03T16:02:07Z

@camd I have facing a problem in persist the exact task inside the exact group
I am currently using the task id to see which group should be expanded
But, the tasks are not unique and several group may have same task
Should I add the testName also to the URL so that we can see can make it unique.This would make the URL very long although
I have already added the 2 params

defaultExpandedTab - whether possible regressions is expanded or known issues
expandedDefaultMetric - This contains the task id to show the details panel

The things seem to be working fine except that many tests are expanded

camd · 2020-08-03T16:18:00Z

@camd I have facing a problem in persist the exact task inside the exact group
I am currently using the task id to see which group should be expanded
But, the tasks are not unique and several group may have same task
Should I add the testName also to the URL so that we can see can make it unique.This would make the URL very long although
I have already added the 2 params
1. defaultExpandedTab - whether possible regressions is expanded or known issues

2. expandedDefaultMetric - This contains the task id to show the details panel
The things seem to be working fine except that many tests are expanded

One way to do this would be to add the id of the test from the FailureLine table in this for loop:

treeherder/treeherder/push_health/tests.py

Line 96 in ed3913a

for failure_line in new_failure_lines:

That way you could just use the id instead of the test name. The unit tests should ensure these things work.

When we move to using mozci as a back-end, some of these things will likely change. So the ids we get for this may be different after that change. But that's ok. There is not a wide use-base yet. And we can break backward compatibility at that time if we need to.

SuyashSalampuria · 2020-08-03T18:31:26Z

@camd Please review this too .
I currently need 4 variables on the params to expand the correct task on the correct test
Please suggest if you can find a better solution too

camd

This seems to be working well. A bit more refinement is needed. But a great start!

camd · 2020-08-04T23:53:10Z

    if (selectedTask === task || !task) {
      this.setState({ selectedTask: null, detailsShowing: false });
    } else {
+      this.props.updateParamsAndState({ expandedDefaultMetric: task.id });


A metric is different than a task. I think this state name should be selectedTask. Or you could use selectedTaskId for the querystring param name.

metricwould be either tests, builds or linting. And we should persist that, too.

The url params are seeming a bit verbose to me. I'm wondering about something closer to this:

selectedTask = task.id
test= test.id (rather than the full test text)
testgroup = one of pr(possible regressions), ki (known issues)
metric = one of tests, linting, or builds

It also seems worthwhile to add these settings to the URL as you expand each one. You may not want to necessarily share just a selected task, but an expanded list of tests in one area.

Those names might seem a bit generic for being part of state. Perhaps selectedTest, selectedTestGroup, etc would be fine.

metricwould be either tests, builds or linting. And we should persist that, too.

I think we can have that without persisting it on the URL. If any of the params, that have been added in this PR is present, tests should be shown by default. The cost of this would be checking max 7 conditions on the React code

test= test.id (rather than the full test text)

The tests are actually grouped by path or platform. So under any tests object in the frontend(which can be expanded or collapse), there are several tests from different failure lines.

This is the screenshot of the props in the Test Component. the test object may have more than one tests from different failure lines, hence different failure.id. This was my understanding from the code. Please rectify me if I am wrong or we have to store the id which has the whole test name in the URL

This is the screenshot of the props in the Test Component. the test object may have more than one tests from different failure lines, hence different failure.id. This was my understanding from the code. Please rectify me if I am wrong or we have to store the id which has the whole test name in the URL

Ahh yes. I see what you mean. That ID won't be consistent, especially if some of the back-end data changes and a task is moved to "known issues" because retriggers come back with more passing than failing.

OK, let's go with the text, then. I'll test this locally a bit today and see if I can find any issues. But this is looking good. :)

SuyashSalampuria · 2020-08-10T18:09:20Z

@camd I just wanted to confirm if we are adding test group to the URL or the current way is fine

camd · 2020-08-11T21:19:25Z

When I use this url: http://localhost:5000/pushhealth.html?repo=try&revision=7b971f5053efbca154a3e3232a85328eb8cd376f&selectedTaskId=312757448&selectedTest=linux1804-64debug&testGroup=pr&regressionsGroupBy=platform

It opens ALL of the tasks on that platform. So I think it needs to also persist the job_type_name in the url.

camd · 2020-08-11T21:20:56Z

Also, it should save when things are only partially expanded. Not only when a task has been selected. It if we are in this state, it should just open to here:

camd · 2020-08-11T21:22:17Z

@camd I just wanted to confirm if we are adding test group to the URL or the current way is fine

It looks like you have added testGroup=pr to the url. I think this is good. Or were you asking about something else?

SuyashSalampuria · 2020-08-13T15:57:37Z

It looks like you have added testGroup=pr to the url. I think this is good. Or were you asking about something else?
Hey @camd
We have some many groupings there. I was speaking of selectedTest=linux1804-64debug , here actually
I have made some changes
It would be great if you could review it once again

camd

I think this is almost there. Just a few small nit-picky changes here and there. :)

Thanks!!!

camd · 2020-08-28T22:51:33Z

+    const newParams = {
+      ...parseQueryParams(location.search),
+    };
+    Object.assign(newParams, stateObj);


Instead of Object.assign, let's use spreading, like you did for newParams. So just add ...stateObj to the creation of newParams in the spread on line 105.

camd · 2020-08-28T22:59:04Z

+      jobs,
+      failure: { jobName, testName },
+    } = this.props;
+    if (selectedJobName === `${testName} ${jobName}`) {


I think this section could be condensed into:

this.setState({ detailsShowing: selectedJobName === `${testName} ${jobName}`, selectedTask: selectedTaskId ? jobs[jobName].filter( (job) => job.id === parseInt(selectedTaskId, 10), ).length > 0 : null, });

Would you give that a shot and see if it works ok?

also, please add a blank line before this.setState

camd

This is working great! I've been doing some testing with it, and it's a really nice addition. Thank you!!

SuyashSalampuria marked this pull request as ready for review July 28, 2020 11:32

sarah-clements reviewed Jul 28, 2020

View reviewed changes

SuyashSalampuria force-pushed the persist-url-changes branch 2 times, most recently from cadb2f3 to 7a60b86 Compare August 3, 2020 15:46

camd requested changes Aug 4, 2020

View reviewed changes

SuyashSalampuria force-pushed the persist-url-changes branch 2 times, most recently from 29ab083 to 5597a08 Compare August 10, 2020 17:47

SuyashSalampuria force-pushed the persist-url-changes branch from 5597a08 to 4324065 Compare August 13, 2020 08:06

SuyashSalampuria requested a review from camd August 21, 2020 18:26

camd requested changes Aug 28, 2020

View reviewed changes

SuyashSalampuria added 8 commits August 29, 2020 16:07

persist group and order by changes in the URL

631108e

Not reloading URL

39793ce

add proper defaults

613d92f

Modify tests for modifying the way of changing orderBy or groupBy

a97a070

add a generic function to update state and params

c2a4c87

Persist changes in the metric in the URL

8aa6565

modify tests

8e42070

make a single tests expanded

4b11675

SuyashSalampuria added 3 commits August 29, 2020 16:08

resolve conflicts and used proper names for state variables

a4acd34

persist the jobName in the URL

fa1eddb

just nits

18ed3a3

SuyashSalampuria force-pushed the persist-url-changes branch from 4324065 to 18ed3a3 Compare August 29, 2020 10:42

SuyashSalampuria requested a review from camd August 29, 2020 10:42

camd approved these changes Sep 2, 2020

View reviewed changes

camd merged commit 030c67a into mozilla:master Sep 2, 2020

Conversation

SuyashSalampuria commented Jul 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Jul 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

SuyashSalampuria commented Jul 28, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sarah-clements Jul 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SuyashSalampuria Jul 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sarah-clements Jul 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sarah-clements commented Jul 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SuyashSalampuria commented Aug 3, 2020

Uh oh!

camd commented Aug 3, 2020

Uh oh!

SuyashSalampuria commented Aug 3, 2020

Uh oh!

camd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SuyashSalampuria Aug 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SuyashSalampuria commented Aug 10, 2020

Uh oh!

camd commented Aug 11, 2020

Uh oh!

camd commented Aug 11, 2020

Uh oh!

camd commented Aug 11, 2020

Uh oh!

SuyashSalampuria commented Aug 13, 2020

Uh oh!

camd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

camd left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

SuyashSalampuria commented Jul 26, 2020 •

edited

Loading

codecov-commenter commented Jul 27, 2020 •

edited

Loading

sarah-clements Jul 28, 2020 •

edited

Loading

SuyashSalampuria Jul 29, 2020 •

edited

Loading

sarah-clements Jul 29, 2020 •

edited

Loading

sarah-clements commented Jul 28, 2020 •

edited

Loading

SuyashSalampuria Aug 5, 2020 •

edited

Loading