Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2537053003:
Changing group reports to use an id for multiple keys. (Closed)
DescriptionChanging group reports to use an id for multiple keys.
When a user selects alerts (from the alerts/ page), their keys are sent (via post) to /group_report which replies with an id (a hash of the keys, which maps to the keys in the datastore).
Then, we redirect to /group_report?id= which (in the ready: function) posts to /group_report_data with the id and receives the applicable keys (and all the other information from before).
Also handles ensuring the correct alerts are selected. Adds corresponding tests.
Note, the original ?keys=, ?bug_id=, and ?rev= work as before.
BUG=catapult:#2876
Review-Url: https://codereview.chromium.org/2537053003
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/b599da01bca9a4e2accd9e3c4ad6266f92f349ec
Patch Set 1 #Patch Set 2 : cleaned up coded and added tests #
Total comments: 22
Patch Set 3 : n #Patch Set 4 : minor text fixes #
Total comments: 4
Patch Set 5 : respose to sullivan@ comments #
Total comments: 3
Patch Set 6 : changed logic to better align with /report #Patch Set 7 : changed logic to better align with /report #
Total comments: 5
Patch Set 8 : cleaning up code #Patch Set 9 : added test #
Depends on Patchset: Messages
Total messages: 34 (16 generated)
Description was changed from ========== Changing group reports to use an id for multiple keys. BUG=catapult:#2876 ========== to ========== Changing group reports to use an id for multiple keys. When a user selects alerts (from the alerts/ page) their keys are sent (via post) to /group_report which replies with an id (a hash of the keys, which maps to the keys in the datastore). Then we redirect to /group_report?id= which (in the ready: function) posts to /group_report_data with the id which returns the applicable keys (and all the other information from before). Also handles ensuring the correct alerts are selected. Adds corresponding tests. Note, the original ?keys=, ?bug_id=, and ?rev= work as before. BUG=catapult:#2876 ==========
The CQ bit was checked by jessimb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Changing group reports to use an id for multiple keys. When a user selects alerts (from the alerts/ page) their keys are sent (via post) to /group_report which replies with an id (a hash of the keys, which maps to the keys in the datastore). Then we redirect to /group_report?id= which (in the ready: function) posts to /group_report_data with the id which returns the applicable keys (and all the other information from before). Also handles ensuring the correct alerts are selected. Adds corresponding tests. Note, the original ?keys=, ?bug_id=, and ?rev= work as before. BUG=catapult:#2876 ========== to ========== Changing group reports to use an id for multiple keys. When a user selects alerts (from the alerts/ page), their keys are sent (via post) to /group_report which replies with an id (a hash of the keys, which maps to the keys in the datastore). Then, we redirect to /group_report?id= which (in the ready: function) posts to /group_report_data with the id and receives the applicable keys (and all the other information from before). Also handles ensuring the correct alerts are selected. Adds corresponding tests. Note, the original ?keys=, ?bug_id=, and ?rev= work as before. BUG=catapult:#2876 ==========
jessimb@chromium.org changed reviewers: + eakuefner@chromium.org
PTAL
https://codereview.chromium.org/2537053003/diff/20001/dashboard/dashboard/ele... File dashboard/dashboard/elements/alerts-table.html (right): https://codereview.chromium.org/2537053003/diff/20001/dashboard/dashboard/ele... dashboard/dashboard/elements/alerts-table.html:364: params['keys'] = ''; nit: write this as params.keys https://codereview.chromium.org/2537053003/diff/20001/dashboard/dashboard/ele... dashboard/dashboard/elements/alerts-table.html:365: for (var alert of alerts) { Instead of this, can you write alerts.map(alert => alert.key).join(',')? https://codereview.chromium.org/2537053003/diff/20001/dashboard/dashboard/ele... dashboard/dashboard/elements/alerts-table.html:366: params['keys'] += (alert['key'] + ','); and here, params.keys and alert.key https://codereview.chromium.org/2537053003/diff/20001/dashboard/dashboard/ele... dashboard/dashboard/elements/alerts-table.html:644: keys.forEach(function(k) { Should be able to write this as a for..of, right? https://codereview.chromium.org/2537053003/diff/20001/dashboard/dashboard/ele... dashboard/dashboard/elements/alerts-table.html:1025: * Opens a new tab to be populated. Then sends a post request nit: POST https://codereview.chromium.org/2537053003/diff/20001/dashboard/dashboard/ele... dashboard/dashboard/elements/alerts-table.html:1033: function(response) { write this an arrow: response => { ... } and then you don't have to .bind(this) https://codereview.chromium.org/2537053003/diff/20001/dashboard/dashboard/ele... dashboard/dashboard/elements/alerts-table.html:1036: function(error) { Rationalize error handling https://codereview.chromium.org/2537053003/diff/20001/dashboard/dashboard/gro... File dashboard/dashboard/group_report.py (right): https://codereview.chromium.org/2537053003/diff/20001/dashboard/dashboard/gro... dashboard/dashboard/group_report.py:65: class GroupReportDataHandler(chart_handler.ChartHandler): Move this into its own file https://codereview.chromium.org/2537053003/diff/20001/dashboard/dashboard/gro... dashboard/dashboard/group_report.py:87: ps = ndb.Key(page_state.PageState, hash_code).get() nit: ps is not the best variable name; maybe state? https://codereview.chromium.org/2537053003/diff/20001/dashboard/dashboard/gro... dashboard/dashboard/group_report.py:155: I don't think this newline is necessary. https://codereview.chromium.org/2537053003/diff/20001/dashboard/dashboard/gro... File dashboard/dashboard/group_report_test.py (right): https://codereview.chromium.org/2537053003/diff/20001/dashboard/dashboard/gro... dashboard/dashboard/group_report_test.py:79: self.assertIn('b2321edb36fa39bcd0de86557365999356a6a971b38e8', hash_code) Why assertIn? Should it not be exactly equal? Let's make the most precise assertion we can here. https://codereview.chromium.org/2537053003/diff/20001/dashboard/dashboard/gro... dashboard/dashboard/group_report_test.py:84: self.assertIn('12345', rev) And here?
Moved group_report_data to it's own file and cleaned up imports. Addressed the other comments as well. https://codereview.chromium.org/2537053003/diff/20001/dashboard/dashboard/ele... File dashboard/dashboard/elements/alerts-table.html (right): https://codereview.chromium.org/2537053003/diff/20001/dashboard/dashboard/ele... dashboard/dashboard/elements/alerts-table.html:364: params['keys'] = ''; On 2016/12/02 at 19:04:00, eakuefner wrote: > nit: write this as params.keys done https://codereview.chromium.org/2537053003/diff/20001/dashboard/dashboard/ele... dashboard/dashboard/elements/alerts-table.html:365: for (var alert of alerts) { On 2016/12/02 at 19:04:00, eakuefner wrote: > Instead of this, can you write > > alerts.map(alert => alert.key).join(',')? done https://codereview.chromium.org/2537053003/diff/20001/dashboard/dashboard/ele... dashboard/dashboard/elements/alerts-table.html:366: params['keys'] += (alert['key'] + ','); On 2016/12/02 at 19:04:00, eakuefner wrote: > and here, params.keys and alert.key line removed https://codereview.chromium.org/2537053003/diff/20001/dashboard/dashboard/ele... dashboard/dashboard/elements/alerts-table.html:644: keys.forEach(function(k) { On 2016/12/02 at 19:04:00, eakuefner wrote: > Should be able to write this as a for..of, right? Wasn't my code, but I will leave it better than I found it :) https://codereview.chromium.org/2537053003/diff/20001/dashboard/dashboard/ele... dashboard/dashboard/elements/alerts-table.html:1025: * Opens a new tab to be populated. Then sends a post request On 2016/12/02 at 19:04:00, eakuefner wrote: > nit: POST done https://codereview.chromium.org/2537053003/diff/20001/dashboard/dashboard/ele... dashboard/dashboard/elements/alerts-table.html:1033: function(response) { On 2016/12/02 at 19:04:00, eakuefner wrote: > write this an arrow: > > response => { > ... > } and then you don't have to .bind(this) done https://codereview.chromium.org/2537053003/diff/20001/dashboard/dashboard/ele... dashboard/dashboard/elements/alerts-table.html:1036: function(error) { On 2016/12/02 at 19:04:00, eakuefner wrote: > Rationalize error handling Error handling now exists. https://codereview.chromium.org/2537053003/diff/20001/dashboard/dashboard/gro... File dashboard/dashboard/group_report.py (right): https://codereview.chromium.org/2537053003/diff/20001/dashboard/dashboard/gro... dashboard/dashboard/group_report.py:65: class GroupReportDataHandler(chart_handler.ChartHandler): On 2016/12/02 at 19:04:00, eakuefner wrote: > Move this into its own file done https://codereview.chromium.org/2537053003/diff/20001/dashboard/dashboard/gro... dashboard/dashboard/group_report.py:87: ps = ndb.Key(page_state.PageState, hash_code).get() On 2016/12/02 at 19:04:00, eakuefner wrote: > nit: ps is not the best variable name; maybe state? done https://codereview.chromium.org/2537053003/diff/20001/dashboard/dashboard/gro... dashboard/dashboard/group_report.py:155: On 2016/12/02 at 19:04:00, eakuefner wrote: > I don't think this newline is necessary. done
eakuefner@chromium.org changed reviewers: + sullivan@chromium.org
lgtm, +Annie for dashboard/ OWNERS
I'd strongly prefer that we use the same URI flow/naming that /report uses: /short_uri is used to convert from sid<->JSON data on the client /report (or in this case /group_report) still returns the dynamic data needed for the page. https://codereview.chromium.org/2537053003/diff/60001/dashboard/dashboard/ele... File dashboard/dashboard/elements/alerts-table.html (right): https://codereview.chromium.org/2537053003/diff/60001/dashboard/dashboard/ele... dashboard/dashboard/elements/alerts-table.html:370: function getGraphUri(alerts) { Shouldn't the name of this function change since it no longer returns a URI? https://codereview.chromium.org/2537053003/diff/60001/dashboard/dashboard/gro... File dashboard/dashboard/group_report.py (right): https://codereview.chromium.org/2537053003/diff/60001/dashboard/dashboard/gro... dashboard/dashboard/group_report.py:24: """Returns a hash of keys as id, or the passed in bug_id or rev.""" This should really say WHY it does that. (The request-too-long issues for the keys, why we would post the bug id and get the bug id back, etc)
On 2016/12/02 22:28:52, sullivan wrote: > I'd strongly prefer that we use the same URI flow/naming that /report uses: Note, I don't think that naming is necessarily better, but the important thing to me is that it be consistent; it's not currently.
I'm not entirely sure what you mean here. Are you referring to how I POST to group_report/ for id, then POST to group_report_data/ for the dynamic data? I can't follow report/ entirely because (IIUC) report/ creates the page state in the GET request which would cause a request too long error for group_reports. I POST from the ready: function in the group-report-page.html because that is how it was done previously, I just link it to a new endpoint and allow id to be passed in as well. I'm also not sure of a different way to do that POST.
Response to comments by sullivan@ https://codereview.chromium.org/2537053003/diff/60001/dashboard/dashboard/ele... File dashboard/dashboard/elements/alerts-table.html (right): https://codereview.chromium.org/2537053003/diff/60001/dashboard/dashboard/ele... dashboard/dashboard/elements/alerts-table.html:370: function getGraphUri(alerts) { On 2016/12/02 at 22:28:52, sullivan wrote: > Shouldn't the name of this function change since it no longer returns a URI? Done https://codereview.chromium.org/2537053003/diff/60001/dashboard/dashboard/gro... File dashboard/dashboard/group_report.py (right): https://codereview.chromium.org/2537053003/diff/60001/dashboard/dashboard/gro... dashboard/dashboard/group_report.py:24: """Returns a hash of keys as id, or the passed in bug_id or rev.""" On 2016/12/02 at 22:28:52, sullivan wrote: > This should really say WHY it does that. > > (The request-too-long issues for the keys, why we would post the bug id and get the bug id back, etc) Updated!
Sorry, it took me a long time to get the time to dig through the existing code. The /report page works like this: When the url is being generated, do a POST request to /short_uri with page_state=<json you want a hash for> and the result goes into the sid param of the url being generated. When the url is being loaded, two steps: 1) Load the general page details, with a POST request to /report 2) Load the list of charts, with a GET request to /short_uri to translate the hash back to the JSON about which charts to load. I think you're confusing the GET/POST parts of short_uri: you post the JSON to get a hash, and you only do a GET request with a hash, so it should never exceed GET request limits. I also think that the two-step XHR from /report is not necessary for your use case. So the specific changes I am asking for are: * Use 'sid' for the param name for the hash to keep consistency with the /report page. (This may not have been the best name, but there are now thousands of links in the wild including it) * Generate the sid param on the client with a POST request to /page_state * Make the format cached via hash pure JSON, not a string of comma-separated IDs * /group_report_data should be removed, and POST functionality should be moved back into /group_report.
Draft comments point out specific places to change. https://codereview.chromium.org/2537053003/diff/80001/dashboard/dashboard/ele... File dashboard/dashboard/elements/alerts-table.html (right): https://codereview.chromium.org/2537053003/diff/80001/dashboard/dashboard/ele... dashboard/dashboard/elements/alerts-table.html:1032: simple_xhr.send('/group_report', getGraphParams(this.checkedAlerts), We should use the built-in /short_uri here: simple_xhr.send('/short_uri', {'page_state': JSON.stringify(this.checkedAlerts.map(alert => alert.key))}); Note that what I've attempted to do here is actually change what gets saved in the page state for alerts 123, 456 from {keys: "123,456"} to [123, 456]. You might want to do something like {keys: [123, 456]} but since it's JSON please don't use a format that requires string parsing. https://codereview.chromium.org/2537053003/diff/80001/dashboard/dashboard/ele... dashboard/dashboard/elements/alerts-table.html:1034: newTab.location.replace('/group_report?id=' + response.hash); Make the param name 'sid' to keep consistency with /report page. https://codereview.chromium.org/2537053003/diff/80001/dashboard/dashboard/gro... File dashboard/dashboard/group_report_data.py (right): https://codereview.chromium.org/2537053003/diff/80001/dashboard/dashboard/gro... dashboard/dashboard/group_report_data.py:53: keys = state.value.split('=')[1][:-1] # Remove trailing ". I think we should store keys as a JSON array instead of doing this parsing.
The CQ bit was checked by jessimb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL I have implemented the changes you suggested, and I agree it is much cleaner now. I didn't fully realize what /short_uri did, but it does exactly what I needed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2537053003/diff/120001/dashboard/dashboard/gr... File dashboard/dashboard/group_report.py (right): https://codereview.chromium.org/2537053003/diff/120001/dashboard/dashboard/gr... dashboard/dashboard/group_report.py:68: self._ShowAlertsForKeys(keys, sid_method) Since the function is called _ShowAlertsForKeys(), it'd be better to just compute the keys in the "# sid takes precedence" block above and always pass them in the same way. https://codereview.chromium.org/2537053003/diff/120001/dashboard/dashboard/gr... File dashboard/dashboard/group_report_test.py (right): https://codereview.chromium.org/2537053003/diff/120001/dashboard/dashboard/gr... dashboard/dashboard/group_report_test.py:99: self.assertIn('No anomalies specified', error) Can you also add a test which POSTs an sid param?
PTAL https://codereview.chromium.org/2537053003/diff/120001/dashboard/dashboard/gr... File dashboard/dashboard/group_report.py (right): https://codereview.chromium.org/2537053003/diff/120001/dashboard/dashboard/gr... dashboard/dashboard/group_report.py:68: self._ShowAlertsForKeys(keys, sid_method) On 2017/01/05 at 17:38:03, sullivan wrote: > Since the function is called _ShowAlertsForKeys(), it'd be better to just compute the keys in the "# sid takes precedence" block above and always pass them in the same way. An obvious, much cleaner solution. Done https://codereview.chromium.org/2537053003/diff/120001/dashboard/dashboard/gr... File dashboard/dashboard/group_report_test.py (right): https://codereview.chromium.org/2537053003/diff/120001/dashboard/dashboard/gr... dashboard/dashboard/group_report_test.py:99: self.assertIn('No anomalies specified', error) On 2017/01/05 at 17:38:03, sullivan wrote: > Can you also add a test which POSTs an sid param? I worked on this for a bit. It would require mocking the datastore with a valid page_state, then retrieving it. IMO, testing this is more of the testing responsibility of /short_uri than of /group_report (report_test.py also does not test sid in this manner).
https://codereview.chromium.org/2537053003/diff/120001/dashboard/dashboard/gr... File dashboard/dashboard/group_report_test.py (right): https://codereview.chromium.org/2537053003/diff/120001/dashboard/dashboard/gr... dashboard/dashboard/group_report_test.py:99: self.assertIn('No anomalies specified', error) On 2017/01/05 18:05:06, jessimb wrote: > On 2017/01/05 at 17:38:03, sullivan wrote: > > Can you also add a test which POSTs an sid param? > > I worked on this for a bit. It would require mocking the datastore with a valid > page_state, then retrieving it. IMO, testing this is more of the testing > responsibility of /short_uri than of /group_report (report_test.py also does not > test sid in this manner). The datastore mocks happen at a very low level, so you should be able to just import page_state module into your test and call its API directly to set this up. I think it's worth having a unit test.
Added in the test. PTAL
lgtm
The CQ bit was checked by jessimb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eakuefner@chromium.org Link to the patchset: https://codereview.chromium.org/2537053003/#ps160001 (title: "added test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1483655697727450,
"parent_rev": "f63ee1c60a67074da0088097725aca76601fc481", "commit_rev":
"b599da01bca9a4e2accd9e3c4ad6266f92f349ec"}
Message was sent while issue was closed.
Description was changed from ========== Changing group reports to use an id for multiple keys. When a user selects alerts (from the alerts/ page), their keys are sent (via post) to /group_report which replies with an id (a hash of the keys, which maps to the keys in the datastore). Then, we redirect to /group_report?id= which (in the ready: function) posts to /group_report_data with the id and receives the applicable keys (and all the other information from before). Also handles ensuring the correct alerts are selected. Adds corresponding tests. Note, the original ?keys=, ?bug_id=, and ?rev= work as before. BUG=catapult:#2876 ========== to ========== Changing group reports to use an id for multiple keys. When a user selects alerts (from the alerts/ page), their keys are sent (via post) to /group_report which replies with an id (a hash of the keys, which maps to the keys in the datastore). Then, we redirect to /group_report?id= which (in the ready: function) posts to /group_report_data with the id and receives the applicable keys (and all the other information from before). Also handles ensuring the correct alerts are selected. Adds corresponding tests. Note, the original ?keys=, ?bug_id=, and ?rev= work as before. BUG=catapult:#2876 Review-Url: https://codereview.chromium.org/2537053003 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
