Automatically add new users to default groups

Review Request #7165 - Created April 4, 2015 and submitted

Stanley Yeo
Review Board
master
5366233...
reviewboard, students

This change is to make a new user to be a member of default groups, when the user is registered or added to a local-site.

Works done are:
1. handle newly registered user
1) catch user_registered djblets.auth.signals.user_registered signal invoked when a new user is registered
2) add the new user to default groups
2. handle new LocalSite-User relation
1) catch m2m_changed (many-to-many relation changed) signal invoked when a LocalSite-User relation changed
2) invoke local_site_user_added for new relations between LocalSite and User
3) make a receiver of local_site_user_added, to add this newly added user to the LocalSite's default groups

Browser tests done:

  1. make new local site
  2. make new group, addign local the local site to it, and make it a default group
  3. make new group and addign local the local site to it (do not make it default group)
  4. add a user to the local site
  5. see if the user is added to group created in #2
  6. see if the user is not added to group created in #3

Unit Tests done:

  1. check if _add_default_groups signal handler is called properly when a user is registered
  2. check if _add_default_groups signal handler is called properly when local_site.users.add(user)
  3. check if _add_default_groups signal handler is called properly when user.local_site.add(local_site)
  4. check if _add_default_groups adds global default review_groups when a user is registered
  5. check if _add_default_groups adds a local_site's default groups when local_site.users.add(user) or user.local_site.add(local_site)

Edge cases:

  1. there's no default group
  2. there's no non-default group
  • 0
  • 0
  • 25
  • 0
  • 25
Description From Last Updated
Stanley Yeo
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/site/signals.py
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/reviews/models/group.py
        reviewboard/reviews/admin.py
        reviewboard/site/models.py
        reviewboard/accounts/models.py
        reviewboard/reviews/evolutions/is_default_group.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/site/signals.py
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/reviews/models/group.py
        reviewboard/reviews/admin.py
        reviewboard/site/models.py
        reviewboard/accounts/models.py
        reviewboard/reviews/evolutions/is_default_group.py
    
    
  2. 
      
Stanley Yeo
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/site/signals.py
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/reviews/models/group.py
        reviewboard/reviews/admin.py
        reviewboard/site/models.py
        reviewboard/accounts/models.py
        reviewboard/reviews/evolutions/is_default_group.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/site/signals.py
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/reviews/models/group.py
        reviewboard/reviews/admin.py
        reviewboard/site/models.py
        reviewboard/accounts/models.py
        reviewboard/reviews/evolutions/is_default_group.py
    
    
  2. 
      
Christian Hammond
  1. Generally-speaking, this looks good. However, this change should not be limited to Local Sites. It must support Local Sites, but also must work in the standard case of one global site. As such, the docs, help text, comments, etc. should not be LocalSite-specific.

  2. reviewboard/accounts/models.py (Diff revision 2)
     
     

    "to a LocalSite"

  3. reviewboard/accounts/models.py (Diff revision 2)
     
     

    "to the default groups"

  4. The start of this file must be:

    from __future__ import unicode_literals
    
    <other imports here>
    
  5. 
      
Barret Rennie
  1. 
      
  2. reviewboard/reviews/models/group.py (Diff revision 2)
     
     
     
     
     

    No hyphen in "local site"

  3. reviewboard/reviews/models/group.py (Diff revision 2)
     
     

    No comma.

  4. reviewboard/site/signals.py (Diff revision 2)
     
     

    This also needs from __future__ import unicode_literals.

  5. reviewboard/site/signals.py (Diff revision 2)
     
     

    Single quotes.

  6. 
      
Stanley Yeo
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/site/signals.py
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/reviews/models/group.py
        reviewboard/reviews/admin.py
        reviewboard/site/models.py
        reviewboard/accounts/models.py
        reviewboard/reviews/evolutions/is_default_group.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/site/signals.py
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/reviews/models/group.py
        reviewboard/reviews/admin.py
        reviewboard/site/models.py
        reviewboard/accounts/models.py
        reviewboard/reviews/evolutions/is_default_group.py
    
    
  2. 
      
Christian Hammond
  1. This is still LocalSite-specific. It needs to be updated to work with the main global site.

    Also, we'll need unit tests as part of this that cover:

    • Newly registered users not on a LocalSite without having any default non-LocalSite groups (just normal groups)
    • Newly registered users not on a LocalSite with having default non-LocalSite groups.
    • Newly registered users not on a LocalSite with having default LocalSite groups.
    • Newly registered users immediately added to a LocalSite, without having any LocalSite default groups (just normal groups).
    • Newly registered users immediately added to a LocalSite, with having default LocalSite default groups.
    • Newly registered users immediately added to a LocalSite, with having default non-LocalSite default groups.
    • Existing users added to a LocalSite, without having any default non-LocalSite groups (just normal groups).
    • Existing users added to a LocalSite, with having default non-LocalSite groups.
    • Existing users added to a LocalSite, with having default LocalSite groups.
    • Actions on either side of that M2M relation.
    • Anything else you can think of that's relevant.
  2. reviewboard/accounts/models.py (Diff revision 3)
     
     
     
     
     
     
     
     
     
     
     
     
     

    This is still LocalSite-specific. You're going to need a version for new users registered on the site. We have a signal for this in djblets.auth.signals.

  3. reviewboard/accounts/models.py (Diff revision 3)
     
     
     

    You don't need to re-save a model when adding to a ManyToManyField relation.

  4. reviewboard/reviews/models/group.py (Diff revision 3)
     
     
     
     
     

    This is still referring to Local Sites.

  5. reviewboard/site/models.py (Diff revision 3)
     
     
     
     
     
     
     
     
     

    I suspect this function will fail to do the right thing if you do:

    user.local_sites.add(...)
    
  6. reviewboard/site/models.py (Diff revision 3)
     
     
     
     

    Is there a reason we need to do this, and can't simply trust pk_set in a post_add call?

  7. reviewboard/site/models.py (Diff revision 3)
     
     
     
     

    No blank line here.

  8. reviewboard/site/signals.py (Diff revision 3)
     
     
     
     

    Two blank lines here.

  9. 
      
Stanley Yeo
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/site/signals.py
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/reviews/models/group.py
        reviewboard/reviews/admin.py
        reviewboard/site/models.py
        reviewboard/accounts/models.py
        reviewboard/reviews/evolutions/is_default_group.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/site/signals.py
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/reviews/models/group.py
        reviewboard/reviews/admin.py
        reviewboard/site/models.py
        reviewboard/accounts/models.py
        reviewboard/reviews/evolutions/is_default_group.py
    
    
  2. 
      
Barret Rennie
  1. 
      
  2. reviewboard/accounts/models.py (Diff revision 4)
     
     
     
     
     

    Docstrings should be of the form:

    """Single line summary.

    Multi-line description.
    """

    The summary should be written in the imperative mood (e.g., "Handle ...").

  3. reviewboard/site/models.py (Diff revision 4)
     
     
     

    This should be in the imperative mood, e.g.:

    "Handle a many-to-many relationship changing between LocalSites and Users."

  4. 
      
Stanley Yeo
Stanley Yeo
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/site/signals.py
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/reviews/tests.py
        reviewboard/reviews/models/group.py
        reviewboard/reviews/admin.py
        reviewboard/site/models.py
        reviewboard/accounts/models.py
        reviewboard/reviews/evolutions/is_default_group.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/site/signals.py
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/reviews/tests.py
        reviewboard/reviews/models/group.py
        reviewboard/reviews/admin.py
        reviewboard/site/models.py
        reviewboard/accounts/models.py
        reviewboard/reviews/evolutions/is_default_group.py
    
    
  2. reviewboard/reviews/tests.py (Diff revision 5)
     
     
     '_on_local_site_users_changed' imported but unused
    
  3. reviewboard/reviews/tests.py (Diff revision 5)
     
     
     local variable 'group' is assigned to but never used
    
  4. reviewboard/reviews/tests.py (Diff revision 5)
     
     
     local variable 'group' is assigned to but never used
    
  5. 
      
Stanley Yeo
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/site/signals.py
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/reviews/tests.py
        reviewboard/reviews/models/group.py
        reviewboard/reviews/admin.py
        reviewboard/site/models.py
        reviewboard/accounts/models.py
        reviewboard/reviews/evolutions/is_default_group.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/site/signals.py
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/reviews/tests.py
        reviewboard/reviews/models/group.py
        reviewboard/reviews/admin.py
        reviewboard/site/models.py
        reviewboard/accounts/models.py
        reviewboard/reviews/evolutions/is_default_group.py
    
    
  2. 
      
Barret Rennie
  1. 
      
  2. reviewboard/accounts/models.py (Diff revision 6)
     
     
     

    This needs to all fit on the first line.

  3. reviewboard/reviews/tests.py (Diff revision 6)
     
     

    How about "triggers" instead of "reaches" ?

  4. reviewboard/site/models.py (Diff revision 6)
     
     
     

    The first sentence of a docstring must fit on the first line.

    How about:
    """Handle the m2m_changed event for LocalSite and User."""

  5. reviewboard/site/models.py (Diff revision 6)
     
     

    I think this wording needs a bit of work. How about:

    "This function handles both the case where users are added to local sites and local sites are added to the set of a user's local sites. In both of these cases, the local_site_user_added signal is dispatched.

  6. 
      
Stanley Yeo
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/site/signals.py
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/reviews/tests.py
        reviewboard/reviews/models/group.py
        reviewboard/reviews/admin.py
        reviewboard/site/models.py
        reviewboard/accounts/models.py
        reviewboard/reviews/evolutions/is_default_group.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/site/signals.py
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/reviews/tests.py
        reviewboard/reviews/models/group.py
        reviewboard/reviews/admin.py
        reviewboard/site/models.py
        reviewboard/accounts/models.py
        reviewboard/reviews/evolutions/is_default_group.py
    
    
  2. 
      
Barret Rennie
  1. This looks good to me!

    I'm going to wait on a review from someone more familiar with signals before landing this.

  2. reviewboard/accounts/models.py (Diff revision 7)
     
     
     

    Either the second sentence should begin on the previous line, or put a blank line between these.

  3. 
      
Stanley Yeo
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/site/signals.py
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/reviews/tests.py
        reviewboard/reviews/models/group.py
        reviewboard/reviews/admin.py
        reviewboard/site/models.py
        reviewboard/accounts/models.py
        reviewboard/reviews/evolutions/is_default_group.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/site/signals.py
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/reviews/tests.py
        reviewboard/reviews/models/group.py
        reviewboard/reviews/admin.py
        reviewboard/site/models.py
        reviewboard/accounts/models.py
        reviewboard/reviews/evolutions/is_default_group.py
    
    
  2. 
      
David Trowbridge
  1. 
      
  2. reviewboard/accounts/models.py (Diff revision 8)
     
     

    This should probably only choose default groups where local_site is None. Otherwise all new users will additionally be added to local-site specific groups even if they're not a member of that local site.

  3. 
      
Stanley Yeo
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/site/signals.py
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/reviews/tests.py
        reviewboard/reviews/models/group.py
        reviewboard/reviews/admin.py
        reviewboard/site/models.py
        reviewboard/accounts/models.py
        reviewboard/reviews/evolutions/is_default_group.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/site/signals.py
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/reviews/tests.py
        reviewboard/reviews/models/group.py
        reviewboard/reviews/admin.py
        reviewboard/site/models.py
        reviewboard/accounts/models.py
        reviewboard/reviews/evolutions/is_default_group.py
    
    
  2. 
      
David Trowbridge
  1. I'm going to make some slight changes to the documentation and push this. Thanks!

  2. 
      
Stanley Yeo
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-2.5.x (dbc39c4)
Loading...