Added backend work for the archive profile setting
Review Request #14003 — Created June 28, 2024 and updated
Began backend implementation for a profile setting that will determine whether
to automatically archive a review request when posted.
This included adding apublish_and_archive
BooleanField
to the
AccountSettingsForm
inreviewboard/accounts/pages.py
, which is initially set
to False.publish_and_archive
was also added to theProfile
in
reviewboard/accounts/models.py
. Lastly, publish_and_archive was also added to
reviewboard/accounts/accounts.py
in thejs_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 |
---|---|---|
045daa0a7492b92fc41c1109d1ee3fa7f7bd9ede | Daniel | |
ebd7fcf3accd3c9d9661ffcecd88ccd287a5da47 | Daniel | |
dc307f916fe70b3da9ad7a4dbcb303c781d1d595 | 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), … |
|
|
You can also change any "Version Added"s that you have from 7.0 to TODO, like you did in your frontend … |
![]() |
|
trailing whitespace Column: 64 Error code: W291 |
![]() |
|
This looks like it can all fit on one line and still be less than 79 chars. |
![]() |
|
Lets be more specific here and say "review requests" instead of "new posts". |
![]() |
|
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 … |
![]() |
|
These need to be moved to the same import group as the other django import on line 5. Files have … |
![]() |
|
This needs a docstring. It can be as simple as """Setup for a unit test.""" |
![]() |
|
This needs an Args: section in the docs and a Returns: section, as well as typing. |
![]() |
|
Missing a -> None: |
![]() |
|
Leftover code that should've been deleted? |
![]() |
|
This is a nit, but we like to sort the keys alphabetically. You could do that here and for the … |
![]() |
|
Add a blank line before the comment. |
![]() |
|
blank line contains whitespace Column: 1 Error code: W293 |
![]() |
|
While you're here, would you mind sorting the keys in alphabetical order? |
|
|
Looks like a leftover from when you had this as a DB field. |
|
|
The first line of the docstring needs to fit on one line here. You can add an additional paragraph after … |
|
|
Same here. |
|
|
The variable definitions and setUp() method should be at the top of the class, not under any test methods. |
|
|
Please add doc comments for these. |
|
|
This method should call super.setUp() as its first line. |
|
|
This looks unused, and even if you did need to do something with siteconfig, you shouldn't need to create your … |
|
|
For legacy reasons, our test case docstrings should be of the format "Testing ..." and not end in a period. … |
|
|
Do we need the or False here? |
![]() |
|
When there are more arguments than just self, each argument should be on their own line like this: publish_and_archive( self, … |
![]() |
|
django and djblets imports should be in the same group (third party imports) so this blank line should be removed. |
![]() |
|
The 2nd and 3rd """ here should be deleted. |
![]() |
|
Instead of putting the instance variable docs in the class docstring, they should instead be written out above each variable … |
![]() |
|
On Python3+ we don't need to include the class name and self when calling super, you can instead do super().setUp(). … |
![]() |
|
Add a blank line before this comment. |
![]() |
|
If we're going to be reformatting all of these lines, we should add type hints while we do it. If … |
|
|
There should be a blank line after a class docstring. |
|
|
For documenting instance members, we format it like this: class X: """...""" ###################### # Instance variables # ###################### #: The … |
|
|
Let's clarify that this is the user's preferred default value for the "Archive after publishing" setting, and not that it … |
|
|
The Model import should be inside an if TYPE_CHECKING: block at the bottom of the import list (you can import … |
|
|
AvatarService should also be inside if TYPE_CHECKING |
|
|
TrophyType should also be inside if TYPE_CHECKING |
|
|
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 … |
|
|
This can go away. |
|
|
Copy/paste-o: ThemeForm -> AccountSettingsForm |
|
|
When concatenating strings like this, our convention is to put the space at the end of the previous line instead … |
|
|
Because this is a @property, we can document it as if it was a value instead of a function. ("Whether … |
|
- Commits:
-
Summary ID Author cfe29bb89d2fb5360a79a582cb42ea5c4a73d996 Daniel 110645ac302fce03f56b9d7331f8a78ccd4f57c7 Daniel d4c8e6df5b28303eee148ab113f92290cebe92ec Daniel c47d6b0ce1e7ac74c8a5fed5a8ee8c01505fd6ca Daniel cfe29bb89d2fb5360a79a582cb42ea5c4a73d996 Daniel 110645ac302fce03f56b9d7331f8a78ccd4f57c7 Daniel d4c8e6df5b28303eee148ab113f92290cebe92ec Daniel c47d6b0ce1e7ac74c8a5fed5a8ee8c01505fd6ca Daniel a5a47c2c5bd39afd1969ac9a6f5ecb5693ca6994 Daniel
Checks run (2 succeeded)
![maubin](https://media-reviews-reviewboard-org.s3.amazonaws.com/uploaded/avatars/m/ma/maubin__28892f51-5581-4db1-b65d-322d3b6117de.jpg?AWSAccessKeyId=AKIAQF2RGG6MLO3L2GME&Signature=ChfrSS%2FR03np%2Fe3FnJR%2FzaqkCYY%3D&Expires=1725754673)
-
This is a great start.
-
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.) -
-
-
from djblets.siteconfig.models import SiteConfiguration
should be placed on the line beforefrom djblets.testing.decorators import add_fixtures
.Imports in the same group should be sorted alphabetically (so S before T).
-
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
-
-
-
-
-
This is a nit, but we like to sort the keys alphabetically. You could do that here and for the other dictionaries below.
-
- Commits:
-
Summary ID Author cfe29bb89d2fb5360a79a582cb42ea5c4a73d996 Daniel 110645ac302fce03f56b9d7331f8a78ccd4f57c7 Daniel d4c8e6df5b28303eee148ab113f92290cebe92ec Daniel c47d6b0ce1e7ac74c8a5fed5a8ee8c01505fd6ca Daniel a5a47c2c5bd39afd1969ac9a6f5ecb5693ca6994 Daniel cfe29bb89d2fb5360a79a582cb42ea5c4a73d996 Daniel 110645ac302fce03f56b9d7331f8a78ccd4f57c7 Daniel d4c8e6df5b28303eee148ab113f92290cebe92ec Daniel c47d6b0ce1e7ac74c8a5fed5a8ee8c01505fd6ca Daniel a5a47c2c5bd39afd1969ac9a6f5ecb5693ca6994 Daniel 3912ae510848532175db3e866b8fec70d9539080 Daniel
- Commits:
-
Summary ID Author cfe29bb89d2fb5360a79a582cb42ea5c4a73d996 Daniel 110645ac302fce03f56b9d7331f8a78ccd4f57c7 Daniel d4c8e6df5b28303eee148ab113f92290cebe92ec Daniel c47d6b0ce1e7ac74c8a5fed5a8ee8c01505fd6ca Daniel a5a47c2c5bd39afd1969ac9a6f5ecb5693ca6994 Daniel 3912ae510848532175db3e866b8fec70d9539080 Daniel cfe29bb89d2fb5360a79a582cb42ea5c4a73d996 Daniel 110645ac302fce03f56b9d7331f8a78ccd4f57c7 Daniel d4c8e6df5b28303eee148ab113f92290cebe92ec Daniel c47d6b0ce1e7ac74c8a5fed5a8ee8c01505fd6ca Daniel a5a47c2c5bd39afd1969ac9a6f5ecb5693ca6994 Daniel 3912ae510848532175db3e866b8fec70d9539080 Daniel 91c2f315aee32a58883b2d361f5ae721b759baa0 Daniel
Checks run (2 succeeded)
- Change Summary:
-
Changed the
publish_and_archive
from aBooleanField
to aproperty
like
ui_theme_id
so thatpublish_and_archive
is not added to the database.
Changes to the tests were also made to accommodate for these changes - Commits:
-
Summary ID Author cfe29bb89d2fb5360a79a582cb42ea5c4a73d996 Daniel 110645ac302fce03f56b9d7331f8a78ccd4f57c7 Daniel d4c8e6df5b28303eee148ab113f92290cebe92ec Daniel c47d6b0ce1e7ac74c8a5fed5a8ee8c01505fd6ca Daniel a5a47c2c5bd39afd1969ac9a6f5ecb5693ca6994 Daniel 3912ae510848532175db3e866b8fec70d9539080 Daniel 91c2f315aee32a58883b2d361f5ae721b759baa0 Daniel cfe29bb89d2fb5360a79a582cb42ea5c4a73d996 Daniel 110645ac302fce03f56b9d7331f8a78ccd4f57c7 Daniel d4c8e6df5b28303eee148ab113f92290cebe92ec Daniel c47d6b0ce1e7ac74c8a5fed5a8ee8c01505fd6ca Daniel a5a47c2c5bd39afd1969ac9a6f5ecb5693ca6994 Daniel 3912ae510848532175db3e866b8fec70d9539080 Daniel 91c2f315aee32a58883b2d361f5ae721b759baa0 Daniel 7ea0769c5f44ba49a005ab4dcb9cb49686347346 Daniel
Checks run (2 succeeded)
-
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.
- Commits:
-
Summary ID Author cfe29bb89d2fb5360a79a582cb42ea5c4a73d996 Daniel 110645ac302fce03f56b9d7331f8a78ccd4f57c7 Daniel d4c8e6df5b28303eee148ab113f92290cebe92ec Daniel c47d6b0ce1e7ac74c8a5fed5a8ee8c01505fd6ca Daniel a5a47c2c5bd39afd1969ac9a6f5ecb5693ca6994 Daniel 3912ae510848532175db3e866b8fec70d9539080 Daniel 91c2f315aee32a58883b2d361f5ae721b759baa0 Daniel 7ea0769c5f44ba49a005ab4dcb9cb49686347346 Daniel 30cd3e40f35bf86e5fff334e205b4a9d7c395a32 Daniel f5642d198399b493640a0be29f41c3c60e372bca Daniel a75b7f76d0770663b0917cd54619ff2e4c78ce6d Daniel 86eafb7560527799c4d0c5bb4c3ee233ff9ae13c Daniel bdaeedea3b36037dd2393783dde199c643a3fd12 Daniel 719420e92218e671ce1439a2c5e15ea4c35acb33 Daniel 58b864eb6e433f433210aea8c9fb7358ebc55750 Daniel a8f699817a20db342ba4f06e66187efa2deb535d Daniel 9a4e5a39cffa34d42733fde47471c07da8b9f6ea Daniel
Checks run (2 succeeded)
-
-
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.
-
-
-
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.
-
-
The variable definitions and setUp() method should be at the top of the class, not under any test methods.
-
-
-
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?
-
For legacy reasons, our test case docstrings should be of the format "Testing ..." and not end in a period. Here and below.
- Change Summary:
-
Addresses most of the issues highlighgted by David. Fixed the comments
throughout the code and reordered variables. - Commits:
-
Summary ID Author 30cd3e40f35bf86e5fff334e205b4a9d7c395a32 Daniel f5642d198399b493640a0be29f41c3c60e372bca Daniel a75b7f76d0770663b0917cd54619ff2e4c78ce6d Daniel 86eafb7560527799c4d0c5bb4c3ee233ff9ae13c Daniel bdaeedea3b36037dd2393783dde199c643a3fd12 Daniel 719420e92218e671ce1439a2c5e15ea4c35acb33 Daniel 58b864eb6e433f433210aea8c9fb7358ebc55750 Daniel a8f699817a20db342ba4f06e66187efa2deb535d Daniel 9a4e5a39cffa34d42733fde47471c07da8b9f6ea Daniel 30cd3e40f35bf86e5fff334e205b4a9d7c395a32 Daniel f5642d198399b493640a0be29f41c3c60e372bca Daniel a75b7f76d0770663b0917cd54619ff2e4c78ce6d Daniel 86eafb7560527799c4d0c5bb4c3ee233ff9ae13c Daniel bdaeedea3b36037dd2393783dde199c643a3fd12 Daniel 719420e92218e671ce1439a2c5e15ea4c35acb33 Daniel 58b864eb6e433f433210aea8c9fb7358ebc55750 Daniel a8f699817a20db342ba4f06e66187efa2deb535d Daniel 9a4e5a39cffa34d42733fde47471c07da8b9f6ea Daniel 0bb80089745beb1fe1765b96099976d636b06f3d Daniel e855ea81bcd4ebfeac8572d8023a881b5efc4399 Daniel 0307ff863048c7b1e59fcfc52ece05eb0d17d575 Daniel
Checks run (2 succeeded)
- Change Summary:
-
Fixed setUp issue in
test_account_settings_form.py
. - Commits:
-
Summary ID Author 30cd3e40f35bf86e5fff334e205b4a9d7c395a32 Daniel f5642d198399b493640a0be29f41c3c60e372bca Daniel a75b7f76d0770663b0917cd54619ff2e4c78ce6d Daniel 86eafb7560527799c4d0c5bb4c3ee233ff9ae13c Daniel bdaeedea3b36037dd2393783dde199c643a3fd12 Daniel 719420e92218e671ce1439a2c5e15ea4c35acb33 Daniel 58b864eb6e433f433210aea8c9fb7358ebc55750 Daniel a8f699817a20db342ba4f06e66187efa2deb535d Daniel 9a4e5a39cffa34d42733fde47471c07da8b9f6ea Daniel 0bb80089745beb1fe1765b96099976d636b06f3d Daniel e855ea81bcd4ebfeac8572d8023a881b5efc4399 Daniel 0307ff863048c7b1e59fcfc52ece05eb0d17d575 Daniel 30cd3e40f35bf86e5fff334e205b4a9d7c395a32 Daniel f5642d198399b493640a0be29f41c3c60e372bca Daniel a75b7f76d0770663b0917cd54619ff2e4c78ce6d Daniel 86eafb7560527799c4d0c5bb4c3ee233ff9ae13c Daniel bdaeedea3b36037dd2393783dde199c643a3fd12 Daniel 719420e92218e671ce1439a2c5e15ea4c35acb33 Daniel 58b864eb6e433f433210aea8c9fb7358ebc55750 Daniel a8f699817a20db342ba4f06e66187efa2deb535d Daniel 9a4e5a39cffa34d42733fde47471c07da8b9f6ea Daniel 0bb80089745beb1fe1765b96099976d636b06f3d Daniel e855ea81bcd4ebfeac8572d8023a881b5efc4399 Daniel 0307ff863048c7b1e59fcfc52ece05eb0d17d575 Daniel 72786f2862a5c144400201a53ff676db0062265c Daniel
Checks run (2 succeeded)
- Commits:
-
Summary ID Author 30cd3e40f35bf86e5fff334e205b4a9d7c395a32 Daniel f5642d198399b493640a0be29f41c3c60e372bca Daniel a75b7f76d0770663b0917cd54619ff2e4c78ce6d Daniel 86eafb7560527799c4d0c5bb4c3ee233ff9ae13c Daniel bdaeedea3b36037dd2393783dde199c643a3fd12 Daniel 719420e92218e671ce1439a2c5e15ea4c35acb33 Daniel 58b864eb6e433f433210aea8c9fb7358ebc55750 Daniel a8f699817a20db342ba4f06e66187efa2deb535d Daniel 9a4e5a39cffa34d42733fde47471c07da8b9f6ea Daniel 0bb80089745beb1fe1765b96099976d636b06f3d Daniel e855ea81bcd4ebfeac8572d8023a881b5efc4399 Daniel 0307ff863048c7b1e59fcfc52ece05eb0d17d575 Daniel 72786f2862a5c144400201a53ff676db0062265c Daniel 92b7a9b08194663313e84ec938a6dd3b22154e86 Daniel d1c2d90d9612899f82a109c92a2f6f26d2128b53 Daniel 4a698faf4da5f514c3a5d527e4990d5cc4ce9e20 Daniel
Checks run (2 succeeded)
![maubin](https://media-reviews-reviewboard-org.s3.amazonaws.com/uploaded/avatars/m/ma/maubin__28892f51-5581-4db1-b65d-322d3b6117de.jpg?AWSAccessKeyId=AKIAQF2RGG6MLO3L2GME&Signature=ChfrSS%2FR03np%2Fe3FnJR%2FzaqkCYY%3D&Expires=1725754673)
-
-
-
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:
-
django
anddjblets
imports should be in the same group (third party imports) so this blank line should be removed. -
-
Instead of putting the instance variable docs in the class docstring, they should instead be written out above each variable using
#:
comments. See theUIThemeRegistryTests
class inreviewboard/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. -
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 callingsuper()
-
- Commits:
-
Summary ID Author 92b7a9b08194663313e84ec938a6dd3b22154e86 Daniel d1c2d90d9612899f82a109c92a2f6f26d2128b53 Daniel 4a698faf4da5f514c3a5d527e4990d5cc4ce9e20 Daniel db8bd3a166e7448670ead6ae85b8ae4ef818bd9e Daniel c837977102290bf537bb6248a929f56089c80255 Daniel 4da6734f76f49b40be1e749e4e3b471dc889251e Daniel
Checks run (2 succeeded)
-
-
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.
-
-
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
- Change Summary:
-
Adjusted the typing for functions in
model.py
. - Commits:
-
Summary ID Author db8bd3a166e7448670ead6ae85b8ae4ef818bd9e Daniel c837977102290bf537bb6248a929f56089c80255 Daniel 4da6734f76f49b40be1e749e4e3b471dc889251e Daniel db8bd3a166e7448670ead6ae85b8ae4ef818bd9e Daniel c837977102290bf537bb6248a929f56089c80255 Daniel 4da6734f76f49b40be1e749e4e3b471dc889251e Daniel ce575d65bc4677836cb219a703b3fc3a995bfec3 Daniel - Diff:
-
Revision 11 (+2200 -5132)
Checks run (2 succeeded)
- Change Summary:
-
Rebased properly with master
- Commits:
-
Summary ID Author db8bd3a166e7448670ead6ae85b8ae4ef818bd9e Daniel c837977102290bf537bb6248a929f56089c80255 Daniel 4da6734f76f49b40be1e749e4e3b471dc889251e Daniel ce575d65bc4677836cb219a703b3fc3a995bfec3 Daniel 045daa0a7492b92fc41c1109d1ee3fa7f7bd9ede Daniel f035550bae4b5959007d349e7374ec0e8f820684 Daniel f3bd08332868680685757d4f8a2ddcf29fc3e8ff Daniel 78d6b279fa2e4a9528710ee10e6744576bce0eef Daniel
Checks run (2 succeeded)
-
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.
-
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.
-
The Model import should be inside an
if TYPE_CHECKING:
block at the bottom of the import list (you can importTYPE_CHECKING
in thefrom typing ...
line) -
-
-
I think we can type
local_site
asOptional[AnyOrAllLocalSites]
(importing that fromreviewboard.site.models
). I don't know whyint
was included in the documentation -- perhaps Christian can shed some light on that.Same for others in this file.
-
-
- Commits:
-
Summary ID Author 045daa0a7492b92fc41c1109d1ee3fa7f7bd9ede Daniel f035550bae4b5959007d349e7374ec0e8f820684 Daniel f3bd08332868680685757d4f8a2ddcf29fc3e8ff Daniel 78d6b279fa2e4a9528710ee10e6744576bce0eef Daniel 045daa0a7492b92fc41c1109d1ee3fa7f7bd9ede Daniel ebd7fcf3accd3c9d9661ffcecd88ccd287a5da47 Daniel dc307f916fe70b3da9ad7a4dbcb303c781d1d595 Daniel