Add UI to archive and mute review requests with dashboard filters
Review Request #7086 — Created March 19, 2015 and submitted
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) |
reviewbot | |
Col: 80 E501 line too long (82 > 79 characters) |
reviewbot | |
Can you change this to be a single quoted string? |
brennie | |
This doesn't make sense grammatically. |
brennie | |
Undo this change. |
brennie | |
"a Hidden Items resource" |
brennie | |
(from David) I don't understand what the visibility attribute is used for. Can you explain? |
CR cristocrat | |
Remove this line. |
brennie | |
Single quotes for all these strings. |
brennie | |
(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 … |
brennie | |
'A' and 'M' should be named constants. |
brennie | |
Single quotes. This should be indented relative to the variable name. You can also do something like var reviewRequest = … |
brennie | |
We use a single var statement at the top of each function. |
brennie | |
See previous comment. |
brennie | |
See previous comment re: multiline strings. |
brennie | |
We use a single var statement at the top of each function. |
brennie | |
We don't wrap HTML at 80. |
brennie | |
We don't wrap HTML at 80. |
brennie | |
Based on your usage, having this be an empty dictionary isn't right. Probably it should be None |
david | |
I really don't like having this variable that third parties are expected to set. It's taken me several readings of … |
david | |
Blank line between these. |
chipx86 | |
Probably should be None. |
chipx86 | |
Can you move this value before the AJAX_SERIAL one? |
chipx86 | |
All var statements should go at the top of the file, like: `var Item, StoredItems; ... Item = ... StoredItems … |
chipx86 | |
Blank line between these. |
chipx86 | |
What's true? This should probably pass on all the arguments instead. It also needs to pass the correct context and … |
chipx86 | |
Same comments as above. |
chipx86 | |
I don't think these belong here. |
chipx86 | |
function() { should be aligned with the other arguments. Same below. |
chipx86 | |
I'll be mentioning this in the related review request, but the API shouldn't return the internal codes for these. Instead, … |
chipx86 | |
This doesn't seem right? |
chipx86 | |
Same. |
chipx86 | |
These are only used once in the function, so rather than assigning a local variable, I'd just use them directly … |
chipx86 | |
One variable per line. Also, initialized variables go before uninitialized variables. Same below. |
chipx86 | |
The localization parser won't handle the addition of strings properly. This needs to actually be one long string (even if … |
chipx86 |
Summary: |
|
||||
---|---|---|---|---|---|
Depends On: |
|
-
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
-
-
-
reviewboard/static/rb/js/models/userSessionModel.js (Diff revision 2) (from David) I don't understand what the visibility attribute is used for. Can you explain?
-
reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 2) (from David) Please use "review request" instead of "change"
-
reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 2) (from David) Please use "review request" instead of "change"
-
-
-
reviewboard/static/rb/js/models/userSessionModel.js (Diff revision 2) This doesn't make sense grammatically.
-
-
-
-
reviewboard/static/rb/js/models/userSessionModel.js (Diff revision 2) Single quotes for all these strings.
-
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. -
reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 2) 'A' and 'M' should be named constants.
-
reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 2) 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. -
reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 2) We use a single
var
statement at the top of each function. -
-
reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 2) See previous comment re: multiline strings.
-
reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 2) We use a single
var
statement at the top of each function. -
-
Depends On: |
|
||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 3 (+244 -46)
|
-
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
Summary: |
|
||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 4 (+350 -46)
|
-
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
-
Can you attach screenshots of the new UI?
-
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
-
reviewboard/static/rb/js/models/userSessionModel.js (Diff revision 4) 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
) toaddImmediately
/removeImmediately
that causes it to trigger achanged
event (based on the listener, I don't see a reason to have two different events)?
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||
Depends On: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||
Groups: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 5 (+362 -46)
|
|||||||||||||||||||||||||||||||||||||||||||||||||||
Added Files: |
-
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
Description: |
|
---|
-
-
-
-
-
reviewboard/static/rb/js/models/userSessionModel.js (Diff revision 5) All
var
statements should go at the top of the file, like:`var Item,
StoredItems;...
Item = ...
StoredItems = ...
``` -
-
reviewboard/static/rb/js/models/userSessionModel.js (Diff revision 5) What's
true
? This should probably pass on all the arguments instead. It also needs to pass the correct context and check thatoptions.success
is set:if (options && _.isFunction(options.success)) { options.success.apply(context, arguments); }
-
-
reviewboard/static/rb/js/models/userSessionModel.js (Diff revision 5) I don't think these belong here.
-
reviewboard/static/rb/js/models/userSessionModel.js (Diff revision 5) function() {
should be aligned with the other arguments.Same below.
-
reviewboard/static/rb/js/models/userSessionModel.js (Diff revision 5) 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 :)
-
reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 5) This doesn't seem right?
-
-
reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 5) These are only used once in the function, so rather than assigning a local variable, I'd just use them directly below.
-
reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 5) One variable per line.
Also, initialized variables go before uninitialized variables.
Same below.
-
reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 5) 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.
Diff: |
Revision 6 (+366 -46)
|
---|
-
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