Avoid a page reload on the review request UI.
Review Request #3389 — Created Oct. 1, 2012 and discarded
Information | |
---|---|
jzambrano | |
Review Board | |
master | |
Reviewers | |
reviewboard | |
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? |
|
|
Please combine these too. |
|
|
This change isn't correct--we already were hiding it using CSS. We don't want it to be present in the case … |
|
|
You removed the spaces inside the {%. Please don't |
|
|
Please put the spaces back before the else. |
|
|
There's no reason for this change. |
|
|
This still isn't correct. It's still including the draft banner in the page for users who should never be able … |
|
|
What happens if you pass this success function to gReviewRequest.close on line ~2619 in reviews.js, where we attach the click … |
|
|
This semicolon is breaking reviews.js - you need to remove it. |
|
|
This isn't needed. Keep it as it was. You only really care about hasOwnProperty when you want to be sure … |
|
|
You need to put the other id "#discard-review-request-link" back into this query. |
|
|
Space before "{" |
|
|
Should use gDraftBanner. |
|
|
It'd be nice to have a variable at the top like gDraftBanner for this. |
|
|
This seems wrong. Note that there's two conditions for showing this form: 1) It has a pending draft. 2) The … |
|
|
The first conditional is checking against 'P', so I don't see how the newly added check against 'D' can ever … |
|
|
nits - space before the equals signs here. |
|
|
The style is a bit off here - should look like: gReviewRequest.publish({ buttons: gDraftBannerButtons, success: function() { gDraftBanner.hide(); } }); |
|
|
space before = |
|
|
Spaces after the ifs, and on either side of the =='s, and before the open braces. Also, I think you … |
|
|
In an effort to prepare for our eventual localization, we probably want to abstract the English text into some constants … |
|
|
So, instead of hiding each editicon, how about shutting down their editable-fieldness? This would mean changing inlineEditor so that it … |
|
|
Same as above. |
|
|
Same comments as above - only in this case, we'd be enabling the inlineEditor. |
|
|
Instead of changing the ID and adding this, let's just keep the old ID. |
|
|
What is this element? I can't find any elements with this ID anywhere... |
|
|
Not entirely sure this is necessary - see below. |
|
|
Why is this ID changing? |
|
|
Please remove this extra line. |
|
|
Hm, so I'm afraid how how fragile this structure is for l10n. Not sure how important that is at this … |
|
|
This looks really, really wrong. That's three things named the same ID, with tiny variations. That's just plain confusing. If … |
|
|
Style is a bit off here. success: function() { gDraftBanner.hide(); } |
|
|
I think I'd feel better if we didn't depend so much on whether the banners were visible, and just checked … |
|
|
This is really confusing - why are we still creating a client-side timestamp here? Why can't we retrieve this information … |
|
|
This can be condensed to: $("span.status").text(newState ? newState : "updated"); |
|
|
Whoa - hold it...why are you passing "disable" as a string? I thought disable was a function that we'd call … |
|
|
Same as above. Also, this function looks *extremely* similar to the one on line 2503. We might want to DRY* … |
|
|
I really don't think this is how we want to be enabling and disabling inlineEditors... |
|
|
Why was this ID changed? |
|
|
Swap these. var should go first. Blank line after the var. |
|
|
loaded should be last. It signifies that we've finished loading. |
|
|
I believe this can possibly fetch fields that aren't on the review request. I think what you want is .review-request … |
|
|
I'm certain this doesn't work. You're never terminating the publish() command. That makes me think that this code path was … |
|
|
There's no way any of this was ever tested. You're using Django template tags in here, but we never go … |
|
|
Indentation problems. |
|
|
Should be alphabetical. |
|
|
Space after the "," |
|
|
You have indentation problems here. We only call this once, so I don't think it's worth having a function for … |
|
|
Same comments here. |
|
|
You can chain this: $('time.timesince') .attr('datetime', gReviewRequest.last_updated) .timesince(); |
|
|
We're not actually closing it. I'd call this "onReviewRequestClosed" instead, since it's really the handler after we've closed. |
|
|
Can you keep the queries grouped and the non-queries grouped? |
|
|
No need for ".editable" |
|
|
onReviewRequestReopened |
|
|
What's the "updated" all about? |
|
|
There can only be one element with a given ID. This query is not reliable. You may need to change … |
|
|
You're missing a ; |
|
|
Same comment as above. |
|
|
Space before "(" |
|
|
The ID is the most specific. No need to append the class names. Needs to wrap to < 80 chars. … |
|
|
I think what you really want is: $('.review-request .editable').reviewRequestFieldEditor(); |
|
|
Keep the blank line. |
|
|
You're defining this button ID multiple times, which won't work. You need to switch to classes and update call sites. |
|
|
Levels of indentation look wrong. The last endif should only have one space, and the first here should have two. … |
|
|
One space before "if" |
|
Change Summary:
-Fixed the indentation -Changed the logic of the template -Got rid of the trailing whitespace
Diff: |
Revision 2 (+33 -44) |
---|
-
-
-
-
reviewboard/templates/reviews/review_header.html (Diff revision 2) 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.
-
reviewboard/templates/reviews/review_header.html (Diff revision 2) You removed the spaces inside the {%. Please don't
-
reviewboard/templates/reviews/review_header.html (Diff revision 2) Please put the spaces back before the else.
-
reviewboard/templates/reviews/review_header.html (Diff revision 2) There's no reason for this change.
-
-
reviewboard/templates/reviews/review_header.html (Diff revision 3) 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).
Change Summary:
-Modified logic of draft-banner in review_header.html. -Modified handler for the case when review-request-discard button is clicked.
Diff: |
Revision 4 (+40 -40) |
---|
-
-
reviewboard/static/rb/js/reviews.js (Diff revision 4) 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?
-
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.
-
reviewboard/static/rb/js/reviews.js (Diff revision 4) This semicolon is breaking reviews.js - you need to remove it.
Change Summary:
- draft-banner and discard-banner now show/hide properly. - Changed the template to address points raised by David. Note: There's a server error message that appears and quickly goes away. It also disappers from the console so i don't get to see what is causing the error message.
Diff: |
Revision 5 (+71 -63) |
---|
Change Summary:
-Fixed server error message. -Page still reloading:success member inside the click handler does not prevent the page reload request in _apiCall()
Diff: |
Revision 6 (+64 -64) |
---|
Change Summary:
-Page reloading is successfully avoided for the discard-draft button. -I've also cleaned up review_header.html
Diff: |
Revision 7 (+42 -45) |
---|
Summary: |
|
||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
||||||||||||
Testing Done: |
|
Description: |
|
---|
-
Hey Jesus, This looks pretty good! Just one major point below, and a question. -Mike
-
-
reviewboard/static/rb/js/reviews.js (Diff revision 8) You need to put the other id "#discard-review-request-link" back into this query.
-
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.
-
reviewboard/static/rb/js/datastore.js (Diff revision 8) 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.
-
-
-
reviewboard/static/rb/js/reviews.js (Diff revision 8) It'd be nice to have a variable at the top like gDraftBanner for this.
-
reviewboard/templates/reviews/review_header.html (Diff revision 8) 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.
-
reviewboard/templates/reviews/review_header.html (Diff revision 8) The first conditional is checking against 'P', so I don't see how the newly added check against 'D' can ever be false.
Change Summary:
Modifications: -Addressed Christian's comments. -"close" option in the nav bar now hides when discard-review request is clicked. Current issues: -Discard-banner: The draft shows two pencil icons instead of one. Also, the "reopen for review" button is broken. Console does not capture anything when the button is clicked.
Diff: |
Revision 9 (+69 -54) |
---|
-
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
Change Summary:
Banners show/hide when appropriate. Next Step: -Resolve issue of multiple pencil icons showing on discard-banner and submitted banner. -Keep functionality the same as before: i.e: a user should lose the ability to edit the request's fields once it's discarded.
Testing Done: |
|
||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 10 (+91 -57) |
Change Summary:
-Hide editable fields when user discards a review request. -Review request status is updated accordingly. current issues: -Testing-done/description fields are still editable when request is discarded. -Two pencil icons on discard-banner/submitted-banner -Can't figure out how to check if a review request is a draft using RB.ReviewRequest
Summary: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 11 (+172 -64) |
-
-
-
reviewboard/static/rb/js/reviews.js (Diff revision 11) The style is a bit off here - should look like: gReviewRequest.publish({ buttons: gDraftBannerButtons, success: function() { gDraftBanner.hide(); } });
-
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?
-
-
reviewboard/static/rb/js/reviews.js (Diff revision 11) 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"); }
-
reviewboard/static/rb/js/reviews.js (Diff revision 11) 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.
-
reviewboard/static/rb/js/reviews.js (Diff revision 11) So, instead of hiding each editicon, how about shutting down their editable-fieldness? This would mean changing inlineEditor so that it can be disabled.
-
-
reviewboard/static/rb/js/reviews.js (Diff revision 11) Same comments as above - only in this case, we'd be enabling the inlineEditor.
-
reviewboard/static/rb/js/reviews.js (Diff revision 11) Instead of changing the ID and adding this, let's just keep the old ID.
-
reviewboard/static/rb/js/reviews.js (Diff revision 11) What is this element? I can't find any elements with this ID anywhere...
-
reviewboard/static/rb/js/reviews.js (Diff revision 11) Not entirely sure this is necessary - see below.
-
-
reviewboard/templates/reviews/review_header.html (Diff revision 11) Did I miss something? Why are we adding this new ability?
-
reviewboard/templates/reviews/review_request_actions_secondary.html (Diff revision 11) Please remove this extra line.
-
reviewboard/templates/reviews/review_request_box.html (Diff revision 11) 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?
Change Summary:
-Fixed issue of multiple pencil icons on the banners. Next Step: -Add a field to RB.ReviewRequest to check if a review request is public/draft.
Diff: |
Revision 12 (+175 -71) |
---|
Change Summary:
Changed reviews.js so all '.editable' fields are bound to the inlineEditor() and enabled/disabled when appropriate.
Diff: |
Revision 13 (+152 -72) |
---|
Change Summary:
-RB.ReviewRequest now pulls in data from the API about the status of a review request. -This solves the issue of showing draft-banner/no banner when a previosuly submitted review request is re-opened
Diff: |
Revision 14 (+151 -80) |
---|
-
-
reviewboard/static/rb/css/reviews.less (Diff revision 14) 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.
-
reviewboard/static/rb/js/reviews.js (Diff revision 14) Style is a bit off here. success: function() { gDraftBanner.hide(); }
-
reviewboard/static/rb/js/reviews.js (Diff revision 14) 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.
-
reviewboard/static/rb/js/reviews.js (Diff revision 14) 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?
-
reviewboard/static/rb/js/reviews.js (Diff revision 14) This can be condensed to: $("span.status").text(newState ? newState : "updated");
-
reviewboard/static/rb/js/reviews.js (Diff revision 14) Whoa - hold it...why are you passing "disable" as a string? I thought disable was a function that we'd call on inlineEditor widgets.
-
reviewboard/static/rb/js/reviews.js (Diff revision 14) 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.
-
reviewboard/static/rb/js/reviews.js (Diff revision 14) I really don't think this is how we want to be enabling and disabling inlineEditors...
-
Change Summary:
Addressed issues from previous review request: -Better IDs for elements that take care of changing description on the banners. -Avoid code repetition.
Diff: |
Revision 15 (+162 -78) |
---|
Change Summary:
-Original data from the review request is pulled when a draft review request is discarded/submitted. -Avoid code repetition. TODO: -More thorough testing. -A banner at the bottom of the page should appear when a review request is updated(discarded/changed/submitted etc).
Diff: |
Revision 16 (+177 -83) |
---|
Change Summary:
TODO: -Display draft-banner properly when a public review request is modified needs work still. -More thorough testing. -A banner at the bottom of the page should appear when a review request is updated(discarded/changed/submitted etc).
Summary: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 17 (+186 -84) |
-
-
reviewboard/static/rb/js/datastore.js (Diff revision 17) Swap these. var should go first. Blank line after the var.
-
reviewboard/static/rb/js/datastore.js (Diff revision 17) loaded should be last. It signifies that we've finished loading.
-
reviewboard/static/rb/js/reviews.js (Diff revision 17) I believe this can possibly fetch fields that aren't on the review request. I think what you want is .review-request .main
-
reviewboard/static/rb/js/reviews.js (Diff revision 17) 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.
-
reviewboard/static/rb/js/reviews.js (Diff revision 17) 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.
-
-
-
-
reviewboard/static/rb/js/reviews.js (Diff revision 17) 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.
-
-
reviewboard/static/rb/js/reviews.js (Diff revision 17) You can chain this: $('time.timesince') .attr('datetime', gReviewRequest.last_updated) .timesince();
-
reviewboard/static/rb/js/reviews.js (Diff revision 17) We're not actually closing it. I'd call this "onReviewRequestClosed" instead, since it's really the handler after we've closed.
-
reviewboard/static/rb/js/reviews.js (Diff revision 17) Can you keep the queries grouped and the non-queries grouped?
-
-
-
-
reviewboard/static/rb/js/reviews.js (Diff revision 17) 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.
-
-
-
-
reviewboard/static/rb/js/reviews.js (Diff revision 17) The ID is the most specific. No need to append the class names. Needs to wrap to < 80 chars. Do: $('#submitted-description') .reviewCloseCommentEditor(...);
-
reviewboard/static/rb/js/reviews.js (Diff revision 17) I think what you really want is: $('.review-request .editable').reviewRequestFieldEditor();
-
-
reviewboard/templates/reviews/review_header.html (Diff revision 17) You're defining this button ID multiple times, which won't work. You need to switch to classes and update call sites.
-
reviewboard/templates/reviews/review_header.html (Diff revision 17) 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.
-
reviewboard/templates/reviews/review_request_actions_secondary.html (Diff revision 17) One space before "if"