Added backend work for the archive profile setting

Review Request #14003 — Created June 28, 2024 and updated

Information

Review Board
master

Reviewers

Began backend implementation for a profile setting that will determine whether
to automatically archive a review request when posted.
This included adding a publish_and_archive BooleanField to the
AccountSettingsForm in reviewboard/accounts/pages.py, which is initially set
to False. publish_and_archive was also added to the Profile in
reviewboard/accounts/models.py. Lastly, publish_and_archive was also added to
reviewboard/accounts/accounts.py in the js_user_session_info() function.

A series of tests were also implmeneted to ensure that the AccountSettingsForm
was initialized, loaded, and saved properly and that it correctly identified
invalid inputs. These tests were added to
reviewboard/accounts/test/test_account_settings_form.py.

All previous passing pytests still passed.

Summary ID Author
Backend for a user setting for Archive Review Request behavior when publishing.
This change introduce a new user setting called `publish_and_archive`, which is add to each user account and model. It also adds a new `BooleanField` option to the profile settings page to enable this feature.
5dc5fee06536328e6bdbbcc172f823d27dccad01 Daniel
Replace `publish_and_archive` `BooleanField` to not alter database.
Any additional `BooleanFields` in `models.py` automatically add that field to the database, which we want to avoid. Instead, a new `@property`, similar to `ui_theme_id`, is now used to handle `publish_and_archive`.
7cbadcf1e02637f75507d76265ccf75f12e47c17 Daniel
Updated AccountSettingsForm unit tests to account for `publish_and_archive`.
Introduced a new suite of unit tests for each feature added to the model. These serve to check that they are initialized, saved, and loaded properly. Tests to check for invalid or missing data were also included. Currently, `timezone` is the only required feature that can also be invalid.
5998c75acbd440e7221729ac31f86398dfcee0c4 Daniel
Description From Last Updated

In the description on your review request (which will get turned into the commit message that's stored for all time), …

daviddavid

You can also change any "Version Added"s that you have from 7.0 to TODO, like you did in your frontend …

maubinmaubin

trailing whitespace Column: 64 Error code: W291

reviewbotreviewbot

This looks like it can all fit on one line and still be less than 79 chars.

maubinmaubin

Lets be more specific here and say "review requests" instead of "new posts".

maubinmaubin

from djblets.siteconfig.models import SiteConfiguration should be placed on the line before from djblets.testing.decorators import add_fixtures. Imports in the same group …

maubinmaubin

These need to be moved to the same import group as the other django import on line 5. Files have …

maubinmaubin

This needs a docstring. It can be as simple as """Setup for a unit test."""

maubinmaubin

This needs an Args: section in the docs and a Returns: section, as well as typing.

maubinmaubin

Missing a -> None:

maubinmaubin

Leftover code that should've been deleted?

maubinmaubin

This is a nit, but we like to sort the keys alphabetically. You could do that here and for the …

maubinmaubin

Add a blank line before the comment.

maubinmaubin

blank line contains whitespace Column: 1 Error code: W293

reviewbotreviewbot

While you're here, would you mind sorting the keys in alphabetical order?

daviddavid

Looks like a leftover from when you had this as a DB field.

daviddavid

The first line of the docstring needs to fit on one line here. You can add an additional paragraph after …

daviddavid

Same here.

daviddavid

The variable definitions and setUp() method should be at the top of the class, not under any test methods.

daviddavid

Please add doc comments for these.

daviddavid

This method should call super.setUp() as its first line.

daviddavid

This looks unused, and even if you did need to do something with siteconfig, you shouldn't need to create your …

daviddavid

For legacy reasons, our test case docstrings should be of the format "Testing ..." and not end in a period. …

daviddavid

Do we need the or False here?

maubinmaubin

When there are more arguments than just self, each argument should be on their own line like this: publish_and_archive( self, …

maubinmaubin

django and djblets imports should be in the same group (third party imports) so this blank line should be removed.

maubinmaubin

The 2nd and 3rd """ here should be deleted.

maubinmaubin

Instead of putting the instance variable docs in the class docstring, they should instead be written out above each variable …

maubinmaubin

On Python3+ we don't need to include the class name and self when calling super, you can instead do super().setUp(). …

maubinmaubin

Add a blank line before this comment.

maubinmaubin

If we're going to be reformatting all of these lines, we should add type hints while we do it. If …

daviddavid

There should be a blank line after a class docstring.

daviddavid

For documenting instance members, we format it like this: class X: """...""" ###################### # Instance variables # ###################### #: The …

daviddavid

Let's clarify that this is the user's preferred default value for the "Archive after publishing" setting, and not that it …

daviddavid

The Model import should be inside an if TYPE_CHECKING: block at the bottom of the import list (you can import …

daviddavid

AvatarService should also be inside if TYPE_CHECKING

daviddavid

TrophyType should also be inside if TYPE_CHECKING

daviddavid

I think we can type local_site as Optional[AnyOrAllLocalSites] (importing that from reviewboard.site.models). I don't know why int was included in …

daviddavid

This can go away.

daviddavid

Copy/paste-o: ThemeForm -> AccountSettingsForm

daviddavid

When concatenating strings like this, our convention is to put the space at the end of the previous line instead …

daviddavid

Because this is a @property, we can document it as if it was a value instead of a function. ("Whether …

daviddavid
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

dan.casares
maubin
  1. This is a great start.

  2. When adding new changes, its good to run the full test suite and not just the relevant tests, so that you know whether your changes lead to any unexpected breakages.

    If a test passes on the main branch, but fails on your branch, then you know that your change broke something. If it fails on both branches, then this is something that was already broken and you don't need to worry about it for this change (but you should still mention it to us.)

  3. reviewboard/accounts/forms/pages.py (Diff revision 2)
     
     
     
    Show all issues

    This looks like it can all fit on one line and still be less than 79 chars.

  4. reviewboard/accounts/models.py (Diff revision 2)
     
     
     
    Show all issues

    Lets be more specific here and say "review requests" instead of "new posts".

  5. Show all issues

    from djblets.siteconfig.models import SiteConfiguration should be placed on the line before from djblets.testing.decorators import add_fixtures.

    Imports in the same group should be sorted alphabetically (so S before T).

  6. Show all issues

    These need to be moved to the same import group as the other django import on line 5. Files have 3 import groups:
    - Python Standard Library imports
    - Third-party module imports (django, kgb, six, pkg_resources, etc.).
    - Project imports.

    See more detail here

  7. Show all issues

    This needs a docstring. It can be as simple as

    """Setup for a unit test."""
    
  8. Show all issues

    This needs an Args: section in the docs and a Returns: section, as well as typing.

  9. Show all issues

    Missing a -> None:

  10. Show all issues

    Leftover code that should've been deleted?

  11. reviewboard/accounts/tests/test_account_settings_form.py (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    This is a nit, but we like to sort the keys alphabetically. You could do that here and for the other dictionaries below.

  12. Show all issues

    Add a blank line before the comment.

  13. 
      
dan.casares
Review request changed
Commits:
Summary ID Author
Added backend work for the archive profile setting
cfe29bb89d2fb5360a79a582cb42ea5c4a73d996 Daniel
Changed publish_and_archive to bee a regular BooleanField.
110645ac302fce03f56b9d7331f8a78ccd4f57c7 Daniel
Added test to test_account_settings_form for previous changes.
d4c8e6df5b28303eee148ab113f92290cebe92ec Daniel
Slight change to single test
c47d6b0ce1e7ac74c8a5fed5a8ee8c01505fd6ca Daniel
removed trailing white space
a5a47c2c5bd39afd1969ac9a6f5ecb5693ca6994 Daniel
Added backend work for the archive profile setting
cfe29bb89d2fb5360a79a582cb42ea5c4a73d996 Daniel
Changed publish_and_archive to bee a regular BooleanField.
110645ac302fce03f56b9d7331f8a78ccd4f57c7 Daniel
Added test to test_account_settings_form for previous changes.
d4c8e6df5b28303eee148ab113f92290cebe92ec Daniel
Slight change to single test
c47d6b0ce1e7ac74c8a5fed5a8ee8c01505fd6ca Daniel
removed trailing white space
a5a47c2c5bd39afd1969ac9a6f5ecb5693ca6994 Daniel
Fixing issues highlighted in review.
3912ae510848532175db3e866b8fec70d9539080 Daniel

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

dan.casares
dan.casares
david
  1. Hey Daniel,

    It looks like you've pulled on your master branch, but your branch is still coming off of a previous revision. The diff you've posted here therefore has a bunch of unrelated stuff.

    You're going to want to rebase your branch onto the latest master. See https://www.notion.so/reviewboard/Keeping-Commit-Histories-Clean-0f717c4e802c4a0ebd852cf9337ce5d2?pvs=25#ef7f7a64bfd64d49b44eea1e066e5e1e for details on that. After rebasing, please post this change again.

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

    In the description on your review request (which will get turned into the commit message that's stored for all time), we don't want to describe the mechanics of the change (added X method to Y file), because that's what the diff is for.

    Instead, we should explain at a high level what the change accomplishes and why we're doing it. In this case, you'd want it to say that we're adding a new setting to the user profile to let them save a default value for the "Archive after publishing" feature, plumbing that setting through to the frontend JS, and adding a new checkbox in the user preferences form to let them change it.

    Similarly, in the testing done field, we don't need to say exactly what files were changed. You can just say that you added new unit tests to cover certain cases. You probably should also run the entire python test suite as well as just your newly-added tests.

  3. reviewboard/accounts/forms/pages.py (Diff revision 6)
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    While you're here, would you mind sorting the keys in alphabetical order?

  4. reviewboard/accounts/forms/pages.py (Diff revision 6)
     
     
    Show all issues

    Looks like a leftover from when you had this as a DB field.

  5. reviewboard/accounts/models.py (Diff revision 6)
     
     
     
    Show all issues

    The first line of the docstring needs to fit on one line here. You can add an additional paragraph after if you need to be more wordy.

  6. reviewboard/accounts/models.py (Diff revision 6)
     
     
     
    Show all issues

    Same here.

  7. reviewboard/accounts/tests/test_account_settings_form.py (Diff revision 6)
     
     
     
     
     
     
     
     
     
     
    Show all issues

    The variable definitions and setUp() method should be at the top of the class, not under any test methods.

  8. Show all issues

    Please add doc comments for these.

  9. Show all issues

    This method should call super.setUp() as its first line.

  10. reviewboard/accounts/tests/test_account_settings_form.py (Diff revision 6)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    This looks unused, and even if you did need to do something with siteconfig, you shouldn't need to create your own instance. Can we just remove this?

  11. Show all issues

    For legacy reasons, our test case docstrings should be of the format "Testing ..." and not end in a period. Here and below.

  12. 
      
dan.casares
dan.casares
dan.casares
maubin
  1. 
      
  2. reviewboard/accounts/forms/pages.py (Diff revision 9)
     
     
    Show all issues

    Do we need the or False here?

  3. reviewboard/accounts/models.py (Diff revision 9)
     
     
    Show all issues

    When there are more arguments than just self, each argument should be on their own line like this:

    publish_and_archive(
        self,
        archive: bool,
    ) -> None:
    
  4. Show all issues

    django and djblets imports should be in the same group (third party imports) so this blank line should be removed.

  5. Show all issues

    The 2nd and 3rd """ here should be deleted.

  6. reviewboard/accounts/tests/test_account_settings_form.py (Diff revision 9)
     
     
     
     
     
     
     
     
    Show all issues

    Instead of putting the instance variable docs in the class docstring, they should instead be written out above each variable using #: comments. See the UIThemeRegistryTests class in reviewboard/themes/tests/test_ui_theme_registry.py for an example of what this looks like. You'll also need to add an "Instance Variables" comment header, which you'll see in that class as well.

  7. Show all issues

    On Python3+ we don't need to include the class name and self when calling super, you can instead do super().setUp(). We also like to put a blank line after calling super()

  8. Show all issues

    Add a blank line before this comment.

  9. 
      
maubin
  1. 
      
  2. Show all issues

    You can also change any "Version Added"s that you have from 7.0 to TODO, like you did in your frontend change.

  3. 
      
dan.casares
david
  1. 
      
  2. reviewboard/accounts/models.py (Diff revision 10)
     
     
     
     
     
     
    Show all issues

    If we're going to be reformatting all of these lines, we should add type hints while we do it. If we don't want to touch these, we should revert these formatting changes.

  3. Show all issues

    There should be a blank line after a class docstring.

  4. reviewboard/accounts/tests/test_account_settings_form.py (Diff revision 10)
     
     
     
     
     
     
     
    Show all issues

    For documenting instance members, we format it like this:

    class X:
        """..."""
    
        ######################
        # Instance variables #
        ######################
    
        #: The HTTP request.
        request: HttpRequest
    
        #: The user profile.
        profile: Profile
    
        #: The user to test with.
        user: User
    
  5. 
      
dan.casares
dan.casares
david
  1. I know if might be a fair bit of work, but it would be really nice to pull the type annotation changes in reviewboard/accounts/models.py out into their own review request. It's best to keep each change focused on one task.

  2. reviewboard/accounts/forms/pages.py (Diff revision 12)
     
     
    Show all issues

    Let's clarify that this is the user's preferred default value for the "Archive after publishing" setting, and not that it will always happen.

  3. reviewboard/accounts/models.py (Diff revision 12)
     
     
    Show all issues

    The Model import should be inside an if TYPE_CHECKING: block at the bottom of the import list (you can import TYPE_CHECKING in the from typing ... line)

  4. reviewboard/accounts/models.py (Diff revision 12)
     
     
    Show all issues

    AvatarService should also be inside if TYPE_CHECKING

  5. reviewboard/accounts/models.py (Diff revision 12)
     
     
    Show all issues

    TrophyType should also be inside if TYPE_CHECKING

  6. reviewboard/accounts/models.py (Diff revision 12)
     
     
    Show all issues

    I think we can type local_site as Optional[AnyOrAllLocalSites] (importing that from reviewboard.site.models). I don't know why int was included in the documentation -- perhaps Christian can shed some light on that.

    Same for others in this file.

  7. Show all issues

    This can go away.

  8. Show all issues

    Copy/paste-o: ThemeForm -> AccountSettingsForm

  9. 
      
dan.casares
david
  1. 
      
  2. reviewboard/accounts/forms/pages.py (Diff revision 13)
     
     
     
    Show all issues

    When concatenating strings like this, our convention is to put the space at the end of the previous line instead of the beginning of the next.

  3. reviewboard/accounts/models.py (Diff revision 13)
     
     
    Show all issues

    Because this is a @property, we can document it as if it was a value instead of a function. ("Whether to ..." instead of "Return whether to ...")

  4. 
      
dan.casares
dan.casares
Review request changed
Testing Done:
   

A series of tests were also implmeneted to ensure that the AccountSettingsForm

    was initialized, loaded, and saved properly and that it correctly identified
    invalid inputs. These tests were added to
    reviewboard/accounts/test/test_account_settings_form.py.

   
~  

Passes all relevant tests.

  ~

All previous passing pytests still passed.