Summary:

Add consolidation of duplicate LocalSiteProfile objects.

Review Request #12471 — Created July 14, 2022 and submitted Aug. 9, 2022, 1 p.m.

Information
Owner:	david
Repository:	Review Board
Branch:	release-5.0.x
Bugs:
Depends On:
Reviewers
Groups:	reviewboard
People:

Description

We had two problems that were potentially causing the existence of
duplicate LocalSiteProfile models. The first of these is that this
wasn't using ConcurrencyManager, which handles integrity issues across
separate threads (or servers). The second, more difficult one, is that
some databases (particularly MySQL) were ignoring our unique_together
constraint when one of the relations was NULL. This meant it was
possible to end up with multiple LocalSiteProfile objects with
local_site=None.

This change moves us over to ConcurrencyManager, and implements a
check for the duplicate case when attempting to use
User.get_site_profile. If we do find duplicates, they'll be
consolidated into a single one before returning.

Testing Done

Ran unit tests.

Commits

Summary
	Add consolidation of duplicate LocalSiteProfile objects.

Screenshots

Issues

Description	From	Last Updated
line too long (80 > 79 characters) Column: 80 Error code: E501	reviewbot	July 14, 2022, 3:15 p.m.
line too long (82 > 79 characters) Column: 80 Error code: E501	reviewbot	July 14, 2022, 3:15 p.m.
'reviewboard.accounts.models.Profile' imported but unused Column: 1 Error code: F401	reviewbot	July 14, 2022, 3:15 p.m.
'reviewboard.site.models.LocalSite' imported but unused Column: 1 Error code: F401	reviewbot	July 14, 2022, 3:15 p.m.
Can we give create_if_missing a default?	chipx86	Aug. 3, 2022, 2:07 p.m.
Most of our code capitalized as "Local Site". Can we have these docs use the same?	chipx86	Aug. 3, 2022, 2:07 p.m.
Let's make this a numeric list. Makes it much easier to read what's in which slot.	chipx86	Aug. 3, 2022, 2:08 p.m.
Can we do one per line? We should treat these like a dictionary when we have multiple keyword args. It's …	chipx86	Aug. 3, 2022, 2:08 p.m.
Same as above, can we do one per line? Minimizes changes if we need to update the query. We may …	chipx86	Aug. 3, 2022, 2:09 p.m.
We should save all this and permissions using update_fields=, I think?	chipx86	Aug. 3, 2022, 2:11 p.m.
Can the paren be moved to the next line, and a trailing comma added? We'll probably need to update this …	chipx86	Aug. 9, 2022, 1 p.m.
Swap these.	chipx86	Aug. 3, 2022, 2:11 p.m.
We should add a step that reloads and verifies resultls.	chipx86	Aug. 3, 2022, 2:12 p.m.
line too long (82 > 79 characters) Column: 80 Error code: E501	reviewbot	Aug. 3, 2022, 2:27 p.m.
line too long (80 > 79 characters) Column: 80 Error code: E501	reviewbot	Aug. 3, 2022, 2:27 p.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/accounts/managers.py (Diff revision 1)

Show all issues

line too long (80 > 79 characters)

Column: 80
Error code: E501

reviewboard/accounts/managers.py (Diff revision 1)

Show all issues

line too long (82 > 79 characters)

Column: 80
Error code: E501

reviewboard/accounts/tests/test_local_site_profile.py (Diff revision 1)

Show all issues

'reviewboard.accounts.models.Profile' imported but unused

Column: 1
Error code: F401

reviewboard/accounts/tests/test_local_site_profile.py (Diff revision 1)

Show all issues

'reviewboard.site.models.LocalSite' imported but unused

Column: 1
Error code: F401

Commits:

	Summary
-		Add consolidation of duplicate LocalSiteProfile objects.
+		Add consolidation of duplicate LocalSiteProfile objects.

Diff:

Revision 2 (+298 -26)

Show changes

	reviewboard/accounts/managers.py
	reviewboard/accounts/models.py
	reviewboard/accounts/tests/test_local_site_profile.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

reviewboard/accounts/managers.py (Diff revision 2)
Show all issues
```
Can we give create_if_missing a default?
```

reviewboard/accounts/managers.py (Diff revision 2)

Show all issues

Most of our code capitalized as "Local Site". Can we have these docs use the same?

reviewboard/accounts/managers.py (Diff revision 2)

Show all issues

Let's make this a numeric list. Makes it much easier to read what's in which slot.

reviewboard/accounts/managers.py (Diff revision 2)

Show all issues

Can we do one per line? We should treat these like a dictionary when we have multiple keyword args. It's much easier to read and to update.

reviewboard/accounts/managers.py (Diff revision 2)

Show all issues

Same as above, can we do one per line? Minimizes changes if we need to update the query.

We may also want to use .only(...) with pk, permissions, and the counters. Small optimization, and this is unlikely to be called much, so I'll leave that up to you.

reviewboard/accounts/managers.py (Diff revision 2)

Show all issues

We should save all this and permissions using update_fields=, I think?

reviewboard/accounts/tests/test_local_site_profile.py (Diff revision 2)
Show all issues
```
Swap these.
```

reviewboard/accounts/tests/test_local_site_profile.py (Diff revision 2)

Show all issues

We should add a step that reloads and verifies resultls.

Commits:

	Summary
-		Add consolidation of duplicate LocalSiteProfile objects.
+		Add consolidation of duplicate LocalSiteProfile objects.

Diff:

Revision 3 (+344 -26)

Show changes

	reviewboard/accounts/managers.py
	reviewboard/accounts/models.py
	reviewboard/accounts/tests/test_local_site_profile.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/accounts/managers.py (Diff revision 3)

Show all issues

line too long (82 > 79 characters)

Column: 80
Error code: E501

reviewboard/accounts/managers.py (Diff revision 3)

Show all issues

line too long (80 > 79 characters)

Column: 80
Error code: E501

Commits:

	Summary
-		Add consolidation of duplicate LocalSiteProfile objects.
+		Add consolidation of duplicate LocalSiteProfile objects.

Diff:

Revision 4 (+348 -26)

Show changes

	reviewboard/accounts/managers.py
	reviewboard/accounts/models.py
	reviewboard/accounts/tests/test_local_site_profile.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

reviewboard/accounts/managers.py (Diff revisions 2 - 4)

Show all issues

Can the paren be moved to the next line, and a trailing comma added? We'll probably need to update this list at some point.

It'd be nice to alphabetize it, too.

Status: Closed (submitted)

Change Summary:

Pushed to release-5.0.x (906cbbc)