Summary

Migrate to using avatar services everywhere

Review Request #7810 — Created Dec. 15, 2015 and submitted Feb. 22, 2016, 11:14 p.m.

Information

Owner

brennie

Repository

Review Board

Branch

release-2.6.x

Bugs

Depends On

~~7809~~

Reviewers

Groups

reviewboard

People

Description

Review Board now uses avatar services everywhere that gravatars were
previously used.

The admin panel has been updated to reflect this chnage. There is no
longer a setting for gravatars, specfically. Instead, there is now an
entire page of avatar service configuration.

Testing Done

Ran unit tests.

Performed the following manual testing:
- Verified enabling/disabling avatars entirely worked correctly.
- Verified the avatar services admin form saves correctly.
- Viewd the search page and datagrids to verify that avatars render
correctly.

Files

Issues

Description	From	Last Updated
So my major complaint about this change is this UI. I think it's kind of confusing that there are a …	david	Feb. 22, 2016, 7:19 a.m.
'six' imported but unused	reviewbot	Dec. 16, 2015, 9:59 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	Dec. 16, 2015, 9:59 p.m.
Hmmm. We'll have to figure out how this interacts with $.retinaGravatar, which expects the .gravatar class. Perhaps we can add …	david	Jan. 6, 2016, 5:47 p.m.
break?	david	Jan. 6, 2016, 5:47 p.m.
typo: resoltuion	misery	Feb. 22, 2016, 7:16 a.m.
typo: emptyu	misery	Feb. 22, 2016, 7:16 a.m.
It would be nicer if we did the fallback for urls in the assignment: var urls = this.get('avatarURLs') \|\| {}; …	david	Feb. 22, 2016, 7:20 a.m.

Tool: Pyflakes
Processed Files:
    reviewboard/admin/forms.py
    reviewboard/datagrids/columns.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/admin/urls.py
    reviewboard/search/views.py
    reviewboard/webapi/resources/user.py

Ignored Files:
    reviewboard/templates/search/_user.html
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/templates/reviews/review_reply.html
    reviewboard/templates/accounts/user_infobox.html
    reviewboard/templates/base/_nav_support_menu.html
    reviewboard/templates/reviews/boxes/change.html
    reviewboard/templates/datagrids/sidebar_user_info.html
    reviewboard/templates/admin/avatar_settings.html
    reviewboard/templates/reviews/review_issue_summary_table.html
    reviewboard/templates/base/_mobile_navbar.html
    reviewboard/templates/admin/widgets/w-actions.html
    reviewboard/static/rb/js/views/reviewReplyEditorView.js
    reviewboard/static/rb/css/pages/reviews.less



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/admin/forms.py
    reviewboard/datagrids/columns.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/admin/urls.py
    reviewboard/search/views.py
    reviewboard/webapi/resources/user.py

Ignored Files:
    reviewboard/templates/search/_user.html
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/templates/reviews/review_reply.html
    reviewboard/templates/accounts/user_infobox.html
    reviewboard/templates/base/_nav_support_menu.html
    reviewboard/templates/reviews/boxes/change.html
    reviewboard/templates/datagrids/sidebar_user_info.html
    reviewboard/templates/admin/avatar_settings.html
    reviewboard/templates/reviews/review_issue_summary_table.html
    reviewboard/templates/base/_mobile_navbar.html
    reviewboard/templates/admin/widgets/w-actions.html
    reviewboard/static/rb/js/views/reviewReplyEditorView.js
    reviewboard/static/rb/css/pages/reviews.less

reviewboard/search/views.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 'six' imported but unused
```
reviewboard/search/views.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```

Change Summary:

PEP8

Diff:

Revision 2 (+264 -101)

Show changes

	reviewboard/admin/forms.py
	reviewboard/admin/urls.py
	reviewboard/datagrids/columns.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/search/views.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/views/reviewReplyEditorView.js
	reviewboard/templates/accounts/user_infobox.html
	11 more

Tool: Pyflakes
Processed Files:
    reviewboard/admin/forms.py
    reviewboard/datagrids/columns.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/admin/urls.py
    reviewboard/search/views.py
    reviewboard/webapi/resources/user.py

Ignored Files:
    reviewboard/templates/search/_user.html
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/templates/reviews/review_reply.html
    reviewboard/templates/accounts/user_infobox.html
    reviewboard/templates/base/_nav_support_menu.html
    reviewboard/templates/reviews/boxes/change.html
    reviewboard/templates/datagrids/sidebar_user_info.html
    reviewboard/templates/admin/avatar_settings.html
    reviewboard/templates/reviews/review_issue_summary_table.html
    reviewboard/templates/base/_mobile_navbar.html
    reviewboard/templates/admin/widgets/w-actions.html
    reviewboard/static/rb/js/views/reviewReplyEditorView.js
    reviewboard/static/rb/css/pages/reviews.less



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/admin/forms.py
    reviewboard/datagrids/columns.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/admin/urls.py
    reviewboard/search/views.py
    reviewboard/webapi/resources/user.py

Ignored Files:
    reviewboard/templates/search/_user.html
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/templates/reviews/review_reply.html
    reviewboard/templates/accounts/user_infobox.html
    reviewboard/templates/base/_nav_support_menu.html
    reviewboard/templates/reviews/boxes/change.html
    reviewboard/templates/datagrids/sidebar_user_info.html
    reviewboard/templates/admin/avatar_settings.html
    reviewboard/templates/reviews/review_issue_summary_table.html
    reviewboard/templates/base/_mobile_navbar.html
    reviewboard/templates/admin/widgets/w-actions.html
    reviewboard/static/rb/js/views/reviewReplyEditorView.js
    reviewboard/static/rb/css/pages/reviews.less

Screenshot(s)?

brennie Dec. 22, 2015, 3:06 p.m.

The UI hasn't changed, but I will upload some.

david Dec. 22, 2015, 3:07 p.m.

That doesn't jive with your change description: "Instead, there is now an entire page of avatar service configuration."

brennie Dec. 22, 2015, 3:11 p.m.

Ah of that :P I thought you meant of where avatars appeared. I derped.

Change Summary:

.container -> .avatar-container

add screenshots

Diff:

Revision 3 (+264 -101)

Show changes

	reviewboard/admin/forms.py
	reviewboard/admin/urls.py
	reviewboard/datagrids/columns.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/search/views.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/views/reviewReplyEditorView.js
	reviewboard/templates/accounts/user_infobox.html
	11 more

Added Files:

Tool: Pyflakes
Processed Files:
    reviewboard/admin/forms.py
    reviewboard/datagrids/columns.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/admin/urls.py
    reviewboard/search/views.py
    reviewboard/webapi/resources/user.py

Ignored Files:
    reviewboard/templates/search/_user.html
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/templates/reviews/review_reply.html
    reviewboard/templates/accounts/user_infobox.html
    reviewboard/templates/base/_nav_support_menu.html
    reviewboard/templates/reviews/boxes/change.html
    reviewboard/templates/datagrids/sidebar_user_info.html
    reviewboard/templates/admin/avatar_settings.html
    reviewboard/templates/reviews/review_issue_summary_table.html
    reviewboard/templates/base/_mobile_navbar.html
    reviewboard/templates/admin/widgets/w-actions.html
    reviewboard/static/rb/js/views/reviewReplyEditorView.js
    reviewboard/static/rb/css/pages/reviews.less



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/admin/forms.py
    reviewboard/datagrids/columns.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/admin/urls.py
    reviewboard/search/views.py
    reviewboard/webapi/resources/user.py

Ignored Files:
    reviewboard/templates/search/_user.html
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/templates/reviews/review_reply.html
    reviewboard/templates/accounts/user_infobox.html
    reviewboard/templates/base/_nav_support_menu.html
    reviewboard/templates/reviews/boxes/change.html
    reviewboard/templates/datagrids/sidebar_user_info.html
    reviewboard/templates/admin/avatar_settings.html
    reviewboard/templates/reviews/review_issue_summary_table.html
    reviewboard/templates/base/_mobile_navbar.html
    reviewboard/templates/admin/widgets/w-actions.html
    reviewboard/static/rb/js/views/reviewReplyEditorView.js
    reviewboard/static/rb/css/pages/reviews.less

reviewboard/datagrids/columns.py (Diff revision 3)

The issue has been resolved. Show all issues

Hmmm. We'll have to figure out how this interacts with $.retinaGravatar, which expects the .gravatar class. Perhaps we can add a data-at2x attribute?

chipx86 Dec. 30, 2015, 2:20 a.m.

A data-at2x would be ideal. In fact... Maybe we should rethink some of the API so that instead of getting a single avatar URL, you get a sort of dictionary of data consisting of the @1x size, @2x size, fallback, etc. Gives us room for expansion for things like @3x down the road (which is already becoming a thing for images on smartphones).

As for this class, I'm fine with us moving to .avatar, but we probably will need .gravatar as a fallback for that very reason, for $.retinaGravatar and such.

Also, I've been bit multiple times by the fact that .gravatar turns into .gravatar-retina. While rethinking avatar support, we should change things to add but not replace the classes, so maybe class="avatar avatar-2x", class="avatar avatar-fallback", etc.

brennie Jan. 4, 2016, 10:37 a.m.

If this change migrates into Djblets, we could replace $.retinaGravatar with $.retinaAvatar which uses this new behaviour with data- attributes. How does that sound?

reviewboard/search/views.py (Diff revision 3)
The issue has been resolved. Show all issues
```
break?
```

Change Summary:

Addressed issues. Rebased off djblets change and changes to 7809.

Diff:

Revision 4 (+374 -123)

Show changes

	reviewboard/admin/forms.py
	reviewboard/admin/urls.py
	reviewboard/avatars/registry.py
	reviewboard/avatars/templatetags/avatars.py
	reviewboard/datagrids/columns.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/search/views.py
	reviewboard/static/rb/css/pages/reviews.less
	16 more

Tool: Pyflakes
Processed Files:
    reviewboard/admin/forms.py
    reviewboard/datagrids/columns.py
    reviewboard/admin/urls.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/avatars/registry.py
    reviewboard/search/views.py
    reviewboard/avatars/templatetags/avatars.py
    reviewboard/webapi/resources/user.py

Ignored Files:
    reviewboard/templates/search/_user.html
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/templates/base.html
    reviewboard/templates/accounts/user_infobox.html
    reviewboard/templates/base/_nav_support_menu.html
    reviewboard/templates/reviews/boxes/change.html
    reviewboard/templates/reviews/review_reply.html
    reviewboard/templates/admin/avatar_settings.html
    reviewboard/templates/reviews/review_issue_summary_table.html
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/templates/datagrids/sidebar_user_info.html
    reviewboard/static/rb/js/views/reviewReplyEditorView.js
    reviewboard/templates/admin/widgets/w-actions.html
    reviewboard/templates/base/_mobile_navbar.html
    reviewboard/static/rb/js/common.js
    reviewboard/static/rb/js/models/userSessionModel.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/admin/forms.py
    reviewboard/datagrids/columns.py
    reviewboard/admin/urls.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/avatars/registry.py
    reviewboard/search/views.py
    reviewboard/avatars/templatetags/avatars.py
    reviewboard/webapi/resources/user.py

Ignored Files:
    reviewboard/templates/search/_user.html
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/templates/base.html
    reviewboard/templates/accounts/user_infobox.html
    reviewboard/templates/base/_nav_support_menu.html
    reviewboard/templates/reviews/boxes/change.html
    reviewboard/templates/reviews/review_reply.html
    reviewboard/templates/admin/avatar_settings.html
    reviewboard/templates/reviews/review_issue_summary_table.html
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/templates/datagrids/sidebar_user_info.html
    reviewboard/static/rb/js/views/reviewReplyEditorView.js
    reviewboard/templates/admin/widgets/w-actions.html
    reviewboard/templates/base/_mobile_navbar.html
    reviewboard/static/rb/js/common.js
    reviewboard/static/rb/js/models/userSessionModel.js

Change Summary:

Forgot to say yes to rbt post -u

Diff:

Revision 5 (+400 -123)

Show changes

	reviewboard/admin/forms.py
	reviewboard/admin/urls.py
	reviewboard/avatars/registry.py
	reviewboard/avatars/templatetags/avatars.py
	reviewboard/datagrids/columns.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/search/views.py
	reviewboard/static/rb/css/pages/reviews.less
	16 more

Tool: Pyflakes
Processed Files:
    reviewboard/admin/forms.py
    reviewboard/datagrids/columns.py
    reviewboard/admin/urls.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/avatars/registry.py
    reviewboard/search/views.py
    reviewboard/avatars/templatetags/avatars.py
    reviewboard/webapi/resources/user.py

Ignored Files:
    reviewboard/templates/search/_user.html
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/templates/base.html
    reviewboard/templates/accounts/user_infobox.html
    reviewboard/templates/base/_nav_support_menu.html
    reviewboard/templates/reviews/boxes/change.html
    reviewboard/templates/reviews/review_reply.html
    reviewboard/templates/admin/avatar_settings.html
    reviewboard/templates/reviews/review_issue_summary_table.html
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/templates/datagrids/sidebar_user_info.html
    reviewboard/static/rb/js/views/reviewReplyEditorView.js
    reviewboard/templates/admin/widgets/w-actions.html
    reviewboard/templates/base/_mobile_navbar.html
    reviewboard/static/rb/js/common.js
    reviewboard/static/rb/js/models/userSessionModel.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/admin/forms.py
    reviewboard/datagrids/columns.py
    reviewboard/admin/urls.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/avatars/registry.py
    reviewboard/search/views.py
    reviewboard/avatars/templatetags/avatars.py
    reviewboard/webapi/resources/user.py

Ignored Files:
    reviewboard/templates/search/_user.html
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/templates/base.html
    reviewboard/templates/accounts/user_infobox.html
    reviewboard/templates/base/_nav_support_menu.html
    reviewboard/templates/reviews/boxes/change.html
    reviewboard/templates/reviews/review_reply.html
    reviewboard/templates/admin/avatar_settings.html
    reviewboard/templates/reviews/review_issue_summary_table.html
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/templates/datagrids/sidebar_user_info.html
    reviewboard/static/rb/js/views/reviewReplyEditorView.js
    reviewboard/templates/admin/widgets/w-actions.html
    reviewboard/templates/base/_mobile_navbar.html
    reviewboard/static/rb/js/common.js
    reviewboard/static/rb/js/models/userSessionModel.js

Change Summary:

Rebase.

Diff:

Revision 6 (+404 -125)

Show changes

	reviewboard/admin/forms.py
	reviewboard/admin/urls.py
	reviewboard/avatars/registry.py
	reviewboard/avatars/templatetags/avatars.py
	reviewboard/datagrids/columns.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/search/views.py
	reviewboard/static/rb/css/pages/reviews.less
	17 more

Tool: Pyflakes
Processed Files:
    reviewboard/admin/forms.py
    reviewboard/datagrids/columns.py
    reviewboard/admin/urls.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/avatars/registry.py
    reviewboard/search/views.py
    reviewboard/avatars/templatetags/avatars.py
    reviewboard/webapi/resources/user.py

Ignored Files:
    reviewboard/templates/js/tests.html
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/templates/base.html
    reviewboard/templates/accounts/user_infobox.html
    reviewboard/templates/base/_nav_support_menu.html
    reviewboard/templates/reviews/boxes/change.html
    reviewboard/templates/search/_user.html
    reviewboard/templates/reviews/review_reply.html
    reviewboard/templates/admin/avatar_settings.html
    reviewboard/templates/reviews/review_issue_summary_table.html
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/templates/datagrids/sidebar_user_info.html
    reviewboard/static/rb/js/views/reviewReplyEditorView.js
    reviewboard/templates/admin/widgets/w-actions.html
    reviewboard/templates/base/_mobile_navbar.html
    reviewboard/static/rb/js/common.js
    reviewboard/static/rb/js/models/userSessionModel.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/admin/forms.py
    reviewboard/datagrids/columns.py
    reviewboard/admin/urls.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/avatars/registry.py
    reviewboard/search/views.py
    reviewboard/avatars/templatetags/avatars.py
    reviewboard/webapi/resources/user.py

Ignored Files:
    reviewboard/templates/js/tests.html
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/templates/base.html
    reviewboard/templates/accounts/user_infobox.html
    reviewboard/templates/base/_nav_support_menu.html
    reviewboard/templates/reviews/boxes/change.html
    reviewboard/templates/search/_user.html
    reviewboard/templates/reviews/review_reply.html
    reviewboard/templates/admin/avatar_settings.html
    reviewboard/templates/reviews/review_issue_summary_table.html
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/templates/datagrids/sidebar_user_info.html
    reviewboard/static/rb/js/views/reviewReplyEditorView.js
    reviewboard/templates/admin/widgets/w-actions.html
    reviewboard/templates/base/_mobile_navbar.html
    reviewboard/static/rb/js/common.js
    reviewboard/static/rb/js/models/userSessionModel.js

admin.png (Revision 1)

The issue has been resolved. Show all issues

So my major complaint about this change is this UI.

I think it's kind of confusing that there are a bunch of "enabled avatar services" and then a separate drop-down for the "default avatar service". It's not at all clear what the two different pieces of UI will do.

AFAICT, only the default avatar service will actually be used? In that case, why do we expose the enabled list at all?

brennie Feb. 4, 2016, 9:47 p.m.

Currently only the default avatar service is used, but I have a patch in the works to allow users to choose avatar services.

This would allow e.g., all users to use gravatars except users like Review Bot who could have a custom uploaded avatar.

brennie Feb. 22, 2016, 7:19 a.m.

I've added more help text to the controls.

reviewboard/static/rb/js/models/userSessionModel.js (Diff revision 6)

The issue has been resolved. Show all issues

It would be nicer if we did the fallback for urls in the assignment:

var urls = this.get('avatarURLs') || {};
return urls.size || {};

reviewboard/avatars/templatetags/avatars.py (Diff revision 6)
The issue has been resolved. Show all issues
```
typo: resoltuion
```
reviewboard/avatars/templatetags/avatars.py (Diff revision 6)
The issue has been resolved. Show all issues
```
typo: emptyu
```

Change Summary:

Address open issues.

Diff:

Revision 7 (+407 -125)

Show changes

	reviewboard/admin/forms.py
	reviewboard/admin/urls.py
	reviewboard/avatars/registry.py
	reviewboard/avatars/templatetags/avatars.py
	reviewboard/datagrids/columns.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/search/views.py
	reviewboard/static/rb/css/pages/reviews.less
	17 more

Tool: Pyflakes
Processed Files:
    reviewboard/admin/forms.py
    reviewboard/datagrids/columns.py
    reviewboard/admin/urls.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/avatars/registry.py
    reviewboard/search/views.py
    reviewboard/avatars/templatetags/avatars.py
    reviewboard/webapi/resources/user.py

Ignored Files:
    reviewboard/templates/js/tests.html
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/templates/base.html
    reviewboard/templates/accounts/user_infobox.html
    reviewboard/templates/base/_nav_support_menu.html
    reviewboard/templates/reviews/boxes/change.html
    reviewboard/templates/search/_user.html
    reviewboard/templates/reviews/review_reply.html
    reviewboard/templates/admin/avatar_settings.html
    reviewboard/templates/reviews/review_issue_summary_table.html
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/templates/datagrids/sidebar_user_info.html
    reviewboard/static/rb/js/views/reviewReplyEditorView.js
    reviewboard/templates/admin/widgets/w-actions.html
    reviewboard/templates/base/_mobile_navbar.html
    reviewboard/static/rb/js/common.js
    reviewboard/static/rb/js/models/userSessionModel.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/admin/forms.py
    reviewboard/datagrids/columns.py
    reviewboard/admin/urls.py
    reviewboard/reviews/templatetags/reviewtags.py
    reviewboard/avatars/registry.py
    reviewboard/search/views.py
    reviewboard/avatars/templatetags/avatars.py
    reviewboard/webapi/resources/user.py

Ignored Files:
    reviewboard/templates/js/tests.html
    reviewboard/templates/reviews/boxes/review.html
    reviewboard/templates/base.html
    reviewboard/templates/accounts/user_infobox.html
    reviewboard/templates/base/_nav_support_menu.html
    reviewboard/templates/reviews/boxes/change.html
    reviewboard/templates/search/_user.html
    reviewboard/templates/reviews/review_reply.html
    reviewboard/templates/admin/avatar_settings.html
    reviewboard/templates/reviews/review_issue_summary_table.html
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/templates/datagrids/sidebar_user_info.html
    reviewboard/static/rb/js/views/reviewReplyEditorView.js
    reviewboard/templates/admin/widgets/w-actions.html
    reviewboard/templates/base/_mobile_navbar.html
    reviewboard/static/rb/js/common.js
    reviewboard/static/rb/js/models/userSessionModel.js

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-2.6.x (ee27598)