Update AvatarService docs and guide to include required fields and methods

Review Request #9493 — Created Jan. 19, 2018 and submitted

Information

Djblets
release-1.0.x
bb870cf...

Reviewers

When an extension for AvatarServices doesn't include an implementation for get_avatar_urls_uncached or get_etag_data, it throws a 500 error. With the changes, we now return a default etag_data and return an empty string for the uncached avatar urls in addition to logging an error.

ran make html and the formatting changes appear.

tests for AvatarServices passed. Added 2 unit tests to ensure get_avatar_urls_uncached logs an error and get_etag_data returns the default [avatar.service_id, user.pk]

Description From Last Updated

Hey Florie - did you try generating the sourcecode for the avatars to make sure that it's properly formatted? If …

mike_conleymike_conley

The Bugs field needs to be filled in with the bugs this references.

chipx86chipx86

The documentation is great, but I think part of the original problem in the report is that the user saw …

chipx86chipx86

The description/testing need additional work to meet our standards. We have a guide on what we expect from these fields: …

chipx86chipx86

Can you update the summary to indicate this is a docs change? Something like: Update AvatarService docs and guide to …

brenniebrennie

W291 trailing whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

There needs to be a blank line separating the list and the text preceding it.

chipx86chipx86

No "and" at the end here. For lists of items in docs, you don't treat each item as part of …

chipx86chipx86

No need for "attributes".

chipx86chipx86

Given the above, you'd want to rewrite this as well.

chipx86chipx86

No need for "method". There needs to be a blank line separating the list and the text preceding it.

chipx86chipx86

This should go before the django imports. We put imports into three groups: from __future__ import unicode_literals #always first import …

brenniebrennie

Missing a period. Should mention that they should override get_etag_data for caching.

brenniebrennie

Undo this change. We want this to raise an error.

brenniebrennie

Can you also change this into a definition list?

brenniebrennie

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 …

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

flake8

FL
Review request changed
Change Summary:

Fixing lint issues

Commit:
b8973c8e62b42144274bc03d77b655857cc15722
3e71986c3209801cec035c692487c7d276f752dd

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

FL
mike_conley
  1. 
      
  2. Show all issues

    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/djblets directory, and running make html. You might have to install some packages (like sphinx).

  3. 
      
FL
chipx86
  1. 
      
  2. Show all issues

    The Bugs field needs to be filled in with the bugs this references.

  3. Show all issues

    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_uncached is called. We should catch the exception, log the error, and return a sane result (like {}).

    The other function, get_etag_data is actually not called in Djblets. We probably should not be raising NotImplementedError here, 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_urls without get_avatar_urls_uncached having 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_data returns that default value.
  4. Show all issues

    The description/testing need additional work to meet our standards. We have a guide on what we expect from these fields: https://www.reviewboard.org/docs/codebase/dev/writing-good-descriptions/

  5. djblets/avatars/services/base.py (Diff revision 4)
     
     
     
    Show all issues

    There needs to be a blank line separating the list and the text preceding it.

  6. djblets/avatars/services/base.py (Diff revision 4)
     
     
     
    Show all issues

    No "and" at the end here. For lists of items in docs, you don't treat each item as part of a sentence.

  7. djblets/avatars/services/base.py (Diff revision 4)
     
     
     
    Show all issues

    No need for "attributes".

  8. djblets/avatars/services/base.py (Diff revision 4)
     
     
    Show all issues

    Given the above, you'd want to rewrite this as well.

  9. djblets/avatars/services/base.py (Diff revision 4)
     
     
     
    Show all issues

    No need for "method".

    There needs to be a blank line separating the list and the text preceding it.

  10. 
      
FL
brennie
  1. 
      
  2. djblets/avatars/services/base.py (Diff revision 5)
     
     
    Show all issues

    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
    
  3. djblets/avatars/services/base.py (Diff revision 5)
     
     
    Show all issues

    Missing a period. Should mention that they should override get_etag_data for caching.

  4. djblets/avatars/services/base.py (Diff revision 5)
     
     
     
     
     
    Show all issues

    Undo this change. We want this to raise an error.

    1. Christian said in an earlier review: One place to do this is in djblets/avatars/base.py, where get_avatar_urls_uncached is called. We should catch the exception, log the error, and return a sane result (like {}).

      So should I keep the exception or have a default implementation?

    2. In that case this is fine.

  5. Show all issues

    Can you also change this into a definition list?

  6. Show all issues

    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_data has a default impl but it should probably be overriden to include appropriate setting values for caching.

  7. 
      
brennie
  1. 
      
  2. Show all issues

    Can you update the summary to indicate this is a docs change? Something like:
    Update AvatarService docs and guide to include required fields and methods

  3. 
      
FL
david
  1. Making some small changes and landing. Thanks!

  2. 
      
FL
Review request changed
Status:
Completed
Change Summary:
Pushed to release-1.0.x (52a4d41)