Rework how and when avatar settings are migrated.

Review Request #9695 — Created Feb. 23, 2018 and submitted

Information

Review Board
release-3.0.x
6078328...

Reviewers

AvatarServiceRegistry.populate() was taking care of the avatar
settings migrations, which required that populate() be called in
virtually every method in the base registry in Djblets. A recent change
redid how we stored settings and when populate() 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 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 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 in load_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 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 is False, 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.

Description From Last Updated

E501 line too long (81 > 79 characters)

reviewbotreviewbot

F841 local variable 'siteconfig_settings' is assigned to but never used

reviewbotreviewbot

typo: 3.04+ -> 3.0.4+

daviddavid

Docstring?

daviddavid
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

chipx86
chipx86
david
  1. 
      
  2. reviewboard/avatars/registry.py (Diff revision 2)
     
     

    typo: 3.04+ -> 3.0.4+

  3. reviewboard/avatars/tests.py (Diff revision 2)
     
     

    Docstring?

  4. 
      
chipx86
david
  1. Ship It!
  2. 
      
chipx86
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (2f03cd4)
Loading...