Frontend changes for new `publish_and_archive` profile setting.
Review Request #14053 — Created July 24, 2024 and updated
When publishing reviews, we have an option for archiving the review request
after the review gets published.These changes are part of a new profile setting which will allow user to have
their review requests automatically archived instead of having to click the
“… and archive the review request” button every time.This means created a new class to standardize the different implementations
of the publish button and connecting it to backend changes to access the
publish_and_archive
user setting.
New Jasmine tests were added to test
publishButtonView
functionality, checkboxes, and click actions.
Whole reviewboard test suite was run successfully.
Manually testing demonstrated that the changes looked and worked as intended.
Summary | ID | Author |
---|---|---|
c54c3e15db74152743160aa3f4d81a0d0b6d9aff | Daniel | |
63a91f0ce9e59ca0de6f79efa7208c1d4855d767 | Daniel | |
ab1af0d8e307ff51675cb8cc630171798a7ce2a0 | Daniel |
Description | From | Last Updated |
---|---|---|
The lines in the description are too long (need to be less than 79 chars). |
maubin | |
This doesn't seem like it should always be True, we should be grabbing the saved value from the user's profile. … |
maubin | |
Add a blank line after this one. |
maubin | |
I think the spacing here should stay the way it was before. |
maubin | |
The spacing here should stay the way it was before. |
maubin | |
The spacing here should stay the way it was before. |
maubin | |
The spacing here should stay the way it was before. Theres a few other instances of this in this file, … |
maubin | |
Missing a trailing colon. |
maubin | |
Add a blank line after this one. |
maubin | |
do not assign a lambda expression, use a def Column: 13 Error code: E731 |
reviewbot | |
do not assign a lambda expression, use a def Column: 13 Error code: E731 |
reviewbot | |
do not assign a lambda expression, use a def Column: 13 Error code: E731 |
reviewbot | |
Since you're not longer adding anything to here, let's revert these particular changes. Sorting this alphabetically is something that's still … |
david | |
We need a "Version Added" here, even if it's still labeled as TODO |
david | |
Add a blank line in between here. |
david | |
Move this up to be just below the @beanbag/spina import. |
david | |
Unindent this 4 spaces. |
david | |
Let's type this as EventsHash. You can import that as type EventsHash inside the @beanbag/spina block. |
david | |
We can remove the type for this. If _getReviewRequestEditor is properly typed, it'll be inferred. |
david | |
Having "show" in the names for these doesn't make sense. |
david | |
We don't need the let definition above and can just define this as const here. |
david | |
This needs a doc comment. |
david | |
This should be typed as returning ReviewRequestEditor. You can import that as: import { type ReviewRequestEditor, } from '../models/reviewRequestEditorModel'; |
david | |
Looks like some automatic formatting messed this up. |
david | |
Looks like some automatic formatting messed this up. |
david | |
Looks like some automatic formatting messed this up. |
david | |
Looks like some automatic formatting messed this up. |
david | |
Looks like some automatic formatting messed this up. |
david | |
Can we call this ReviewDialogPublishButtonView? Explicit is generally better, even when it feels verbose. |
david | |
We should also change the typing here for ReviewRequestEditor |
david | |
The formatting changes here aren't wrong, but it's just code movement for no reason. Please undo this. |
david | |
Looks like some automatic formatting messed this up. |
david | |
Looks like some automatic formatting messed this up. |
david | |
Looks like some automatic formatting messed this up. |
david | |
If we keep options optional in _saveReview then we don't need this change. |
david | |
If we keep options optional in _saveReview then we don't need this change. |
david | |
Let's keep the options structure optional here. |
david | |
No blank line here. |
david | |
Please add types for these. |
david | |
No blank line here. |
david | |
No blank line here. |
david | |
No blank line here. |
david | |
Add another blank line here. |
david | |
Can we call this BannerPublishButtonView? |
david | |
Undo this formatting change. |
david | |
Undo this formatting change. |
david | |
Undo this formatting change. |
david | |
Undo this formatting change. |
david | |
Undo this formatting change. |
david | |
This needs to be defined in the ReviewRequestEditorAttrs interface too. |
david | |
Leftover debug code. |
david | |
This can just be "Return the review request editor." |
david | |
This can probably just say "Trigger the publish operation." |
david | |
These should start with lower case letters. |
david | |
Because it spans multiple lines, let's wrap the whole value in parens: trivial: (sendEmailItem !== null && !sendEmailItem.get('checked')) |
david | |
If we change the publish button to specify the model as ReviewRequestEditor I think this doesn't need to be imported … |
david | |
This should use PublishButtonView<ReviewRequestEditor> |
david | |
Let's just say "Return the review request editor." |
david | |
Change this to say ReviewRequestEditor. |
david | |
Same here with parens. |
david | |
Please undo this change. |
david | |
Please undo this change (you removed two spaces of indentation on this line). |
david | |
What's left to do for these commented-out lines? |
david | |
We sort alphabetically based on filename, so this should go just above the import of type ReviewRequestEditorView. |
david | |
This needs a doc comment. |
david | |
Ping on this TODO |
maubin | |
We should add some extra info to the description to let people know this needs to be implemented by the … |
maubin | |
Nit: Use "Return" instead of "Returns". |
maubin | |
Nit: Use "Call" instead of "Calls". |
maubin | |
While you're here, can you update the docs to include an "Option Args" section. You can see examples if you … |
maubin | |
To be more specific about what differentiates this test from the previous one and what the purpose is, you could … |
maubin | |
This can stay at the indentation level that it was at before. |
maubin |
-
-
-
This doesn't seem like it should always be
True
, we should be grabbing the saved value from the user's profile. You can get the user's profile by doingrequest.user.get_profile()
and then grab thepublish_and_archive
setting from the profile.Also, since we access
request.user
multiple times in this function, its a good idea to pull this out into a variable (user = request.user
) and replace any instances ofrequest.user
with this newuser
variable. Calling the variable is less expensive than accessing theuser
property on therequest
. -
-
-
-
-
The spacing here should stay the way it was before. Theres a few other instances of this in this file, they can all be changed back to the way it was before.
-
-
- Description:
-
~ When publishing reviews, we have an option for archiving the review request after the review gets published.
~ When publishing reviews, we have an option for archiving the review request
+ after the review gets published. ~ These changes are part of a new profile setting which will allow user to have their review requests automatically archived instead of having to click the “… and archive the review request” button every time.
~ These changes are part of a new profile setting which will allow user to have
+ their review requests automatically archived instead of having to click the + “… and archive the review request” button every time. ~ This means created a new class to standardize the different implementations of the publish button and connecting it to backend changes to access the
publish_and_archive
user setting.~ This means created a new class to standardize the different implementations
+ of the publish button and connecting it to backend changes to access the + publish_and_archive
user setting. - Commits:
-
Summary ID Author b59391a5c242f1518dc97846d7c22c2c2d9b8fab Daniel 2a60079a790133bbdd2bb52777084af8af0834c9 Daniel 695dffc66af9ba31cff24a64d8931695a0a66c63 Daniel 1e28c07979386e5acc5ef7ed58ee9e66e2e296ba Daniel 9063b7cc02f95130c2183ad89477c4d901bd45cf Daniel
- Change Summary:
-
Properly rebased with master branch.
- Commits:
-
Summary ID Author 695dffc66af9ba31cff24a64d8931695a0a66c63 Daniel 1e28c07979386e5acc5ef7ed58ee9e66e2e296ba Daniel 9063b7cc02f95130c2183ad89477c4d901bd45cf Daniel d4a8f4fff3d7df30759c88ffc160590072da2ff6 Daniel 638fe6254cc6a658f1e5738a8fd34e71f9dc9476 Daniel 2d9b716bf66aaadf4a6b5843110610d75cd0ba92 Daniel - Diff:
-
Revision 3 (+1026 -294)
Checks run (2 succeeded)
- Commits:
-
Summary ID Author d4a8f4fff3d7df30759c88ffc160590072da2ff6 Daniel 638fe6254cc6a658f1e5738a8fd34e71f9dc9476 Daniel 2d9b716bf66aaadf4a6b5843110610d75cd0ba92 Daniel a387bdf247988d6692be9e92e2d41cfba6bf380e Daniel 29e3b4ce118d24d23a88479a42a7de946d916f22 Daniel 0c3cee74dd14c9cd4c8d972cf74e5102cd81797b Daniel - Diff:
-
Revision 4 (+2517 -5267)
Checks run (2 succeeded)
- Change Summary:
-
Rebased properly with master
- Commits:
-
Summary ID Author a387bdf247988d6692be9e92e2d41cfba6bf380e Daniel 29e3b4ce118d24d23a88479a42a7de946d916f22 Daniel 0c3cee74dd14c9cd4c8d972cf74e5102cd81797b Daniel f08c75b174780bdf5ffc6448ab021bf5faffdfb1 Daniel c8ebeeda5d32e21f7a1ed1b1d5e359981dbbb978 Daniel a74e0c9b52c9ed33f3b3122b72b9cf11c7cd362d Daniel - Diff:
-
Revision 5 (+1025 -309)
Checks run (2 succeeded)
- Testing Done:
-
~ No new tests were added so far.
~ Whole reviewboard test suite was run and no new tests failed. ~ New Jasmine tests were added to test
publishButtonView
functionality, checkboxes, and click actions.~ Whole reviewboard test suite was run successfully. Manually testing demonstrated that the changes looked and worked as intended.
-
This is generally looking pretty good. There are a bunch of unrelated formatting changes that slipped in which do not conform to our coding style, which seems like maybe your editor is trying to do some auto-formatting.
-
Since you're not longer adding anything to here, let's revert these particular changes. Sorting this alphabetically is something that's still valuable, but it doesn't belong in this change anymore.
-
-
-
-
-
Let's type this as
EventsHash
. You can import that astype EventsHash
inside the@beanbag/spina
block. -
-
-
-
-
This should be typed as returning
ReviewRequestEditor
. You can import that as:import { type ReviewRequestEditor, } from '../models/reviewRequestEditorModel';
-
-
-
-
-
-
Can we call this
ReviewDialogPublishButtonView
? Explicit is generally better, even when it feels verbose. -
-
The formatting changes here aren't wrong, but it's just code movement for no reason. Please undo this.
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
- Change Summary:
-
Fixed auto-formatting issues, imports, and other issues highlighted by David.
- Commits:
-
Summary ID Author f08c75b174780bdf5ffc6448ab021bf5faffdfb1 Daniel c8ebeeda5d32e21f7a1ed1b1d5e359981dbbb978 Daniel a74e0c9b52c9ed33f3b3122b72b9cf11c7cd362d Daniel f08c75b174780bdf5ffc6448ab021bf5faffdfb1 Daniel 55ed50f13cbac858a41b81a0a133e1c709a20e89 Daniel f84b102fd15a42fdd581740ad1d82c87d7cba094 Daniel - Diff:
-
Revision 6 (+1001 -281)
Checks run (2 succeeded)
-
-
-
-
-
-
-
Because it spans multiple lines, let's wrap the whole value in parens:
trivial: (sendEmailItem !== null && !sendEmailItem.get('checked'))
-
If we change the publish button to specify the model as
ReviewRequestEditor
I think this doesn't need to be imported anymore. -
-
-
-
-
-
-
-
We sort alphabetically based on filename, so this should go just above the import of
type ReviewRequestEditorView
. -
- Commits:
-
Summary ID Author f08c75b174780bdf5ffc6448ab021bf5faffdfb1 Daniel 55ed50f13cbac858a41b81a0a133e1c709a20e89 Daniel f84b102fd15a42fdd581740ad1d82c87d7cba094 Daniel c54c3e15db74152743160aa3f4d81a0d0b6d9aff Daniel 63a91f0ce9e59ca0de6f79efa7208c1d4855d767 Daniel ab1af0d8e307ff51675cb8cc630171798a7ce2a0 Daniel - Diff:
-
Revision 7 (+952 -276)
Checks run (2 succeeded)
-
-
-
We should add some extra info to the description to let people know this needs to be implemented by the subclass:
/** * Update the state of the publish button. * * This should be implemented by subclasses. */
-
-
-
While you're here, can you update the docs to include an "Option Args" section. You can see examples if you search for "Option Args" in the codebase.
-
To be more specific about what differentiates this test from the previous one and what the purpose is, you could say "should trigger the publish event handler when publishAndArchive is false". And above you can say "should trigger the publish event handler when publishAndArchive is true"
-