Frontend changes for new `publish_and_archive` profile setting.

Review Request #14053 — Created July 24, 2024 and updated

Information

Review Board
master

Reviewers

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
Created a new class to standardize the publish buttons across ReviewBoard.
Currently, there are 2 different versions of the publish review button in use. First, there is `UnifiedBannerView`, in which the publish button has its own class and includes dropdown checkboxes for archiving and sending emails. The second, in `ReviewDialogView`, the publish button is an html `MenuButtonView` that has dropdown buttons that are NOT checkboxes, which can publish and archive, or publish and send email, but not both. This new class will standardize the structure of the publish button, make it easily moddable, and make each publish button have checkboxes.
c54c3e15db74152743160aa3f4d81a0d0b6d9aff Daniel
Introduced new publishButton to reviewDialogView to add dropdown checkboxes.
These changes included making a new subclass of `PublishButtonView` called `RDPublishButtonView`, which obtains the reviewRequestEditor differently due to different models being passed into the `ReviewDialogView` and triggers a different function in `_action()` due to the previous publishing structure in `ReviewDialogView`. This new publish button replaces the old publish button, which did not have checkboxes, only action buttons. This change also meant that the arguments passed into `_saveReview()` could no longer be optional when saving and discarding.
63a91f0ce9e59ca0de6f79efa7208c1d4855d767 Daniel
Created Jasmine Tests.
This commit has the new Jasmine Unit Tests for the PublishButtonView. They test each aspect of this new class, from appearance, to checkbox availablility, to actions.
ab1af0d8e307ff51675cb8cc630171798a7ce2a0 Daniel
Description From Last Updated

The lines in the description are too long (need to be less than 79 chars).

maubinmaubin

This doesn't seem like it should always be True, we should be grabbing the saved value from the user's profile. …

maubinmaubin

Add a blank line after this one.

maubinmaubin

I think the spacing here should stay the way it was before.

maubinmaubin

The spacing here should stay the way it was before.

maubinmaubin

The spacing here should stay the way it was before.

maubinmaubin

The spacing here should stay the way it was before. Theres a few other instances of this in this file, …

maubinmaubin

Missing a trailing colon.

maubinmaubin

Add a blank line after this one.

maubinmaubin

do not assign a lambda expression, use a def Column: 13 Error code: E731

reviewbotreviewbot

do not assign a lambda expression, use a def Column: 13 Error code: E731

reviewbotreviewbot

do not assign a lambda expression, use a def Column: 13 Error code: E731

reviewbotreviewbot

Since you're not longer adding anything to here, let's revert these particular changes. Sorting this alphabetically is something that's still …

daviddavid

We need a "Version Added" here, even if it's still labeled as TODO

daviddavid

Add a blank line in between here.

daviddavid

Move this up to be just below the @beanbag/spina import.

daviddavid

Unindent this 4 spaces.

daviddavid

Let's type this as EventsHash. You can import that as type EventsHash inside the @beanbag/spina block.

daviddavid

We can remove the type for this. If _getReviewRequestEditor is properly typed, it'll be inferred.

daviddavid

Having "show" in the names for these doesn't make sense.

daviddavid

We don't need the let definition above and can just define this as const here.

daviddavid

This needs a doc comment.

daviddavid

This should be typed as returning ReviewRequestEditor. You can import that as: import { type ReviewRequestEditor, } from '../models/reviewRequestEditorModel';

daviddavid

Looks like some automatic formatting messed this up.

daviddavid

Looks like some automatic formatting messed this up.

daviddavid

Looks like some automatic formatting messed this up.

daviddavid

Looks like some automatic formatting messed this up.

daviddavid

Looks like some automatic formatting messed this up.

daviddavid

Can we call this ReviewDialogPublishButtonView? Explicit is generally better, even when it feels verbose.

daviddavid

We should also change the typing here for ReviewRequestEditor

daviddavid

The formatting changes here aren't wrong, but it's just code movement for no reason. Please undo this.

daviddavid

Looks like some automatic formatting messed this up.

daviddavid

Looks like some automatic formatting messed this up.

daviddavid

Looks like some automatic formatting messed this up.

daviddavid

If we keep options optional in _saveReview then we don't need this change.

daviddavid

If we keep options optional in _saveReview then we don't need this change.

daviddavid

Let's keep the options structure optional here.

daviddavid

No blank line here.

daviddavid

Please add types for these.

daviddavid

No blank line here.

daviddavid

No blank line here.

daviddavid

No blank line here.

daviddavid

Add another blank line here.

daviddavid

Can we call this BannerPublishButtonView?

daviddavid

Undo this formatting change.

daviddavid

Undo this formatting change.

daviddavid

Undo this formatting change.

daviddavid

Undo this formatting change.

daviddavid

Undo this formatting change.

daviddavid

This needs to be defined in the ReviewRequestEditorAttrs interface too.

daviddavid

Leftover debug code.

daviddavid

This can just be "Return the review request editor."

daviddavid

This can probably just say "Trigger the publish operation."

daviddavid

These should start with lower case letters.

daviddavid

Because it spans multiple lines, let's wrap the whole value in parens: trivial: (sendEmailItem !== null && !sendEmailItem.get('checked'))

daviddavid

If we change the publish button to specify the model as ReviewRequestEditor I think this doesn't need to be imported …

daviddavid

This should use PublishButtonView<ReviewRequestEditor>

daviddavid

Let's just say "Return the review request editor."

daviddavid

Change this to say ReviewRequestEditor.

daviddavid

Same here with parens.

daviddavid

Please undo this change.

daviddavid

Please undo this change (you removed two spaces of indentation on this line).

daviddavid

What's left to do for these commented-out lines?

daviddavid

We sort alphabetically based on filename, so this should go just above the import of type ReviewRequestEditorView.

daviddavid

This needs a doc comment.

daviddavid

Ping on this TODO

maubinmaubin

We should add some extra info to the description to let people know this needs to be implemented by the …

maubinmaubin

Nit: Use "Return" instead of "Returns".

maubinmaubin

Nit: Use "Call" instead of "Calls".

maubinmaubin

While you're here, can you update the docs to include an "Option Args" section. You can see examples if you …

maubinmaubin

To be more specific about what differentiates this test from the previous one and what the purpose is, you could …

maubinmaubin

This can stay at the indentation level that it was at before.

maubinmaubin
maubin
  1. 
      
  2. Show all issues

    The lines in the description are too long (need to be less than 79 chars).

  3. reviewboard/reviews/context.py (Diff revision 1)
     
     
    Show all issues

    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 doing request.user.get_profile() and then grab the publish_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 of request.user with this new user variable. Calling the variable is less expensive than accessing the user property on the request.

    1. This setting is what decides whether the archvie option will show up as an option, similar to how the email option is enabled by an admin setting. These system wide settings are accessed thorugh reviewRequestEditor.get('showSendEmail') and reviewRequestEditor.get('showPublishAndArchive') in the PublishButtonView, while the user profile settings for archiving are accessed through UserSession.instance.get('publishAndArchive').
      Currently, there is no admin setting for enabling or disabling arching review requests, but in case one is added in the future, I implemented it similar to showSendEmail, just with its value always set to true.

    2. I don't think this is something that's valuable for an admin to be able to turn off. If for some reason we decide to do that in the future, we can add the plumbing for it. It's generally better to keep things as simple as possible to start, and then later on if we need to reevaluate, we can do so with more information.

    3. So I should just have the archive option always show?

    4. Yes.

  4. Show all issues

    Add a blank line after this one.

  5. Show all issues

    I think the spacing here should stay the way it was before.

  6. Show all issues

    The spacing here should stay the way it was before.

  7. Show all issues

    The spacing here should stay the way it was before.

  8. Show all issues

    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.

  9. Show all issues

    Missing a trailing colon.

  10. Show all issues

    Add a blank line after this one.

  11. 
      
dan.casares
Review request changed
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
Created a new class to standardize the publish buttons across ReviewBoard.
Currently, there are 2 different versions of the publish review button in use. First, there is `UnifiedBannerView`, in which the publish button has its own class and includes dropdown checkboxes for archiving and sending emails. The second, in `ReviewDialogView`, the publish button is an html `MenuButtonView` that has dropdown buttons that are NOT checkboxes, which can publish and archive, or publish and send email, but not both. This new class will standardize the structure of the publish button, make it easily moddable, and make each publish button have checkboxes.
b59391a5c242f1518dc97846d7c22c2c2d9b8fab Daniel
Introduced new publishButton to reviewDialogView to add dropdown checkboxes.
These changes included making a new subclass of `PublishButtonView` called `RDPublishButtonView`, which obtains the reviewRequestEditor differently due to different models being passed into the `ReviewDialogView` and triggers a different function in `_action()` due to the previous publishing structure in `ReviewDialogView`. This new publish button replaces the old publish button, which did not have checkboxes, only action buttons. This change also meant that the arguments passed into `_saveReview()` could no longer be optional when saving and discarding.
2a60079a790133bbdd2bb52777084af8af0834c9 Daniel
Created a new class to standardize the publish buttons across ReviewBoard.
Currently, there are 2 different versions of the publish review button in use. First, there is `UnifiedBannerView`, in which the publish button has its own class and includes dropdown checkboxes for archiving and sending emails. The second, in `ReviewDialogView`, the publish button is an html `MenuButtonView` that has dropdown buttons that are NOT checkboxes, which can publish and archive, or publish and send email, but not both. This new class will standardize the structure of the publish button, make it easily moddable, and make each publish button have checkboxes.
695dffc66af9ba31cff24a64d8931695a0a66c63 Daniel
Introduced new publishButton to reviewDialogView to add dropdown checkboxes.
These changes included making a new subclass of `PublishButtonView` called `RDPublishButtonView`, which obtains the reviewRequestEditor differently due to different models being passed into the `ReviewDialogView` and triggers a different function in `_action()` due to the previous publishing structure in `ReviewDialogView`. This new publish button replaces the old publish button, which did not have checkboxes, only action buttons. This change also meant that the arguments passed into `_saveReview()` could no longer be optional when saving and discarding.
1e28c07979386e5acc5ef7ed58ee9e66e2e296ba Daniel
Created Jasmine Tests.
This commit has the new Jasmine Unit Tests for the PublishButtonView. They test each aspect of this new class, from appearance, to checkbox availablility, to actions.
9063b7cc02f95130c2183ad89477c4d901bd45cf Daniel

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

dan.casares
dan.casares
dan.casares
dan.casares
david
  1. 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.

  2. reviewboard/reviews/context.py (Diff revision 5)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    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.

  3. Show all issues

    We need a "Version Added" here, even if it's still labeled as TODO

  4. Show all issues

    Add a blank line in between here.

  5. Show all issues

    Move this up to be just below the @beanbag/spina import.

  6. Show all issues

    Unindent this 4 spaces.

  7. Show all issues

    Let's type this as EventsHash. You can import that as type EventsHash inside the @beanbag/spina block.

  8. Show all issues

    We can remove the type for this. If _getReviewRequestEditor is properly typed, it'll be inferred.

  9. Show all issues

    Having "show" in the names for these doesn't make sense.

  10. Show all issues

    We don't need the let definition above and can just define this as const here.

  11. Show all issues

    This needs a doc comment.

  12. Show all issues

    This should be typed as returning ReviewRequestEditor. You can import that as:

    import {
        type ReviewRequestEditor,
    } from '../models/reviewRequestEditorModel';
    
  13. Show all issues

    Looks like some automatic formatting messed this up.

  14. reviewboard/static/rb/js/reviews/views/reviewDialogView.ts (Diff revision 5)
     
     
     
     
     
     
     
    Show all issues

    Looks like some automatic formatting messed this up.

  15. Show all issues

    Looks like some automatic formatting messed this up.

  16. reviewboard/static/rb/js/reviews/views/reviewDialogView.ts (Diff revision 5)
     
     
     
     
     
     
     
    Show all issues

    Looks like some automatic formatting messed this up.

  17. Show all issues

    Looks like some automatic formatting messed this up.

  18. Show all issues

    Can we call this ReviewDialogPublishButtonView? Explicit is generally better, even when it feels verbose.

  19. Show all issues

    We should also change the typing here for ReviewRequestEditor

  20. reviewboard/static/rb/js/reviews/views/reviewDialogView.ts (Diff revision 5)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    The formatting changes here aren't wrong, but it's just code movement for no reason. Please undo this.

  21. reviewboard/static/rb/js/reviews/views/reviewDialogView.ts (Diff revision 5)
     
     
     
     
     
     
     
    Show all issues

    Looks like some automatic formatting messed this up.

  22. reviewboard/static/rb/js/reviews/views/reviewDialogView.ts (Diff revision 5)
     
     
     
     
     
     
     
    Show all issues

    Looks like some automatic formatting messed this up.

  23. reviewboard/static/rb/js/reviews/views/reviewDialogView.ts (Diff revision 5)
     
     
     
     
     
     
     
    Show all issues

    Looks like some automatic formatting messed this up.

  24. Show all issues

    If we keep options optional in _saveReview then we don't need this change.

  25. Show all issues

    If we keep options optional in _saveReview then we don't need this change.

  26. Show all issues

    Let's keep the options structure optional here.

  27. Show all issues

    No blank line here.

  28. Show all issues

    Please add types for these.

  29. Show all issues

    No blank line here.

  30. Show all issues

    No blank line here.

  31. Show all issues

    No blank line here.

  32. Show all issues

    Add another blank line here.

  33. Show all issues

    Can we call this BannerPublishButtonView?

  34. Show all issues

    Undo this formatting change.

  35. Show all issues

    Undo this formatting change.

  36. Show all issues

    Undo this formatting change.

  37. Show all issues

    Undo this formatting change.

  38. reviewboard/static/rb/js/reviews/views/unifiedBannerView.ts (Diff revision 5)
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Undo this formatting change.

  39. 
      
dan.casares
david
  1. 
      
  2. Show all issues

    This needs to be defined in the ReviewRequestEditorAttrs interface too.

    1. This actually needed to be removed, since publishAndArchvie is always shown.

  3. Show all issues

    Leftover debug code.

  4. Show all issues

    This can just be "Return the review request editor."

  5. Show all issues

    This can probably just say "Trigger the publish operation."

  6. Show all issues

    These should start with lower case letters.

  7. Show all issues

    Because it spans multiple lines, let's wrap the whole value in parens:

    trivial: (sendEmailItem !== null &&
              !sendEmailItem.get('checked'))
    
  8. Show all issues

    If we change the publish button to specify the model as ReviewRequestEditor I think this doesn't need to be imported anymore.

  9. Show all issues

    This should use PublishButtonView<ReviewRequestEditor>

  10. Show all issues

    Let's just say "Return the review request editor."

  11. Show all issues

    Change this to say ReviewRequestEditor.

  12. Show all issues

    Same here with parens.

  13. Show all issues

    Please undo this change.

  14. Show all issues

    Please undo this change (you removed two spaces of indentation on this line).

  15. reviewboard/static/rb/js/reviews/views/tests/publishButtonViewTests.ts (Diff revision 6)
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    What's left to do for these commented-out lines?

  16. Show all issues

    We sort alphabetically based on filename, so this should go just above the import of type ReviewRequestEditorView.

  17. Show all issues

    This needs a doc comment.

  18. 
      
dan.casares
Review request changed
Commits:
Summary ID Author
Created a new class to standardize the publish buttons across ReviewBoard.
Currently, there are 2 different versions of the publish review button in use. First, there is `UnifiedBannerView`, in which the publish button has its own class and includes dropdown checkboxes for archiving and sending emails. The second, in `ReviewDialogView`, the publish button is an html `MenuButtonView` that has dropdown buttons that are NOT checkboxes, which can publish and archive, or publish and send email, but not both. This new class will standardize the structure of the publish button, make it easily moddable, and make each publish button have checkboxes.
f08c75b174780bdf5ffc6448ab021bf5faffdfb1 Daniel
Introduced new publishButton to reviewDialogView to add dropdown checkboxes.
These changes included making a new subclass of `PublishButtonView` called `RDPublishButtonView`, which obtains the reviewRequestEditor differently due to different models being passed into the `ReviewDialogView` and triggers a different function in `_action()` due to the previous publishing structure in `ReviewDialogView`. This new publish button replaces the old publish button, which did not have checkboxes, only action buttons. This change also meant that the arguments passed into `_saveReview()` could no longer be optional when saving and discarding.
55ed50f13cbac858a41b81a0a133e1c709a20e89 Daniel
Created Jasmine Tests.
This commit has the new Jasmine Unit Tests for the PublishButtonView. They test each aspect of this new class, from appearance, to checkbox availablility, to actions.
f84b102fd15a42fdd581740ad1d82c87d7cba094 Daniel
Created a new class to standardize the publish buttons across ReviewBoard.
Currently, there are 2 different versions of the publish review button in use. First, there is `UnifiedBannerView`, in which the publish button has its own class and includes dropdown checkboxes for archiving and sending emails. The second, in `ReviewDialogView`, the publish button is an html `MenuButtonView` that has dropdown buttons that are NOT checkboxes, which can publish and archive, or publish and send email, but not both. This new class will standardize the structure of the publish button, make it easily moddable, and make each publish button have checkboxes.
c54c3e15db74152743160aa3f4d81a0d0b6d9aff Daniel
Introduced new publishButton to reviewDialogView to add dropdown checkboxes.
These changes included making a new subclass of `PublishButtonView` called `RDPublishButtonView`, which obtains the reviewRequestEditor differently due to different models being passed into the `ReviewDialogView` and triggers a different function in `_action()` due to the previous publishing structure in `ReviewDialogView`. This new publish button replaces the old publish button, which did not have checkboxes, only action buttons. This change also meant that the arguments passed into `_saveReview()` could no longer be optional when saving and discarding.
63a91f0ce9e59ca0de6f79efa7208c1d4855d767 Daniel
Created Jasmine Tests.
This commit has the new Jasmine Unit Tests for the PublishButtonView. They test each aspect of this new class, from appearance, to checkbox availablility, to actions.
ab1af0d8e307ff51675cb8cc630171798a7ce2a0 Daniel

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
maubin
  1. 
      
  2. Show all issues

    Ping on this TODO

  3. Show all issues

    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.
     */
    
  4. Show all issues

    Nit: Use "Return" instead of "Returns".

  5. Show all issues

    Nit: Use "Call" instead of "Calls".

  6. Show all issues

    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.

  7. Show all issues

    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"

  8. Show all issues

    This can stay at the indentation level that it was at before.

  9.