• 
      

    Support users in multiple review groups in API responses

    Review Request #8458 — Created Oct. 12, 2016 and submitted

    Information

    Review Board
    release-2.0.x
    4d6411e...

    Reviewers

    The patch that fixed the review group user resource links broke when a
    user was a member of more than one group (which is a likely occurance).
    This was because we were using a model_parent_key that was a many-to-
    many field (and therefore the reverse relation was not unique),
    resulting in a MultipleObjectsReturned exception. This has been fixed
    by customizing how we retrieve the parent object IDs.

    Ran unit tests.

    Description From Last Updated

    "many-to-many"

    chipx86chipx86

    "have to manually"

    chipx86chipx86

    So as mentioned on Slack, this will result in an extra query per result (the Group.objects.get). It looks like the …

    chipx86chipx86

    undefined name 'parent_id_key'

    reviewbotreviewbot

    undefined name 'parent_id_key'

    reviewbotreviewbot

    This should check the response for stat=ok, item count, and the expected user.

    chipx86chipx86
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          reviewboard/webapi/resources/review_group_user.py
          reviewboard/webapi/tests/test_review_group_user.py
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          reviewboard/webapi/resources/review_group_user.py
          reviewboard/webapi/tests/test_review_group_user.py
      
      
    2. 
        
    chipx86
    1. 
        
    2. Show all issues

      "many-to-many"

    3. Show all issues

      "have to manually"

    4. reviewboard/webapi/resources/review_group_user.py (Diff revision 1)
       
       
       
       
       
       
       
       
       
       
       
       
       
       
      Show all issues

      So as mentioned on Slack, this will result in an extra query per result (the Group.objects.get). It looks like the reason this is being done is so it can chain up a list of parents, which is fine in a general sense but not necessary for groups (which won't have a parent -- Local Sites don't count, since they're not part of the resource tree).

      We have a few things going for us that the default get_href_parent_ids doesn't have. We know the value used in the URL, and we know the key needed for looking up that value and returning it. We also know there isn't a parent in ReviewGroupResource. So we can short-circuit a lot of the default implementation and do our own thing.

      The default implementation doesn't know which ID we need, and it needs to handle any number of parents and corner cases involving the IDs. So it's going to do more work. It's going to check if the parent resource has a model_parent_key, and if set, it will fetch the object (much like you're doing here) and try to gets its parents (ReviewGroupResource will be a no-op here, since it doesn't have a model_parent_key). Then it will populate an entry in the ID map for parent.uri_object_key -> parent_obj[parent.model_object_key]. In the case of a Review Group, that'd be group_name: child.group.name.

      But since we already know that group name, and are building a specific implementation (as opposed to that general implementation), we don't need to do any of that work.

      I think this whole method can become:

      parent_id_key = self._parent_resource.uri_object_key
      
      return {
          self._parent_resource.uri_object_key: kwargs[parent_id_key],
      }
      
    5. 
        
    brennie
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          reviewboard/webapi/resources/review_group_user.py
          reviewboard/webapi/tests/test_review_group_user.py
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          reviewboard/webapi/resources/review_group_user.py
          reviewboard/webapi/tests/test_review_group_user.py
      
      
    2. Show all issues
       undefined name 'parent_id_key'
      
    3. Show all issues
       undefined name 'parent_id_key'
      
    4. 
        
    brennie
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          reviewboard/webapi/resources/review_group_user.py
          reviewboard/webapi/tests/test_review_group_user.py
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          reviewboard/webapi/resources/review_group_user.py
          reviewboard/webapi/tests/test_review_group_user.py
      
      
    2. 
        
    chipx86
    1. 
        
    2. Show all issues

      This should check the response for stat=ok, item count, and the expected user.

    3. 
        
    brennie
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          reviewboard/webapi/resources/review_group_user.py
          reviewboard/webapi/tests/test_review_group_user.py
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          reviewboard/webapi/resources/review_group_user.py
          reviewboard/webapi/tests/test_review_group_user.py
      
      
    2. 
        
    chipx86
    1. Ship It!
    2. 
        
    brennie
    Review request changed
    Status:
    Completed
    Change Summary:
    Pushed to release-2.0.x (a516969)