Summary

[WIP] Add timezone to user in API

Review Request #7881 — Created Jan. 16, 2016 and discarded Jan. 29, 2019, 1:43 p.m.

Information

Owner

adilally

Repository

Review Board

Branch

master

Bugs

4044

Depends On

Reviewers

Groups

reviewboard, students

People

Description

Timezone has been added to the User resource in the API, this was a request.

The possible values of timezone are the timezone options available in the User settings page.

The User's timezone is queried along with the rest of the User's information in order to reduce the number of queries made to the database.

Testing Done

Accessed a list of Users (/api/users/) and a single user (/api/users/admin/) in order to confirm correct data (Local Review Board had 2 users total with different Timezones).

Performed a unit test to ensure that a minimal amount of queries are performed (when a User is queried, their profile is joined).
Unit test consisted of starting with the "unoptimized" version (which made an extra query) and fixing until the data was retrieved without the extra query being made.

Issues

Description	From	Last Updated
Col: 19 E231 missing whitespace after ':'	reviewbot	Jan. 16, 2016, 9:50 a.m.
Col: 26 E231 missing whitespace after ':'	reviewbot	Jan. 16, 2016, 9:50 a.m.
Col: 24 E203 whitespace before ':'	reviewbot	Jan. 16, 2016, 9:50 a.m.
local variable 'e' is assigned to but never used	reviewbot	Jan. 16, 2016, 9:50 a.m.
Col: 35 E127 continuation line over-indented for visual indent	reviewbot	Jan. 16, 2016, 9:50 a.m.
Col: 23 E203 whitespace before ':'	reviewbot	Jan. 16, 2016, 9:59 a.m.
Col: 29 E127 continuation line over-indented for visual indent	reviewbot	Jan. 16, 2016, 9:58 a.m.
Col: 35 E127 continuation line over-indented for visual indent	reviewbot	Jan. 16, 2016, 9:59 a.m.
Do we think folks will care about keeping their timezones private? Should we add timezone to this list?	mike_conley	Jan. 16, 2016, 11:20 a.m.
It's usually not a good idea to catch Exception, since that's a really generic Exception, and might get raised for …	mike_conley	Jan. 16, 2016, 12:43 p.m.
We're doing the subquery twice, which we can avoid by making use of a couple extra parameters to extra(): return …	chipx86	Jan. 16, 2016, 1:03 p.m.
Should have a trailing comma in the last list item.	chipx86	Jan. 16, 2016, 1:54 p.m.

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/resources/user.py



Tool: Pyflakes
Processed Files:
    reviewboard/webapi/resources/user.py

reviewboard/webapi/resources/user.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 19
 E231 missing whitespace after ':'
```
reviewboard/webapi/resources/user.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 26
 E231 missing whitespace after ':'
```
reviewboard/webapi/resources/user.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 24
 E203 whitespace before ':'
```
reviewboard/webapi/resources/user.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 local variable 'e' is assigned to but never used
```
reviewboard/webapi/resources/user.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 35
 E127 continuation line over-indented for visual indent
```

Change Summary:

Fixed issues outlined by reviewbot

Diff:

Revision 2 (+16)

Show changes

reviewboard/webapi/resources/user.py

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/resources/user.py



Tool: Pyflakes
Processed Files:
    reviewboard/webapi/resources/user.py

reviewboard/webapi/resources/user.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 23
 E203 whitespace before ':'
```
reviewboard/webapi/resources/user.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 29
 E127 continuation line over-indented for visual indent
```
reviewboard/webapi/resources/user.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 35
 E127 continuation line over-indented for visual indent
```

Change Summary:

Fixed misunderstood errors highlighted by Reviewbot.

Diff:

Revision 3 (+16)

Show changes

reviewboard/webapi/resources/user.py

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/resources/user.py



Tool: Pyflakes
Processed Files:
    reviewboard/webapi/resources/user.py

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

The issue has been dropped. Show all issues

Do we think folks will care about keeping their timezones private? Should we add timezone to this list?

mike_conley Jan. 16, 2016, 11:20 a.m.

Willing to drop this - apparently we're okay exposing this (according to brennie).

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

According to the Django 1.9 docs, we're supposed to avoid extra at all costs, but I guess if it's good enough for is_private, it's probably fine...

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

The issue has been resolved. Show all issues

It's usually not a good idea to catch Exception, since that's a really generic Exception, and might get raised for reasons other than the fact that the user does not exist when accessing timezone.
If we're certain we want to handle this case (I'm not sure how it could happen), maybe we should do:

if not user:
    logging.error(...)

return user.timezone


If we don't care about this case, then I'm pretty sure we don't need this serializer.

david Jan. 16, 2016, 11:19 a.m.

Should catch Profile.DoesNotExist instead.

david Jan. 16, 2016, 11:20 a.m.

And we don't need to log anything here--it's valid for the profile not to exist.

adilally Jan. 16, 2016, 12:17 p.m.

The user's timezone is queried along with the user's other information, so unfortunately Profile.DoesNotExist is not thrown, but if a profile doesn't exist, the timezone attribute won't exist and will cause an error. Switching to either hasattr(object, 'property') or getattr(object, 'property', default_value) in order to avoid the exception issue, unless there is a better alternative.

adilally Jan. 16, 2016, 12:43 p.m.

Published possible fix, please comment/re-open if issue has not been resolved.

Also, on your next pass, would you mind altering your Summary / Description / Testing Done to adhere to this document? Thanks!

Groups:

+	students

Change Summary:

Attempting to adhere to documentation principles.

Summary:

-	-Added User's Timezone to API (/api/user/<user>
+	Add timezone to user in API

Bugs:

+	4044

Description:

~		-Fixed missing brackets for user.getprofile().timezone
	~	Timezone has been added to the User resource in the API.
-
-
-
-		-Catch Exception if a profile doesn't exist
-
-
-
-		-Removed miscellaenous code -Need to Prefetch profiles for the Users Queryset
-
-
-
-		-Reworked Timezone implementation to reduce the number of queries made to the Database.

		Timezone is now an attribute that is accessible from the User resource.
~		The possible values of Timezone are the timezone options available in the User settings page.
	~	The possible values of timezone are the timezone options available in the User settings page.
	+
	+	The User's timezone is queried along with the rest of the User's information in order to reduce the number of queries made to the database.

~		An example of the attribute in XML format, omitting other User resource details, would be:
	~	This feature was a request
-		<timezone>US/Eastern</timezone>

Change Summary:

Changed the way timezone is retrieved in order to avoid exceptions and ensure proper handling in the case that a Profile does not exist.

Diff:

Revision 4 (+11 -1)

Show changes

reviewboard/webapi/resources/user.py

Tool: Pyflakes
Processed Files:
    reviewboard/webapi/resources/user.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/resources/user.py

Change Summary:

Fixing description.

Description:

~		Timezone has been added to the User resource in the API.
	~	Timezone has been added to the User resource in the API, this was a request.

~		Timezone is now an attribute that is accessible from the User resource.
	~	The possible values of timezone are the timezone options available in the User settings page.
-		The possible values of timezone are the timezone options available in the User settings page.

		The User's timezone is queried along with the rest of the User's information in order to reduce the number of queries made to the database.
-
-		This feature was a request

Ship it!

This looks good to me, but I'm only 80% confident that getattr is the right move. Let's wait for at least one other mentor to chime in. Thanks!

chipx86 Jan. 16, 2016, 1 p.m.

It'll always be there, given the query, so you don't even need the serialize function for it.

adilally Jan. 16, 2016, 1:18 p.m.

You guys were right, the serialize function isn't necessary, removing it in the next update.

adilally Jan. 17, 2016, 6:19 a.m.

I think I may have jumped the gun. I don't believe that I've fully wrapped my head around Profiles and Users. It seems that if I expose this attribute without serializing it, the instances where a User is "accessed" without the corresponding Profile, will throw an error and fail the unit tests.

reviewboard/webapi/resources/user.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Should have a trailing comma in the last list item.
```

Fix it!

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

An issue was opened. Show all issues

We're doing the subquery twice, which we can avoid by making use of a couple extra parameters to extra():

return query.extra(
    tables=['accounts_profile'],
    where=['accounts_profile.user_id=auth_user.id'],
    extra={
        'is_private': 'accounts_profile.is_private',
        'timezone': 'accounts_profile.timezone',
    }
)

Change Summary:

Added missing trailing comma.
Removed serialized function.

Need to expose timezone in the User resource!

Diff:

Revision 5 (+3 -1)

Show changes

reviewboard/webapi/resources/user.py

Tool: Pyflakes
Processed Files:
    reviewboard/webapi/resources/user.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/resources/user.py

Change Summary:

Changed to WIP until functionality is restored.

Summary:

-	Add timezone to user in API
+	[WIP] Add timezone to user in API

Christian's feedback still stands - can you combine the two selects?

adilally Jan. 17, 2016, 7:07 a.m.

Unfortunately it fails a unit test, I'm investigating to see what exactly is causing it to fail.

mike_conley Jan. 17, 2016, 7:14 a.m.

Ah, cool. Can you please post the test failure output? If you don't get anywhere with it in the next half hour, please let us know.

adilally Jan. 17, 2016, 7:28 a.m.

Sure, ignore the error reported by the unit testing, the failure is caused by combining the select statements.

'''

======================================================================

FAIL: Testing the GET users/ API

Traceback (most recent call last):

  File "/src/djblets/djblets/webapi/testing/decorators.py", line 16, in _call

    return test_func(args, *kwargs)

  File "/src/reviewboard/reviewboard/webapi/tests/mixins.py", line 458, in test_get

    self.assertEqual(len(items), len(items_rsp))

AssertionError: 4 != 3

Ran 2343 tests in 682.680s
FAILED (SKIP=50, errors=1, failures=1)

'''

mike_conley March 21, 2016, 3:21 p.m.

Hey Adil, did you ever get anywhere with this investigation?

adilally March 23, 2016, 5 p.m.

Hi Mike, I was able to work with Christian to tackle the issue here, but we weren't able to come up with an efficient solution for my implementation during the Code Sprint at the beginning of the semester. Because of that, Christian advised me to work on the API Explorer until he had a chance to come up with a proper way of handling the implementation.

To explain the issue in more detail: The User Resource is made up of a User Model and Profile Model (not sure if model is the correct term). Unfortunately, not every User, has a Profile, which means that we do not have a guarantee that every User Resource will have a Profile model associated with it. This means that the User's without a Profile will not have a timezone field associated with their resource. In the original implementation, I had made a query to the database for the timezone field but Christian noticed that we were already making a query to the database, and a second query would be too inefficient. We reduced the two queries into one, but we ran into issues due to the lack of guarantee by the User Resource for the timezone field, and that's when Christian told me to focus on the API Explorer.

Hopefully that makes sense and wasn't too lengthy, apologies in advanced. If necessary, I'm sure Christian can always enlighten us to the issue at hand here.

Review Board 7.0 alpha 0 (dev)

[WIP] Add timezone to user in API

Information

Reviewers

Change Summary:

Diff:

Change Summary:

Diff:

Groups:

Change Summary:

Summary:

Bugs:

Description:

Change Summary:

Diff:

Change Summary:

Description:

Change Summary:

Diff:

Change Summary:

Summary:

Status: Discarded