Additional Branch and Revision optional fields within Submission Banner

Review Request #3965 — Created March 13, 2013 and discarded

Information

Review Board
master

Reviewers

Feature
- This feature adds two additional optional fields to the Submit banner alongside the original "describe your submission"; being "Revision" and Branch". 
- These new fields are dynamically addressed in the change log found at the bottom of a review request.

NOTE: This review request is currently bugged, as it is not allowing the removal of the following issues from the tracker:
File 17, 18, 33, 34, 38. Consider these issues dealt with.
THE FOLLOWING OCCUR AFTER CURSORING OVER THE CLOSE DROP DOWN AND THEN CLICKING DISCARDED
- Typing nothing proceeds with empty bottom log other then signal for status change, as expected.

- Take a newly published request, cursor over the Close drop down, click discarded. The discard banner appears, as expected. Typing in a reason for discard proceeds as normal.

THE FOLLOWING OCCUR AFTER CURSORING OVER THE CLOSE DROP DOWN AND THEN CLICKING SUBMITTED
- Typing nothing proceeds with empty bottom log other then signal for status change, as expected.

- Typing only in the description area, the text area expands, takes in input and then detracts as required. The bottom change log adjusts to display the newly typed description under its proper heading.

- Typing only in the revision/branch area, the text label switches to edit mode, takes in input and then detracts as required. The bottom change log adjusts to display the newly typed revision/branch under its proper heading.

- Typing in all three areas, the text labels switch to edit mode, takes in input and then detracts as required. The bottom change log adjusts to display the newly typed revision/branch/description under their proper headings.

- Typing in any of the labels as described previously, but then changing your mind and overwriting to another input. The bottomt change log adjusts to the newest version of text.

Description From Last Updated

I'm not sure this is the best format to use. The description is by no means restricted to a single …

SM smacleod

I think we should probably use single line input boxes for these (similar to the branch field describing the review …

SM smacleod

It's hard to tell which screenshots are of the current project. If you can remove any that don't represent the …

chipx86chipx86

The "(optional)" is too wordy and inconsistent from the rest of the UI. Every field is optional unless it has …

chipx86chipx86

I meant the labels themselves, not the values. Like on a review request, how "Branch" and "Submitter" and stuff are …

chipx86chipx86

Col: 80 E501 line too long (83 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (83 > 79 characters)

reviewbotreviewbot

Col: 48 W291 trailing whitespace

reviewbotreviewbot

Col: 24 E711 comparison to None should be 'if cond is not None

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 21 E711 comparison to None should be 'if cond is not None

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 19 E711 comparison to None should be 'if cond is not None

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 53 E711 comparison to None should be 'if cond is not None

reviewbotreviewbot

Col: 58 W291 trailing whitespace

reviewbotreviewbot

Col: 50 E711 comparison to None should be 'if cond is not None

reviewbotreviewbot

Col: 56 W291 trailing whitespace

reviewbotreviewbot

Col: 48 E711 comparison to None should be 'if cond is not None

reviewbotreviewbot

Col: 80 E501 line too long (80 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (80 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (85 > 79 characters)

reviewbotreviewbot

Col: 68 W291 trailing whitespace

reviewbotreviewbot

This max_length is probably too short.

SM smacleod

This max length is definitely too short, sha1s are twice this length.

SM smacleod

Again, probably too short

SM smacleod

I'm thinking we probably don't want to do it this way. It would be nicer to have each of these …

SM smacleod

Trailing whitespace (Review Bot only checks .py files, so you'll have to manually inspect the js, or enable stripping of …

SM smacleod

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 56 W291 trailing whitespace

reviewbotreviewbot

Col: 69 W291 trailing whitespace

reviewbotreviewbot

Col: 61 E272 multiple spaces before keyword

reviewbotreviewbot

Please alphabetically order these properties, and get rid of the commented out property.

mike_conleymike_conley

Needs 2 space indentation here, and a space between : and inline-block.

mike_conleymike_conley

Too much indent here.

mike_conleymike_conley

Each of these variables need "var" in front of them. Plus, we might as well use "revision", "branch". "desc" is …

mike_conleymike_conley

var in front of these

mike_conleymike_conley

Needs var in front of these, and use "revision" and "branch".

mike_conleymike_conley

I don't know that we need new fields for this. Any additional fields are supposed to lie in fields_changed.

chipx86chipx86

Not sure we need this. We already will be storing the information in the change description, and will be parsing …

chipx86chipx86

So this is completely changing how we handle these fields, and will break every existing review request out there. The …

chipx86chipx86

This is the perfect case of where you want to target a class instead of IDs.

chipx86chipx86

Can you just do: if (options.revision) Or will that not work?

chipx86chipx86

Why is this different from revision?

chipx86chipx86

Remove the magic number 1, and just use Rb.ReviewRequest.CLOSE_DISCARDED.

mike_conleymike_conley

I've just realized how fragile this is. Is jQuery.closest() not workable here? Same with the other parent().parent().parent() bit.

mike_conleymike_conley

These are really hairy and scary. You should be able to make use of the ID for the banner.

chipx86chipx86

Same as above.

chipx86chipx86

Too much redundancy with the above. I think really, this entire function should be part of the above function. Any …

chipx86chipx86

We shouldn't do this query more than once. How about: $('#submitted-banner .editable') .reviewCloseCommentEditor(RB.ReviewRequest.CLOSE_SUBMITTED);

chipx86chipx86

Can we rename these IDs to "revision-description" and stuff? Using the "-". I know "changedescription" is there, but for new …

chipx86chipx86

Just submitted. We don't want revision or branch for the discard banner, just submitted.

chipx86chipx86

Should enforce that this only works for submitting.

chipx86chipx86

Col: 80 E501 line too long (83 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (87 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (81 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (83 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (84 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (107 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (82 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (103 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (83 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (87 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (81 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (83 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (84 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (107 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (82 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (103 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (83 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (87 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (81 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (83 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (84 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (107 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (82 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (103 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (83 > 79 characters)

reviewbotreviewbot

Col: 73 W291 trailing whitespace

reviewbotreviewbot

Col: 80 E501 line too long (81 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (84 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (107 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (82 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (103 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (83 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (81 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (84 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (107 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (82 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (103 > 79 characters)

reviewbotreviewbot
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/reviews/views.py
        reviewboard/webapi/resources.py
        reviewboard/reviews/models.py
      Ignored Files:
        reviewboard/static/rb/js/reviews.js
        reviewboard/static/rb/css/reviews.less
        reviewboard/static/rb/js/datastore.js
        reviewboard/templates/reviews/review_header.html
    
    
  2. reviewboard/reviews/models.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (83 > 79 characters)
    
  3. reviewboard/reviews/models.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (83 > 79 characters)
    
  4. reviewboard/reviews/models.py (Diff revision 1)
     
     
    Col: 48
     W291 trailing whitespace
    
  5. reviewboard/reviews/models.py (Diff revision 1)
     
     
    Col: 24
     E711 comparison to None should be 'if cond is not None
  6. reviewboard/reviews/models.py (Diff revision 1)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  7. reviewboard/reviews/models.py (Diff revision 1)
     
     
    Col: 21
     E711 comparison to None should be 'if cond is not None
  8. reviewboard/reviews/models.py (Diff revision 1)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  9. reviewboard/reviews/models.py (Diff revision 1)
     
     
    Col: 19
     E711 comparison to None should be 'if cond is not None
  10. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  11. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Col: 53
     E711 comparison to None should be 'if cond is not None
  12. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Col: 58
     W291 trailing whitespace
    
  13. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Col: 50
     E711 comparison to None should be 'if cond is not None
  14. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Col: 56
     W291 trailing whitespace
    
  15. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Col: 48
     E711 comparison to None should be 'if cond is not None
  16. reviewboard/webapi/resources.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  17. reviewboard/webapi/resources.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  18. reviewboard/webapi/resources.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (85 > 79 characters)
    
  19. 
      
DE
DE
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/reviews/views.py
        reviewboard/webapi/resources.py
        reviewboard/reviews/models.py
      Ignored Files:
        reviewboard/static/rb/js/reviews.js
        reviewboard/static/rb/css/reviews.less
        reviewboard/static/rb/js/datastore.js
        reviewboard/templates/reviews/review_header.html
    
    
  2. reviewboard/webapi/resources.py (Diff revision 2)
     
     
    Col: 68
     W291 trailing whitespace
    
  3. 
      
DE
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/reviews/views.py
        reviewboard/webapi/resources.py
        reviewboard/reviews/models.py
      Ignored Files:
        reviewboard/static/rb/js/reviews.js
        reviewboard/static/rb/css/reviews.less
        reviewboard/static/rb/js/datastore.js
        reviewboard/templates/reviews/review_header.html
    
    
  2. 
      
SM
  1. 
      
  2. I'm not sure this is the best format to use. The description is by no means restricted to a single line, and this could look really strange with a multi-liner.
    
    Also, it'd probably be easier to pick out the branch and revision if they were separate fields.
    1. I think I'd be happiest with:
      
      "Submitted in revision 10 on branch 'reviews'"
      
      Try making the "Submitted in revision" and "branch" look like the label style used for other fields, so it stands out from the description text.
    2. I've addressed the first suggestion in my latest changes that will be resubmitted shortly, and I am now attempt to address the second suggestion.
  3. I think we should probably use single line input boxes for these (similar to the branch field describing the review request). I think it would look nicer, more compact, and it's all the functionality we need.
    1. Agreed. This was the main thing that struck me when looking at the change.
    2. This is now my current target. 
  4. reviewboard/reviews/models.py (Diff revision 3)
     
     
    This max_length is probably too short.
  5. reviewboard/reviews/models.py (Diff revision 3)
     
     
    This max length is definitely too short, sha1s are twice this length.
  6. reviewboard/reviews/models.py (Diff revision 3)
     
     
    Again, probably too short
  7. reviewboard/reviews/models.py (Diff revision 3)
     
     
     
     
     
     
     
     
     
    I'm thinking we probably don't want to do it this way. It would be nicer to have each of these fields actually stored separately and displayed, instead of generating a message and sticking it in the text where description used to go.
    1. So I have the model storing each of the inputs, but for now I've address one of the earlier comments suggesting a readability improvement in the current style of output. Check out the new screenshots to see if it's more to your liking. 
      
      In terms of moving forward, the saved model fields definately could be used to produce a more developed means of output, but should that be left for a future project, either by myself or another student, keeping in mind the up coming deadlines? Just a thought.
    2. Set as fixed as the first part was addressed, and the second has been reviewed in a more recent comment. Just trying to avoid duplicate review issues.
  8. reviewboard/static/rb/js/reviews.js (Diff revision 3)
     
     
    Trailing whitespace (Review Bot only checks .py files, so you'll have to manually inspect the js, or enable stripping of trailing whitespace in your editor)
  9. 
      
DE
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/reviews/views.py
        reviewboard/webapi/resources.py
        reviewboard/reviews/models.py
      Ignored Files:
        reviewboard/static/rb/js/reviews.js
        reviewboard/static/rb/css/reviews.less
        reviewboard/static/rb/js/datastore.js
        reviewboard/templates/reviews/review_header.html
    
    
  2. 
      
DE
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/reviews/views.py
        reviewboard/webapi/resources.py
        reviewboard/reviews/models.py
      Ignored Files:
        reviewboard/static/rb/js/reviews.js
        reviewboard/static/rb/css/reviews.less
        reviewboard/static/rb/js/datastore.js
        reviewboard/templates/reviews/review_header.html
    
    
  2. reviewboard/reviews/models.py (Diff revision 5)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  3. 
      
DE
DE
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/reviews/views.py
        reviewboard/webapi/resources.py
        reviewboard/reviews/models.py
      Ignored Files:
        reviewboard/static/rb/js/reviews.js
        reviewboard/static/rb/css/reviews.less
        reviewboard/static/rb/js/datastore.js
        reviewboard/templates/reviews/review_header.html
    
    
  2. 
      
DE
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/reviews/views.py
        reviewboard/webapi/resources.py
        reviewboard/reviews/models.py
      Ignored Files:
        reviewboard/static/rb/js/reviews.js
        reviewboard/static/rb/css/reviews.less
        reviewboard/static/rb/js/datastore.js
        reviewboard/templates/reviews/review_header.html
    
    
  2. 
      
DE
DE
DE
chipx86
  1. 
      
  2. It's hard to tell which screenshots are of the current project. If you can remove any that don't represent the current state, that would help.
    
    In this particular one, I'd rather not try to be smart and turn this into a big sentence. The description should be very distinct from the revision and branch.
    
    The revision and branch should be marked up better so that they look like labels in the forms of what we have for every other field. Consistency is important here.
    1. All the screenshots are new. The old ones were deleted at the same time as these were added.
      
      I'll look into changing it up as I was a little confused due to another comment.
    2. Also, this review/comment/issue encompasses two of the previous issues. However, it seems that I am not able to comment/reply to those older review threads. Very odd. Basically, if you see one of those older review issues set to fixed, it's because their first suggestion was dealt with, but the overarching issue remains, but their reviews are not relevant anymore since this is the latest and most descriptive response.
  3. 
      
chipx86
  1. 
      
  2. The "(optional)" is too wordy and inconsistent from the rest of the UI. Every field is optional unless it has an asterisk, so in this case, just remove "(optional)" on these fields.
    
    Also, I'd prefer the labels be marked up like they are on the review request fields, so that there's some visual indication.
    1. By marked up, do you mean with the white background, with the 1px border, etc? At least for me, the review request fields don't seem to have a border when not in edit mode, as the actual area background is in white, hence my confusion on the matter. I'll change it to have the visualized boxes, it's a quick fix.
  3. 
      
DE
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/reviews/views.py
        reviewboard/webapi/resources.py
        reviewboard/reviews/models.py
      Ignored Files:
        reviewboard/static/rb/js/reviews.js
        reviewboard/static/rb/css/reviews.less
        reviewboard/static/rb/js/datastore.js
        reviewboard/templates/reviews/review_header.html
    
    
  2. 
      
DE
chipx86
  1. 
      
  2. I meant the labels themselves, not the values. Like on a review request, how "Branch" and "Submitter" and stuff are bold and non-black. I'd suggest making the labels bold and the label color something like #333333 (dark gray).
  3. 
      
DE
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/reviews/views.py
        reviewboard/webapi/resources.py
        reviewboard/reviews/models.py
      Ignored Files:
        reviewboard/static/rb/js/reviews.js
        reviewboard/static/rb/css/reviews.less
        reviewboard/static/rb/js/datastore.js
        reviewboard/templates/reviews/review_header.html
    
    
    1. Hmm, it's not letting me select "Fixed issue" for the issues: File 17, 18, 34, and 38.
      
      This might be a continuation/extension of the issues I was having with my replies earlier. :/ 
  2. 
      
DE
DE
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/reviews/views.py
        reviewboard/webapi/resources.py
        reviewboard/changedescs/models.py
        reviewboard/reviews/models.py
      Ignored Files:
        reviewboard/static/rb/js/reviews.js
        reviewboard/templates/reviews/review_detail.html
        reviewboard/static/rb/js/datastore.js
        reviewboard/templates/reviews/review_header.html
        reviewboard/static/rb/css/reviews.less
    
    
  2. reviewboard/reviews/models.py (Diff revision 10)
     
     
    Col: 56
     W291 trailing whitespace
    
  3. reviewboard/reviews/models.py (Diff revision 10)
     
     
    Col: 69
     W291 trailing whitespace
    
  4. reviewboard/reviews/models.py (Diff revision 10)
     
     
    Col: 61
     E272 multiple spaces before keyword
    
  5. 
      
DE
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/reviews/views.py
        reviewboard/webapi/resources.py
        reviewboard/changedescs/models.py
        reviewboard/reviews/models.py
      Ignored Files:
        reviewboard/static/rb/js/reviews.js
        reviewboard/static/rb/css/reviews.less
        reviewboard/static/rb/js/datastore.js
        reviewboard/templates/reviews/review_header.html
        reviewboard/templates/reviews/review_detail.html
    
    
  2. 
      
mike_conley
  1. Patch looks pretty good! Please fill in the "Testing Done" section of this review request, detailing the testing you've performed on this change.
  2. reviewboard/static/rb/css/reviews.less (Diff revision 11)
     
     
     
     
     
     
     
     
     
     
    Please alphabetically order these properties, and get rid of the commented out property.
  3. reviewboard/static/rb/css/reviews.less (Diff revision 11)
     
     
     
     
     
     
    Needs 2 space indentation here, and a space between : and inline-block.
  4. reviewboard/static/rb/css/reviews.less (Diff revision 11)
     
     
     
    Too much indent here.
  5. reviewboard/static/rb/js/reviews.js (Diff revision 11)
     
     
     
     
    Each of these variables need "var" in front of them.
    
    Plus, we might as well use "revision", "branch". "desc" is fine.
  6. reviewboard/static/rb/js/reviews.js (Diff revision 11)
     
     
     
     
    var in front of these
  7. reviewboard/static/rb/js/reviews.js (Diff revision 11)
     
     
     
     
    Needs var in front of these, and use "revision" and "branch".
  8. P
DE
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/reviews/views.py
        reviewboard/webapi/resources.py
        reviewboard/changedescs/models.py
        reviewboard/reviews/models.py
      Ignored Files:
        reviewboard/templates/reviews/review_detail.html
        reviewboard/static/rb/js/reviews.js
        reviewboard/static/rb/js/datastore.js
        reviewboard/templates/reviews/review_header.html
        reviewboard/static/rb/css/reviews.less
    
    
  2. 
      
DE
DE
mike_conley
  1. Hey Jon, I know it's late in the game, but I have two more comments. 
  2. reviewboard/static/rb/js/reviews.js (Diff revision 12)
     
     
    Remove the magic number 1, and just use Rb.ReviewRequest.CLOSE_DISCARDED.
    1. Fixed in Diff r13!
  3. reviewboard/static/rb/js/reviews.js (Diff revision 12)
     
     
     
     
    I've just realized how fragile this is. Is jQuery.closest() not workable here?
    
    Same with the other parent().parent().parent() bit.
    1. Fixed in Diff r14!
  4. 
      
chipx86
  1. 
      
  2. reviewboard/changedescs/models.py (Diff revision 12)
     
     
     
    I don't know that we need new fields for this. Any additional fields are supposed to lie in fields_changed.
    1. Fixed in Diff r13!
  3. reviewboard/reviews/models.py (Diff revision 12)
     
     
     
     
     
     
     
    Not sure we need this. We already will be storing the information in the change description, and will be parsing it out from there. See my comment below about how we're doing that.
    
    (If we did keep this, it'd be extremely important that the submitted_description matches the 'text' field type from ChangeDescription.)
    1. Fixed in Diff r13!
  4. reviewboard/reviews/views.py (Diff revision 12)
     
     
     
     
     
     
     
    So this is completely changing how we handle these fields, and will break every existing review request out there.
    
    The new code is saying we're going to set the descriptions from the review request, but that's not where the change description has ever lived. It's always lived in the ChangeDescription. No new review request will have change_description set on the ReviewRequest, so they'll end up with blank descriptions, losing a ton of history.
    
    What we should do is not put this data in ReviewRequest, and instead pull from latest_changedesc, like we used to.
    
    So, close_description should come from latest_changedesc.text.
    
    revision should come from latest_changedesc.fields_changed['submitted_revision']['new'], if that 'submitted_revision' is in there.
    
    Likewise, branch should come from there.
    
    We should only pull these fields if status == ReviewRequest.SUBMITTED, since a discarded review request shouldn't have these fields.
    
    There's also an odd naming here and elsewhere in the change. "branch_description" and "revision_description." "change_description" makes sense, because it's a description of the change, btu these are really more like "submitted_branch" and "submitted_revision."
    1. Fixed in Diff r13!
  5. reviewboard/static/rb/css/reviews.less (Diff revision 12)
     
     
    This is the perfect case of where you want to target a class instead of IDs.
    1. Fixed in Diff r13!
  6. reviewboard/static/rb/js/datastore.js (Diff revision 12)
     
     
     
     
    Can you just do:
    
        if (options.revision)
    
    Or will that not work?
    1. Fixed in Diff r15!
  7. reviewboard/static/rb/js/datastore.js (Diff revision 12)
     
     
    Why is this different from revision?
    1. Fixed in Diff r15!
  8. reviewboard/static/rb/js/reviews.js (Diff revision 12)
     
     
     
     
    These are really hairy and scary. You should be able to make use of the ID for the banner.
    1. Fixed in Diff r14!
  9. reviewboard/static/rb/js/reviews.js (Diff revision 12)
     
     
     
     
    Same as above.
    1. Fixed in Diff r14!
  10. reviewboard/static/rb/js/reviews.js (Diff revision 12)
     
     
     
     
     
     
    Too much redundancy with the above. 
    
    I think really, this entire function should be part of the above function. Any customization can be done within the function.
    
    So, one function for any field on the banner, which will simplify the code below that calls into it, as I'll demonstrate below.
    1. Fixed in Diff r14!
  11. reviewboard/static/rb/js/reviews.js (Diff revision 12)
     
     
     
     
    We shouldn't do this query more than once. How about:
    
        $('#submitted-banner .editable')
            .reviewCloseCommentEditor(RB.ReviewRequest.CLOSE_SUBMITTED);
    1. Fixed in Diff r13!
  12. Can we rename these IDs to "revision-description" and stuff? Using the "-". I know "changedescription" is there, but for new ones, we should fix it.
    1. Fixed in Diff r13!
  13. reviewboard/webapi/resources.py (Diff revision 12)
     
     
    Just submitted. We don't want revision or branch for the discard banner, just submitted.
    1. Fixed in Diff r13!
  14. reviewboard/webapi/resources.py (Diff revision 12)
     
     
     
    Should enforce that this only works for submitting.
    1. Fixed in Diff r15!
  15. 
      
DE
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/reviews/views.py
        reviewboard/webapi/resources.py
        reviewboard/reviews/models.py
      Ignored Files:
        reviewboard/static/rb/js/reviews.js
        reviewboard/static/rb/css/reviews.less
        reviewboard/static/rb/js/datastore.js
        reviewboard/templates/reviews/review_header.html
        reviewboard/templates/reviews/review_detail.html
    
    
  2. reviewboard/reviews/models.py (Diff revision 13)
     
     
    Col: 80
     E501 line too long (83 > 79 characters)
    
  3. reviewboard/reviews/models.py (Diff revision 13)
     
     
    Col: 80
     E501 line too long (87 > 79 characters)
    
  4. reviewboard/reviews/models.py (Diff revision 13)
     
     
    Col: 80
     E501 line too long (81 > 79 characters)
    
  5. reviewboard/reviews/models.py (Diff revision 13)
     
     
    Col: 80
     E501 line too long (83 > 79 characters)
    
  6. reviewboard/reviews/views.py (Diff revision 13)
     
     
    Col: 80
     E501 line too long (84 > 79 characters)
    
  7. reviewboard/reviews/views.py (Diff revision 13)
     
     
    Col: 80
     E501 line too long (107 > 79 characters)
    
  8. reviewboard/reviews/views.py (Diff revision 13)
     
     
    Col: 80
     E501 line too long (82 > 79 characters)
    
  9. reviewboard/reviews/views.py (Diff revision 13)
     
     
    Col: 80
     E501 line too long (103 > 79 characters)
    
  10. 
      
DE
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/reviews/views.py
        reviewboard/webapi/resources.py
        reviewboard/reviews/models.py
      Ignored Files:
        reviewboard/static/rb/js/reviews.js
        reviewboard/static/rb/css/reviews.less
        reviewboard/static/rb/js/datastore.js
        reviewboard/templates/reviews/review_header.html
        reviewboard/templates/reviews/review_detail.html
    
    
  2. reviewboard/reviews/models.py (Diff revision 14)
     
     
    Col: 80
     E501 line too long (83 > 79 characters)
    
  3. reviewboard/reviews/models.py (Diff revision 14)
     
     
    Col: 80
     E501 line too long (87 > 79 characters)
    
  4. reviewboard/reviews/models.py (Diff revision 14)
     
     
    Col: 80
     E501 line too long (81 > 79 characters)
    
  5. reviewboard/reviews/models.py (Diff revision 14)
     
     
    Col: 80
     E501 line too long (83 > 79 characters)
    
  6. reviewboard/reviews/views.py (Diff revision 14)
     
     
    Col: 80
     E501 line too long (84 > 79 characters)
    
  7. reviewboard/reviews/views.py (Diff revision 14)
     
     
    Col: 80
     E501 line too long (107 > 79 characters)
    
  8. reviewboard/reviews/views.py (Diff revision 14)
     
     
    Col: 80
     E501 line too long (82 > 79 characters)
    
  9. reviewboard/reviews/views.py (Diff revision 14)
     
     
    Col: 80
     E501 line too long (103 > 79 characters)
    
  10. 
      
DE
DE
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/reviews/views.py
        reviewboard/webapi/resources.py
        reviewboard/reviews/models.py
      Ignored Files:
        reviewboard/static/rb/js/reviews.js
        reviewboard/static/rb/css/reviews.less
        reviewboard/static/rb/js/datastore.js
        reviewboard/templates/reviews/review_header.html
        reviewboard/templates/reviews/review_detail.html
    
    
  2. reviewboard/reviews/models.py (Diff revision 15)
     
     
    Col: 80
     E501 line too long (83 > 79 characters)
    
  3. reviewboard/reviews/models.py (Diff revision 15)
     
     
    Col: 80
     E501 line too long (87 > 79 characters)
    
  4. reviewboard/reviews/models.py (Diff revision 15)
     
     
    Col: 80
     E501 line too long (81 > 79 characters)
    
  5. reviewboard/reviews/models.py (Diff revision 15)
     
     
    Col: 80
     E501 line too long (83 > 79 characters)
    
  6. reviewboard/reviews/views.py (Diff revision 15)
     
     
    Col: 80
     E501 line too long (84 > 79 characters)
    
  7. reviewboard/reviews/views.py (Diff revision 15)
     
     
    Col: 80
     E501 line too long (107 > 79 characters)
    
  8. reviewboard/reviews/views.py (Diff revision 15)
     
     
    Col: 80
     E501 line too long (82 > 79 characters)
    
  9. reviewboard/reviews/views.py (Diff revision 15)
     
     
    Col: 80
     E501 line too long (103 > 79 characters)
    
  10. 
      
DE
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/reviews/views.py
        reviewboard/webapi/resources.py
        reviewboard/reviews/models.py
      Ignored Files:
        reviewboard/static/rb/js/reviews.js
        reviewboard/static/rb/css/reviews.less
        reviewboard/static/rb/js/datastore.js
        reviewboard/templates/reviews/review_header.html
        reviewboard/templates/reviews/review_detail.html
    
    
  2. reviewboard/reviews/models.py (Diff revision 16)
     
     
    Col: 80
     E501 line too long (83 > 79 characters)
    
  3. reviewboard/reviews/models.py (Diff revision 16)
     
     
    Col: 73
     W291 trailing whitespace
    
  4. reviewboard/reviews/models.py (Diff revision 16)
     
     
    Col: 80
     E501 line too long (81 > 79 characters)
    
  5. reviewboard/reviews/views.py (Diff revision 16)
     
     
    Col: 80
     E501 line too long (84 > 79 characters)
    
  6. reviewboard/reviews/views.py (Diff revision 16)
     
     
    Col: 80
     E501 line too long (107 > 79 characters)
    
  7. reviewboard/reviews/views.py (Diff revision 16)
     
     
    Col: 80
     E501 line too long (82 > 79 characters)
    
  8. reviewboard/reviews/views.py (Diff revision 16)
     
     
    Col: 80
     E501 line too long (103 > 79 characters)
    
  9. 
      
DE
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/reviews/views.py
        reviewboard/webapi/resources.py
        reviewboard/reviews/models.py
      Ignored Files:
        reviewboard/static/rb/js/reviews.js
        reviewboard/static/rb/css/reviews.less
        reviewboard/static/rb/js/datastore.js
        reviewboard/templates/reviews/review_header.html
        reviewboard/templates/reviews/review_detail.html
    
    
  2. reviewboard/reviews/models.py (Diff revision 17)
     
     
    Col: 80
     E501 line too long (83 > 79 characters)
    
  3. reviewboard/reviews/models.py (Diff revision 17)
     
     
    Col: 80
     E501 line too long (81 > 79 characters)
    
  4. reviewboard/reviews/views.py (Diff revision 17)
     
     
    Col: 80
     E501 line too long (84 > 79 characters)
    
  5. reviewboard/reviews/views.py (Diff revision 17)
     
     
    Col: 80
     E501 line too long (107 > 79 characters)
    
  6. reviewboard/reviews/views.py (Diff revision 17)
     
     
    Col: 80
     E501 line too long (82 > 79 characters)
    
  7. reviewboard/reviews/views.py (Diff revision 17)
     
     
    Col: 80
     E501 line too long (103 > 79 characters)
    
  8. 
      
DE
Review request changed

Status: Discarded

Loading...