Summary

Add the avatar services registry

Review Request #7809 — Created Dec. 15, 2015 and submitted Feb. 22, 2016, 7:21 a.m.

Information

Owner

brennie

Repository

Review Board

Branch

release-2.6.x

Bugs

Depends On

~~7844~~

Blocks

7810

Reviewers

Groups

reviewboard

People

Description

The avatar services registry is a new object for managing avatar
services, which are defined to provide Review Board multiple ways of
displaying user avatars. Currently, the only supported avatar service
is the Gravatar (http://gravatar.com) service, which is what Review
Board supported previously.

This patch, however, does not migrate to use the new avatar services.

Generated serach indexes have also been added to .gitignore.

Testing Done

Ran unit tests.
Used this in an upcoming patch.

Issues

Description	From	Last Updated
Should use ugettext_lazy	david	Dec. 23, 2015, 6:21 p.m.
Docstring?	david	Dec. 23, 2015, 6:21 p.m.
Since we render with a size specified, we should probably include width and height parameters in this tag. Also, can …	david	Dec. 23, 2015, 6:42 p.m.
We should mark_safe this.	david	Dec. 22, 2015, 2:52 p.m.
Col: 25 E241 multiple spaces after ':'	reviewbot	Dec. 16, 2015, 9:57 p.m.
Col: 29 E241 multiple spaces after ':'	reviewbot	Dec. 16, 2015, 9:57 p.m.
Col: 19 E241 multiple spaces after ':'	reviewbot	Dec. 16, 2015, 9:57 p.m.
Col: 26 E241 multiple spaces after ':'	reviewbot	Dec. 16, 2015, 9:57 p.m.
Col: 23 E241 multiple spaces after ':'	reviewbot	Dec. 16, 2015, 9:57 p.m.
Col: 28 E241 multiple spaces after ':'	reviewbot	Dec. 16, 2015, 9:57 p.m.
Col: 30 E241 multiple spaces after ':'	reviewbot	Dec. 16, 2015, 9:57 p.m.
Col: 30 E241 multiple spaces after ':'	reviewbot	Dec. 16, 2015, 9:57 p.m.
Col: 26 E241 multiple spaces after ':'	reviewbot	Dec. 16, 2015, 9:57 p.m.
Col: 26 E241 multiple spaces after ':'	reviewbot	Dec. 16, 2015, 9:57 p.m.
Col: 22 E241 multiple spaces after ':'	reviewbot	Dec. 16, 2015, 9:57 p.m.
Col: 22 E241 multiple spaces after ':'	reviewbot	Dec. 16, 2015, 9:57 p.m.
Col: 17 E241 multiple spaces after ':'	reviewbot	Dec. 16, 2015, 9:57 p.m.
Col: 26 E241 multiple spaces after ':'	reviewbot	Dec. 16, 2015, 9:57 p.m.
Col: 28 E241 multiple spaces after ':'	reviewbot	Dec. 16, 2015, 9:57 p.m.
Col: 30 E241 multiple spaces after ':'	reviewbot	Dec. 16, 2015, 9:57 p.m.
Col: 31 E241 multiple spaces after ':'	reviewbot	Dec. 16, 2015, 9:57 p.m.
Col: 31 E241 multiple spaces after ':'	reviewbot	Dec. 16, 2015, 9:57 p.m.
Col: 23 E241 multiple spaces after ':'	reviewbot	Dec. 16, 2015, 9:57 p.m.
Col: 30 E241 multiple spaces after ':'	reviewbot	Dec. 16, 2015, 9:57 p.m.
Col: 32 E241 multiple spaces after ':'	reviewbot	Dec. 16, 2015, 9:57 p.m.
I know it wasn't already, but can we alphabetize this list?	david	Dec. 23, 2015, 6:27 p.m.
Perhaps we should rename these so they start with avatar_service so they all bunch together?	david	Jan. 4, 2016, 4:08 p.m.
Let's go into a little more detail on this, at the very least saying "avatar services," but ideally clarifying what …	chipx86	Jan. 4, 2016, 11:02 p.m.
Swap these.	chipx86	Jan. 4, 2016, 10:59 a.m.
Ordering is backwards.	chipx86	Jan. 4, 2016, 10:59 a.m.
Similar comment to my other review. We should have a capital letter after the ":", since that's typically how errors …	chipx86	Jan. 4, 2016, 4:11 p.m.
Blank line between these.	chipx86	Jan. 4, 2016, 4:21 p.m.
A property should generally have the same type as both the getter and setter. In this case, we're using a …	chipx86	Jan. 4, 2016, 4:18 p.m.
~ before the class path?	chipx86	Jan. 4, 2016, 4:19 p.m.
Localized?	chipx86	Jan. 4, 2016, 4:19 p.m.
Seeing a lot of inconsistency with the class paths in the docs. Some say reviewboard.accounts.avatars.services, some reviewboard.avatars.service, and some reviewboard.avatars.services.	chipx86	Jan. 4, 2016, 10:59 a.m.
Might wrap better as: raise ItemLookupError( '...' % service) Same below. Also, these should be localized.	chipx86	Jan. 4, 2016, 4:23 p.m.
I'm not sure it's clear what this really means, as written. Same below. Can we say what saving means, or …	chipx86	Jan. 4, 2016, 4:24 p.m.
Sort of hitting a parse error with this sentence.	chipx86	Jan. 4, 2016, 4:25 p.m.
Not necessarily true. Maybe: Whether the registry of avatars was newly populated by this call. If ``False``, the registry was …	chipx86	Jan. 4, 2016, 4:26 p.m.
We know we're saving below, so probably no need to do that here.	chipx86	Jan. 4, 2016, 4:27 p.m.
Should pass the service ID as a parameter, rather than a format string.	chipx86	Jan. 4, 2016, 11:02 p.m.
"set"/"unsetting" doesn't mean a lot outside of the internals of the registries, I don't think. Maybe we can reword this?	chipx86	Jan. 4, 2016, 11:02 p.m.
This is an internal detail I'd really rather not expose. It's cheap to fetch the SiteConfiguration, since we cache it, …	chipx86	Jan. 4, 2016, 11:02 p.m.
Since this is something that developers will be interacting with, this should go into a lot more detail on what …	chipx86	Jan. 4, 2016, 11:04 p.m.
This should go into more detail on what's expected and what's provided to the template.	chipx86	Jan. 4, 2016, 11:04 p.m.
How about "The user whose avatar URL will be retrieved."	chipx86	Jan. 4, 2016, 11:04 p.m.
Let's add "in pixels," and that this will be applied to both the width and the height.	chipx86	Jan. 4, 2016, 11:04 p.m.
"... to HTML." This should go into more detail about how it's rendered (using template), and that it can be …	chipx86	Jan. 4, 2016, 11:20 p.m.
Let's cache these on the request, taking into account the user ID and size. This will give us a nice …	chipx86	Jan. 5, 2016, 3 p.m.
Let's have this in its own file. Refer to my thoughts at the top about directory structure.	chipx86	Jan. 4, 2016, 11:22 p.m.
Two blank lines.	chipx86	Jan. 5, 2016, 2:46 p.m.
This is kind of oddly written, and doesn't make it clear that service_id can impact this. This should be more …	chipx86	Jan. 5, 2016, 3 p.m.
Let's pass the values as keyword arguments, in case we extend down the road.	chipx86	Jan. 5, 2016, 3 p.m.
Col: 23 E241 multiple spaces after ':'	reviewbot	Dec. 30, 2015, 12:28 p.m.
Col: 25 E241 multiple spaces after ':'	reviewbot	Dec. 30, 2015, 12:28 p.m.
Col: 26 E241 multiple spaces after ':'	reviewbot	Dec. 30, 2015, 12:28 p.m.
Col: 23 E241 multiple spaces after ':'	reviewbot	Dec. 30, 2015, 12:28 p.m.
Col: 19 E241 multiple spaces after ':'	reviewbot	Dec. 30, 2015, 12:28 p.m.
Col: 30 E241 multiple spaces after ':'	reviewbot	Dec. 30, 2015, 12:28 p.m.
Col: 28 E241 multiple spaces after ':'	reviewbot	Dec. 30, 2015, 12:28 p.m.
Col: 30 E241 multiple spaces after ':'	reviewbot	Dec. 30, 2015, 12:28 p.m.
Col: 26 E241 multiple spaces after ':'	reviewbot	Dec. 30, 2015, 12:28 p.m.
Col: 26 E241 multiple spaces after ':'	reviewbot	Dec. 30, 2015, 12:28 p.m.
Col: 29 E241 multiple spaces after ':'	reviewbot	Dec. 30, 2015, 12:28 p.m.
Col: 30 E241 multiple spaces after ':'	reviewbot	Dec. 30, 2015, 12:28 p.m.
Col: 31 E241 multiple spaces after ':'	reviewbot	Dec. 30, 2015, 12:28 p.m.
Col: 30 E241 multiple spaces after ':'	reviewbot	Dec. 30, 2015, 12:28 p.m.
Col: 31 E241 multiple spaces after ':'	reviewbot	Dec. 30, 2015, 12:28 p.m.
Col: 32 E241 multiple spaces after ':'	reviewbot	Dec. 30, 2015, 12:28 p.m.
Col: 22 E241 multiple spaces after ':'	reviewbot	Dec. 30, 2015, 12:28 p.m.
Col: 26 E241 multiple spaces after ':'	reviewbot	Dec. 30, 2015, 12:28 p.m.
Col: 28 E241 multiple spaces after ':'	reviewbot	Dec. 30, 2015, 12:28 p.m.
Col: 17 E241 multiple spaces after ':'	reviewbot	Dec. 30, 2015, 12:28 p.m.
Col: 22 E241 multiple spaces after ':'	reviewbot	Dec. 30, 2015, 12:28 p.m.
Col: 31 E241 multiple spaces after ':'	reviewbot	Jan. 6, 2016, 10:03 p.m.
Col: 39 E241 multiple spaces after ':'	reviewbot	Jan. 6, 2016, 10:03 p.m.
Col: 32 E241 multiple spaces after ':'	reviewbot	Jan. 6, 2016, 10:03 p.m.
Col: 25 E241 multiple spaces after ':'	reviewbot	Jan. 6, 2016, 10:03 p.m.
Col: 26 E241 multiple spaces after ':'	reviewbot	Jan. 6, 2016, 10:03 p.m.
Col: 23 E241 multiple spaces after ':'	reviewbot	Jan. 6, 2016, 10:03 p.m.
Col: 19 E241 multiple spaces after ':'	reviewbot	Jan. 6, 2016, 10:03 p.m.
Col: 30 E241 multiple spaces after ':'	reviewbot	Jan. 6, 2016, 10:03 p.m.
Col: 28 E241 multiple spaces after ':'	reviewbot	Jan. 6, 2016, 10:03 p.m.
Col: 30 E241 multiple spaces after ':'	reviewbot	Jan. 6, 2016, 10:04 p.m.
Col: 26 E241 multiple spaces after ':'	reviewbot	Jan. 6, 2016, 10:04 p.m.
Col: 26 E241 multiple spaces after ':'	reviewbot	Jan. 6, 2016, 10:04 p.m.
Col: 29 E241 multiple spaces after ':'	reviewbot	Jan. 6, 2016, 10:04 p.m.
Col: 30 E241 multiple spaces after ':'	reviewbot	Jan. 6, 2016, 10:04 p.m.
Col: 31 E241 multiple spaces after ':'	reviewbot	Jan. 6, 2016, 10:04 p.m.
Col: 22 E241 multiple spaces after ':'	reviewbot	Jan. 6, 2016, 10:04 p.m.
Col: 26 E241 multiple spaces after ':'	reviewbot	Jan. 6, 2016, 10:04 p.m.
Col: 28 E241 multiple spaces after ':'	reviewbot	Jan. 6, 2016, 10:04 p.m.
Col: 17 E241 multiple spaces after ':'	reviewbot	Jan. 6, 2016, 10:04 p.m.
Col: 22 E241 multiple spaces after ':'	reviewbot	Jan. 6, 2016, 10:04 p.m.
Two blank lines.	chipx86	Jan. 16, 2016, 10:15 a.m.
This needs more detail on what this does and how it works, for people not familiar with what we're doing.	chipx86	Jan. 16, 2016, 10:59 a.m.
This shouldn't have to be called here. It's already managed by the middleware and app initialize stuff, and when setting …	chipx86	Jan. 16, 2016, 10:15 a.m.
This makes me nervous. Can we just set the things we need?	chipx86	Jan. 16, 2016, 10:15 a.m.
'django_reset' imported but unused	reviewbot	Jan. 6, 2016, 10:04 p.m.
'from settings_local import *' used; unable to detect undefined names	reviewbot	Jan. 6, 2016, 10:04 p.m.
'PIPELINE_CSS' imported but unused	reviewbot	Jan. 6, 2016, 10:04 p.m.
'PIPELINE_JS' imported but unused	reviewbot	Jan. 6, 2016, 10:04 p.m.
Col: 23 E241 multiple spaces after ':'	reviewbot	Jan. 17, 2016, 6:25 a.m.
Things are misaligned here. Can we just shift this to be pep-8 style (even though it's not quite as pretty …	david	Jan. 27, 2016, 6:35 p.m.
Col: 31 E241 multiple spaces after ':'	reviewbot	Jan. 17, 2016, 6:25 a.m.
Col: 24 E241 multiple spaces after ':'	reviewbot	Jan. 17, 2016, 6:25 a.m.
Col: 25 E241 multiple spaces after ':'	reviewbot	Jan. 17, 2016, 6:25 a.m.
Col: 26 E241 multiple spaces after ':'	reviewbot	Jan. 17, 2016, 6:25 a.m.
Col: 23 E241 multiple spaces after ':'	reviewbot	Jan. 17, 2016, 6:25 a.m.
Col: 19 E241 multiple spaces after ':'	reviewbot	Jan. 17, 2016, 6:25 a.m.
Col: 30 E241 multiple spaces after ':'	reviewbot	Jan. 17, 2016, 6:25 a.m.
Col: 28 E241 multiple spaces after ':'	reviewbot	Jan. 17, 2016, 6:25 a.m.
Col: 30 E241 multiple spaces after ':'	reviewbot	Jan. 17, 2016, 6:25 a.m.
Col: 26 E241 multiple spaces after ':'	reviewbot	Jan. 17, 2016, 6:25 a.m.
Col: 26 E241 multiple spaces after ':'	reviewbot	Jan. 17, 2016, 6:25 a.m.
Col: 29 E241 multiple spaces after ':'	reviewbot	Jan. 17, 2016, 6:25 a.m.
Col: 30 E241 multiple spaces after ':'	reviewbot	Jan. 17, 2016, 6:25 a.m.
Col: 31 E241 multiple spaces after ':'	reviewbot	Jan. 17, 2016, 6:25 a.m.
Col: 22 E241 multiple spaces after ':'	reviewbot	Jan. 17, 2016, 6:25 a.m.
Col: 26 E241 multiple spaces after ':'	reviewbot	Jan. 17, 2016, 6:25 a.m.
Col: 28 E241 multiple spaces after ':'	reviewbot	Jan. 17, 2016, 6:25 a.m.
Col: 17 E241 multiple spaces after ':'	reviewbot	Jan. 17, 2016, 6:25 a.m.
Col: 22 E241 multiple spaces after ':'	reviewbot	Jan. 17, 2016, 6:25 a.m.
This should all fit on one line.	david	Jan. 27, 2016, 6:35 p.m.
'django_reset' imported but unused	reviewbot	Jan. 17, 2016, 6:25 a.m.
'from settings_local import *' used; unable to detect undefined names	reviewbot	Jan. 17, 2016, 6:25 a.m.
'PIPELINE_CSS' imported but unused	reviewbot	Jan. 17, 2016, 6:25 a.m.
'PIPELINE_JS' imported but unused	reviewbot	Jan. 17, 2016, 6:25 a.m.
'django_reset' imported but unused	reviewbot	Jan. 27, 2016, 7:12 p.m.
'from settings_local import *' used; unable to detect undefined names	reviewbot	Jan. 27, 2016, 7:12 p.m.
'PIPELINE_CSS' imported but unused	reviewbot	Jan. 27, 2016, 7:12 p.m.
'PIPELINE_JS' imported but unused	reviewbot	Jan. 27, 2016, 7:12 p.m.

Tool: Pyflakes
Processed Files:
    reviewboard/accounts/avatars/tests.py
    reviewboard/accounts/avatars/registry.py
    reviewboard/admin/siteconfig.py
    reviewboard/accounts/avatars/services.py
    reviewboard/accounts/avatars/errors.py
    reviewboard/accounts/templatetags/avatars.py

Ignored Files:
    reviewboard/accounts/avatars/__init__.py
    reviewboard/accounts/templatetags/__init__.py
    .gitignore



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/accounts/avatars/tests.py
    reviewboard/accounts/avatars/registry.py
    reviewboard/admin/siteconfig.py
    reviewboard/accounts/avatars/services.py
    reviewboard/accounts/avatars/errors.py
    reviewboard/accounts/templatetags/avatars.py

Ignored Files:
    reviewboard/accounts/avatars/__init__.py
    reviewboard/accounts/templatetags/__init__.py
    .gitignore

reviewboard/admin/siteconfig.py (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 25
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 29
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 19
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 26
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 23
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 28
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 30
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 30
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 26
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 26
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 22
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 22
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 17
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 26
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 28
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 30
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 31
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 31
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 23
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 30
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 32
 E241 multiple spaces after ':'
```

reviewboard/accounts/avatars/registry.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Should use ugettext_lazy
```
reviewboard/accounts/avatars/registry.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Docstring?
```

reviewboard/accounts/avatars/services.py (Diff revision 1)

The issue has been resolved. Show all issues

Since we render with a size specified, we should probably include width and height parameters in this tag.

Also, can you double check that things are properly escaped when rendering this?

reviewboard/accounts/avatars/services.py (Diff revision 1)
The issue has been dropped. Show all issues
```
We should mark_safe this.
```
1. brennie Dec. 22, 2015, 2:53 p.m.
  Template.render returns a SafeText.
reviewboard/admin/siteconfig.py (Diff revision 1)
The issue has been resolved. Show all issues
```
I know it wasn't already, but can we alphabetize this list?
```

Change Summary:

Address David's issues.

Diff:

Revision 2 (+653 -17)

Show changes

	reviewboard/accounts/avatars/__init__.py
	reviewboard/accounts/avatars/errors.py
	reviewboard/accounts/avatars/registry.py
	reviewboard/accounts/avatars/services.py
	reviewboard/accounts/avatars/tests.py
	reviewboard/accounts/templatetags/__init__.py
	reviewboard/accounts/templatetags/avatars.py
	reviewboard/admin/siteconfig.py

reviewboard/admin/siteconfig.py (Diff revisions 1 - 2)

The issue has been resolved. Show all issues

Perhaps we should rename these so they start with avatar_service so they all bunch together?

The directory structure's feeling a bit deep. I get that avatars relate to accounts, but they're not necessarily a part of an account. I can see, for instance, us enhancing this later to let us get an avatar for a remote service, like a GitHub account, without having a local account. Maybe.
Anyway, room for debate here, but I feel like reviewboard.avatars is a fine namespace for this. Allows this to really be solely about avatars, without needing to be, like, a utility service of accounts (which the current directory structure implies).
It also allows us to more easily break services.py up into services/base.py and services/gravatar.py. That'll be good down the road, in case we want to add some other built-in services (github.py, custom_url.py, or whatever).

reviewboard/accounts/avatars/errors.py (Diff revision 2)

The issue has been resolved. Show all issues

Let's go into a little more detail on this, at the very least saying "avatar services," but ideally clarifying what this means more.

reviewboard/accounts/avatars/registry.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Swap these.
```
reviewboard/accounts/avatars/registry.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Ordering is backwards.
```

reviewboard/accounts/avatars/registry.py (Diff revision 2)

The issue has been resolved. Show all issues

Similar comment to my other review. We should have a capital letter after the ":", since that's typically how errors will be represented (as another exception's message is often used as the "reason" for an error here).

We also usually have a form more like:

Could not register the avatar service %(item)s: This service is already registered.

Also, this one is missing a trailing period.

reviewboard/accounts/avatars/registry.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Blank line between these.
```
1. brennie Dec. 30, 2015, 12:29 p.m.
  Why?
2. chipx86 Jan. 4, 2016, 4:20 p.m.
  Most of our code follows that. Not all, but most. So primarily, consistency.

reviewboard/accounts/avatars/registry.py (Diff revision 2)

The issue has been resolved. Show all issues

A property should generally have the same type as both the getter and setter. In this case, we're using a set value of a list, and a get of a generator. I feel consistency is preferable here, particularly since we're never going to really have more than one or two services enabled here.

reviewboard/accounts/avatars/registry.py (Diff revision 2)
The issue has been resolved. Show all issues
```
~ before the class path?
```
reviewboard/accounts/avatars/registry.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Localized?
```

reviewboard/accounts/avatars/registry.py (Diff revision 2)

The issue has been resolved. Show all issues

Seeing a lot of inconsistency with the class paths in the docs. Some say reviewboard.accounts.avatars.services, some reviewboard.avatars.service, and some reviewboard.avatars.services.

reviewboard/accounts/avatars/registry.py (Diff revision 2)

The issue has been resolved. Show all issues

Might wrap better as:

raise ItemLookupError(
    '...'
    % service)

Same below.

Also, these should be localized.

reviewboard/accounts/avatars/registry.py (Diff revision 2)

The issue has been resolved. Show all issues

I'm not sure it's clear what this really means, as written. Same below.

Can we say what saving means, or maybe rewrite it if it's something like "Whether or not the list of enabled services should be saved to the database"? "Registry" isn't necessarily as clear.

reviewboard/accounts/avatars/registry.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Sort of hitting a parse error with this sentence.
```

reviewboard/accounts/avatars/registry.py (Diff revision 2)

The issue has been resolved. Show all issues

Not necessarily true. Maybe:

Whether the registry of avatars was newly populated by this call. If ``False``, the registry was already populated by a previosu call.

Also, do we use this anywhere? Might have missed it.

brennie Dec. 30, 2015, 12:29 p.m.

If you want to override populate and call the super method, you have to check super(type(self), self).populate() to see if it is already populated; if it is, then we can exit early. See line 218.

reviewboard/accounts/avatars/registry.py (Diff revision 2)
The issue has been resolved. Show all issues
```
We know we're saving below, so probably no need to do that here.
```
reviewboard/accounts/avatars/registry.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Should pass the service ID as a parameter, rather than a format string.
```

reviewboard/accounts/avatars/registry.py (Diff revision 2)

The issue has been resolved. Show all issues

"set"/"unsetting" doesn't mean a lot outside of the internals of the registries, I don't think. Maybe we can reword this?

reviewboard/accounts/avatars/registry.py (Diff revision 2)

The issue has been resolved. Show all issues

This is an internal detail I'd really rather not expose. It's cheap to fetch the SiteConfiguration, since we cache it, so let's just do that here.

reviewboard/accounts/avatars/services.py (Diff revision 2)

The issue has been resolved. Show all issues

Since this is something that developers will be interacting with, this should go into a lot more detail on what this is responsible for, how it's used, and what's expected by an implementor.

reviewboard/accounts/avatars/services.py (Diff revision 2)
The issue has been resolved. Show all issues
```
This should go into more detail on what's expected and what's provided to the template.
```
reviewboard/accounts/avatars/services.py (Diff revision 2)
The issue has been resolved. Show all issues
```
How about "The user whose avatar URL will be retrieved."
```
reviewboard/accounts/avatars/services.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Let's add "in pixels," and that this will be applied to both the width and the height.
```

reviewboard/accounts/avatars/services.py (Diff revision 2)

The issue has been resolved. Show all issues

"... to HTML."

This should go into more detail about how it's rendered (using template), and that it can be overridden for custom behavior.

reviewboard/accounts/avatars/services.py (Diff revision 2)

The issue has been resolved. Show all issues

Let's cache these on the request, taking into account the user ID and size. This will give us a nice speed boost. We should also mention that in the docs.

reviewboard/accounts/avatars/services.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Let's have this in its own file. Refer to my thoughts at the top about directory structure.
```
reviewboard/accounts/templatetags/avatars.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Two blank lines.
```

reviewboard/accounts/templatetags/avatars.py (Diff revision 2)

The issue has been resolved. Show all issues

This is kind of oddly written, and doesn't make it clear that service_id can impact this. This should be more explicit.

reviewboard/accounts/templatetags/avatars.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Let's pass the values as keyword arguments, in case we extend down the road.
```

Oh, also, I feel like this would be a great thing to have in Djblets, since avatars are a very generic concept.

Tool: Pyflakes
Processed Files:
    reviewboard/accounts/avatars/tests.py
    reviewboard/accounts/avatars/registry.py
    reviewboard/admin/siteconfig.py
    reviewboard/accounts/avatars/services.py
    reviewboard/accounts/avatars/errors.py
    reviewboard/accounts/templatetags/avatars.py

Ignored Files:
    reviewboard/accounts/avatars/__init__.py
    reviewboard/accounts/templatetags/__init__.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/accounts/avatars/tests.py
    reviewboard/accounts/avatars/registry.py
    reviewboard/admin/siteconfig.py
    reviewboard/accounts/avatars/services.py
    reviewboard/accounts/avatars/errors.py
    reviewboard/accounts/templatetags/avatars.py

Ignored Files:
    reviewboard/accounts/avatars/__init__.py
    reviewboard/accounts/templatetags/__init__.py

reviewboard/admin/siteconfig.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Col: 23
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Col: 25
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Col: 26
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Col: 23
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Col: 19
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Col: 30
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Col: 28
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Col: 30
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Col: 26
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Col: 26
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Col: 29
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Col: 30
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Col: 31
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Col: 30
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Col: 31
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Col: 32
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Col: 22
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Col: 26
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Col: 28
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Col: 17
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Col: 22
 E241 multiple spaces after ':'
```

Change Summary:

Move most of the registry to Djblets (https://reviews.reviewboard.org/r/7844/)

Address issues.

Depends On:

~~7844 - Add an Avatar Service Registry to Djblets~~

Diff:

Revision 3 (+336 -17)

Show changes

	reviewboard/settings.py
	reviewboard/admin/siteconfig.py
	reviewboard/avatars/__init__.py
	reviewboard/avatars/registry.py
	reviewboard/avatars/tests.py
	reviewboard/avatars/templatetags/__init__.py
	reviewboard/avatars/templatetags/avatars.py

Tool: Pyflakes
Processed Files:
    reviewboard/admin/siteconfig.py
    reviewboard/avatars/__init__.py
    reviewboard/settings.py
    reviewboard/avatars/registry.py
    reviewboard/avatars/templatetags/avatars.py
    reviewboard/avatars/tests.py

Ignored Files:
    reviewboard/avatars/templatetags/__init__.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/admin/siteconfig.py
    reviewboard/avatars/__init__.py
    reviewboard/settings.py
    reviewboard/avatars/registry.py
    reviewboard/avatars/templatetags/avatars.py
    reviewboard/avatars/tests.py

Ignored Files:
    reviewboard/avatars/templatetags/__init__.py

reviewboard/admin/siteconfig.py (Diff revision 3)
The issue has been dropped. Show all issues
```
Col: 31
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 3)
The issue has been dropped. Show all issues
```
Col: 39
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 3)
The issue has been dropped. Show all issues
```
Col: 32
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 3)
The issue has been dropped. Show all issues
```
Col: 25
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 3)
The issue has been dropped. Show all issues
```
Col: 26
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 3)
The issue has been dropped. Show all issues
```
Col: 23
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 3)
The issue has been dropped. Show all issues
```
Col: 19
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 3)
The issue has been dropped. Show all issues
```
Col: 30
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 3)
The issue has been dropped. Show all issues
```
Col: 28
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 3)
The issue has been dropped. Show all issues
```
Col: 30
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 3)
The issue has been dropped. Show all issues
```
Col: 26
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 3)
The issue has been dropped. Show all issues
```
Col: 26
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 3)
The issue has been dropped. Show all issues
```
Col: 29
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 3)
The issue has been dropped. Show all issues
```
Col: 30
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 3)
The issue has been dropped. Show all issues
```
Col: 31
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 3)
The issue has been dropped. Show all issues
```
Col: 22
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 3)
The issue has been dropped. Show all issues
```
Col: 26
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 3)
The issue has been dropped. Show all issues
```
Col: 28
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 3)
The issue has been dropped. Show all issues
```
Col: 17
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 3)
The issue has been dropped. Show all issues
```
Col: 22
 E241 multiple spaces after ':'
```
reviewboard/settings.py (Diff revision 3)
The issue has been dropped. Show all issues
```
 'django_reset' imported but unused
```
reviewboard/settings.py (Diff revision 3)
The issue has been dropped. Show all issues
```
 'from settings_local import *' used; unable to detect undefined names
```
reviewboard/settings.py (Diff revision 3)
The issue has been dropped. Show all issues
```
 'PIPELINE_CSS' imported but unused
```
reviewboard/settings.py (Diff revision 3)
The issue has been dropped. Show all issues
```
 'PIPELINE_JS' imported but unused
```

Typo in the description: "serach". That should probably just be its own trivial change, though.

reviewboard/avatars/__init__.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Two blank lines.
```

reviewboard/avatars/registry.py (Diff revision 3)

The issue has been resolved. Show all issues

This needs more detail on what this does and how it works, for people not familiar with what we're doing.

reviewboard/avatars/registry.py (Diff revision 3)

The issue has been resolved. Show all issues

This shouldn't have to be called here. It's already managed by the middleware and app initialize stuff, and when setting new configuration for the site. This should be able to rely on the existing calls.

reviewboard/avatars/tests.py (Diff revision 3)
The issue has been resolved. Show all issues
```
This makes me nervous. Can we just set the things we need?
```

Change Summary:

Address Christians issues.

Diff:

Revision 4 (+345 -17)

Show changes

	reviewboard/settings.py
	reviewboard/admin/siteconfig.py
	reviewboard/avatars/__init__.py
	reviewboard/avatars/registry.py
	reviewboard/avatars/tests.py
	reviewboard/avatars/templatetags/__init__.py
	reviewboard/avatars/templatetags/avatars.py

Tool: Pyflakes
Processed Files:
    reviewboard/admin/siteconfig.py
    reviewboard/avatars/__init__.py
    reviewboard/settings.py
    reviewboard/avatars/registry.py
    reviewboard/avatars/templatetags/avatars.py
    reviewboard/avatars/tests.py

Ignored Files:
    reviewboard/avatars/templatetags/__init__.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/admin/siteconfig.py
    reviewboard/avatars/__init__.py
    reviewboard/settings.py
    reviewboard/avatars/registry.py
    reviewboard/avatars/templatetags/avatars.py
    reviewboard/avatars/tests.py

Ignored Files:
    reviewboard/avatars/templatetags/__init__.py

reviewboard/admin/siteconfig.py (Diff revision 4)
The issue has been dropped. Show all issues
```
Col: 23
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 4)
The issue has been dropped. Show all issues
```
Col: 31
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 4)
The issue has been dropped. Show all issues
```
Col: 24
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 4)
The issue has been dropped. Show all issues
```
Col: 25
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 4)
The issue has been dropped. Show all issues
```
Col: 26
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 4)
The issue has been dropped. Show all issues
```
Col: 23
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 4)
The issue has been dropped. Show all issues
```
Col: 19
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 4)
The issue has been dropped. Show all issues
```
Col: 30
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 4)
The issue has been dropped. Show all issues
```
Col: 28
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 4)
The issue has been dropped. Show all issues
```
Col: 30
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 4)
The issue has been dropped. Show all issues
```
Col: 26
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 4)
The issue has been dropped. Show all issues
```
Col: 26
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 4)
The issue has been dropped. Show all issues
```
Col: 29
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 4)
The issue has been dropped. Show all issues
```
Col: 30
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 4)
The issue has been dropped. Show all issues
```
Col: 31
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 4)
The issue has been dropped. Show all issues
```
Col: 22
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 4)
The issue has been dropped. Show all issues
```
Col: 26
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 4)
The issue has been dropped. Show all issues
```
Col: 28
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 4)
The issue has been dropped. Show all issues
```
Col: 17
 E241 multiple spaces after ':'
```
reviewboard/admin/siteconfig.py (Diff revision 4)
The issue has been dropped. Show all issues
```
Col: 22
 E241 multiple spaces after ':'
```
reviewboard/settings.py (Diff revision 4)
The issue has been dropped. Show all issues
```
 'django_reset' imported but unused
```
reviewboard/settings.py (Diff revision 4)
The issue has been dropped. Show all issues
```
 'from settings_local import *' used; unable to detect undefined names
```
reviewboard/settings.py (Diff revision 4)
The issue has been dropped. Show all issues
```
 'PIPELINE_CSS' imported but unused
```
reviewboard/settings.py (Diff revision 4)
The issue has been dropped. Show all issues
```
 'PIPELINE_JS' imported but unused
```

reviewboard/admin/siteconfig.py (Diff revision 4)

The issue has been resolved. Show all issues

Things are misaligned here. Can we just shift this to be pep-8 style (even though it's not quite as pretty as aligned)?

reviewboard/avatars/templatetags/avatars.py (Diff revision 4)
The issue has been resolved. Show all issues
```
This should all fit on one line.
```

Diff:

Revision 5 (+342 -16)

Show changes

	reviewboard/settings.py
	reviewboard/admin/siteconfig.py
	reviewboard/avatars/__init__.py
	reviewboard/avatars/registry.py
	reviewboard/avatars/tests.py
	reviewboard/avatars/templatetags/__init__.py
	reviewboard/avatars/templatetags/avatars.py

Tool: Pyflakes
Processed Files:
    reviewboard/admin/siteconfig.py
    reviewboard/avatars/__init__.py
    reviewboard/settings.py
    reviewboard/avatars/registry.py
    reviewboard/avatars/templatetags/avatars.py
    reviewboard/avatars/tests.py

Ignored Files:
    reviewboard/avatars/templatetags/__init__.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/admin/siteconfig.py
    reviewboard/avatars/__init__.py
    reviewboard/settings.py
    reviewboard/avatars/registry.py
    reviewboard/avatars/templatetags/avatars.py
    reviewboard/avatars/tests.py

Ignored Files:
    reviewboard/avatars/templatetags/__init__.py

reviewboard/settings.py (Diff revision 5)
The issue has been dropped. Show all issues
```
 'django_reset' imported but unused
```
reviewboard/settings.py (Diff revision 5)
The issue has been dropped. Show all issues
```
 'from settings_local import *' used; unable to detect undefined names
```
reviewboard/settings.py (Diff revision 5)
The issue has been dropped. Show all issues
```
 'PIPELINE_CSS' imported but unused
```
reviewboard/settings.py (Diff revision 5)
The issue has been dropped. Show all issues
```
 'PIPELINE_JS' imported but unused
```

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-2.6.x (0335bfc)

Status:: Re-opened

Status:: Completed
Change Summary:: Pushed to release-2.6.x (4b9342b)