• 
      

    Add UI to archive and mute review requests with dashboard filters

    Review Request #7086 — Created March 19, 2015 and submitted

    Information

    Review Board
    master
    50295d6...

    Reviewers

    This adds the following UI:

    • archive and mute buttons in a Hide dropdown menu in a review request view
    • banner that indicates that review request is archived with an unarchive button
    • banner that indicates that review request is muted with an unmute button

    Review request visibility is now passed as context data to templates. If visibility is VISIBLE, the Hide dropdown menu becomes visibile. If visibility is ARCHIVED or MUTED, the Hide dropdown menu is hidden and the appropriate archive or mute banner appears. When visibility is changed, a page refresh is triggered.

    This also augments the UserSession WatchedItem (renamed StoredItem) to also include archivedReviewRequests and mutedReviewRequests. The only difference these objects are their URL and their error messages (addError and removeError methods were added in order to set different errors).

    Archived and mute banner tests were added to reviewRequestEditorViewTests.js, all tests passed.


    Description From Last Updated

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

    reviewbotreviewbot

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

    reviewbotreviewbot

    Can you change this to be a single quoted string?

    brenniebrennie

    This doesn't make sense grammatically.

    brenniebrennie

    Undo this change.

    brenniebrennie

    "a Hidden Items resource"

    brenniebrennie

    (from David) I don't understand what the visibility attribute is used for. Can you explain?

    CR cristocrat

    Remove this line.

    brenniebrennie

    Single quotes for all these strings.

    brenniebrennie

    (from David) Please use "review request" instead of "change"

    CR cristocrat

    (from David) Please use "review request" instead of "change"

    CR cristocrat

    Since we ignore the previous block for deciding the banner class, we should join these two blocks, with this new …

    brenniebrennie

    'A' and 'M' should be named constants.

    brenniebrennie

    Single quotes. This should be indented relative to the variable name. You can also do something like var reviewRequest = …

    brenniebrennie

    We use a single var statement at the top of each function.

    brenniebrennie

    See previous comment.

    brenniebrennie

    See previous comment re: multiline strings.

    brenniebrennie

    We use a single var statement at the top of each function.

    brenniebrennie

    We don't wrap HTML at 80.

    brenniebrennie

    We don't wrap HTML at 80.

    brenniebrennie

    Based on your usage, having this be an empty dictionary isn't right. Probably it should be None

    daviddavid

    I really don't like having this variable that third parties are expected to set. It's taken me several readings of …

    daviddavid

    Blank line between these.

    chipx86chipx86

    Probably should be None.

    chipx86chipx86

    Can you move this value before the AJAX_SERIAL one?

    chipx86chipx86

    All var statements should go at the top of the file, like: `var Item, StoredItems; ... Item = ... StoredItems …

    chipx86chipx86

    Blank line between these.

    chipx86chipx86

    What's true? This should probably pass on all the arguments instead. It also needs to pass the correct context and …

    chipx86chipx86

    Same comments as above.

    chipx86chipx86

    I don't think these belong here.

    chipx86chipx86

    function() { should be aligned with the other arguments. Same below.

    chipx86chipx86

    I'll be mentioning this in the related review request, but the API shouldn't return the internal codes for these. Instead, …

    chipx86chipx86

    This doesn't seem right?

    chipx86chipx86

    Same.

    chipx86chipx86

    These are only used once in the function, so rather than assigning a local variable, I'd just use them directly …

    chipx86chipx86

    One variable per line. Also, initialized variables go before uninitialized variables. Same below.

    chipx86chipx86

    The localization parser won't handle the addition of strings properly. This needs to actually be one long string (even if …

    chipx86chipx86
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          reviewboard/reviews/views.py
          reviewboard/datagrids/grids.py
          reviewboard/accounts/evolutions/__init__.py
          reviewboard/accounts/evolutions/reviewrequestvisit_visibility.py
          reviewboard/accounts/models.py
      
      Ignored Files:
          reviewboard/templates/base.html
          reviewboard/templates/reviews/review_header.html
          reviewboard/templates/datagrids/hideable_review_request_listview.html
          reviewboard/static/rb/js/views/reviewRequestEditorView.js
          reviewboard/static/rb/css/pages/reviews.less
          reviewboard/templates/reviews/reviewable_page_data.js
          reviewboard/static/rb/css/common.less
          reviewboard/templates/reviews/review_request_actions_secondary.html
          reviewboard/static/rb/js/models/userSessionModel.js
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          reviewboard/reviews/views.py
          reviewboard/datagrids/grids.py
          reviewboard/accounts/evolutions/__init__.py
          reviewboard/accounts/evolutions/reviewrequestvisit_visibility.py
          reviewboard/accounts/models.py
      
      Ignored Files:
          reviewboard/templates/base.html
          reviewboard/templates/reviews/review_header.html
          reviewboard/templates/datagrids/hideable_review_request_listview.html
          reviewboard/static/rb/js/views/reviewRequestEditorView.js
          reviewboard/static/rb/css/pages/reviews.less
          reviewboard/templates/reviews/reviewable_page_data.js
          reviewboard/static/rb/css/common.less
          reviewboard/templates/reviews/review_request_actions_secondary.html
          reviewboard/static/rb/js/models/userSessionModel.js
      
      
    2. reviewboard/datagrids/grids.py (Diff revision 1)
       
       
      Show all issues
      Col: 80
       E501 line too long (82 > 79 characters)
      
    3. 
        
    CR
    CR
    CR
    reviewbot
    1. Tool: PEP8 Style Checker
      Processed Files:
          reviewboard/reviews/views.py
          reviewboard/datagrids/grids.py
      
      Ignored Files:
          reviewboard/templates/base.html
          reviewboard/templates/reviews/review_header.html
          reviewboard/templates/datagrids/hideable_review_request_listview.html
          reviewboard/static/rb/js/views/reviewRequestEditorView.js
          reviewboard/static/rb/css/pages/reviews.less
          reviewboard/templates/reviews/reviewable_page_data.js
          reviewboard/static/rb/css/common.less
          reviewboard/templates/reviews/review_request_actions_secondary.html
          reviewboard/static/rb/js/models/userSessionModel.js
      
      
      
      Tool: Pyflakes
      Processed Files:
          reviewboard/reviews/views.py
          reviewboard/datagrids/grids.py
      
      Ignored Files:
          reviewboard/templates/base.html
          reviewboard/templates/reviews/review_header.html
          reviewboard/templates/datagrids/hideable_review_request_listview.html
          reviewboard/static/rb/js/views/reviewRequestEditorView.js
          reviewboard/static/rb/css/pages/reviews.less
          reviewboard/templates/reviews/reviewable_page_data.js
          reviewboard/static/rb/css/common.less
          reviewboard/templates/reviews/review_request_actions_secondary.html
          reviewboard/static/rb/js/models/userSessionModel.js
      
      
    2. reviewboard/datagrids/grids.py (Diff revision 2)
       
       
      Show all issues
      Col: 80
       E501 line too long (82 > 79 characters)
      
    3. 
        
    CR
    1. 
        
    2. Show all issues

      (from David) I don't understand what the visibility attribute is used for. Can you explain?

    3. Show all issues

      (from David) Please use "review request" instead of "change"

    4. Show all issues

      (from David) Please use "review request" instead of "change"

    5. 
        
    brennie
    1. 
        
    2. reviewboard/reviews/views.py (Diff revision 2)
       
       
      Show all issues

      Can you change this to be a single quoted string?

    3. Show all issues

      This doesn't make sense grammatically.

    4. Show all issues

      Undo this change.

    5. Show all issues

      "a Hidden Items resource"

    6. Show all issues

      Remove this line.

    7. reviewboard/static/rb/js/models/userSessionModel.js (Diff revision 2)
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
      Show all issues

      Single quotes for all these strings.

    8. reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 2)
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
      Show all issues

      Since we ignore the previous block for deciding the banner class, we should join these two blocks, with this new block coming first and the prior block being else-if-ified.

    9. Show all issues

      'A' and 'M' should be named constants.

    10. Show all issues

      Single quotes. This should be indented relative to the variable name.

      You can also do something like

              var reviewRequest = this.model.get('reviewRequest'),
                  confirmText = getText('Are you sure you want to archive this ' +
                                        'review request?');
      

      You can also use a list of strings a .join them together, but + should be okay for just two strings.

    11. Show all issues

      We use a single var statement at the top of each function.

    12. Show all issues

      See previous comment.

    13. Show all issues

      See previous comment re: multiline strings.

    14. Show all issues

      We use a single var statement at the top of each function.

    15. Show all issues

      We don't wrap HTML at 80.

      1. Sorry, could you explain more? Not sure I understand

    16. Show all issues

      We don't wrap HTML at 80.

    17. 
        
    CR
    reviewbot
    1. Tool: PEP8 Style Checker
      Processed Files:
          reviewboard/reviews/views.py
      
      Ignored Files:
          reviewboard/templates/reviews/reviewable_page_data.js
          reviewboard/static/rb/js/views/reviewRequestEditorView.js
          reviewboard/templates/reviews/review_header.html
          reviewboard/static/rb/css/pages/reviews.less
          reviewboard/templates/reviews/review_request_actions_secondary.html
          reviewboard/static/rb/js/models/userSessionModel.js
      
      
      
      Tool: Pyflakes
      Processed Files:
          reviewboard/reviews/views.py
      
      Ignored Files:
          reviewboard/templates/reviews/reviewable_page_data.js
          reviewboard/static/rb/js/views/reviewRequestEditorView.js
          reviewboard/templates/reviews/review_header.html
          reviewboard/static/rb/css/pages/reviews.less
          reviewboard/templates/reviews/review_request_actions_secondary.html
          reviewboard/static/rb/js/models/userSessionModel.js
      
      
    2. 
        
    CR
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          reviewboard/reviews/views.py
      
      Ignored Files:
          reviewboard/templates/base.html
          reviewboard/templates/reviews/reviewable_page_data.js
          reviewboard/static/rb/js/views/reviewRequestEditorView.js
          reviewboard/templates/reviews/review_header.html
          reviewboard/static/rb/css/pages/reviews.less
          reviewboard/templates/reviews/review_request_actions_secondary.html
          reviewboard/static/rb/js/models/userSessionModel.js
          reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          reviewboard/reviews/views.py
      
      Ignored Files:
          reviewboard/templates/base.html
          reviewboard/templates/reviews/reviewable_page_data.js
          reviewboard/static/rb/js/views/reviewRequestEditorView.js
          reviewboard/templates/reviews/review_header.html
          reviewboard/static/rb/css/pages/reviews.less
          reviewboard/templates/reviews/review_request_actions_secondary.html
          reviewboard/static/rb/js/models/userSessionModel.js
          reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
      
      
    2. 
        
    david
    1. Can you attach screenshots of the new UI?

    2. reviewboard/reviews/views.py (Diff revision 4)
       
       
      Show all issues

      Based on your usage, having this be an empty dictionary isn't right. Probably it should be None

    3. Show all issues

      I really don't like having this variable that third parties are expected to set. It's taken me several readings of the code to figure out what it meant. How about we just make a new argument (maybe called trigger) to addImmediately / removeImmediately that causes it to trigger a changed event (based on the listener, I don't see a reason to have two different events)?

      1. Could you go into more detail about this? I'm having trouble wrapping my head around triggers and listeners through several layers of objects and callbacks. I'm also trying to keep in mind what Christian said about how the add/remove functions should emit more general signals about items being added/removed, and pass the item in the callback.

      2. OK, after reading this more closely, I think we should remove the visibility variable entirely. Instead of:

        if (this.visibility) {
            this.trigger('hidden');
        }
        

        We should do:

        this.trigger('itemAdded', item);
        

        (and correspondingly have an itemRemoved event).

        This would give us generic events that can be used for anything, and then the handler for those would interpret 'added' as "I should refresh"

    4. 
        
    CR
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          reviewboard/reviews/views.py
      
      Ignored Files:
          reviewboard/templates/base.html
          reviewboard/templates/reviews/reviewable_page_data.js
          reviewboard/static/rb/js/views/reviewRequestEditorView.js
          reviewboard/templates/reviews/review_header.html
          reviewboard/static/rb/css/pages/reviews.less
          reviewboard/templates/reviews/review_request_actions_secondary.html
          reviewboard/static/rb/js/models/userSessionModel.js
          reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          reviewboard/reviews/views.py
      
      Ignored Files:
          reviewboard/templates/base.html
          reviewboard/templates/reviews/reviewable_page_data.js
          reviewboard/static/rb/js/views/reviewRequestEditorView.js
          reviewboard/templates/reviews/review_header.html
          reviewboard/static/rb/css/pages/reviews.less
          reviewboard/templates/reviews/review_request_actions_secondary.html
          reviewboard/static/rb/js/models/userSessionModel.js
          reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
      
      
    2. 
        
    CR
    chipx86
    1. 
        
    2. reviewboard/reviews/views.py (Diff revision 5)
       
       
       
      Show all issues

      Blank line between these.

    3. reviewboard/reviews/views.py (Diff revision 5)
       
       
      Show all issues

      Probably should be None.

    4. reviewboard/reviews/views.py (Diff revision 5)
       
       
      Show all issues

      Can you move this value before the AJAX_SERIAL one?

    5. Show all issues

      All var statements should go at the top of the file, like:

      `var Item,
      StoredItems;

      ...

      Item = ...

      StoredItems = ...
      ```

    6. Show all issues

      Blank line between these.

    7. Show all issues

      What's true? This should probably pass on all the arguments instead. It also needs to pass the correct context and check that options.success is set:

      if (options && _.isFunction(options.success)) {
          options.success.apply(context, arguments);
      }
      
    8. reviewboard/static/rb/js/models/userSessionModel.js (Diff revision 5)
       
       
       
       
       
       
       
      Show all issues

      Same comments as above.

    9. Show all issues

      I don't think these belong here.

    10. Show all issues

      function() { should be aligned with the other arguments.

      Same below.

    11. Show all issues

      I'll be mentioning this in the related review request, but the API shouldn't return the internal codes for these. Instead, it should return boolean fields for archived vs. muted, probably. See my other review :)

      1. Oh, I thought this was coming from the API, and just realized it's passed to the template.

        Well, I'd still use more human-readable names: 'archived' and 'muted'.

      2. Sorry, shouldn't I keep using the characters I used in the ReviewRequestVisit model ('A', 'M')?

    12. Show all issues

      This doesn't seem right?

    13. Show all issues

      Same.

    14. Show all issues

      These are only used once in the function, so rather than assigning a local variable, I'd just use them directly below.

    15. Show all issues

      One variable per line.

      Also, initialized variables go before uninitialized variables.

      Same below.

    16. Show all issues

      The localization parser won't handle the addition of strings properly. This needs to actually be one long string (even if that means going over the 79 character limit).

      Same below.

    17. 
        
    CR
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          reviewboard/reviews/views.py
      
      Ignored Files:
          reviewboard/templates/base.html
          reviewboard/templates/reviews/reviewable_page_data.js
          reviewboard/static/rb/js/views/reviewRequestEditorView.js
          reviewboard/templates/reviews/review_header.html
          reviewboard/static/rb/css/pages/reviews.less
          reviewboard/templates/reviews/review_request_actions_secondary.html
          reviewboard/static/rb/js/models/userSessionModel.js
          reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          reviewboard/reviews/views.py
      
      Ignored Files:
          reviewboard/templates/base.html
          reviewboard/templates/reviews/reviewable_page_data.js
          reviewboard/static/rb/js/views/reviewRequestEditorView.js
          reviewboard/templates/reviews/review_header.html
          reviewboard/static/rb/css/pages/reviews.less
          reviewboard/templates/reviews/review_request_actions_secondary.html
          reviewboard/static/rb/js/models/userSessionModel.js
          reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
      
      
    2. 
        
    CR
    Review request changed
    Status:
    Completed
    Change Summary:
    Pushed to ucosp/cristocrat/archive-and-mute (029d534)