Bug 1465987 - Convert the BugFiler to ReactJS #3878
Conversation
| @@ -144,7 +144,7 @@ export default class ErrorLine extends React.Component { | |||
| */ | |||
| onOptionChange(option) { | |||
| this.initOption(option); | |||
| this.setState({ selectedOption: option }); | |||
There was a problem hiding this comment.
This makes it so that the new bug # that comes back from the BugFiler gets added to the manual bug input field in autoclassify.
This was actually an older bug I'd missed. When setting state, you need to set it to a new Object not just the same object that had a field modified, as we did in this case. So doing the spread remedies that.
|
@sarah-clements : Since Ed's on PTO, you're the only one on board to review this. Sorry it's another whopper, but it's fairly contained. This functionality is only available in two places:
To be honest, since the |
ui/job-view/details/BugFiler.jsx
Outdated
| name="modalProductFinderSearch" | ||
| id="modalProductFinderSearch" | ||
| onKeyDown={this.productSearchEnter} | ||
| onChange={evt => this.setState({ productSearch: evt.target.value })} |
There was a problem hiding this comment.
I thought calling setState in render was problematic due to the state/render cycle? Is that not the case if its in-lined in jsx? (seems to work fine when testing)
ui/css/treeherder-bugfiler.css
Outdated
| line-height: 16px; | ||
| } | ||
|
|
||
| #suggestedProducts { |
There was a problem hiding this comment.
These margin attributes seem quite precise, but I wonder if you couldn't use some of the bootstrap utility spacing classes for some or if it'd be worth making them into general spacing classes for future modals and forms to use?
There was a problem hiding this comment.
Yeah, this is a really good point. I spent some time laying this out with just the bootstrap classes. Very educational. :) But I eliminated most of the CSS file in the process.
| @@ -133,3 +133,9 @@ export const getRepoUrl = function getRepoUrl(newRepoName) { | |||
| params.set('repo', newRepoName); | |||
| return `${uiJobsUrlBase}?${params.toString()}`; | |||
| }; | |||
|
|
|||
| export const bzBaseUrl = 'https://bugzilla.mozilla.org/'; | |||
There was a problem hiding this comment.
The existing bugzilla.mozilla.org instances in this file can be switched to use this at some point (perhaps after some more of the react changes land, we can have a cleanup of the helpers since we'll have a better idea of what's used?).
There was a problem hiding this comment.
Yeah, I'm certain a good audit/cleanup will be in order on the helpers after all this. We're getting close! :) I wish there was a better name than helpers. I almost prefer lib but not really sure. Still kind of meaninglessly generic... Names are hard..
| // submit the new bug. Only request the versions because some products | ||
| // take quite a long time to fetch the full object | ||
| try { | ||
| const productResp = await fetch(`${bzBaseUrl}rest/product/${product}?include_fields=versions`); |
There was a problem hiding this comment.
I think this could use bugzillaBugsApi() from url helpers
| suggestions={[errorLine.data.bug_suggestions]} | ||
| fullLog={logUrl} | ||
| parsedLog={`${location.origin}/${getLogViewerUrl(job.id, repoName)}`} | ||
| reftestUrl={isReftest(job) ? `${getReftestUrl(logUrl)}&only_show_unexpected=1` : ''} |
There was a problem hiding this comment.
I think the &only_show_unexpected=1 should be moved into getReftestUrl() (since it's duplicated at several of the getReftestUrl() callsites)
There was a problem hiding this comment.
Heh, ends up that param was already in the helper. So these were adding it twice. Just had to remove them.
There was a problem hiding this comment.
Maybe "utils" or else just put them in "shared"?
Bug 1465987
This is a straight over conversion. I had to re-write quite a bit of the
BugFilerbut tried to keep at least some of the structure of functions the same.A big chunk of this change is the tests. I had to modify them to from Angular to React, of course, but the content of what they test didn't change. Still the same input and expected outputs. The only new test is the last one. So you can largely skip reviewing them in detail, if you like. :)
Testing
This calls to the Treeherder API which then, in turn,
POSTs to the bug to bugzilla. So to test, I stubbed out the back-end to just print out the payloadJSONthat would be sent to BZ. Then I created a bug with the original code and then this new code and compared the payloads to ensure they were identical.