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)
     
     
    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)
     
     
    Col: 80
     E501 line too long (82 > 79 characters)
    
  3. 
      
CR
  1. 
      
  2. (from David) I don't understand what the visibility attribute is used for. Can you explain?

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

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

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

    Can you change this to be a single quoted string?

  3. This doesn't make sense grammatically.

  4. Undo this change.

  5. "a Hidden Items resource"

  6. Remove this line.

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

    Single quotes for all these strings.

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

    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. 'A' and 'M' should be named constants.

  10. 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. We use a single var statement at the top of each function.

  12. See previous comment.

  13. See previous comment re: multiline strings.

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

  15. We don't wrap HTML at 80.

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

  16. 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)
     
     

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

  3. 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)
     
     
     

    Blank line between these.

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

    Probably should be None.

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

    Can you move this value before the AJAX_SERIAL one?

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

    `var Item,
    StoredItems;

    ...

    Item = ...

    StoredItems = ...
    ```

  6. Blank line between these.

  7. 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)
     
     
     
     
     
     
     

    Same comments as above.

  9. I don't think these belong here.

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

    Same below.

  11. 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. This doesn't seem right?

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

  14. One variable per line.

    Also, initialized variables go before uninitialized variables.

    Same below.

  15. 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.

  16. 
      
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: Closed (submitted)

Change Summary:

Pushed to ucosp/cristocrat/archive-and-mute (029d534)
Loading...