Expose Toggles for Opening Issues in Review Summary Dialog

Review Request #2827 — Created Jan. 26, 2012 and submitted

Information

Review Board

Reviewers

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
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
Description From Last Updated

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

chipx86chipx86

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

mike_conleymike_conley

Didn't we want to use something like: $(issueField[0]).is(":checked") ?

mike_conleymike_conley

Same as above, re: is(":checked")

mike_conleymike_conley

Same as above, re: .is(":checked")

mike_conleymike_conley

Same as above, re: .is(":checked")

mike_conleymike_conley

Same as above, re: .is(":checked")

mike_conleymike_conley
AM
  1. 
      
  2. 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).
    1. "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
      
    2. 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!
    3. 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
  3. 
      
chipx86
  1. 
      
  2. 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.
    1. Definitely, I'll make the change
  3. 
      
JI
JI
david
  1. Can you list out the steps that you tested?
    1. Sure. It is done. Let me know if you need more information
  2. 
      
JI
david
  1. And when you publish the review, everything is as you expect?
    1. Actually, I think I just discover yet another bug...let me investigate a bit on this
  2. 
      
mike_conley
  1. 
      
  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.
  3. 
      
JI
mike_conley
  1. 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
    1. 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.
    2. Jim:
      
      True - though I should point out, in Christians review, he writes:
      
      "Agreed with the is(":checked")."...
    3. 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.
    4. *shrugs*, alright - I won't press the point. If Christian and David are cool with it, feel free to drop my issues.
  2. Show all issues
    Didn't we want to use something like:
    
    $(issueField[0]).is(":checked")
    
    ?
  3. Show all issues
    Same as above, re: is(":checked")
  4. Show all issues
    Same as above, re: .is(":checked")
  5. Show all issues
    Same as above, re: .is(":checked")
  6. Show all issues
    Same as above, re: .is(":checked")
  7. 
      
mike_conley
  1. This looks good to me.  Thanks!
    
    -Mike
  2. 
      
david
  1. 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.
  2. 
      
david
  1. Ship It!
  2. 
      
JI
Review request changed
Status:
Completed
Change Summary:
Pushed to master (68ec6cd). Thanks!