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.

Passes all relevant tests.

Summary ID Author
Added backend work for the archive profile setting
30cd3e40f35bf86e5fff334e205b4a9d7c395a32 Daniel
Changed publish_and_archive to bee a regular BooleanField.
f5642d198399b493640a0be29f41c3c60e372bca Daniel
Added test to test_account_settings_form for previous changes.
a75b7f76d0770663b0917cd54619ff2e4c78ce6d Daniel
Slight change to single test
86eafb7560527799c4d0c5bb4c3ee233ff9ae13c Daniel
removed trailing white space
bdaeedea3b36037dd2393783dde199c643a3fd12 Daniel
Fixing issues highlighted in review.
719420e92218e671ce1439a2c5e15ea4c35acb33 Daniel
Removed whitespace
58b864eb6e433f433210aea8c9fb7358ebc55750 Daniel
Changed publish_and_archive to be property and not BooleanField to not be added to the database. Also fixed resulting errors in tests.
a8f699817a20db342ba4f06e66187efa2deb535d Daniel
Fixed indentation
9a4e5a39cffa34d42733fde47471c07da8b9f6ea Daniel
Rearanged Imports
0bb80089745beb1fe1765b96099976d636b06f3d Daniel
Small changes for review request
e855ea81bcd4ebfeac8572d8023a881b5efc4399 Daniel
Added more comments.
0307ff863048c7b1e59fcfc52ece05eb0d17d575 Daniel
Added super().setup()
72786f2862a5c144400201a53ff676db0062265c 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

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
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
Review request changed
Change Summary:

Fixed setUp issue in test_account_settings_form.py.

Commits:
Summary ID Author
Added backend work for the archive profile setting
30cd3e40f35bf86e5fff334e205b4a9d7c395a32 Daniel
Changed publish_and_archive to bee a regular BooleanField.
f5642d198399b493640a0be29f41c3c60e372bca Daniel
Added test to test_account_settings_form for previous changes.
a75b7f76d0770663b0917cd54619ff2e4c78ce6d Daniel
Slight change to single test
86eafb7560527799c4d0c5bb4c3ee233ff9ae13c Daniel
removed trailing white space
bdaeedea3b36037dd2393783dde199c643a3fd12 Daniel
Fixing issues highlighted in review.
719420e92218e671ce1439a2c5e15ea4c35acb33 Daniel
Removed whitespace
58b864eb6e433f433210aea8c9fb7358ebc55750 Daniel
Changed publish_and_archive to be property and not BooleanField to not be added to the database. Also fixed resulting errors in tests.
a8f699817a20db342ba4f06e66187efa2deb535d Daniel
Fixed indentation
9a4e5a39cffa34d42733fde47471c07da8b9f6ea Daniel
Rearanged Imports
0bb80089745beb1fe1765b96099976d636b06f3d Daniel
Small changes for review request
e855ea81bcd4ebfeac8572d8023a881b5efc4399 Daniel
Added more comments.
0307ff863048c7b1e59fcfc52ece05eb0d17d575 Daniel
Added backend work for the archive profile setting
30cd3e40f35bf86e5fff334e205b4a9d7c395a32 Daniel
Changed publish_and_archive to bee a regular BooleanField.
f5642d198399b493640a0be29f41c3c60e372bca Daniel
Added test to test_account_settings_form for previous changes.
a75b7f76d0770663b0917cd54619ff2e4c78ce6d Daniel
Slight change to single test
86eafb7560527799c4d0c5bb4c3ee233ff9ae13c Daniel
removed trailing white space
bdaeedea3b36037dd2393783dde199c643a3fd12 Daniel
Fixing issues highlighted in review.
719420e92218e671ce1439a2c5e15ea4c35acb33 Daniel
Removed whitespace
58b864eb6e433f433210aea8c9fb7358ebc55750 Daniel
Changed publish_and_archive to be property and not BooleanField to not be added to the database. Also fixed resulting errors in tests.
a8f699817a20db342ba4f06e66187efa2deb535d Daniel
Fixed indentation
9a4e5a39cffa34d42733fde47471c07da8b9f6ea Daniel
Rearanged Imports
0bb80089745beb1fe1765b96099976d636b06f3d Daniel
Small changes for review request
e855ea81bcd4ebfeac8572d8023a881b5efc4399 Daniel
Added more comments.
0307ff863048c7b1e59fcfc52ece05eb0d17d575 Daniel
Added super().setup()
72786f2862a5c144400201a53ff676db0062265c Daniel

Checks run (2 succeeded)

flake8 passed.
JSHint passed.