Rework how and when avatar settings are migrated.
Review Request #9695 — Created Feb. 23, 2018 and submitted
AvatarServiceRegistry.populate()
was taking care of the avatar
settings migrations, which required thatpopulate()
be called in
virtually every method in the base registry in Djblets. A recent change
redid how we stored settings and whenpopulate()
was called, and this
led to a situation where some siteconfig settings wrappers (such as the
default_service
property on the registry) could produce bad results
due to our settings migration not having yet occurred.This was reproduceable by having non-migrated settings and no custom
avatar settings for a user with a freshly-started development server and
then fetching the API resource for the user. This would call
AvatarServiceRegistry.for_user()
, which would return the result of
default_service
, since there were no user-specific settings.
default_service
would just wrap siteconfig settings. This meant no
calls topopulate()
.Since the concept of migration of settings is specific to Review Board,
Djblets is doing nothing wrong. Instead, Review Board was migrating too
late. Now,load_site_config()
(which handles other migration tasks)
calls the newAvatarServiceRegistry.migrate_settings()
, which handles
the migration. This ensures any avatar-dependent code will see a proper
state of the world.This exposed a new problem, though, which is that databases defaulted to
legacy settings, requiring at least one migration. While not a big deal
on a new install, migrating settings inload_site_config()
did end up
impacting query counts for unit tests, since a migration had to be
performed before every test (normally,load_site_config()
doesn't need
to save anything on a default database).To address this, and to simplify things going forward, the way we handle
default avatar settings and migration has changed. Instead of having a
blank slate for default avatar settings, we set them to the desired
post-migration settings, turning on avatars, setting the enabled
backends, and defaulting to Gravatars. When migrating, we check if
there's a storedavatars_enabled
state and, if so, assume we're good
(this will be the case on Review Board 3.0 through 3.0.3).If there's not, then it's assumed we're using legacy settings, so we
checkintegration_gravatars
. If this isFalse
, then we explicitly
store that avatars are disabled. Otherwise, we leave the setting alone
in siteconfig. We then get rid of the legac yintegration_gravatars
and
avatars_migrated
keys, since they're no longer needed.This allows us to optimistically make use of the siteconfig defaults,
instead of having to write all new state, and to be modern-by-default.
It also means that we can add new default avatar services and have them
show up as defaults for most installs (unless they've explicitly saved
new avatar settings).
All unit tests pass. These include new unit tests that test migration
from various versions of avatar settings state (pre-2.0, 2.0/2.5,
3.0 through 3.0.3, and 3.0.4+) and combinations of settings states.Tested the repro case of a new database with no user avatar state and
no migrated avatar settings. Started the dev server and accessed the API
for a user. Saw the new settings, and verified the correct state in the
database.
- Change Summary:
-
- Fixed a line length issue.
- Removed an unused variable in a unit test.
- Commit:
-
917dc32e18cf0e13c730ebb7bc47f1c38af9fb982bad02a1b446b183ff85cec3266a771627d18843
Checks run (2 succeeded)
- Description:
-
AvatarServiceRegistry.populate()
was taking care of the avatarsettings migrations, which required that populate()
be called invirtually every method in the base registry in Djblets. A recent change redid how we stored settings and when populate()
was called, and thisled to a situation where some siteconfig settings wrappers (such as the default_service
property on the registry) could produce bad resultsdue to our settings migration not having yet occurred. This was reproduceable by having non-migrated settings and no custom
avatar settings for a user with a freshly-started development server and then fetching the API resource for the user. This would call AvatarServiceRegistry.for_user()
, which would return the result ofdefault_service
, since there were no user-specific settings.default_service
would just wrap siteconfig settings. This meant nocalls to populate()
.Since the concept of migration of settings is specific to Review Board,
Djblets is doing nothing wrong. Instead, Review Board was migrating too late. Now, load_site_config()
(which handles other migration tasks)~ calls the new AvatarServiceRegistry.migrate_settings()
, which hanles~ calls the new AvatarServiceRegistry.migrate_settings()
, which handlesthe migration. This ensures any avatar-dependent code will see a proper state of the world. This exposed a new problem, though, which is that databases defaulted to
legacy settings, requiring at least one migration. While not a big deal on a new install, migrating settings in load_site_config()
did end upimpacting query counts for unit tests, since a migration had to be performed before every test (normally, load_site_config()
doesn't needto save anything on a default database). To address this, and to simplify things going forward, the way we handle
default avatar settings and migration has changed. Instead of having a blank slate for default avatar settings, we set them to the desired post-migration settings, turning on avatars, setting the enabled backends, and defaulting to Gravatars. When migrating, we check if there's a stored avatars_enabled
state and, if so, assume we're good(this will be the case on Review Board 3.0 through 3.0.3). If there's not, then it's assumed we're using legacy settings, so we
check integration_gravatars
. If this isFalse
, then we explicitlystore that avatars are disabled. Otherwise, we leave the setting alone in siteconfig. We then get rid of the legac y integration_gravatars
andavatars_migrated
keys, since they're no longer needed.This allows us to optimistically make use of the siteconfig defaults,
instead of having to write all new state, and to be modern-by-default. It also means that we can add new default avatar services and have them show up as defaults for most installs (unless they've explicitly saved new avatar settings).