Toggle for "Open an Issue"

Review Request #2828 — Created Jan. 28, 2012 and submitted

wilsonyeung
Review Board
2282
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.

mike_conleymike_conley

Just added the spacer?

WI wilsonyeung

As discussed in Jim's bug, I think we want to use: issueField.checked = gOpenAnIssue;

mike_conleymike_conley

This doesn't look like part of your project...

mike_conleymike_conley

This doesn't look like part of your project...

mike_conleymike_conley

Not sure what I can do about it.

WI wilsonyeung

This doesn't look like part of your project.

mike_conleymike_conley

This doesn't look like part of your project.

mike_conleymike_conley

This doesn't look like part of your project.

mike_conleymike_conley

This doesn't look like part of your project.

mike_conleymike_conley

This doesn't look like part of your project.

mike_conleymike_conley

This doesn't look like part of your project.

mike_conleymike_conley

Why is this in here?

mike_conleymike_conley

This doesn't look like part of your project.

mike_conleymike_conley

This evolution can be axed completely.

mike_conleymike_conley

We can get rid of these extra newlines.

mike_conleymike_conley

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 ...

mike_conleymike_conley

I think we want these formatted like they were before, and your evolution, since it was last, should be at ...

mike_conleymike_conley
WI
AM
  1. 
      
    1. I think Wilson's commit got mixed up with other commits. He needs to re-sync with master/origin.
      
      Yazan
  2. 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.
  3. Should the scope of the imports be narrowed to the specific objects that are being used?
  4. reviewboard/accounts/models.py (Diff revision 1)
     
     
    This line is over 80 characters long.
  5. I feel like this line should be kept.
  6. 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.
  7. 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.
  8. You can do something like "var gOpenAnIssue = {{user.get_profile.open_an_issuelower}};
  9. 
      
mike_conley
  1. 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
  2. Correct - we can just blow the always_issue.py evolution away.
  3. As discussed in Jim's bug, I think we want to use:
    
    issueField.checked = gOpenAnIssue;
    
  4. This doesn't look like part of your project...
  5. reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 1)
     
     
     
     
     
     
    This doesn't look like part of your project...
  6. reviewboard/notifications/email.py (Diff revision 1)
     
     
     
     
    This doesn't look like part of your project.
  7. reviewboard/notifications/email.py (Diff revision 1)
     
     
     
     
    This doesn't look like part of your project.
  8. reviewboard/notifications/email.py (Diff revision 1)
     
     
    This doesn't look like part of your project.
  9. reviewboard/notifications/email.py (Diff revision 1)
     
     
    This doesn't look like part of your project.
  10. reviewboard/notifications/email.py (Diff revision 1)
     
     
    This doesn't look like part of your project.
  11. reviewboard/reviews/datagrids.py (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    This doesn't look like part of your project.
  12. reviewboard/settings.py (Diff revision 1)
     
     
    Why is this in here?
  13. reviewboard/templates/accounts/prefs.html (Diff revision 1)
     
     
     
     
     
    This looks good.
  14. reviewboard/templates/admin/widgets/w-actions.html (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    This doesn't look like part of your project.
  15. 
      
WI
chipx86
  1. 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.
  2. 
      
WI
WI
  1. 
      
    1. Hey Wilson, I'm not exactly sure what's going on here. 
      
      It seems like you reviewed the wrong revision of the Diff, or you're responding to other people's reviews.
      If you are responding to other people's issues and comments, you can simply press the "Add comment" link found on the far right of your window under the person's comment.
      
      It looks like you finally got your branch in sync with origin master, which is great. You should update the review description to reflect that.
  2. Just deleted the file. I was playing around with evolutions and realized I named the variable poorly.
  3. Good call
  4. Just added the spacer?
  5. reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 1)
     
     
     
     
     
     
    Not sure what I can do about it.
  6. I need to find a way to change the initial state of the checkbox, not the final state as it closes
  7. Tried:
    {% if gOpenAnIssue %} check="checked" {% endif %}
  8. 
      
ME
  1. 
      
  2. reviewboard/templates/reviews/comments_dlg.html (Diff revision 3)
     
     
     
     
     
     
    Trailing white space and extra lines should be removed.
  3. 
      
mike_conley
  1. 
      
  2. This evolution can be axed completely.
  3. reviewboard/accounts/models.py (Diff revision 3)
     
     
     
    We can get rid of these extra newlines.
  4. 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.
  5. 
      
WI
WI
WI
ME
  1. Good job, just a few small things worth mentioning.
    
    -Yazan
  2. Empty line should be removed.
  3. reviewboard/accounts/forms.py (Diff revision 4)
     
     
    Maybe elaborate more in the label. "Default to opening an issue" isn't very intuitive.
  4. Remove space after "checked".
    
    ie: checked="checked"/>
    1. Feel free to drop this one, I just noticed you removed the checked attribute. My bad.
  5. 
      
mike_conley
  1. I'm happy with this, and it seems to work.  Great job, Wilson!
    
    -Mike
    1. (once you've addressed the issues that Yazan brought up)
  2. 
      
WI
mike_conley
  1. 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
  2. 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.
  3. 
      
WI
ME
  1. Ship It!
  2. 
      
mike_conley
  1. Just one minor issue, Wilson.  With that fixed, I'll be ready to give my ship-it.
  2. 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'
  3. 
      
WI
mike_conley
  1. I'm good with this.  Thanks Wilson!
  2. 
      
WI
Review request changed

Status: Closed (submitted)

Change Summary:

Committed to master as 33983aa9.
Loading...