Summary

Added mass submit/discard to dashboard

Review Request #3799 — Created Feb. 5, 2013 and submitted Jan. 9, 2014, 11 p.m.

Information

Owner

gcoupar

Repository

Review Board

Branch

master

Bugs

579

Depends On

Reviewers

Groups

reviewboard

People

Description

Added mass submit/discard to dashboard

This commit adds a CheckboxColumn to the datagrid in the dashboard, as
well as some links in the datagrid header to submit or discard all the
selected reviews.  When clicked, the links will send API requests to
close each of the selected reviews and will then reload the page.

This code depends on the CheckboxColumn being added to djblets, which
I've submitted in another review: http://reviews.reviewboard.org/r/3796/

Testing Done

- Confirmed that submit/discard selected buttons closes the reviews then reloads the page
- Confirmed that check/uncheck all works

Files

Issues

Description	From	Last Updated
Can you make this another paragraph? That way there dialog isn't as wide, and easier to scan.	chipx86	Feb. 9, 2013, 12:46 p.m.
Col: 80 E501 line too long (83 > 79 characters)	reviewbot	Jan. 27, 2013, 8:48 a.m.
Should only have one import, and then import CheckboxColumn on a second line, using \ for line continuation. CheckboxColumn would …	chipx86	Jan. 27, 2013, 1:28 p.m.
Labels are meant to be localized. This one should be. However, that means you'll have a problem with looking it …	chipx86	Jan. 28, 2013, 2:15 p.m.
What does all this cover?	chipx86	Jan. 27, 2013, 11:06 a.m.
Comments should be proper sentences and end in a trailing period. (Yes, it's nitpicky, but we strive for nitpicky consistency …	chipx86	Jan. 27, 2013, 1:29 p.m.
Really, this is "registerDashboardActions".	chipx86	Jan. 27, 2013, 1:29 p.m.
Should use one var statement, and comma-separate them (one per line). This query should be more limited in scope. It …	chipx86	Jan. 27, 2013, 1:32 p.m.
Trailing whitespace.	chipx86	Jan. 27, 2013, 1:32 p.m.
blank line between these.	chipx86	Jan. 27, 2013, 1:33 p.m.
Same comment about vars. Also, blank line after vars.No need for the 'self' one though.	chipx86	Jan. 27, 2013, 1:34 p.m.
Blank line.	chipx86	Jan. 27, 2013, 1:34 p.m.
Can you fix these vars while you're at it?	chipx86	Jan. 27, 2013, 1:34 p.m.
One space indentation. I think it'd be best if these IDs were more specific so they can't conflict. Prefix with …	chipx86	Jan. 27, 2013, 3:50 p.m.

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/reviews/datagrids.py
  Ignored Files:
    reviewboard/static/rb/css/dashboard.less
    reviewboard/static/rb/js/common.js
    reviewboard/static/rb/js/datastore.js
    reviewboard/templates/reviews/dashboard_listview.html

reviewboard/reviews/datagrids.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (83 > 79 characters)
```

Change Summary:

Fixed PEP8 warning

Diff:

Revision 2 (+82 -2)

Show changes

	reviewboard/reviews/datagrids.py
	reviewboard/static/rb/css/dashboard.less
	reviewboard/static/rb/js/common.js
	reviewboard/static/rb/js/datastore.js
	reviewboard/templates/reviews/dashboard_listview.html

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/reviews/datagrids.py
  Ignored Files:
    reviewboard/static/rb/css/dashboard.less
    reviewboard/static/rb/js/common.js
    reviewboard/static/rb/js/datastore.js
    reviewboard/templates/reviews/dashboard_listview.html

Fix it!

Can you provide screenshots of this? One showing it off, one with the menus opened.

One more addition I think is important is to prompt before making the action happen. It should ask an "Are you sure" question along the lines of our existing ones for, say, deletion (when you're an admin).

reviewboard/reviews/datagrids.py (Diff revision 2)

The issue has been resolved. Show all issues

Should only have one import, and then import CheckboxColumn on a second line, using \ for line continuation.

CheckboxColumn would align with Column. ReviewBot is going to yell at you about this, but ignore it. It's one of those things we can't cleanly turn off...

reviewboard/reviews/datagrids.py (Diff revision 2)

The issue has been resolved. Show all issues

Labels are meant to be localized. This one should be. However, that means you'll have a problem with looking it up later. I think you may need to rethink how to do that.

reviewboard/static/rb/css/dashboard.less (Diff revision 2)

An issue was opened. Show all issues

What does all this cover?

gcoupar Feb. 5, 2013, 3:17 p.m.

I think (sorry, did this bit a month or 2 ago) I mostly copied and adapted it from the actions on a review, but removed the left border because it didn't look particularly good.

The img bit definitely isn't needed, so I've removed that.

As for the rest of it: floats the actions to the right hand side of the header, removes list style, makes Think you should be able to see most of it from the screenshot.  

I'm definitely open to suggestions for improving how it looks - it functions ok at the moment but I've had trouble getting it to look as slick as the actions on the review page.

chipx86 Feb. 9, 2013, 12:46 p.m.

Ideally we'd share as much as possible with the existing action rules, instead of doing a big copy/paste.

reviewboard/static/rb/js/common.js (Diff revision 2)

The issue has been resolved. Show all issues

Comments should be proper sentences and end in a trailing period. (Yes, it's nitpicky, but we strive for nitpicky consistency :)

reviewboard/static/rb/js/common.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Really, this is "registerDashboardActions".
```

reviewboard/static/rb/js/common.js (Diff revision 2)

The issue has been resolved. Show all issues

Should use one var statement, and comma-separate them (one per line).

This query should be more limited in scope. It should only check within the dashboard's datagrid, and check input[data-...].

Name should be "checkedBoxes"

reviewboard/static/rb/js/common.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Trailing whitespace.
```
reviewboard/static/rb/js/common.js (Diff revision 2)
The issue has been resolved. Show all issues
```
blank line between these.
```

reviewboard/static/rb/js/common.js (Diff revision 2)

The issue has been resolved. Show all issues

Same comment about vars. Also, blank line after vars.No need for the 'self' one though.

reviewboard/static/rb/js/common.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Blank line.
```
reviewboard/static/rb/js/datastore.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Can you fix these vars while you're at it?
```

reviewboard/templates/reviews/dashboard_listview.html (Diff revision 2)

The issue has been resolved. Show all issues

One space indentation.

I think it'd be best if these IDs were more specific so they can't conflict. Prefix with dashboard- I guess.

Please add bug 579 to the "bugs" field here.

Can you attach some screenshots of this?

Change Summary:

- Fixed comments from review
- Added bug 579
- Added some screenshots

Bugs:

579

Diff:

Revision 3 (+102 -5)

Show changes

	reviewboard/reviews/datagrids.py
	reviewboard/static/rb/css/dashboard.less
	reviewboard/static/rb/js/common.js
	reviewboard/static/rb/js/datastore.js
	reviewboard/templates/reviews/dashboard_listview.html

Added Files:

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/reviews/datagrids.py
  Ignored Files:
    reviewboard/static/rb/css/dashboard.less
    reviewboard/static/rb/js/common.js
    reviewboard/static/rb/js/datastore.js
    reviewboard/templates/reviews/dashboard_listview.html

Fix it!

Submit prompt

An issue was opened. Show all issues

Can you make this another paragraph? That way there dialog isn't as wide, and easier to scan.

I love the feature, but I'd like to address the UI a bit.

The location and state of the command links (Submit Selected, Discard Selected) isn't optimal. It's confusing that they're in a position pretty far away from the check-boxes themselves (where the user's mouse is), and I don't think it looks very good to just have the text floating there in the header, especially when nothing is selected.

Ideally, I'd like something to appear close to the mouse when the first box is checked (maybe slide out from the selection box in the column headers) and otherwise be hidden when there's nothing selected.

Also, is it possible to put a check-box in the column selection drop-down? I think that would make it a lot more intuitive than just saying "Select".

Ship it!

Christian took this, updated it for the 2.0 code, and addressed some of my UI concerns. It's been submitted for some time now.

Status:: Completed