Update AvatarService docs and guide to include required fields and methods
Review Request #9493 — Created Jan. 19, 2018 and submitted
When an extension for AvatarServices doesn't include an implementation for
get_avatar_urls_uncachedor
get_etag_data, it throws a 500 error. With the changes, we now return a default
etag_dataand return an empty string for the uncached avatar urls in addition to logging an error.
ran
make htmland the formatting changes appear.
tests for AvatarServices passed. Added 2 unit tests to ensure
get_avatar_urls_uncachedlogs an error and
get_etag_datareturns the default
[avatar.service_id, user.pk]
Change Summary:
Fixing lint issues
Checks run (1 failed, 1 succeeded)
flake8
Hey Florie - did you try generating the sourcecode for the avatars to make sure that it's properly formatted? If so, you should add that to Testing Done. If not, I suggest doing that.
I think you can do that from within the
djblets/docs/djbletsdirectory, and running
make html. You might have to install some packages (like
sphinx).
The description/testing need additional work to meet our standards. We have a guide on what we expect from these fields:
The Bugs field needs to be filled in with the bugs this references.
The documentation is great, but I think part of the original problem in the report is that the user saw a crash and didn't know what caused it immediately. So, along with the documentation improvements, we should make sure things do not crash, but rather that raised exceptions are logged.
One place to do this is in
djblets/avatars/base.py, where
get_avatar_urls_uncachedis called. We should catch the exception, log the error, and return a sane result (like
{}).
The other function,
get_etag_datais actually not called in Djblets. We probably should not be raising
NotImplementedErrorhere, since we can provide a sane default. So, I'd recommend altering the function to return a sane default, like:
return [self.avatar_service_id, user.pk]
This would probably be an okay default, and would prevent blowing up.
Unit tests need to be added to:
- Test calling
get_avatar_urlswithout
get_avatar_urls_uncachedhaving an implementation, and ensuring that we get a sane result back and that data is logged (you'll use kgb for this -- ask David or Barret about how to do this).
- Test that
get_etag_datareturns that default value.
There needs to be a blank line separating the list and the text preceding it.
No "and" at the end here. For lists of items in docs, you don't treat each item as part of a sentence.
No need for "attributes".
Given the above, you'd want to rewrite this as well.
No need for "method".
There needs to be a blank line separating the list and the text preceding it.
This should go before the django imports.
We put imports into three groups:
from __future__ import unicode_literals #always first import logging # Python standard library import os from django.conf import foo # 3-rd Party imports from djblets.foo import bar # Project imports
Missing a period. Should mention that they should override
get_etag_datafor caching.
Undo this change. We want this to raise an error.
Can you also change this into a definition list?
This should use a definition list:
Subclasses must also override: :py:meth:`~base.AvatarService.get_avatar_urls_uncached`: This method computes and returns the avatar URLs for the given user at a requested size at different resolutions :py:meth:`~base.AvatarService.get_etag_data`: This method returns a list of unicode strings that uniquely identify the avatar services and its configuration.
We should mention
get_etag_datahas a default impl but it should probably be overriden to include appropriate setting values for caching.