Summary

Serialize avatar URL in user resource only when URLs available

Review Request #9457 — Created Dec. 19, 2017 and submitted Jan. 8, 2018, 7:25 p.m.

Information

Owner

brennie

Repository

Review Board

Branch

release-3.0.x

Bugs

4616

Depends On

Commit

ed1a82f...

Reviewers

Groups

reviewboard

People

Description

Previously we assumed that all avatar services would return URLs.

However, it is possible that the service has yet to be configured for a

user (e.g., they haven't uploaded an avatar yet for the

FileUploadAvatarService) or the avatar service returns bad data. In

these cases, we now wrap the serialization in a try..except to ensure

that we don't cause exceptions that crash the resource.
Additionally, we also ensure that we serialize the avatar_urls field

as {} instead of None when there are no URLs to simplify

deserialization logic.

Testing Done

Ran unit tests.

Issues

Description	From	Last Updated
Can you update the description to also mention the change to the avatar_urls field? That'll make it easier to remember …	chipx86	Dec. 21, 2017, 2:54 p.m.
This whole thing could be just return urls and urls.get('1x')	david	Dec. 19, 2017, 4:47 p.m.
Single quotes.	david	Dec. 19, 2017, 4:47 p.m.
Maybe worth having this return {} when there are no URLs? Simplifies the above code, simplifies clients who no longer …	chipx86	Dec. 20, 2017, 7 p.m.
Plurals are wrong here. "avatar service"?	chipx86	Dec. 20, 2017, 7 p.m.

flake8 passed.

JSHint passed.

reviewboard/webapi/resources/user.py (Diff revision 1)
The issue has been resolved. Show all issues
```
This whole thing could be just return urls and urls.get('1x')
```
reviewboard/webapi/tests/test_user.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Single quotes.
```

Change Summary:

Addressed David's issues.

Commit:

-	dcb9006e835285b8cef9925ecd3bbbb6b5e2e7e1
+	357e9fb2bf34aa34508375603bd833d1c3b717a0

Diff:

Revision 2 (+36 -7)

Show changes

	reviewboard/webapi/resources/user.py
	reviewboard/webapi/tests/test_user.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

```
Ship It!
```

Change Summary:

Dont return {} from no urls

Commit:

-	357e9fb2bf34aa34508375603bd833d1c3b717a0
+	ea89767a873a2ad4e17c6abd3c9f930f73004fc3

Diff:

Revision 3 (+37 -4)

Show changes

	reviewboard/webapi/resources/user.py
	reviewboard/webapi/tests/test_user.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/webapi/resources/user.py (Diff revision 3)

The issue has been resolved. Show all issues

Maybe worth having this return {} when there are no URLs? Simplifies the above code, simplifies clients who no longer need to check for null but can instead just check for the presence of certain avatars (or check falsiness).

reviewboard/webapi/tests/test_user.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Plurals are wrong here. "avatar service"?
```

Change Summary:

Addressed Christian's issues.

Commit:

-	ea89767a873a2ad4e17c6abd3c9f930f73004fc3
+	ed1a82f689642e00f5d10fe569e6db27f5a13347

Diff:

Revision 4 (+38 -5)

Show changes

	reviewboard/webapi/resources/user.py
	reviewboard/webapi/tests/test_user.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

The issue has been resolved. Show all issues

Can you update the description to also mention the change to the avatar_urls field? That'll make it easier to remember while writing release notes, and when coming across the modification down the road.

Change Summary:

Addressed Christian's issue.

Description:

		Previously we assumed that all avatar services would return URLs.
		However, it is possible that the service has yet to be configured for a
		user (e.g., they haven't uploaded an avatar yet for the
		`FileUploadAvatarService`) or the avatar service returns bad data. In
		these cases, we now wrap the serialization in a `try..except` to ensure
		that we don't cause exceptions that crash the resource.
	+
	+	Additionally, we also ensure that we serialize the `avatar_urls` field
	+	as `{}` instead of `None` when there are no URLs to simplify
	+	deserialization logic.

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (f50e598)