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: Closed (submitted)

Change Summary:

Pushed to release-2.0.x (a516969)
Loading...