[WIP] Add timezone to user in API

Review Request #7881 — Created Jan. 16, 2016 and discarded

Information

Review Board
master

Reviewers

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.

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.

Description From Last Updated

Col: 19 E231 missing whitespace after ':'

reviewbotreviewbot

Col: 26 E231 missing whitespace after ':'

reviewbotreviewbot

Col: 24 E203 whitespace before ':'

reviewbotreviewbot

local variable 'e' is assigned to but never used

reviewbotreviewbot

Col: 35 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Col: 23 E203 whitespace before ':'

reviewbotreviewbot

Col: 29 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Col: 35 E127 continuation line over-indented for visual indent

reviewbotreviewbot

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

mike_conleymike_conley

It's usually not a good idea to catch Exception, since that's a really generic Exception, and might get raised for …

mike_conleymike_conley

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

chipx86chipx86

Should have a trailing comma in the last list item.

chipx86chipx86
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/user.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/user.py
    
    
  2. reviewboard/webapi/resources/user.py (Diff revision 1)
     
     
    Show all issues
    Col: 19
     E231 missing whitespace after ':'
    
  3. reviewboard/webapi/resources/user.py (Diff revision 1)
     
     
    Show all issues
    Col: 26
     E231 missing whitespace after ':'
    
  4. reviewboard/webapi/resources/user.py (Diff revision 1)
     
     
    Show all issues
    Col: 24
     E203 whitespace before ':'
    
  5. reviewboard/webapi/resources/user.py (Diff revision 1)
     
     
    Show all issues
     local variable 'e' is assigned to but never used
    
  6. reviewboard/webapi/resources/user.py (Diff revision 1)
     
     
    Show all issues
    Col: 35
     E127 continuation line over-indented for visual indent
    
  7. 
      
AD
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/user.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/user.py
    
    
  2. reviewboard/webapi/resources/user.py (Diff revision 2)
     
     
    Show all issues
    Col: 23
     E203 whitespace before ':'
    
  3. reviewboard/webapi/resources/user.py (Diff revision 2)
     
     
    Show all issues
    Col: 29
     E127 continuation line over-indented for visual indent
    
  4. reviewboard/webapi/resources/user.py (Diff revision 2)
     
     
    Show all issues
    Col: 35
     E127 continuation line over-indented for visual indent
    
  5. 
      
AD
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/user.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/user.py
    
    
  2. 
      
mike_conley
  1. 
      
  2. reviewboard/webapi/resources/user.py (Diff revision 3)
     
     
    Show all issues

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

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

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

  4. reviewboard/webapi/resources/user.py (Diff revision 3)
     
     
    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.

    1. Should catch Profile.DoesNotExist instead.

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

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

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

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

  2. 
      
AD
AD
AD
AD
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/user.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/user.py
    
    
  2. 
      
AD
mike_conley
  1. 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!

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

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

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

  2. 
      
chipx86
  1. 
      
  2. reviewboard/webapi/resources/user.py (Diff revision 4)
     
     
     
    Show all issues

    Should have a trailing comma in the last list item.

  3. 
      
chipx86
  1. 
      
  2. reviewboard/webapi/resources/user.py (Diff revision 4)
     
     
     
     
     
     
     
    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',
        }
    )
    
  3. 
      
AD
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/user.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/user.py
    
    
  2. 
      
AD
mike_conley
  1. Christian's feedback still stands - can you combine the two selects?

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

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

    3. 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)
      '''

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

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

  2. 
      
david
Review request changed
Status:
Discarded