Avoid a page reload on the review request UI.

Review Request #3389 — Created Oct. 1, 2012 and discarded

Information

Review Board
master

Reviewers

The idea here is to have both discard and draft banners on the window so that by using jQuery we can hide/show the appropriate banner instead of fully re-loading the page.
Note- Testing pertains banners only and it was performed as follows:
-Analyse expected behaviour of a review request's state in demo.reviewboard.
-Reproduce the review request's state locally making sure it behaves as expected(minus the page reload).

* Brand new, not yet published:
Discard-Review-Request button: draft-banner and "close" option in the nav bar hide. Discard-banner shows 
Discard-Review-Request link: draft-banner and "close" option in the nav bar hide. Discard-banner shows.
Publish button: draft-banner hides and review-request gets published as expected.

* Published and unchanged
None of the banners are displayed- as expected.

* Published with a change causing a draft
Draft-banner is displayed as expected.
"ok" button: saves description entered as expected
"cancel" button: description entered is discarded as expected
"publish changes" button: draft banner hides and request is published as expected.
"discard changes" button: draft banner hides and request is discarded as expected.

* Discarded
discard-banner is displayed as expected.
"ok" button: saves description entered as expected
"cancel" button: description entered is discarded as expected

* Reopened after discarded
"reopen for review" button: discard-banner hides, draft-banner is displayed and request becomes a draft.

* Submitted
submitted-banner is displayed as expected.
"ok" button: saves description entered as expected
"cancel" button: description entered is discarded as expected

* Reopened after submitted
-submitted request was previously a draft: "reopen" button hides submitted-banner and displays draft-banner
-submitted request was previously public: "reopen" button hides submitted-banner.
Description From Last Updated

Can you combine these two lines?

daviddavid

Please combine these too.

daviddavid

This change isn't correct--we already were hiding it using CSS. We don't want it to be present in the case …

daviddavid

You removed the spaces inside the {%. Please don't

daviddavid

Please put the spaces back before the else.

daviddavid

There's no reason for this change.

daviddavid

This still isn't correct. It's still including the draft banner in the page for users who should never be able …

daviddavid

What happens if you pass this success function to gReviewRequest.close on line ~2619 in reviews.js, where we attach the click …

mike_conleymike_conley

This semicolon is breaking reviews.js - you need to remove it.

mike_conleymike_conley

This isn't needed. Keep it as it was. You only really care about hasOwnProperty when you want to be sure …

chipx86chipx86

You need to put the other id "#discard-review-request-link" back into this query.

mike_conleymike_conley

Space before "{"

chipx86chipx86

Should use gDraftBanner.

chipx86chipx86

It'd be nice to have a variable at the top like gDraftBanner for this.

chipx86chipx86

This seems wrong. Note that there's two conditions for showing this form: 1) It has a pending draft. 2) The …

chipx86chipx86

The first conditional is checking against 'P', so I don't see how the newly added check against 'D' can ever …

chipx86chipx86

nits - space before the equals signs here.

mike_conleymike_conley

The style is a bit off here - should look like: gReviewRequest.publish({ buttons: gDraftBannerButtons, success: function() { gDraftBanner.hide(); } });

mike_conleymike_conley

space before =

mike_conleymike_conley

Spaces after the ifs, and on either side of the =='s, and before the open braces. Also, I think you …

mike_conleymike_conley

In an effort to prepare for our eventual localization, we probably want to abstract the English text into some constants …

mike_conleymike_conley

So, instead of hiding each editicon, how about shutting down their editable-fieldness? This would mean changing inlineEditor so that it …

mike_conleymike_conley

Same as above.

mike_conleymike_conley

Same comments as above - only in this case, we'd be enabling the inlineEditor.

mike_conleymike_conley

Instead of changing the ID and adding this, let's just keep the old ID.

mike_conleymike_conley

What is this element? I can't find any elements with this ID anywhere...

mike_conleymike_conley

Not entirely sure this is necessary - see below.

mike_conleymike_conley

Why is this ID changing?

mike_conleymike_conley

Please remove this extra line.

mike_conleymike_conley

Hm, so I'm afraid how how fragile this structure is for l10n. Not sure how important that is at this …

mike_conleymike_conley

This looks really, really wrong. That's three things named the same ID, with tiny variations. That's just plain confusing. If …

mike_conleymike_conley

Style is a bit off here. success: function() { gDraftBanner.hide(); }

mike_conleymike_conley

I think I'd feel better if we didn't depend so much on whether the banners were visible, and just checked …

mike_conleymike_conley

This is really confusing - why are we still creating a client-side timestamp here? Why can't we retrieve this information …

mike_conleymike_conley

This can be condensed to: $("span.status").text(newState ? newState : "updated");

mike_conleymike_conley

Whoa - hold it...why are you passing "disable" as a string? I thought disable was a function that we'd call …

mike_conleymike_conley

Same as above. Also, this function looks *extremely* similar to the one on line 2503. We might want to DRY* …

mike_conleymike_conley

I really don't think this is how we want to be enabling and disabling inlineEditors...

mike_conleymike_conley

Why was this ID changed?

mike_conleymike_conley

Swap these. var should go first. Blank line after the var.

chipx86chipx86

loaded should be last. It signifies that we've finished loading.

chipx86chipx86

I believe this can possibly fetch fields that aren't on the review request. I think what you want is .review-request …

chipx86chipx86

I'm certain this doesn't work. You're never terminating the publish() command. That makes me think that this code path was …

chipx86chipx86

There's no way any of this was ever tested. You're using Django template tags in here, but we never go …

chipx86chipx86

Indentation problems.

chipx86chipx86

Should be alphabetical.

chipx86chipx86

Space after the ","

chipx86chipx86

You have indentation problems here. We only call this once, so I don't think it's worth having a function for …

chipx86chipx86

Same comments here.

chipx86chipx86

You can chain this: $('time.timesince') .attr('datetime', gReviewRequest.last_updated) .timesince();

chipx86chipx86

We're not actually closing it. I'd call this "onReviewRequestClosed" instead, since it's really the handler after we've closed.

chipx86chipx86

Can you keep the queries grouped and the non-queries grouped?

chipx86chipx86

No need for ".editable"

chipx86chipx86

onReviewRequestReopened

chipx86chipx86

What's the "updated" all about?

chipx86chipx86

There can only be one element with a given ID. This query is not reliable. You may need to change …

chipx86chipx86

You're missing a ;

chipx86chipx86

Same comment as above.

chipx86chipx86

Space before "("

chipx86chipx86

The ID is the most specific. No need to append the class names. Needs to wrap to < 80 chars. …

chipx86chipx86

I think what you really want is: $('.review-request .editable').reviewRequestFieldEditor();

chipx86chipx86

Keep the blank line.

chipx86chipx86

You're defining this button ID multiple times, which won't work. You need to switch to classes and update call sites.

chipx86chipx86

Levels of indentation look wrong. The last endif should only have one space, and the first here should have two. …

chipx86chipx86

One space before "if"

chipx86chipx86
mike_conley
  1. Hey Jesus,
    
    A few general comments:
    
    1) For HTML and Django templates, we tend to only use 2 spaces for indenting per level
    2) You're going to need to axe the trailing whitespace (see the big red areas in the diff)
    
    I'm a little confused about the logic for the discard banner. Notice that it's contained within a conditional which *ensures* that review_request.status == "S".
    
    Remember - we want all three banners (submitted, discard, and draft) rendered in the template (so no wrapping conditionals around those). Instead, we want to control the *visibility* of those banners by putting the conditionals within the style attribute of the DIVs, to set display:none; if the block should not be displayed.
    
    Keep digging,
    
    -Mike
  2. 
      
JZ
david
  1. 
      
  2. Show all issues
    Can you combine these two lines?
  3. Show all issues
    Please combine these too.
  4. Show all issues
    This change isn't correct--we already were hiding it using CSS. We don't want it to be present in the case where the user isn't the owner/can-edit for the review.
  5. Show all issues
    You removed the spaces inside the {%. Please don't 
  6. Show all issues
    Please put the spaces back before the else.
  7. reviewboard/templates/reviews/review_header.html (Diff revision 2)
     
     
     
     
    Show all issues
    There's no reason for this change.
  8. 
      
JZ
david
  1. 
      
  2. Show all issues
    This still isn't correct. It's still including the draft banner in the page for users who should never be able to see it (and doing so with two style="display: none" attributes).
  3. 
      
JZ
mike_conley
  1. 
      
  2. reviewboard/static/rb/js/reviews.js (Diff revision 4)
     
     
     
     
     
    Show all issues
    What happens if you pass this success function to gReviewRequest.close on line ~2619 in reviews.js, where we attach the click handler for #btn-review-request-discard and #discard-review-request-link?
    1. I tried that and i'm still having the same issue :/
  3. 
      
mike_conley
  1. I think you may have found a bug in datastore.js - at this line:
    
    https://github.com/reviewboard/reviewboard/blob/master/reviewboard/static/rb/js/datastore.js#L660
    
    if (!options.success) is evaluating to true, even if a function is passed.
    
    We can fix that by using hasOwnProperty. So change that line to be:
    
    if (!options.hasOwnProperty("success")) {
      // ...
    }
    
    That makes the banner go away for me.
  2. reviewboard/static/rb/js/reviews.js (Diff revision 4)
     
     
    Show all issues
    This semicolon is breaking reviews.js - you need to remove it.
  3. 
      
JZ
JZ
JZ
JZ
JZ
JZ
mike_conley
  1. Hey Jesus,
    
    This looks pretty good! Just one major point below, and a question.
    
    -Mike
  2. reviewboard/static/rb/js/reviews.js (Diff revision 8)
     
     
    Why was this removed?
    1. Hmm, i don't remember removing that. I must have done it by mistake? I'll add it back. 
  3. reviewboard/static/rb/js/reviews.js (Diff revision 8)
     
     
    Show all issues
    You need to put the other id "#discard-review-request-link" back into this query.
    1. Yes thanks, I forgot about that.
  4. 
      
chipx86
  1. Hi Jesus,
    
    I'd like to see a complete, detailed, step-by-step set of tests in Testing Done that describes every permutation of states you're working with. We need to know what testing and that testing needs to occur with every draft, since this is something very core that we can't get wrong.
  2. reviewboard/static/rb/js/datastore.js (Diff revision 8)
     
     
    Show all issues
    This isn't needed. Keep it as it was. You only really care about hasOwnProperty when you want to be sure that this specific base-level instance has it and not a prototype backing it.
    1. This is my bad - I suggested it a while back when we were debugging something, and I think this was a red-herring.
  3. reviewboard/static/rb/js/reviews.js (Diff revision 8)
     
     
    Show all issues
    Space before "{"
  4. reviewboard/static/rb/js/reviews.js (Diff revision 8)
     
     
    Show all issues
    Should use gDraftBanner.
  5. reviewboard/static/rb/js/reviews.js (Diff revision 8)
     
     
    Show all issues
    It'd be nice to have a variable at the top like gDraftBanner for this.
  6. Show all issues
    This seems wrong. Note that there's two conditions for showing this form:
    
    1) It has a pending draft.
    2) The user viewing it has permissions to edit this review request.
    
    #1 is dynamic. That state will change when there's an edit or when a review request is reopened from a discarded state.
    
    #2 is not dynamic. This will never change while the page is open. So that original check should remain, and if the user doesn't have permission to edit the review request, this form should never be in the HTML.
    1. I'm not 100% sure i understand your comment. Are you looking for something like this?
      
      http://pastie.org/5182871
    2. Close, but the user check and permission check should always be together. So that'd be the outer check that the draft banner would be within.
      
      The template is rendered on page access. There's no way the user will change while the page is shown, or the submitter will change. And if neither of these conditions are true, there's no legitimate way the draft banner can be displayed (unless there's a bug introduced).
  7. Show all issues
    The first conditional is checking against 'P', so I don't see how the newly added check against 'D' can ever be false.
    1. That's true. But the draft-banner was showing when opening an existing discarded review request. So I added that condition and it no longer showed. Same behaviour occurs with submitted review requests.
      
      I am not sure why this is happening because as you said, we are checking against 'P' in the first conditional.
    2. So the reason is that "and" has higher precedence, so even the original was wrong.
      
      Instead, break this up. The first {% if %} should only do the submitter/permission check. The second should do check against 'P' and not '!= D'.
  8. 
      
JZ
JZ
JZ
chipx86
  1. I'd also like to see detailed testing showing things are working for when the review request is in the following states:
    
    * Brand new, not yet published
    * Published and unchanged
    * Published with a change causing a draft
    * Discarded
    * Roepened after discarded
    * Submitted
    * Reopened after submitted
  2. 
      
JZ
JZ
mike_conley
  1. 
      
  2. reviewboard/static/rb/js/datastore.js (Diff revision 11)
     
     
     
    Show all issues
    nits - space before the equals signs here.
  3. reviewboard/static/rb/js/reviews.js (Diff revision 11)
     
     
     
     
    Show all issues
    The style is a bit off here - should look like:
    
    gReviewRequest.publish({
        buttons: gDraftBannerButtons,
        success: function() {
            gDraftBanner.hide();
        }
    });
  4. reviewboard/static/rb/js/reviews.js (Diff revision 11)
     
     
     
     
    Maybe I missed something, but why are we generating this updated timestamp on the client side, instead of reading it from the returned object once the API call is successful?
  5. reviewboard/static/rb/js/reviews.js (Diff revision 11)
     
     
    Show all issues
    space before =
  6. reviewboard/static/rb/js/reviews.js (Diff revision 11)
     
     
     
     
     
     
     
     
     
     
    Show all issues
    Spaces after the ifs, and on either side of the =='s, and before the open braces.
    
    Also, I think you can simplify your replacements here like this:
    
    if (newState) {
      $("span.status").text(newState);
    } else {
      $("span.status").text("updated");
    }
  7. reviewboard/static/rb/js/reviews.js (Diff revision 11)
     
     
    Show all issues
    In an effort to prepare for our eventual localization, we probably want to abstract the English text into some constants that can be easily flipped.
  8. reviewboard/static/rb/js/reviews.js (Diff revision 11)
     
     
     
     
     
     
    Show all issues
    So, instead of hiding each editicon, how about shutting down their editable-fieldness?
    
    This would mean changing inlineEditor so that it can be disabled.
  9. reviewboard/static/rb/js/reviews.js (Diff revision 11)
     
     
     
     
     
     
    Show all issues
    Same as above.
  10. reviewboard/static/rb/js/reviews.js (Diff revision 11)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues
    Same comments as above - only in this case, we'd be enabling the inlineEditor.
  11. reviewboard/static/rb/js/reviews.js (Diff revision 11)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues
    Instead of changing the ID and adding this, let's just keep the old ID.
  12. reviewboard/static/rb/js/reviews.js (Diff revision 11)
     
     
    Show all issues
    What is this element? I can't find any elements with this ID anywhere...
  13. reviewboard/static/rb/js/reviews.js (Diff revision 11)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues
    Not entirely sure this is necessary - see below.
    1. I added this because i needed to differentiate between re-opening a previously public review request and a review request that was previously a draft. That's because the draft-banner is shown if the review request is re-opened and was never published. And if the review request was public then no banner is shown. I'm not really sure how i could do this differently.
      
      
  14. Show all issues
    Why is this ID changing?
  15. reviewboard/templates/reviews/review_header.html (Diff revision 11)
     
     
     
     
    Did I miss something? Why are we adding this new ability?
  16. Show all issues
    Please remove this extra line.
  17. reviewboard/templates/reviews/review_request_box.html (Diff revision 11)
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues
    Hm, so I'm afraid how how fragile this structure is for l10n. Not sure how important that is at this stage... David or Christian?
  18. 
      
JZ
JZ
JZ
mike_conley
  1. 
      
  2. reviewboard/static/rb/css/reviews.less (Diff revision 14)
     
     
    Show all issues
    This looks really, really wrong. That's three things named the same ID, with tiny variations. That's just plain confusing.
    
    If you really need these things to share these styles, you should use a class - not a series of IDs.
    1. How could I then differentiate each of them is from a different banner? Because otherwise, the editIcon to change the description on the discard-banner also appears on the submitted-banner and vice-versa. 
    2. Perhaps they can have different IDs (though they'd need more accurate / precise naming) - but since the style is the same, they could use a common class.
  3. reviewboard/static/rb/js/reviews.js (Diff revision 14)
     
     
     
     
    Show all issues
    Style is a bit off here.
    
    success: function() {
        gDraftBanner.hide();
    }
  4. reviewboard/static/rb/js/reviews.js (Diff revision 14)
     
     
    Show all issues
    I think I'd feel better if we didn't depend so much on whether the banners were visible, and just checked the review request object to see if it was in a state that allowed for editable fields.
  5. reviewboard/static/rb/js/reviews.js (Diff revision 14)
     
     
     
     
     
    Show all issues
    This is really confusing - why are we still creating a client-side timestamp here? Why can't we retrieve this information from the review request instead?
  6. reviewboard/static/rb/js/reviews.js (Diff revision 14)
     
     
     
     
     
     
     
    Show all issues
    This can be condensed to:
    
    $("span.status").text(newState ? newState : "updated");
  7. reviewboard/static/rb/js/reviews.js (Diff revision 14)
     
     
    Show all issues
    Whoa - hold it...why are you passing "disable" as a string? I thought disable was a function that we'd call on inlineEditor widgets.
    1. So i should just call disable() on the '.editable' fields because they are already also inline editors?
    2. That's how you call functions in jQuery-UI widgets. It's weird.
  8. reviewboard/static/rb/js/reviews.js (Diff revision 14)
     
     
    Show all issues
    Same as above.
    
    Also, this function looks *extremely* similar to the one on line 2503. We might want to DRY* it up.
    
    * Don't repeat yourself.
  9. reviewboard/static/rb/js/reviews.js (Diff revision 14)
     
     
     
    Show all issues
    I really don't think this is how we want to be enabling and disabling inlineEditors...
  10. Show all issues
    Why was this ID changed?
    1. That was the solution i came up with to the problem of multiple editIcon on each banner.
  11. 
      
JZ
JZ
mike_conley
  1. Jesus - is this still a WIP patch, or can we start deeply reviewing it?
    1. I'm trying to get the script in review_header.html to work. If will take the patch out of WIP if i can't figure it out in the next 20 minutes.
  2. 
      
JZ
chipx86
  1. 
      
  2. reviewboard/static/rb/js/datastore.js (Diff revision 17)
     
     
     
    Show all issues
    Swap these. var should go first. Blank line after the var.
  3. reviewboard/static/rb/js/datastore.js (Diff revision 17)
     
     
    Show all issues
    loaded should be last. It signifies that we've finished loading.
  4. reviewboard/static/rb/js/reviews.js (Diff revision 17)
     
     
    Show all issues
    I believe this can possibly fetch fields that aren't on the review request. I think what you want is .review-request .main
  5. reviewboard/static/rb/js/reviews.js (Diff revision 17)
     
     
     
     
     
     
    Show all issues
    I'm certain this doesn't work. You're never terminating the publish() command. That makes me think that this code path was never tested.
  6. reviewboard/static/rb/js/reviews.js (Diff revision 17)
     
     
     
     
     
     
     
     
     
    Show all issues
    There's no way any of this was ever tested.
    
    You're using Django template tags in here, but we never go through Django. That clearly can never work.
    
    What are you attempting to do here? Why can't we reuse the banner we'd otherwise have? That should just be in the HTML and shown.
  7. reviewboard/static/rb/js/reviews.js (Diff revision 17)
     
     
     
     
     
    Show all issues
    Indentation problems.
  8. reviewboard/static/rb/js/reviews.js (Diff revision 17)
     
     
     
     
     
    Show all issues
    Should be alphabetical.
  9. reviewboard/static/rb/js/reviews.js (Diff revision 17)
     
     
    Show all issues
    Space after the ","
  10. reviewboard/static/rb/js/reviews.js (Diff revision 17)
     
     
     
     
    Show all issues
    You have indentation problems here.
    
    We only call this once, so I don't think it's worth having a function for it. Just do this logic where it's needed.
  11. reviewboard/static/rb/js/reviews.js (Diff revision 17)
     
     
     
     
     
     
    Show all issues
    Same comments here.
  12. reviewboard/static/rb/js/reviews.js (Diff revision 17)
     
     
     
    Show all issues
    You can chain this:
    
    $('time.timesince')
        .attr('datetime', gReviewRequest.last_updated)
        .timesince();
  13. reviewboard/static/rb/js/reviews.js (Diff revision 17)
     
     
    Show all issues
    We're not actually closing it. I'd call this "onReviewRequestClosed" instead, since it's really the handler after we've closed.
  14. reviewboard/static/rb/js/reviews.js (Diff revision 17)
     
     
     
     
     
     
     
    Show all issues
    Can you keep the queries grouped and the non-queries grouped?
  15. reviewboard/static/rb/js/reviews.js (Diff revision 17)
     
     
     
     
     
     
     
     
    Show all issues
    No need for ".editable"
  16. reviewboard/static/rb/js/reviews.js (Diff revision 17)
     
     
    Show all issues
    onReviewRequestReopened
  17. reviewboard/static/rb/js/reviews.js (Diff revision 17)
     
     
    Show all issues
    What's the "updated" all about?
  18. reviewboard/static/rb/js/reviews.js (Diff revision 17)
     
     
    Show all issues
    There can only be one element with a given ID. This query is not reliable. You may need to change button IDs to classes and make sure nothing breaks.
  19. reviewboard/static/rb/js/reviews.js (Diff revision 17)
     
     
    Show all issues
    You're missing a ;
  20. reviewboard/static/rb/js/reviews.js (Diff revision 17)
     
     
    Show all issues
    Same comment as above.
  21. reviewboard/static/rb/js/reviews.js (Diff revision 17)
     
     
    Show all issues
    Space before "("
  22. reviewboard/static/rb/js/reviews.js (Diff revision 17)
     
     
     
    Show all issues
    The ID is the most specific. No need to append the class names.
    
    Needs to wrap to < 80 chars. Do:
    
    $('#submitted-description')
        .reviewCloseCommentEditor(...);
  23. reviewboard/static/rb/js/reviews.js (Diff revision 17)
     
     
    Show all issues
    I think what you really want is:
    
    $('.review-request .editable').reviewRequestFieldEditor();
  24. reviewboard/static/rb/js/reviews.js (Diff revision 17)
     
     
    Show all issues
    Keep the blank line.
  25. Show all issues
    You're defining this button ID multiple times, which won't work. You need to switch to classes and update call sites.
  26. Show all issues
    Levels of indentation look wrong. The last endif should only have one space, and the first here should have two.
    
    Make sure the template tags above only indent by 1 space each time and that they're all correct.
  27. Show all issues
    One space before "if"
  28. 
      
JZ
Review request changed
Status:
Discarded