Change Summary:
Thanks for the review! Instead of rendering the buttons and then hiding them, we now only render buttons on the inline comments if the issues are interactive.
Diff: |
Revision 2 (+32 -7) |
|---|
Users with no editing ability on a review request should not see issue buttons on inline comments
Review Request #2419 — Created June 23, 2011 and submitted
I noticed that after I'd created a review with some issues, when I went and viewed my comments in the diff viewer, the buttons for dropping / fixing the issues were visible to me, even though I was not the review requester. This patch fixes that bug, as well as putting the issue status message in the inline comments. Not sure how that one slipped by.
Manual testing.
| Description | From | Last Updated |
|---|---|---|
|
Indentation is wrong. |
 chipx86
|
|
|
We don't deal with scoping issues, so no need for self anymore. |
chipx86
|
|
|
You can return this. |
chipx86
|
|
|
First line of a comment should be blank. So: /* * Wraps an ... */ |
chipx86
|
|
|
Doesn't look like self is needed, since we never deal with scoping issues from callbacks. You can just use this … |
chipx86
|
|
|
Can just use this. |
chipx86
|
-
-
reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 1) should we even be rendering/creating the buttons in the case where the current user shouldn't see them? Rather than hiding after?
-
-
-
reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 2) We don't deal with scoping issues, so no need for self anymore.
-
-
reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 2) First line of a comment should be blank. So: /* * Wraps an ... */
-
reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 2) Doesn't look like self is needed, since we never deal with scoping issues from callbacks. You can just use this below.
-
-
reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 2) Is this actually replacing issue with some new element? If not, we can just do issue.issueButtons().
-
reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 2) Same question here. Though, in this case, I don't see where issue is even used anymore. So I think we can remove the assignment.
Change Summary:
Thanks for the review - updated with style changes. While I was patching this, I also noticed that the issue state for an inline comment wasn't being persisted on closing and re-opening the comment window, so I've fixed that by registering a simple callback with the issue manager.
Diff: |
Revision 3 (+38 -8) |
|---|
