WIP: Adding UI for manual trigger for Review Bot

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

Allyshia
ReviewBot
reviewbot
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)
     
     
    This comment should be changed. Will do in next checkin.
  3. 
      
AL
mike_conley
  1. 
      
  2. Nit: indent 4 spaces per level.
  3. extension/reviewbotext/htdocs/js/reviewbot.js (Diff revision 2)
     
     
     
     
    Nit - title is indented too far. Should be:
    
    var modal = {
      title: "Review Bot"
    };
  4. 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. .html($("<div/>").attr("id", "dialogContent")
                     .attr("class", "modalbox-contents"))
  6. Isn't it too soon to trigger ready if the tools aren't loaded yet?
  7. $("#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)
     
     
     
     
     
     
     
     
     
    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)
     
     
     
     
    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. I'm not really a fan of this here, can we make it not happen.
  4. 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. I feel like we might want to namespace this a bit more. It's feasible something else could make elements with id='checkbox_<#>'.
  6. 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)
     
     
     
    Put an empty line between these.
  9. extension/reviewbotext/resources.py (Diff revision 3)
     
     
    We should change this to something like 'reviewbot_tool_id'
  10. 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)'
  11. 
      
AL
AL
AL
AL
mike_conley
  1. 
      
  2. 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.
  3. Nit: alignment
  4. Space after if, before {
  5. What's the first ( for?
  6. Nit - space on either side of +
  7. 
      
SL
  1. 
      
  2. extension/reviewbotext/extension.py (Diff revision 4)
     
     
     
     
    Alphabetical here
  3. extension/reviewbotext/extension.py (Diff revision 4)
     
     
     
     
    Alphabetical here
  4. extension/reviewbotext/extension.py (Diff revision 4)
     
     
     
     
    Indentation here should be:
    
    register_resource_for_model(ReviewBotTool,
                                review_bot_installed_tool_resource)
  5. extension/reviewbotext/extension.py (Diff revision 4)
     
     
     
     
     
    Same comment as above
  6. extension/reviewbotext/extension.py (Diff revision 4)
     
     
    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. 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)
     
     
    Col: 18
     E203 whitespace before '
  3. extension/reviewbotext/extension.py (Diff revision 5)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  4. extension/reviewbotext/resources.py (Diff revision 5)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  5. extension/reviewbotext/resources.py (Diff revision 5)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  6. extension/reviewbotext/resources.py (Diff revision 5)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  7. extension/reviewbotext/resources.py (Diff revision 5)
     
     
    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)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. extension/reviewbotext/resources.py (Diff revision 6)
     
     
    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)
     
     
    Col: 51
     E226 missing optional whitespace around operator
    
  3. 
      
AL
  1. 
      
  2. 
      
AL
  1. 
      
  2. 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).
  3. 
      
AL
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to rework (c54b1ab3a9094360233aaf42ae5fc2410bc183ac)
Loading...