Summary

Expose Toggles for Opening Issues in Review Summary Dialog

Review Request #2827 — Created Jan. 26, 2012 and submitted Feb. 11, 2012, 6:18 a.m.

Information

Owner

jimrrchen

Repository

Review Board

Branch

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

In the review summary dialog, there is now an "open an issue" checkbox after each comment.
Also fixed the way we get checkbox states in reviews.js

Testing Done

Tested in my local linux machine with Chrome using the following steps
1. take any review request, view its diffs
2. comment on any line of the diff, open an issue
3. comment on some other line of the diff, don't open an issue
4. there should be a green bar on top, click the "edit review" button
5. in the summary, uncheck the "open an issue" in the first comment, don't modify the comment content
6. check the "open an issue" in the second comment, don't modify the comment content
7. save the review
8. refresh the page, and see that the issue states have been changed accordingly

Issues

Description	From	Last Updated
Agreed with the is(":checked"). Also, you should pull out this check so we only do it once.	chipx86	Jan. 27, 2012, 4:35 a.m.
It kind of blows that we have to do this every time we set the issue_opened property. It feels error …	mike_conley	Jan. 28, 2012, 3:12 p.m.
Didn't we want to use something like: $(issueField[0]).is(":checked") ?	mike_conley	Feb. 11, 2012, 6:04 a.m.
Same as above, re: is(":checked")	mike_conley	Feb. 11, 2012, 6:09 a.m.
Same as above, re: .is(":checked")	mike_conley	Feb. 11, 2012, 5:58 a.m.
Same as above, re: .is(":checked")	mike_conley	Feb. 11, 2012, 6:09 a.m.
Same as above, re: .is(":checked")	mike_conley	Feb. 11, 2012, 6:09 a.m.

reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 1)

I realize that this idiom was used earlier in the code, but I'm not entirely sure why the ternary expression is necessary.

Additionally, I believe it should be changed to "issueOption.is(':checked')" (as recommended by http://api.jquery.com/prop/) as the behaviour for attr() will change in jQuery 1.6+ (and it's slightly more readable).

jimrrchen Jan. 27, 2012, 4:35 a.m.

"I realize that this idiom was used earlier in the code, but I'm not entirely sure why the ternary expression is necessary."

Aha...if you did remove the ternary expression, you would face the same problem that I faced yesterday. 
So basically if we save and pass "true"/"false" string instead of 1/0 to the API, because of the way API is written (check line 210 in /djblets/djblets/webapi/decorators.py), it only accepts (1, "1", True, "True") as true values and everything else as false. Therefore, string "true"/"false" would be rejected as false and hence the ternary expression. The better way of course is to modify djblets, which is out of the scope of this check-in.

"Additionally, I believe it should be changed to "issueOption.is(':checked')" (as recommended by http://api.jquery.com/prop/) as the behaviour for attr() will change in jQuery 1.6+ (and it's slightly more readable)."
I see...I was following what was used previously in the code. I'll make the change and update all usage of it in review.js

medanat Jan. 27, 2012, 5:15 a.m.

Can you change all the instances you find of .attr('checked') to .is(':checked') as I plan to try and update jquery to a more recent version sometime soon. Thanks!

jimrrchen Jan. 27, 2012, 5:27 a.m.

It is already done in my latest check-in. The .attr('checked') mostly exists in reviews.js (I did a grep "checked" on all *.js)
I used element.checked instead of .is(':checked'), mostly because it is what's used in other *.js files

reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 1)

Show all issues

Agreed with the is(":checked"). Also, you should pull out this check so we only do it once.

JI

jimrrchen Jan. 27, 2012, 4:49 a.m.
```
Definitely, I'll make the change
```

Diff:

Revision 2 (+13 -6)

Show changes

	reviewboard/htdocs/media/rb/js/reviews.js
	reviewboard/templates/reviews/review_draft_inline_form.html

Change Summary:

Edited descriptions to reflect last changes

Description:

~		In the review summary dialog, there is now an "open an issue" checkbox after each comment.
	~	In the review summary dialog, there is now an "open an issue" checkbox after each comment.
	+	Also fixed the way we get checkbox states in reviews.js

Can you list out the steps that you tested?

jimrrchen Jan. 28, 2012, 6:36 a.m.

Sure. It is done. Let me know if you need more information

Testing Done:

~		Tested in my local linux machine with Chrome
	~	Tested in my local linux machine with Chrome using the following steps
	+	1. take any review request, view its diffs
	+	2. comment on any line of the diff, open an issue
	+	3. comment on some other line of the diff, don't open an issue
	+	4. there should be a green bar on top, click the "edit review" button
	+	5. in the summary, uncheck the "open an issue" in the first comment, don't modify the comment content
	+	6. check the "open an issue" in the second comment, don't modify the comment content
	+	7. save the review
	+	8. refresh the page, and see that the issue states have been changed accordingly

And when you publish the review, everything is as you expect?

jimrrchen Jan. 28, 2012, 3:05 p.m.

Actually, I think I just discover yet another bug...let me investigate a bit on this

reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 2)

Show all issues

It kind of blows that we have to do this every time we set the issue_opened property.  It feels error prone.

We might want to update the comment object in datastore.js that's actually responsible for sending the data to the server.  If we give *it* the responsibility of converting the boolean to the 0/1, I think we'd be in a better position.

Change Summary:

removed the 1/0 conversions since web API now accepts "true"

Diff:

Revision 3 (+14 -7)

Show changes

	reviewboard/htdocs/media/rb/js/reviews.js
	reviewboard/templates/reviews/review_draft_inline_form.html

Jim:

This is really really close - I'm just wondering if we wanted to switch to .is(":checked"), as stated a few reviews back.

-Mike

jimrrchen Feb. 4, 2012, 1:29 p.m.

Hey Mike,

I think they both are actually equivalent, as recommended here http://api.jquery.com/prop/. The preferred cross-browser-compatible way is one of the following:
if ( elem.checked )
if ( $(elem).prop("checked") )
if ( $(elem).is(":checked") )

I just happen to choose the first one because a) the same elem.checked style is also used in repositoryform.js b) personally, coming for a non-jQuery background, I think this is more "javascript native"(as in doesn't depend on jQuery) and easier to understand.

mike_conley Feb. 5, 2012, 2:16 a.m.

Jim:

True - though I should point out, in Christians review, he writes:

"Agreed with the is(":checked")."...

jimrrchen Feb. 5, 2012, 1:29 p.m.

I think he mostly just means not using attr(). :)
Also, think about it. Previously we use attr() and jQuery decides to change its behavior in a later version. The same thing could happen to .is(). It is really to our benefit to not depend on external libraries for some really basic operations as much as we can.

mike_conley Feb. 6, 2012, 8:50 a.m.

*shrugs*, alright - I won't press the point. If Christian and David are cool with it, feel free to drop my issues.

reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 3)

Show all issues

Didn't we want to use something like:

$(issueField[0]).is(":checked")

?

reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 3)
Show all issues
```
Same as above, re: is(":checked")
```
reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 3)
Show all issues
```
Same as above, re: .is(":checked")
```
reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 3)
Show all issues
```
Same as above, re: .is(":checked")
```
reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 3)
Show all issues
```
Same as above, re: .is(":checked")
```

Ship it!

```
This looks good to me.  Thanks!

-Mike
```

I just applied this patch and tested it. If I make several comments with issues, and then click "edit review" in the green bar, none of the check-boxes are checked.

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to master (68ec6cd). Thanks!