Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2619883003:
More structure for speed releasing. (Closed)
DescriptionMore structure for speed releasing.
This CL brings creates the speed-releasing-table element for basic TableConfig retrieval. It also ensures the viewer has permissions to see the data. Corresponding tests have been added as well.
Demo:
https://dev-jessimb-7264ff2f-dot-chromeperf.appspot.com/speed_releasing (when you log in, you see a longer list)
https://dev-jessimb-7264ff2f-dot-chromeperf.appspot.com/speed_releasing/InternalOnlyIsTrue (must be logged in)
BUG=catapult:#3141
Review-Url: https://codereview.chromium.org/2619883003
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/643b80d5f3e6db3f6f184473b2355cb02fd1de66
Patch Set 1 #
Total comments: 5
Patch Set 2 : Speed releasing pulls from tableLinks entities only #Patch Set 3 : response to comments #
Total comments: 2
Patch Set 4 : updated regex #
Total comments: 12
Patch Set 5 : repsonse to comments #
Total comments: 7
Patch Set 6 : wip #Patch Set 7 : Cleaning up code, added table element. Still wip #Patch Set 8 : added testing #Patch Set 9 : Removing debug statements #
Total comments: 14
Patch Set 10 : wip #Patch Set 11 : wip #Patch Set 12 : finished separating page logic #
Total comments: 10
Patch Set 13 : response to comments #Depends on Patchset: Messages
Total messages: 34 (17 generated)
Description was changed from ========== Creates the create_health_report endpoint under admin. Upon filling out this form and POSTing, the values go to the datastore. These values can be retrieved by going to /speed_releasing/name (where name is the name you put in the form). This CL does not do any error handling/input checking. ========== to ========== Allow for creating and retrieving table_config values. Creates the create_health_report endpoint under admin. Upon filling out this form and POSTing, the values go to the datastore. These values can be retrieved by going to /speed_releasing/name (where name is the name you put in the form). This CL does not do any error handling/input checking. ==========
Description was changed from ========== Allow for creating and retrieving table_config values. Creates the create_health_report endpoint under admin. Upon filling out this form and POSTing, the values go to the datastore. These values can be retrieved by going to /speed_releasing/name (where name is the name you put in the form). This CL does not do any error handling/input checking. ========== to ========== Creates the create_health_report endpoint under admin. Upon filling out this form and POSTing, the values go to the datastore. These values can be retrieved by going to /speed_releasing/name (where name is the name you put in the form). This CL does not do any error handling/input checking. ==========
Description was changed from ========== Creates the create_health_report endpoint under admin. Upon filling out this form and POSTing, the values go to the datastore. These values can be retrieved by going to /speed_releasing/name (where name is the name you put in the form). This CL does not do any error handling/input checking. ========== to ========== Creates the /create_health_report endpoint under admin. Upon filling out this form and POSTing, the values go to the datastore. These values can be retrieved by going to /speed_releasing/name (where name is the name you put in the form). This CL does not do any error handling/input checking. ==========
Description was changed from ========== Creates the /create_health_report endpoint under admin. Upon filling out this form and POSTing, the values go to the datastore. These values can be retrieved by going to /speed_releasing/name (where name is the name you put in the form). This CL does not do any error handling/input checking. ========== to ========== Creates the /create_health_report endpoint under admin. Upon filling out this form and POSTing, the values go to the datastore. These values can be retrieved by going to /speed_releasing/name (where name is the name you put in the form). This CL does not do any input checking (the data format is likely to change and it would be a wasted effort). ==========
Description was changed from ========== Creates the /create_health_report endpoint under admin. Upon filling out this form and POSTing, the values go to the datastore. These values can be retrieved by going to /speed_releasing/name (where name is the name you put in the form). This CL does not do any input checking (the data format is likely to change and it would be a wasted effort). ========== to ========== Creates the /create_health_report endpoint under admin. Upon filling out this form and POSTing, the values go to the datastore. These values can be retrieved by going to /speed_releasing/name (where name is the name you put in the form). This CL does not do any input checking (the data format is likely to change and it would be a wasted effort). Demo: https://dev-jessimb-acc12b6e-dot-chromeperf.appspot.com/create_health_report https://dev-jessimb-acc12b6e-dot-chromeperf.appspot.com/speed_releasing/jessi... (the second link is of a table_config I created with the first link) ==========
Description was changed from ========== Creates the /create_health_report endpoint under admin. Upon filling out this form and POSTing, the values go to the datastore. These values can be retrieved by going to /speed_releasing/name (where name is the name you put in the form). This CL does not do any input checking (the data format is likely to change and it would be a wasted effort). Demo: https://dev-jessimb-acc12b6e-dot-chromeperf.appspot.com/create_health_report https://dev-jessimb-acc12b6e-dot-chromeperf.appspot.com/speed_releasing/jessi... (the second link is of a table_config I created with the first link) ========== to ========== Creates the /create_health_report endpoint under admin. Upon filling out this form and POSTing, the values go to the datastore. These values can be retrieved by going to /speed_releasing/name (where name is the name you put in the form). This CL does not do any input checking (the data format is likely to change making it a wasted effort). Demo: https://dev-jessimb-acc12b6e-dot-chromeperf.appspot.com/create_health_report https://dev-jessimb-acc12b6e-dot-chromeperf.appspot.com/speed_releasing/jessi... (the second link is of a table_config I created with the first link) ==========
jessimb@chromium.org changed reviewers: + eakuefner@chromium.org
The next step in speed_releasing has arrived. PTAL.
eakuefner@chromium.org changed reviewers: + sullivan@chromium.org
+Annie to answer specific questions I'm posing in the comments. Jessi, can you split this into two CLs, one that handles the report creation functionality, and another that shows you the data from the datastore, to be landed in that order? https://codereview.chromium.org/2619883003/diff/1/dashboard/dashboard/create_... File dashboard/dashboard/create_health_report.py (right): https://codereview.chromium.org/2619883003/diff/1/dashboard/dashboard/create_... dashboard/dashboard/create_health_report.py:31: self.RenderHtml('create_health_report.html', Annie, is there a better way to do this, preferably using Polymer? I don't know if it necessarily needs to block this CL but this is a pattern elsewhere in the code too. It would be nice to be able to do this kind of data binding without serverside templating. https://codereview.chromium.org/2619883003/diff/1/dashboard/dashboard/dispatc... File dashboard/dashboard/dispatcher.py (right): https://codereview.chromium.org/2619883003/diff/1/dashboard/dashboard/dispatc... dashboard/dashboard/dispatcher.py:106: (r'/speed_releasing/([a-zA-Z0-9-]*)', probably we don't need such a restrictive regex. https://codereview.chromium.org/2619883003/diff/1/dashboard/dashboard/element... File dashboard/dashboard/elements/speed-releasing-page.html (right): https://codereview.chromium.org/2619883003/diff/1/dashboard/dashboard/element... dashboard/dashboard/elements/speed-releasing-page.html:91: var path = window.location.pathname.split('/')[2]; I wonder if it would be preferable to do this without POSTing. It would require a second endpoint, since GET /speed_releasing already is used to give you the static speed releasing page. https://codereview.chromium.org/2619883003/diff/1/dashboard/dashboard/speed_r... File dashboard/dashboard/speed_releasing.py (right): https://codereview.chromium.org/2619883003/diff/1/dashboard/dashboard/speed_r... dashboard/dashboard/speed_releasing.py:16: def get(self, table_name): # pylint: disable=unused-argument nit: two spaces between : and # actually, i'm pretty sure you don't need the disable if you just name table_name _ instead.
Description was changed from ========== Creates the /create_health_report endpoint under admin. Upon filling out this form and POSTing, the values go to the datastore. These values can be retrieved by going to /speed_releasing/name (where name is the name you put in the form). This CL does not do any input checking (the data format is likely to change making it a wasted effort). Demo: https://dev-jessimb-acc12b6e-dot-chromeperf.appspot.com/create_health_report https://dev-jessimb-acc12b6e-dot-chromeperf.appspot.com/speed_releasing/jessi... (the second link is of a table_config I created with the first link) ========== to ========== The tableLinks values can be retrieved by going to /speed_releasing/name (where name is the name you put in the form). ==========
PTAL this is the smaller CL that views a tableLinks entity (see: https://codereview.chromium.org/2622303003) https://codereview.chromium.org/2619883003/diff/1/dashboard/dashboard/dispatc... File dashboard/dashboard/dispatcher.py (right): https://codereview.chromium.org/2619883003/diff/1/dashboard/dashboard/dispatc... dashboard/dashboard/dispatcher.py:106: (r'/speed_releasing/([a-zA-Z0-9-]*)', On 2017/01/11 at 19:08:11, eakuefner wrote: > probably we don't need such a restrictive regex. Agreed
lgtm to land after the other CL. also, you should update your title/description. https://codereview.chromium.org/2619883003/diff/40001/dashboard/dashboard/dis... File dashboard/dashboard/dispatcher.py (right): https://codereview.chromium.org/2619883003/diff/40001/dashboard/dashboard/dis... dashboard/dashboard/dispatcher.py:104: (r'/speed_releasing/(*)', .*, right?
Also, how about BUG=?
https://codereview.chromium.org/2619883003/diff/40001/dashboard/dashboard/spe... File dashboard/dashboard/speed_releasing.py (right): https://codereview.chromium.org/2619883003/diff/40001/dashboard/dashboard/spe... dashboard/dashboard/speed_releasing.py:21: def post(self, table_name): Also, as I mused about on the other CL, I'm not sure if using POST for this is the right thing. It allows us to use the same endpoint, true, but isn't strictly necessary. Don't have a strong opinion either way, just something I'm wondering about.
Description was changed from ========== The tableLinks values can be retrieved by going to /speed_releasing/name (where name is the name you put in the form). ========== to ========== Provides a way to view the tableConfig entity. A tableConfig entity stores the configuration of the table (see https://codereview.chromium.org/2622303003). These values can be retrieved by going to /speed_releasing/name (where name is the name you put in the form). BUG=catapult:#3141 ==========
https://codereview.chromium.org/2619883003/diff/60001/dashboard/dashboard/ele... File dashboard/dashboard/elements/speed-releasing-page.html (right): https://codereview.chromium.org/2619883003/diff/60001/dashboard/dashboard/ele... dashboard/dashboard/elements/speed-releasing-page.html:53: <p>{{tableBots}}<br>{{tableTests}}<br>{{tableLayout}}</p> Seems like it would be pretty quick and much more correct for the future to just write these into the JSON as arrays on the server and use dom-repeat to display as lists here?? https://codereview.chromium.org/2619883003/diff/60001/dashboard/dashboard/ele... dashboard/dashboard/elements/speed-releasing-page.html:83: }, Should be arrays https://codereview.chromium.org/2619883003/diff/60001/dashboard/dashboard/ele... dashboard/dashboard/elements/speed-releasing-page.html:85: type: String, Still don't understand this type. https://codereview.chromium.org/2619883003/diff/60001/dashboard/dashboard/spe... File dashboard/dashboard/speed_releasing.py (right): https://codereview.chromium.org/2619883003/diff/60001/dashboard/dashboard/spe... dashboard/dashboard/speed_releasing.py:37: 'table_layout': table_values[0].table_layout, Why not just write out the full arrays? https://codereview.chromium.org/2619883003/diff/60001/dashboard/dashboard/spe... dashboard/dashboard/speed_releasing.py:44: 'table_bots': 'TODO jessimb, list links for health reports.' Why is this called table_bots? why add a TODO instead of putting it in later?
PTAL https://codereview.chromium.org/2619883003/diff/60001/dashboard/dashboard/ele... File dashboard/dashboard/elements/speed-releasing-page.html (right): https://codereview.chromium.org/2619883003/diff/60001/dashboard/dashboard/ele... dashboard/dashboard/elements/speed-releasing-page.html:53: <p>{{tableBots}}<br>{{tableTests}}<br>{{tableLayout}}</p> On 2017/01/11 at 21:49:41, sullivan wrote: > Seems like it would be pretty quick and much more correct for the future to just write these into the JSON as arrays on the server and use dom-repeat to display as lists here?? This was more of a sanity check that I could retrieve these values and makes for a nice demo. They will be gone with the next level of this feature, since I'll be using tableLayout to populate the table. https://codereview.chromium.org/2619883003/diff/60001/dashboard/dashboard/ele... dashboard/dashboard/elements/speed-releasing-page.html:83: }, On 2017/01/11 at 21:49:41, sullivan wrote: > Should be arrays Done. https://codereview.chromium.org/2619883003/diff/60001/dashboard/dashboard/ele... dashboard/dashboard/elements/speed-releasing-page.html:85: type: String, On 2017/01/11 at 21:49:41, sullivan wrote: > Still don't understand this type. I added more comments to table_config.py, and the /create_health_report has a bit more info on how to write this. https://codereview.chromium.org/2619883003/diff/60001/dashboard/dashboard/spe... File dashboard/dashboard/speed_releasing.py (right): https://codereview.chromium.org/2619883003/diff/60001/dashboard/dashboard/spe... dashboard/dashboard/speed_releasing.py:37: 'table_layout': table_values[0].table_layout, On 2017/01/11 at 21:49:41, sullivan wrote: > Why not just write out the full arrays? Again this was just for demo purposes. I guess I'm not sure what you mean? table_values[0].bots returns a list of bots? https://codereview.chromium.org/2619883003/diff/60001/dashboard/dashboard/spe... dashboard/dashboard/speed_releasing.py:44: 'table_bots': 'TODO jessimb, list links for health reports.' On 2017/01/11 at 21:49:41, sullivan wrote: > Why is this called table_bots? why add a TODO instead of putting it in later? It was an easy way to port the message to the front end without creating another property. Ideally the /speed_releasing (no param) page will be links of possible reports to view. I needed something to be on the page for testing (the / and /name do different things). I will remove it.
Yesterday I tried to do quick reviews and I missed some important high-level stuff, even though this is meant to be a high-level structural CL. I'm really sorry about that. I'm really happy you broke up this into small CLs so I can review just the code structure here; I think it's easy to hand-wave over that when there's too much business logic to check. I added comments below (on the previous patch, but they still apply) about how we could get better code separation for '/' vs '/name'. My naming is just the first thing that came into my head, better names welcome :) I also think this CL is the perfect place to add a basic structure for tests, so that you can make sure your basic structure is working and set a foundation for future CLs. Specific tests I would like to see: speed_releasing_test.py: testPost_WithTableName: adds a table to the datastore, posts the table name, and verifies that the JSON is what's in the table. testPost_ListPage: adds a table to the datastore, posts with no parameters, and verifies the JSON is what's expected for the main page (we didn't somehow pull data from the table we added). speed-releasing-page.html: (mocking functions in testing_common.html, but it's okay to monkeypatch our uri class) testNoTable: mock uri.html to return the path for '/' (I think '/speed_releasing'?). mock simple_xhr to return the expected response for the main page. assert that you don't have a speed-releasing-table testTable: mock uri.html to return the path with a name (I think '/speed_releasing/my_table'?). mock simple_xhr to return the expected response for a dummy table. Assert that you have a speed-releasing-table with the expected name. Adding this foundation at the beginning will allow you to do test-driven development so you can iterate more quickly. You'll be able to pull up the devserver and tweak the UI. You'll be able to check your logic quickly for each change you make to the backend. https://codereview.chromium.org/2619883003/diff/60001/dashboard/dashboard/spe... File dashboard/dashboard/speed_releasing.py (right): https://codereview.chromium.org/2619883003/diff/60001/dashboard/dashboard/spe... dashboard/dashboard/speed_releasing.py:44: 'table_bots': 'TODO jessimb, list links for health reports.' On 2017/01/12 02:30:29, jessimb wrote: > On 2017/01/11 at 21:49:41, sullivan wrote: > > Why is this called table_bots? why add a TODO instead of putting it in later? > > It was an easy way to port the message to the front end without creating another > property. > Ideally the /speed_releasing (no param) page will be links of possible reports > to view. I needed something to be on the page for testing (the / and /name do > different things). I will remove it. Ah, I didn't see that this is meant to be the UI for both / and /name. So the reason we're breaking this up into small CLs is to make sure that we get the high-level structure right from the get go. Either CL comments or direct clear code comments would be super helpful for grokking the plan for the structure. With that in mind, I think we should take the time in this CL to lay down a clear foundation for the two pages to have separate logic. I think having both pages in the same file is fine, but I'd like to see clearer code separation through the following: 1) In this file, separate functions for filling in the data for each page: if table_name: self.ShowTable() else: self.ShowTableList() 2) In the polymer, separate polymer elements/files for the core UX of each component (more comments in that file) https://codereview.chromium.org/2619883003/diff/80001/dashboard/dashboard/ele... File dashboard/dashboard/elements/speed-releasing-page.html (right): https://codereview.chromium.org/2619883003/diff/80001/dashboard/dashboard/ele... dashboard/dashboard/elements/speed-releasing-page.html:53: <p>{{tableBots}}<br>{{tableTests}}<br>{{tableLayout}}</p> I know it seems like overkill, but since the idea for this CL is to lay down the code structure for viewing either the main page or the tables, I think we should: 1) Create two polymer elements: <speed-releasing-table> properties: table-name <speed-releasing-list> 2) Change the ready function to set a name property based on the url. 3) Move the logic for doing the XHR to /speed_releasing/ + path and filling in parameters and all the table-related parameters to the speed-releasing-table element. This will make it much simpler to do step 4, get a list of bots and show them, in the speed-releasing-list element in a future CL. https://codereview.chromium.org/2619883003/diff/80001/dashboard/dashboard/ele... dashboard/dashboard/elements/speed-releasing-page.html:91: var path = window.location.pathname.split('/')[2]; You should add a 'getPathName' function to uri.html so this can be mocked in unit tests. https://codereview.chromium.org/2619883003/diff/80001/dashboard/dashboard/spe... File dashboard/dashboard/speed_releasing.py (right): https://codereview.chromium.org/2619883003/diff/80001/dashboard/dashboard/spe... dashboard/dashboard/speed_releasing.py:29: table_config.TableConfig.name == table_name) So there can be multiple tables with the same name, and we'll only ever display the first one? Wouldn't it be better to use the table name as the key, so you could do: ndb.Key('TableConfig', table_name).get() Also you should check if it's None.
Description was changed from ========== Provides a way to view the tableConfig entity. A tableConfig entity stores the configuration of the table (see https://codereview.chromium.org/2622303003). These values can be retrieved by going to /speed_releasing/name (where name is the name you put in the form). BUG=catapult:#3141 ========== to ========== More structure for speed releasing. This CL brings creates the speed-releasing-table element for basic TableConfig retrieval. It also ensures the viewer has permissions to see the data. Corresponding tests have been added as well. Demo: https://dev-jessimb-39f59e47-dot-chromeperf.appspot.com/speed_releasing (when you log in, you see a longer list) https://dev-jessimb-39f59e47-dot-chromeperf.appspot.com/speed_releasing/Inter... (must be logged in) BUG=catapult:#3141 ==========
I fleshed out the structure of this CL and added testing. I didn't follow all of your advice exactly, so let me know if anything sticks out as being wrong! Thanks!! https://codereview.chromium.org/2619883003/diff/60001/dashboard/dashboard/spe... File dashboard/dashboard/speed_releasing.py (right): https://codereview.chromium.org/2619883003/diff/60001/dashboard/dashboard/spe... dashboard/dashboard/speed_releasing.py:44: 'table_bots': 'TODO jessimb, list links for health reports.' On 2017/01/12 at 13:48:29, sullivan wrote: > On 2017/01/12 02:30:29, jessimb wrote: > > On 2017/01/11 at 21:49:41, sullivan wrote: > > > Why is this called table_bots? why add a TODO instead of putting it in later? > > > > It was an easy way to port the message to the front end without creating another > > property. > > Ideally the /speed_releasing (no param) page will be links of possible reports > > to view. I needed something to be on the page for testing (the / and /name do > > different things). I will remove it. > > Ah, I didn't see that this is meant to be the UI for both / and /name. > > So the reason we're breaking this up into small CLs is to make sure that we get the high-level structure right from the get go. Either CL comments or direct clear code comments would be super helpful for grokking the plan for the structure. > > With that in mind, I think we should take the time in this CL to lay down a clear foundation for the two pages to have separate logic. I think having both pages in the same file is fine, but I'd like to see clearer code separation through the following: > > 1) In this file, separate functions for filling in the data for each page: > > if table_name: > self.ShowTable() > else: > self.ShowTableList() > > 2) In the polymer, separate polymer elements/files for the core UX of each component (more comments in that file) Agreed. Done. https://codereview.chromium.org/2619883003/diff/80001/dashboard/dashboard/ele... File dashboard/dashboard/elements/speed-releasing-page.html (right): https://codereview.chromium.org/2619883003/diff/80001/dashboard/dashboard/ele... dashboard/dashboard/elements/speed-releasing-page.html:53: <p>{{tableBots}}<br>{{tableTests}}<br>{{tableLayout}}</p> On 2017/01/12 at 13:48:29, sullivan wrote: > I know it seems like overkill, but since the idea for this CL is to lay down the code structure for viewing either the main page or the tables, I think we should: > > 1) Create two polymer elements: > <speed-releasing-table> > properties: > table-name > <speed-releasing-list> > > 2) Change the ready function to set a name property based on the url. > > 3) Move the logic for doing the XHR to /speed_releasing/ + path and filling in parameters and all the table-related parameters to the speed-releasing-table element. > > This will make it much simpler to do step 4, get a list of bots and show them, in the speed-releasing-list element in a future CL. Agreed. It became clear to me as I went on to implement more that this would be necessary. Instead of splitting the POSTs, the entire TableConfig is passed. Let me know if you think that is an okay way to do it or if you would prefer having two POSTs (one in the table element to retrieve the config values, one in the page element to confirm that is a valid table/show the list). At this point the table element gets the entire tableConfig (and will eventually parse it itself), but I could see passing just the bots, tests, and values and having a dom-repeat on tables. I'm not sure which is the best approach. https://codereview.chromium.org/2619883003/diff/80001/dashboard/dashboard/ele... dashboard/dashboard/elements/speed-releasing-page.html:91: var path = window.location.pathname.split('/')[2]; On 2017/01/12 at 13:48:29, sullivan wrote: > You should add a 'getPathName' function to uri.html so this can be mocked in unit tests. In the unit tests I was able to make it work by giving it the piece of the path the above code pulled out (i.e. tests.html). Is that acceptable, or should I add a way to mock the path? https://codereview.chromium.org/2619883003/diff/80001/dashboard/dashboard/spe... File dashboard/dashboard/speed_releasing.py (right): https://codereview.chromium.org/2619883003/diff/80001/dashboard/dashboard/spe... dashboard/dashboard/speed_releasing.py:29: table_config.TableConfig.name == table_name) On 2017/01/12 at 13:48:29, sullivan wrote: > So there can be multiple tables with the same name, and we'll only ever display the first one? Wouldn't it be better to use the table name as the key, so you could do: > > ndb.Key('TableConfig', table_name).get() > > Also you should check if it's None. Between the other CL and this one it's done.
Just a few small changes left... https://codereview.chromium.org/2619883003/diff/80001/dashboard/dashboard/ele... File dashboard/dashboard/elements/speed-releasing-page.html (right): https://codereview.chromium.org/2619883003/diff/80001/dashboard/dashboard/ele... dashboard/dashboard/elements/speed-releasing-page.html:53: <p>{{tableBots}}<br>{{tableTests}}<br>{{tableLayout}}</p> On 2017/01/19 00:43:20, jessimb wrote: > On 2017/01/12 at 13:48:29, sullivan wrote: > > I know it seems like overkill, but since the idea for this CL is to lay down > the code structure for viewing either the main page or the tables, I think we > should: > > > > 1) Create two polymer elements: > > <speed-releasing-table> > > properties: > > table-name > > <speed-releasing-list> > > > > 2) Change the ready function to set a name property based on the url. > > > > 3) Move the logic for doing the XHR to /speed_releasing/ + path and filling in > parameters and all the table-related parameters to the speed-releasing-table > element. > > > > This will make it much simpler to do step 4, get a list of bots and show them, > in the speed-releasing-list element in a future CL. > > Agreed. It became clear to me as I went on to implement more that this would be > necessary. Instead of splitting the POSTs, the entire TableConfig is passed. Let > me know if you think that is an okay way to do it or if you would prefer having > two POSTs (one in the table element to retrieve the config values, one in the > page element to confirm that is a valid table/show the list). > At this point the table element gets the entire tableConfig (and will eventually > parse it itself), but I could see passing just the bots, tests, and values and > having a dom-repeat on tables. I'm not sure which is the best approach. I'd still really prefer to do the following: 1) Parse the path to get the value of showTable property in ready() 2) Keep the template that shows the element based on value of showTable 3) Have each element make its own XHR What is going to happen is that each XHR response will slowly build up more and more data, and it'll be easier to figure out which property goes with which XHR/element if you move the XHR to the element itself. https://codereview.chromium.org/2619883003/diff/160001/dashboard/dashboard/el... File dashboard/dashboard/elements/speed-releasing-page-test.html (right): https://codereview.chromium.org/2619883003/diff/160001/dashboard/dashboard/el... dashboard/dashboard/elements/speed-releasing-page-test.html:32: JSON.stringify(mockResponse)); The reason I wanted to mock the URI is so that you could calculate show_list on the client and the XHRs would be created based on that. I think it's still best practice to mock the URI in case someone makes a change to the test harness and the tests.html URI changes. https://codereview.chromium.org/2619883003/diff/160001/dashboard/dashboard/el... File dashboard/dashboard/elements/speed-releasing-page.html (right): https://codereview.chromium.org/2619883003/diff/160001/dashboard/dashboard/el... dashboard/dashboard/elements/speed-releasing-page.html:62: <a href="/speed_releasing/{{item}}">{{item}}</a> This should be its own element, even though it is currently tiny. Probably want a <br> after this or a <p> or <div> around it for new lines? (It's going to get bigger fast!) https://codereview.chromium.org/2619883003/diff/160001/dashboard/dashboard/el... File dashboard/dashboard/elements/speed-releasing-table.html (right): https://codereview.chromium.org/2619883003/diff/160001/dashboard/dashboard/el... dashboard/dashboard/elements/speed-releasing-table.html:36: {{tableConfig.name}} Don't you need to have all text in a table contained within a cell? Shouldn't this be something like: <tr><th colspan="2">{{tableConfig.name}}</th></tr> https://codereview.chromium.org/2619883003/diff/160001/dashboard/dashboard/el... dashboard/dashboard/elements/speed-releasing-table.html:42: <td class="">{{tests}}</td> Why the empty class="" here and above? https://codereview.chromium.org/2619883003/diff/160001/dashboard/dashboard/el... dashboard/dashboard/elements/speed-releasing-table.html:43: </tr> I don't think this is valid HTML either, to have <tr> nested inside <tr>? Shouldn't you be using something like colspan/rowspan attributes? https://codereview.chromium.org/2619883003/diff/160001/dashboard/dashboard/sp... File dashboard/dashboard/speed_releasing.py (right): https://codereview.chromium.org/2619883003/diff/160001/dashboard/dashboard/sp... dashboard/dashboard/speed_releasing.py:31: if table_name: Please factor these branches each into a separate function (one that writes out the response for the table, one that writes out the response for the list)
Description was changed from ========== More structure for speed releasing. This CL brings creates the speed-releasing-table element for basic TableConfig retrieval. It also ensures the viewer has permissions to see the data. Corresponding tests have been added as well. Demo: https://dev-jessimb-39f59e47-dot-chromeperf.appspot.com/speed_releasing (when you log in, you see a longer list) https://dev-jessimb-39f59e47-dot-chromeperf.appspot.com/speed_releasing/Inter... (must be logged in) BUG=catapult:#3141 ========== to ========== More structure for speed releasing. This CL brings creates the speed-releasing-table element for basic TableConfig retrieval. It also ensures the viewer has permissions to see the data. Corresponding tests have been added as well. Demo: https://dev-jessimb-a617cae6-dot-chromeperf.appspot.com/speed_releasing (when you log in, you see a longer list) https://dev-jessimb-a617cae6-dot-chromeperf.appspot.com/speed_releasing/Inter... (must be logged in) BUG=catapult:#3141 ==========
PTAL https://codereview.chromium.org/2619883003/diff/160001/dashboard/dashboard/el... File dashboard/dashboard/elements/speed-releasing-page-test.html (right): https://codereview.chromium.org/2619883003/diff/160001/dashboard/dashboard/el... dashboard/dashboard/elements/speed-releasing-page-test.html:32: JSON.stringify(mockResponse)); On 2017/01/19 at 14:46:10, sullivan wrote: > The reason I wanted to mock the URI is so that you could calculate show_list on the client and the XHRs would be created based on that. > > I think it's still best practice to mock the URI in case someone makes a change to the test harness and the tests.html URI changes. Done! Makes testing much easier. https://codereview.chromium.org/2619883003/diff/160001/dashboard/dashboard/el... File dashboard/dashboard/elements/speed-releasing-page.html (right): https://codereview.chromium.org/2619883003/diff/160001/dashboard/dashboard/el... dashboard/dashboard/elements/speed-releasing-page.html:62: <a href="/speed_releasing/{{item}}">{{item}}</a> On 2017/01/19 at 14:46:10, sullivan wrote: > This should be its own element, even though it is currently tiny. > > Probably want a <br> after this or a <p> or <div> around it for new lines? (It's going to get bigger fast!) Done. https://codereview.chromium.org/2619883003/diff/160001/dashboard/dashboard/el... File dashboard/dashboard/elements/speed-releasing-table.html (right): https://codereview.chromium.org/2619883003/diff/160001/dashboard/dashboard/el... dashboard/dashboard/elements/speed-releasing-table.html:36: {{tableConfig.name}} On 2017/01/19 at 14:46:10, sullivan wrote: > Don't you need to have all text in a table contained within a cell? Shouldn't this be something like: > > <tr><th colspan="2">{{tableConfig.name}}</th></tr> This is meant to be a heading. I will add h3 tags. https://codereview.chromium.org/2619883003/diff/160001/dashboard/dashboard/el... dashboard/dashboard/elements/speed-releasing-table.html:42: <td class="">{{tests}}</td> On 2017/01/19 at 14:46:10, sullivan wrote: > Why the empty class="" here and above? Very good questions. Removed. https://codereview.chromium.org/2619883003/diff/160001/dashboard/dashboard/el... dashboard/dashboard/elements/speed-releasing-table.html:43: </tr> On 2017/01/19 at 14:46:10, sullivan wrote: > I don't think this is valid HTML either, to have <tr> nested inside <tr>? Shouldn't you be using something like colspan/rowspan attributes? It made for a cool visual - mapping bots to tests. The structure of the table itself will be changing heavily as I bring tableLayout into it. I don't wanna futz too much with it now, but I will remove the nesting. https://codereview.chromium.org/2619883003/diff/160001/dashboard/dashboard/sp... File dashboard/dashboard/speed_releasing.py (right): https://codereview.chromium.org/2619883003/diff/160001/dashboard/dashboard/sp... dashboard/dashboard/speed_releasing.py:31: if table_name: On 2017/01/19 at 14:46:10, sullivan wrote: > Please factor these branches each into a separate function (one that writes out the response for the table, one that writes out the response for the list) Done https://codereview.chromium.org/2619883003/diff/220001/dashboard/dashboard/el... File dashboard/dashboard/elements/speed-releasing-table.html (right): https://codereview.chromium.org/2619883003/diff/220001/dashboard/dashboard/el... dashboard/dashboard/elements/speed-releasing-table.html:12: <link rel="import" href="/components/paper-item/paper-item.html"> Ignore the 'before' of this file; I copied a lot from speed-releasing-page so it thinks that it is an edit of that. https://codereview.chromium.org/2619883003/diff/220001/dashboard/dashboard/el... dashboard/dashboard/elements/speed-releasing-table.html:98: var path = uri.getPathName(); If I use this.tableName, I can't make the html test work - I can't change the value before the ready function runs with document.createElement.
Really really close! Just a few more nits. https://codereview.chromium.org/2619883003/diff/160001/dashboard/dashboard/el... File dashboard/dashboard/elements/speed-releasing-table.html (right): https://codereview.chromium.org/2619883003/diff/160001/dashboard/dashboard/el... dashboard/dashboard/elements/speed-releasing-table.html:36: {{tableConfig.name}} On 2017/01/20 20:13:08, jessimb wrote: > On 2017/01/19 at 14:46:10, sullivan wrote: > > Don't you need to have all text in a table contained within a cell? Shouldn't > this be something like: > > > > <tr><th colspan="2">{{tableConfig.name}}</th></tr> > > This is meant to be a heading. I will add h3 tags. It's not valid HTML though: if you look in the inspector you'll see something like: <h3>table name</h3> <table>...</table> The HTML parser pulls the table name out of the table because it's not in a cell. So please either: * Use <thead> or <th> as I suggested above to make it a table header * Just pull it outside of the table https://codereview.chromium.org/2619883003/diff/220001/dashboard/dashboard/el... File dashboard/dashboard/elements/speed-releasing-page.html (right): https://codereview.chromium.org/2619883003/diff/220001/dashboard/dashboard/el... dashboard/dashboard/elements/speed-releasing-page.html:116: else { // POST in speed-releasing-table will get the values. Nit: usually the style is: } else { https://codereview.chromium.org/2619883003/diff/220001/dashboard/dashboard/el... File dashboard/dashboard/elements/speed-releasing-table.html (right): https://codereview.chromium.org/2619883003/diff/220001/dashboard/dashboard/el... dashboard/dashboard/elements/speed-releasing-table.html:98: var path = uri.getPathName(); On 2017/01/20 20:13:08, jessimb wrote: > If I use this.tableName, I can't make the html test work - I can't change the > value before the ready function runs with document.createElement. In the test, do you set tableName before or after calling this.addHTMLOutput(table);? I thought it would work if you set it before. But if not, perhaps the right place for this code is in attached()? https://www.polymer-project.org/1.0/docs/devguide/registering-elements#lifecy... https://codereview.chromium.org/2619883003/diff/220001/dashboard/dashboard/sp... File dashboard/dashboard/speed_releasing.py (right): https://codereview.chromium.org/2619883003/diff/220001/dashboard/dashboard/sp... dashboard/dashboard/speed_releasing.py:34: self._ShowHomePage() Nit: Should probably use consistent naming here. These functions each do the same type of thing. Are they showing a page? Or returning values? Maybe something like _OutputTableJson() and _OutputHomePageJson()? https://codereview.chromium.org/2619883003/diff/220001/dashboard/dashboard/sp... dashboard/dashboard/speed_releasing.py:56: all_entities = table_config.TableConfig.query().fetch() If this properly uses InternalOnlyModel, datastore_hooks should return only the entities the user can see. Can you test and then get rid of the internal checks below?
Description was changed from ========== More structure for speed releasing. This CL brings creates the speed-releasing-table element for basic TableConfig retrieval. It also ensures the viewer has permissions to see the data. Corresponding tests have been added as well. Demo: https://dev-jessimb-a617cae6-dot-chromeperf.appspot.com/speed_releasing (when you log in, you see a longer list) https://dev-jessimb-a617cae6-dot-chromeperf.appspot.com/speed_releasing/Inter... (must be logged in) BUG=catapult:#3141 ========== to ========== More structure for speed releasing. This CL brings creates the speed-releasing-table element for basic TableConfig retrieval. It also ensures the viewer has permissions to see the data. Corresponding tests have been added as well. Demo: https://dev-jessimb-7264ff2f-dot-chromeperf.appspot.com/speed_releasing (when you log in, you see a longer list) https://dev-jessimb-7264ff2f-dot-chromeperf.appspot.com/speed_releasing/Inter... (must be logged in) BUG=catapult:#3141 ==========
PTAL https://codereview.chromium.org/2619883003/diff/160001/dashboard/dashboard/el... File dashboard/dashboard/elements/speed-releasing-table.html (right): https://codereview.chromium.org/2619883003/diff/160001/dashboard/dashboard/el... dashboard/dashboard/elements/speed-releasing-table.html:36: {{tableConfig.name}} On 2017/01/20 at 20:52:28, sullivan wrote: > On 2017/01/20 20:13:08, jessimb wrote: > > On 2017/01/19 at 14:46:10, sullivan wrote: > > > Don't you need to have all text in a table contained within a cell? Shouldn't > > this be something like: > > > > > > <tr><th colspan="2">{{tableConfig.name}}</th></tr> > > > > This is meant to be a heading. I will add h3 tags. > > It's not valid HTML though: if you look in the inspector you'll see something like: > > <h3>table name</h3> > <table>...</table> > > The HTML parser pulls the table name out of the table because it's not in a cell. So please either: > * Use <thead> or <th> as I suggested above to make it a table header > * Just pull it outside of the table I thought I had pulled it out. Fixed. https://codereview.chromium.org/2619883003/diff/220001/dashboard/dashboard/el... File dashboard/dashboard/elements/speed-releasing-page.html (right): https://codereview.chromium.org/2619883003/diff/220001/dashboard/dashboard/el... dashboard/dashboard/elements/speed-releasing-page.html:116: else { // POST in speed-releasing-table will get the values. On 2017/01/20 at 20:52:28, sullivan wrote: > Nit: usually the style is: > > } else { Oh shoot my bad. https://codereview.chromium.org/2619883003/diff/220001/dashboard/dashboard/el... File dashboard/dashboard/elements/speed-releasing-table.html (right): https://codereview.chromium.org/2619883003/diff/220001/dashboard/dashboard/el... dashboard/dashboard/elements/speed-releasing-table.html:98: var path = uri.getPathName(); On 2017/01/20 at 20:52:28, sullivan wrote: > On 2017/01/20 20:13:08, jessimb wrote: > > If I use this.tableName, I can't make the html test work - I can't change the > > value before the ready function runs with document.createElement. > > In the test, do you set tableName before or after calling this.addHTMLOutput(table);? I thought it would work if you set it before. But if not, perhaps the right place for this code is in attached()? > > https://www.polymer-project.org/1.0/docs/devguide/registering-elements#lifecy... I tried adding it before, but I think the document.createElement itself calls the ready function. Attached works for testing, but not when I deploy. When I deploy, attached is called, I can see the response as correct, and this.tableConfig is being assigned correctly, but it never updates the UI. I'm stumped on why the binding isn't working here. I have resorted to using ready:, this.tableName (which is the preferred method), and for testing I just pass in 'undefined' which allows the xhr to work. https://codereview.chromium.org/2619883003/diff/220001/dashboard/dashboard/sp... File dashboard/dashboard/speed_releasing.py (right): https://codereview.chromium.org/2619883003/diff/220001/dashboard/dashboard/sp... dashboard/dashboard/speed_releasing.py:34: self._ShowHomePage() On 2017/01/20 at 20:52:28, sullivan wrote: > Nit: Should probably use consistent naming here. These functions each do the same type of thing. Are they showing a page? Or returning values? > > Maybe something like _OutputTableJson() and _OutputHomePageJson()? Agreed. Done. https://codereview.chromium.org/2619883003/diff/220001/dashboard/dashboard/sp... dashboard/dashboard/speed_releasing.py:56: all_entities = table_config.TableConfig.query().fetch() On 2017/01/20 at 20:52:28, sullivan wrote: > If this properly uses InternalOnlyModel, datastore_hooks should return only the entities the user can see. Can you test and then get rid of the internal checks below? Fun fact, you need to add the kind of datastore entity to track in datastore_hooks. Furthermore, you need to InstallHooks() to get them to work in a test. Now I understand why it didn't work before (hence adding it in myself).
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/2619883003/#ps240001 (title: "response to comments")
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": 240001, "attempt_start_ts": 1484951432827640,
"parent_rev": "b1f8958159a70e075b06de71bf80033749735fa3", "commit_rev":
"643b80d5f3e6db3f6f184473b2355cb02fd1de66"}
Message was sent while issue was closed.
Description was changed from ========== More structure for speed releasing. This CL brings creates the speed-releasing-table element for basic TableConfig retrieval. It also ensures the viewer has permissions to see the data. Corresponding tests have been added as well. Demo: https://dev-jessimb-7264ff2f-dot-chromeperf.appspot.com/speed_releasing (when you log in, you see a longer list) https://dev-jessimb-7264ff2f-dot-chromeperf.appspot.com/speed_releasing/Inter... (must be logged in) BUG=catapult:#3141 ========== to ========== More structure for speed releasing. This CL brings creates the speed-releasing-table element for basic TableConfig retrieval. It also ensures the viewer has permissions to see the data. Corresponding tests have been added as well. Demo: https://dev-jessimb-7264ff2f-dot-chromeperf.appspot.com/speed_releasing (when you log in, you see a longer list) https://dev-jessimb-7264ff2f-dot-chromeperf.appspot.com/speed_releasing/Inter... (must be logged in) BUG=catapult:#3141 Review-Url: https://codereview.chromium.org/2619883003 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu... |
