Summary

fix broken default avatar when avatar service is file upload

Review Request #10690 — Created Sept. 6, 2019 and discarded Sept. 8, 2019, 8:22 a.m.

Information

Owner

cathyzhang

Repository

Djblets

Branch

release-1.0.x

Bugs

4812

Depends On

Blocks

10692

Reviewers

Groups

djblets, students

People

Description

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

Testing Done

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

Commits

Summary	ID	Author
fix broken default avatar	64e07565d426eddef97b3da53c2233984687b4ca	cathyQinQin

Issues

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	Sept. 6, 2019, 3:55 p.m.
It's going to also be important to add unit tests for any changes.	chipx86	Sept. 6, 2019, 3:55 p.m.
E231 missing whitespace after ','	reviewbot	Sept. 6, 2019, 1:47 p.m.
E501 line too long (86 > 79 characters)	reviewbot	Sept. 6, 2019, 1:48 p.m.
E231 missing whitespace after ','	reviewbot	Sept. 6, 2019, 1:49 p.m.
E231 missing whitespace after ','	reviewbot	Sept. 6, 2019, 1:49 p.m.
W293 blank line contains whitespace	reviewbot	Sept. 6, 2019, 1:50 p.m.
E501 line too long (96 > 79 characters)	reviewbot	Sept. 6, 2019, 1:52 p.m.
W291 trailing whitespace	reviewbot	Sept. 6, 2019, 2:37 p.m.
E128 continuation line under-indented for visual indent	reviewbot	Sept. 6, 2019, 2:37 p.m.
E122 continuation line missing indentation or outdented	reviewbot	Sept. 6, 2019, 2:37 p.m.
New variables must be documented in "Args" in the docstring.	chipx86	Sept. 6, 2019, 3:55 p.m.
Blank line between blocks (the for loop) and statements (the return).	chipx86	Sept. 6, 2019, 3:55 p.m.
This needs to be updated to say it'll return None if the service can't provide an avatar.	chipx86	Sept. 6, 2019, 3:55 p.m.
This needs to be updated to say it'll return None if the service can't provide an avatar.	chipx86	Sept. 6, 2019, 3:55 p.m.
You can shorten this by doing: return url_value or None What that's saying is that "if url_value has something in …	chipx86	Sept. 6, 2019, 3:55 p.m.

flake8 failed.

JSHint passed.

flake8

djblets/avatars/registry.py (Diff revision 1)
Show all issues
```
E231 missing whitespace after ','
```
djblets/avatars/registry.py (Diff revision 1)
Show all issues
```
E501 line too long (86 > 79 characters)
```
djblets/avatars/registry.py (Diff revision 1)
Show all issues
```
E231 missing whitespace after ','
```
djblets/avatars/registry.py (Diff revision 1)
Show all issues
```
E231 missing whitespace after ','
```
djblets/avatars/registry.py (Diff revision 1)
Show all issues
```
W293 blank line contains whitespace
```
djblets/avatars/services/url.py (Diff revision 1)
Show all issues
```
E501 line too long (96 > 79 characters)
```

Commits:

	Summary	ID	Author
	fix broken default avatar	e577bab74389ffc2757d1ac1734cc343d519c700	cathyQinQin
	fix broken default avatar	fd5bdb460d447ce0791e24e6076530fd661c2c5b	cathyQinQin

Diff:

Revision 2 (+26 -20)

Show changes

	djblets/avatars/registry.py
	djblets/avatars/services/base.py
	djblets/avatars/services/file_upload.py
	djblets/avatars/services/url.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

djblets/avatars/registry.py (Diff revision 2)
Show all issues
```
W291 trailing whitespace
```

djblets/avatars/registry.py (Diff revision 2)

Show all issues

E128 continuation line under-indented for visual indent

djblets/avatars/services/url.py (Diff revision 2)

Show all issues

E122 continuation line missing indentation or outdented

Commits:

	Summary	ID	Author
	fix broken default avatar	fd5bdb460d447ce0791e24e6076530fd661c2c5b	cathyQinQin
	fix broken default avatar	64e07565d426eddef97b3da53c2233984687b4ca	cathyQinQin

Diff:

Revision 3 (+24 -14)

Show changes

	djblets/avatars/registry.py
	djblets/avatars/services/base.py
	djblets/avatars/services/file_upload.py
	djblets/avatars/services/url.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Description:

~		fix broken default avatar
	~	fix broken default avatar when avatar service is file upload

Testing Done:

I test it on http://localhost:8080/users/

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

Summary:

-	fix broken default avatar
+	fix broken default avatar when avatar service is file upload

Testing Done:

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

Bugs:

+	4812

Fix it!

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 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.

Show all issues

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.

Show all issues

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

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

Show all issues

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

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

Show all issues

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

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

Show all issues

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

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

Show all issues

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

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

Show all issues

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."

Review Board 6.0.3 alpha 0 (dev)

fix broken default avatar when avatar service is file upload

Information

Reviewers

flake8

Commits:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Commits:

Diff:

Checks run (2 succeeded)

Description:

Testing Done:

Description:

Testing Done:

Summary:

Testing Done:

Bugs:

Status: Discarded