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	ID
Add consolidation of duplicate LocalSiteProfile objects. 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.	a86e6b05cdefb853369e80ef04d5582c4db09faf

Summary

Add consolidation of duplicate LocalSiteProfile objects.

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.

a86e6b05cdefb853369e80ef04d5582c4db09faf

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)
The issue has been dropped. Show all issues
```
line too long (80 > 79 characters)

Column: 80
Error code: E501
```
reviewboard/accounts/managers.py (Diff revision 1)
The issue has been dropped. 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)
The issue has been dropped. 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)
The issue has been dropped. Show all issues
```
'reviewboard.site.models.LocalSite' imported but unused

Column: 1
Error code: F401
```

Commits:

	Summary	ID
	Add consolidation of duplicate LocalSiteProfile objects. 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.	99b5d129979cc434333bd6a717ef60fb7fb91cbf
	Add consolidation of duplicate LocalSiteProfile objects. 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.	f75d2fa605a47d7485491004c54a5ae8a0f12905

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)
The issue has been resolved. Show all issues
```
Can we give create_if_missing a default?
```
reviewboard/accounts/managers.py (Diff revision 2)
The issue has been resolved. 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)

The issue has been resolved. 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)

The issue has been resolved. 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)

The issue has been resolved. 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)

The issue has been resolved. 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)
The issue has been resolved. Show all issues
```
Swap these.
```

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

The issue has been resolved. Show all issues

We should add a step that reloads and verifies resultls.

Commits:

	Summary	ID
	Add consolidation of duplicate LocalSiteProfile objects. 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.	f75d2fa605a47d7485491004c54a5ae8a0f12905
	Add consolidation of duplicate LocalSiteProfile objects. 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.	9c6f01e643cc3bfd0d23976c2d481edd2e97c419

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)
The issue has been dropped. Show all issues
```
line too long (82 > 79 characters)

Column: 80
Error code: E501
```
reviewboard/accounts/managers.py (Diff revision 3)
The issue has been dropped. Show all issues
```
line too long (80 > 79 characters)

Column: 80
Error code: E501
```

Commits:

	Summary	ID
	Add consolidation of duplicate LocalSiteProfile objects. 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.	9c6f01e643cc3bfd0d23976c2d481edd2e97c419
	Add consolidation of duplicate LocalSiteProfile objects. 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.	a86e6b05cdefb853369e80ef04d5582c4db09faf

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)

The issue has been resolved. 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:: Completed
Change Summary:: Pushed to release-5.0.x (906cbbc)