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)