fix broken default avatar when avatar service is file upload
Review Request #10690 — Created Sept. 6, 2019 and discarded
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 | ID | Author |
|---|---|---|
| 64e07565d426eddef97b3da53c2233984687b4ca | 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 … |
 chipx86
|
|
|
It's going to also be important to add unit tests for any changes. |
chipx86
|
|
|
E231 missing whitespace after ',' |
 reviewbot
|
|
|
E501 line too long (86 > 79 characters) |
reviewbot
|
|
|
E231 missing whitespace after ',' |
reviewbot
|
|
|
E231 missing whitespace after ',' |
reviewbot
|
|
|
W293 blank line contains whitespace |
reviewbot
|
|
|
E501 line too long (96 > 79 characters) |
reviewbot
|
|
|
W291 trailing whitespace |
reviewbot
|
|
|
E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
E122 continuation line missing indentation or outdented |
reviewbot
|
|
|
New variables must be documented in "Args" in the docstring. |
chipx86
|
|
|
Blank line between blocks (the for loop) and statements (the return). |
chipx86
|
|
|
This needs to be updated to say it'll return None if the service can't provide an avatar. |
chipx86
|
|
|
This needs to be updated to say it'll return None if the service can't provide an avatar. |
chipx86
|
|
|
You can shorten this by doing: return url_value or None What that's saying is that "if url_value has something in … |
chipx86
|
- Commits:
-
Summary ID Author e577bab74389ffc2757d1ac1734cc343d519c700 cathyQinQin fd5bdb460d447ce0791e24e6076530fd661c2c5b cathyQinQin
- Commits:
-
Summary ID Author fd5bdb460d447ce0791e24e6076530fd661c2c5b cathyQinQin 64e07565d426eddef97b3da53c2233984687b4ca cathyQinQin
Checks run (2 succeeded)
- Description:
-
~ fix broken default avatar when avatar service is file upload
~ fix broken default avatar for users when avatar service in admin is chosen as file upload
- Testing Done:
-
~ I test it on http://localhost:8080/users/
~ After selecting file upload as avatar service, all default avatars in user page works well
- Bugs:
-
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:
- 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.
- 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 thisget_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-Nonevalue. -
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.
-
-
-
-
-
-
You can shorten this by doing:
return url_value or NoneWhat that's saying is that "if
url_valuehas something in it (isn't an empty dictionary orNone), then return it. Otherwise, returnNone."
