WIP: Adding UI for manual trigger for Review Bot

Review Request #3575 — Created Nov. 27, 2012 and submitted

Information

ReviewBot

Reviewers

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_conleymike_conley

Nit - title is indented too far. Should be: var modal = { title: "Review Bot" };

mike_conleymike_conley

Perhaps it'd be better to inject the dialog content inside a display:none; node within the page body? Then just manipulate …

mike_conleymike_conley

.html($("").attr("id", "dialogContent") .attr("class", "modalbox-contents"))

mike_conleymike_conley

Isn't it too soon to trigger ready if the tools aren't loaded yet?

mike_conleymike_conley

$("#dialogContent").html("Installed tools:") .append(toolList);

mike_conleymike_conley

The indentation is a bit wonky here - lines 52 to 58 look like they're indented too far.

mike_conleymike_conley

Same wonky indentation as above.

mike_conleymike_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_conleymike_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_conleymike_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_conleymike_conley

What's the first ( for?

mike_conleymike_conley

Nit - space on either side of +

mike_conleymike_conley

Col: 18 E203 whitespace before '

reviewbotreviewbot

Col: 80 E501 line too long (80 > 79 characters)

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 70 W292 no newline at end of file

reviewbotreviewbot

Col: 80 E501 line too long (80 > 79 characters)

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

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

reviewbotreviewbot
AL
  1. 
      
  2. extension/reviewbotext/resources.py (Diff revision 1)
     
     
    Show all issues
    This comment should be changed. Will do in next checkin.
  3. 
      
AL
mike_conley
  1. 
      
  2. Show all issues
    Nit: indent 4 spaces per level.
  3. extension/reviewbotext/htdocs/js/reviewbot.js (Diff revision 2)
     
     
     
     
    Show all issues
    Nit - title is indented too far. Should be:
    
    var modal = {
      title: "Review Bot"
    };
  4. Show all issues
    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?
  5. Show all issues
    .html($("<div/>").attr("id", "dialogContent")
                     .attr("class", "modalbox-contents"))
  6. Show all issues
    Isn't it too soon to trigger ready if the tools aren't loaded yet?
  7. Show all issues
    $("#dialogContent").html("Installed tools:")
                       .append(toolList);
    1. not
       
      $("#dialogContent")
          .html("Installed tools:")
          .append(toolList);
      
      ?
    2. Ah, yep - sorry, I do believe you're correct.
  8. extension/reviewbotext/htdocs/js/reviewbot.js (Diff revision 2)
     
     
     
     
     
     
     
     
     
    Show all issues
    The indentation is a bit wonky here - lines 52 to 58 look like they're indented too far.
  9. extension/reviewbotext/htdocs/js/reviewbot.js (Diff revision 2)
     
     
     
     
    Show all issues
    Same wonky indentation as above.
  10. 
      
AL
SM
  1. 
      
  2. 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?
  3. Show all issues
    I'm not really a fan of this here, can we make it not happen.
  4. Show all issues
    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.
  5. Show all issues
    I feel like we might want to namespace this a bit more. It's feasible something else could make elements with id='checkbox_<#>'.
  6. Show all issues
    Something more accurate might be: 'No tools available to run manually.'
  7. 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.
    
  8. extension/reviewbotext/resources.py (Diff revision 3)
     
     
     
    Show all issues
    Put an empty line between these.
  9. extension/reviewbotext/resources.py (Diff revision 3)
     
     
    Show all issues
    We should change this to something like 'reviewbot_tool_id'
  10. extension/reviewbotext/resources.py (Diff revision 3)
     
     
    Show all issues
    This should be moved into the initialization of the extension.
    
    Also, when the extension is "shutdown" we should call 'unregister_resource_for_model(ReviewBotTool)'
  11. 
      
AL
AL
AL
AL
mike_conley
  1. 
      
  2. extension/reviewbotext/extension.py (Diff revision 4)
     
     
    Show all issues
    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.
  3. Show all issues
    Nit: alignment
  4. Show all issues
    Space after if, before {
  5. Show all issues
    What's the first ( for?
  6. Show all issues
    Nit - space on either side of +
  7. 
      
SL
  1. 
      
  2. extension/reviewbotext/extension.py (Diff revision 4)
     
     
     
     
    Show all issues
    Alphabetical here
  3. extension/reviewbotext/extension.py (Diff revision 4)
     
     
     
     
    Show all issues
    Alphabetical here
  4. extension/reviewbotext/extension.py (Diff revision 4)
     
     
     
     
    Show all issues
    Indentation here should be:
    
    register_resource_for_model(ReviewBotTool,
                                review_bot_installed_tool_resource)
  5. extension/reviewbotext/extension.py (Diff revision 4)
     
     
     
     
     
    Show all issues
    Same comment as above
  6. extension/reviewbotext/extension.py (Diff revision 4)
     
     
    Show all issues
    I'm not sure: should this also unregister 'review_bot_installed_tool_resource'?
    1. I believe that this only needs to be called on the model itself, not on the resource.
    2. You're correct Allyshia.
  7. Show all issues
    I think RB js styles strives to use just 1 `var`, and separates the variables by comma.
  8. To mentors:
    
    Is there a difference between using:
    
    {% get_media_prefix %}
    
    vs.
    
    {{MEDIA_URL}}
    
    ?
    1. Yes - MEDIA_URL is deprecated in favour of {% get_media_prefix %}.
  9. 
      
AL
reviewbot
  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
    
    
  2. extension/reviewbotext/extension.py (Diff revision 5)
     
     
    Show all issues
    Col: 18
     E203 whitespace before '
  3. extension/reviewbotext/extension.py (Diff revision 5)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  4. extension/reviewbotext/resources.py (Diff revision 5)
     
     
    Show all issues
    Col: 1
     E302 expected 2 blank lines, found 1
    
  5. extension/reviewbotext/resources.py (Diff revision 5)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  6. extension/reviewbotext/resources.py (Diff revision 5)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  7. extension/reviewbotext/resources.py (Diff revision 5)
     
     
    Show all issues
    Col: 70
     W292 no newline at end of file
    
  8. 
      
AL
reviewbot
  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
    
    
  2. extension/reviewbotext/extension.py (Diff revision 6)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. extension/reviewbotext/resources.py (Diff revision 6)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  4. 
      
AL
reviewbot
  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
    
    
  2. 
      
AL
reviewbot
  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
    
    
  2. extension/reviewbotext/resources.py (Diff revision 8)
     
     
    Show all issues
    Col: 51
     E226 missing optional whitespace around operator
    
  3. 
      
AL
  1. 
      
  2. 
      
AL
  1. 
      
  2. extension/reviewbotext/extension.py (Diff revision 8)
     
     
    Show all issues
    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).
  3. 
      
AL
Review request changed
Status:
Completed
Change Summary:
Pushed to rework (c54b1ab3a9094360233aaf42ae5fc2410bc183ac)