-
-
extension/reviewbotext/resources.py (Diff revision 1) This comment should be changed. Will do in next checkin.
WIP: Adding UI for manual trigger for Review Bot
Review Request #3575 — Created Nov. 27, 2012 and submitted
Hook in Review Bot extension into the Review Board UI and adding a lightbox for manual trigger of Review Bot tools. Also includes new API resource for pulling down installed tools.
Tested the new API resource in Firefox to ensure the expected data is being pulled down. Manually tested that the tools lightbox loads with the list of all enabled tools that are also enabled to be run manually. Ensured that the UI experience works as expected : correct buttons displayed for correct options, run button disabled if no tools are checked, appropriate text in light boxes per situation.
| Description | From | Last Updated |
|---|---|---|
|
I'm not really a fan of this here, can we make it not happen. |
SM smacleod | |
|
We might want to include version information here. It's possible to have multiple versions of the same tool installed on … |
SM smacleod | |
|
This comment should be changed. Will do in next checkin. |
AL Allyshia | |
|
Nit: indent 4 spaces per level. |
 mike_conley
|
|
|
Nit - title is indented too far. Should be: var modal = { title: "Review Bot" }; |
mike_conley
|
|
|
Perhaps it'd be better to inject the dialog content inside a display:none; node within the page body? Then just manipulate … |
mike_conley
|
|
|
.html($("").attr("id", "dialogContent") .attr("class", "modalbox-contents")) |
mike_conley
|
|
|
Isn't it too soon to trigger ready if the tools aren't loaded yet? |
mike_conley
|
|
|
$("#dialogContent").html("Installed tools:") .append(toolList); |
mike_conley
|
|
|
The indentation is a bit wonky here - lines 52 to 58 look like they're indented too far. |
mike_conley
|
|
|
Same wonky indentation as above. |
mike_conley
|
|
|
I feel like we might want to namespace this a bit more. It's feasible something else could make elements with … |
SM smacleod | |
|
Something more accurate might be: 'No tools available to run manually.' |
SM smacleod | |
|
Put an empty line between these. |
SM smacleod | |
|
We should change this to something like 'reviewbot_tool_id' |
SM smacleod | |
|
This should be moved into the initialization of the extension. Also, when the extension is "shutdown" we should call 'unregister_resource_for_model(ReviewBotTool)' |
SM smacleod | |
|
Alphabetical here |
SL slchen | |
|
Alphabetical here |
SL slchen | |
|
Why the newline at the end of this? Why not: register_resource_for_model(ReviewBotTool, review_bot_installed_tool_resource) ? Same below with the TemplateHook. |
mike_conley
|
|
|
Indentation here should be: register_resource_for_model(ReviewBotTool, review_bot_installed_tool_resource) |
SL slchen | |
|
Same comment as above |
SL slchen | |
|
I'm not sure: should this also unregister 'review_bot_installed_tool_resource'? |
SL slchen | |
|
Nit: alignment |
mike_conley
|
|
|
I think RB js styles strives to use just 1 `var`, and separates the variables by comma. |
SL slchen | |
|
Space after if, before { |
mike_conley
|
|
|
What's the first ( for? |
mike_conley
|
|
|
Nit - space on either side of + |
mike_conley
|
|
|
Col: 18 E203 whitespace before ' |
 reviewbot
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
Col: 1 E302 expected 2 blank lines, found 1 |
reviewbot
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
Col: 70 W292 no newline at end of file |
reviewbot
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
We probably don't want to pass here. Will we assume that not passing in any selected_tools argument means that we … |
AL Allyshia | |
|
Col: 51 E226 missing optional whitespace around operator |
reviewbot
|
AL
-
-
-
extension/reviewbotext/htdocs/js/reviewbot.js (Diff revision 2) Nit - title is indented too far. Should be: var modal = { title: "Review Bot" }; -
extension/reviewbotext/htdocs/js/reviewbot.js (Diff revision 2) Perhaps it'd be better to inject the dialog content inside a display:none; node within the page body? Then just manipulate the tool list on load?
-
extension/reviewbotext/htdocs/js/reviewbot.js (Diff revision 2) .html($("<div/>").attr("id", "dialogContent") .attr("class", "modalbox-contents")) -
extension/reviewbotext/htdocs/js/reviewbot.js (Diff revision 2) Isn't it too soon to trigger ready if the tools aren't loaded yet?
-
extension/reviewbotext/htdocs/js/reviewbot.js (Diff revision 2) $("#dialogContent").html("Installed tools:") .append(toolList); -
extension/reviewbotext/htdocs/js/reviewbot.js (Diff revision 2) The indentation is a bit wonky here - lines 52 to 58 look like they're indented too far.
-
AL
Change Summary:
- addressed styling issues. - added logic to disable run button if no tools are checked. - moved creation of dialog div so that it is created only once.
Summary: |
|
||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
||||||||||||
Testing Done: |
|
||||||||||||
Diff: |
Revision 3 (+140 -1) |
||||||||||||
Added Files: |
SM
-
-
Maybe we could style this in a way which will allow expansion. Eventually I'd like us to add information about which tools have already been executed, or have been trigger to execute. Maybe a table/grid would be a better style?
-
-
We might want to include version information here. It's possible to have multiple versions of the same tool installed on separate workers. If that happens, we couldn't differentiate here.
-
extension/reviewbotext/htdocs/js/reviewbot.js (Diff revision 3) I feel like we might want to namespace this a bit more. It's feasible something else could make elements with id='checkbox_<#>'.
-
extension/reviewbotext/htdocs/js/reviewbot.js (Diff revision 3) Something more accurate might be: 'No tools available to run manually.'
-
extension/reviewbotext/resources.py (Diff revision 3) I know this was just a rudimentary resource to get things working, so you can make the front-end, but I want to put some notes for the future here. Feel free to ignore these for now, and focus on the front-end work: - This should be combined with the resource above (ReviewBotToolResource). - Right now this pulls all tools, we should add arguments to the resource which allow you to pull only tools you want (ones which can run manually). - Eventually we're going to be restricting which tools users can execute manually based on permissions. It might be best to only display tools which the user has permissions for.
-
-
extension/reviewbotext/resources.py (Diff revision 3) We should change this to something like 'reviewbot_tool_id'
-
extension/reviewbotext/resources.py (Diff revision 3) This should be moved into the initialization of the extension. Also, when the extension is "shutdown" we should call 'unregister_resource_for_model(ReviewBotTool)'
AL
Change Summary:
- Addressing comments from last review : style, registering/unregistering API resources, dialog logic and display. - Tested manually that manual trigger popped dialog and that it updated without page refresh if certain tools were enabled or disabled while the review request page was still open.
Diff: |
Revision 4 (+211 -1) |
|---|
-
-
extension/reviewbotext/extension.py (Diff revision 4) Why the newline at the end of this? Why not: register_resource_for_model(ReviewBotTool, review_bot_installed_tool_resource) ? Same below with the TemplateHook. -
-
-
-
SL
-
-
-
-
extension/reviewbotext/extension.py (Diff revision 4) Indentation here should be: register_resource_for_model(ReviewBotTool, review_bot_installed_tool_resource) -
-
extension/reviewbotext/extension.py (Diff revision 4) I'm not sure: should this also unregister 'review_bot_installed_tool_resource'?
-
extension/reviewbotext/htdocs/js/reviewbot.js (Diff revision 4) I think RB js styles strives to use just 1 `var`, and separates the variables by comma.
-
extension/reviewbotext/templates/reviewbot_hook_action.html (Diff revision 4) To mentors: Is there a difference between using: {% get_media_prefix %} vs. {{MEDIA_URL}} ?
AL
Change Summary:
- Addressed style issues - Added manual trigger menu item to diff view Still TODO : merge API resource for installed tools with resource ReviewBotToolResource
Summary: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 5 (+209 -1) |
||||
Added Files: |
-
This is a review from Review Bot. Tool: PEP8 Style Checker Processed Files: extension/reviewbotext/resources.py extension/reviewbotext/extension.py Ignored Files: extension/reviewbotext/htdocs/css/reviewbot.css extension/reviewbotext/templates/reviewbot_hook_action.html extension/reviewbotext/htdocs/js/reviewbot.js -
-
extension/reviewbotext/extension.py (Diff revision 5) Col: 80 E501 line too long (80 > 79 characters)
-
-
-
-
AL
Change Summary:
Merging ReviewBotInstalledToolResource into existing resource ReviewBotToolResource, leaving only one resource for review bot tools.
Summary: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 6 (+201 -1) |
-
This is a review from Review Bot. Tool: PEP8 Style Checker Processed Files: extension/reviewbotext/resources.py extension/reviewbotext/extension.py Ignored Files: extension/reviewbotext/htdocs/css/reviewbot.css extension/reviewbotext/templates/reviewbot_hook_action.html extension/reviewbotext/htdocs/js/reviewbot.js -
extension/reviewbotext/extension.py (Diff revision 6) Col: 80 E501 line too long (80 > 79 characters)
-
AL
Change Summary:
removing trailing whitespace and shortening lines over 79 chars.
Diff: |
Revision 7 (+202 -1) |
|---|
-
This is a review from Review Bot. Tool: PEP8 Style Checker Processed Files: extension/reviewbotext/resources.py extension/reviewbotext/extension.py Ignored Files: extension/reviewbotext/htdocs/css/reviewbot.css extension/reviewbotext/templates/reviewbot_hook_action.html extension/reviewbotext/htdocs/js/reviewbot.js
AL
Change Summary:
- adding API resource for the manual trigger.
- modifying 'notify' method of the ReviewBotExtension class to accommodate a list of selected tools.
- integrating manual trigger UI with back-end trigger functionality.
- still TODO:
* prevent manual trigger functionality from being "triggerable"f no diff has been published
* decide whether manual trigger functionality should be available on latest published diff if there is a draft still to be published
* implement error message visible to user if tool cannot be run
Summary: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 8 (+329 -6) |
-
This is a review from Review Bot. Tool: PEP8 Style Checker Processed Files: extension/reviewbotext/resources.py extension/reviewbotext/extension.py Ignored Files: extension/reviewbotext/htdocs/css/reviewbot.css extension/reviewbotext/templates/reviewbot_hook_action.html extension/reviewbotext/htdocs/js/reviewbot.js -
extension/reviewbotext/resources.py (Diff revision 8) Col: 51 E226 missing optional whitespace around operator
AL
-
-
extension/reviewbotext/extension.py (Diff revision 8) We probably don't want to pass here. Will we assume that not passing in any selected_tools argument means that we are in the automatic trigger case? Probably should also push error msg to the user if there is a problem running the selected tool(s).