fix broken default avatar when avatar service is file upload

Review Request #10690 — Created Sept. 6, 2019 and discarded

cathyzhang
Djblets
release-1.0.x
4812
10692
djblets, students

fix broken default avatar for users when avatar service in admin is chosen as file upload

After selecting file upload as avatar service, all default avatars in user page works well.

Summary Author
fix broken default avatar
cathyQinQin
Description From Last Updated

Hi Qin, Make sure you're following our guide on how we want to see change descriptions. You should be telling ...

chipx86chipx86

It's going to also be important to add unit tests for any changes.

chipx86chipx86

E231 missing whitespace after ','

reviewbotreviewbot

E501 line too long (86 > 79 characters)

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E501 line too long (96 > 79 characters)

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E122 continuation line missing indentation or outdented

reviewbotreviewbot

New variables must be documented in "Args" in the docstring.

chipx86chipx86

Blank line between blocks (the for loop) and statements (the return).

chipx86chipx86

This needs to be updated to say it'll return None if the service can't provide an avatar.

chipx86chipx86

This needs to be updated to say it'll return None if the service can't provide an avatar.

chipx86chipx86

You can shorten this by doing: return url_value or None What that's saying is that "if url_value has something in ...

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

flake8

cathyzhang
Review request changed

Commits:

Summary Author
-
fix broken default avatar
cathyQinQin
+
fix broken default avatar
cathyQinQin

Diff:

Revision 2 (+26 -20)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

cathyzhang
cathyzhang
cathyzhang
cathyzhang
cathyzhang
cathyzhang
chipx86
  1. It looks like this isn't quite implementing the fix the way I thought we were going to do it.

    This is asking for avatar URLs for each service, but there are two problems with this:

    1. Fetching avatar URLs might be expensive (it might require talking to a third-party service), so we don't want to actually do this until we need to.
    2. Avatars don't necessarily return URLs. They may just return HTML, which this won't check for.

    What this change needs to do instead is to provide a new replacement for for_user(). Let's call this get_services_for_user().

    That function should do what this function does, but instead of limiting to 1 result, it returns all results.

    The existing for_user() would wrap this, returning only one result, as before. However, the code handling the rendering of avatars would fetch all the services and try to render each one until we get a non-None value.

  2. Hi Qin,

    Make sure you're following our guide on how we want to see change descriptions.

    You should be telling a story with your change, describing what the problem was and how you solved it (at a high level).

    You'll also need to keep lines to <= 72 characters.

  3. It's going to also be important to add unit tests for any changes.

  4. djblets/avatars/registry.py (Diff revision 3)
     
     
     

    New variables must be documented in "Args" in the docstring.

  5. djblets/avatars/registry.py (Diff revision 3)
     
     
     

    Blank line between blocks (the for loop) and statements (the return).

  6. djblets/avatars/services/file_upload.py (Diff revision 3)
     
     
     
     

    This needs to be updated to say it'll return None if the service can't provide an avatar.

  7. djblets/avatars/services/url.py (Diff revision 3)
     
     
     
     

    This needs to be updated to say it'll return None if the service can't provide an avatar.

  8. djblets/avatars/services/url.py (Diff revision 3)
     
     
     
     

    You can shorten this by doing:

    return url_value or None
    

    What that's saying is that "if url_value has something in it (isn't an empty dictionary or None), then return it. Otherwise, return None."

  9. 
      
david
Review request changed

Status: Discarded

Loading...