Toggle for "Open an Issue"
Review Request #2828 — Created Jan. 28, 2012 and submitted
Information | |
---|---|
wilsonyeung | |
Review Board | |
2282 | |
Reviewers | |
reviewboard | |
Problem: When opening a comment box, there is always a default to "Open an issue" ie that box is checked. Solution: Create an additional property that is configurable by the user to swap that default on or off. 1. Evolution to add property: open_an_issue 2. Modified all relevant files to allow it to be stored in the database. 3. Made open_an_issue reflect in the comment dialogue when changed in account settings.
The "My account" page saves the preference just fine. You can toggle it on and off, and it's reflected on the comment dialogue. Tested from off to on, on to off.
Description | From | Last Updated |
---|---|---|
What is the purpose of this evolution? I might be missing something, but it seems like this (and the DeleteField … |
AM ammok | |
Should the scope of the imports be narrowed to the specific objects that are being used? |
AM ammok | |
This line is over 80 characters long. |
AM ammok | |
Agreed. |
|
|
Just added the spacer? |
WI wilsonyeung | |
As discussed in Jim's bug, I think we want to use: issueField.checked = gOpenAnIssue; |
|
|
This doesn't look like part of your project... |
|
|
This doesn't look like part of your project... |
|
|
Not sure what I can do about it. |
WI wilsonyeung | |
This doesn't look like part of your project. |
|
|
This doesn't look like part of your project. |
|
|
This doesn't look like part of your project. |
|
|
This doesn't look like part of your project. |
|
|
This doesn't look like part of your project. |
|
|
This doesn't look like part of your project. |
|
|
Why is this in here? |
|
|
This doesn't look like part of your project. |
|
|
This evolution can be axed completely. |
|
|
We can get rid of these extra newlines. |
|
|
I need to find a way to change the initial state of the checkbox, not the final state as it … |
WI wilsonyeung | |
Tried: {% if gOpenAnIssue %} check="checked" {% endif %} |
WI wilsonyeung | |
Trailing white space and extra lines should be removed. |
ME medanat | |
Empty line should be removed. |
ME medanat | |
Maybe elaborate more in the label. "Default to opening an issue" isn't very intuitive. |
ME medanat | |
Remove space after "checked". ie: checked="checked"/> |
ME medanat | |
Hold on a second... I think I missed this my first pass through. What happens if we try editing a … |
|
|
I think we want these formatted like they were before, and your evolution, since it was last, should be at … |
|
-
-
reviewboard/accounts/evolutions/always_issue.py (Diff revision 1) What is the purpose of this evolution? I might be missing something, but it seems like this (and the DeleteField below) can be removed from the final commit.
-
reviewboard/accounts/evolutions/open_an_issue.py (Diff revision 1) Should the scope of the imports be narrowed to the specific objects that are being used?
-
-
-
reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 1) I feel like this would work better around page 1136 in the this.open callback. If you move it, I believe you can do away with the changes in comments_dlg.html.
-
reviewboard/templates/reviews/comments_dlg.html (Diff revision 1) While I do like documentation, I'm not entirely sure if this comment adds clarity. The extra 2 lines at the end seem out of place.
-
reviewboard/templates/reviews/reviewable_base.html (Diff revision 1) You can do something like "var gOpenAnIssue = {{user.get_profile.open_an_issuelower}};
-
Hey Wilson, It looks close - but I think you need to untangle your project from a few other commits, because I think there are some things in this diff that don't belong. Let me know in the meeting today if you need assistance in sorting this out, and we can go over it after the meeting. -Mike
-
reviewboard/accounts/evolutions/always_issue.py (Diff revision 1) Correct - we can just blow the always_issue.py evolution away.
-
-
reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 1) As discussed in Jim's bug, I think we want to use: issueField.checked = gOpenAnIssue;
-
reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 1) This doesn't look like part of your project...
-
reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 1) This doesn't look like part of your project...
-
-
-
-
-
-
-
-
-
reviewboard/templates/admin/widgets/w-actions.html (Diff revision 1) This doesn't look like part of your project.
Change Summary:
No real changes, just making sure my code is properly updated.
-
Just a few tips when posting review requests: * Always have a descriptive summary and description. I should be able to have a sense as to what this change is about from the dashboard, and have full clarity when reading the description. * View your diffs before publishing. If changes look tangled, get help first before posting. * If something changes with your code (say, you've untangled the commits), update the description.
Change Summary:
Fixed: - open_an_issue works on the database - cleaned up some code Roadblock: - Can't seem to use open_an_issue data-field or effectively for that matter.
Diff: |
Revision 3 (+36 -3)
|
---|
-
-
reviewboard/accounts/evolutions/always_issue.py (Diff revision 1) Just deleted the file. I was playing around with evolutions and realized I named the variable poorly.
-
-
-
-
reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 3) I need to find a way to change the initial state of the checkbox, not the final state as it closes
-
reviewboard/templates/reviews/comments_dlg.html (Diff revision 3) Tried: {% if gOpenAnIssue %} check="checked" {% endif %}
-
-
reviewboard/templates/reviews/comments_dlg.html (Diff revision 3) Trailing white space and extra lines should be removed.
-
-
reviewboard/accounts/evolutions/always_issue.py (Diff revision 3) This evolution can be axed completely.
-
-
reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 3) There's a function that opens the dialog box - you could put your code in there, to set the state of the checkbox each time the dialog opens.
Change Summary:
Modified the comment dialogue to change the check box.
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 4 (+24 -2)
|
-
Good job, just a few small things worth mentioning. -Yazan
-
-
reviewboard/accounts/forms.py (Diff revision 4) Maybe elaborate more in the label. "Default to opening an issue" isn't very intuitive.
-
reviewboard/templates/reviews/comments_dlg.html (Diff revision 4) Remove space after "checked". ie: checked="checked"/>
Change Summary:
Addressed fixes mentioned in reviews.
Diff: |
Revision 5 (+23 -2)
|
---|
-
Wilson: Hey - I think this patch has bitrotted - I can't apply and test it properly. I went through the code again for one final pass through, and I may have found a problem. Can you comment on the issue I've brought up? Thanks, -Mike
-
reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 5) Hold on a second... I think I missed this my first pass through. What happens if we try editing a pre-existing comment that has an issue opened? If our user default is to not open an issue, will the comment's issue checkbot be overridden? If so, that's incorrect. We should persist the original comments issue opened state.
Change Summary:
Fixed bug that didn't reflect changes after a comment dialogue had been saved previously.
Diff: |
Revision 6 (+27 -5)
|
---|
-
Just one minor issue, Wilson. With that fixed, I'll be ready to give my ship-it.
-
reviewboard/accounts/evolutions/__init__.py (Diff revision 6) I think we want these formatted like they were before, and your evolution, since it was last, should be at the end, like: 'is_private', 'timezone', 'open_an_issue'
Diff: |
Revision 7 (+27 -3)
|
---|