Automatically add new users to default groups

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

Information

Review Board
master
5366233...

Reviewers

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
Description From Last Updated

"to a LocalSite"

chipx86chipx86

"to the default groups"

chipx86chipx86

The start of this file must be: from __future__ import unicode_literals <other imports here>

chipx86chipx86

No hyphen in "local site"

brenniebrennie

No comma.

brenniebrennie

This also needs from __future__ import unicode_literals.

brenniebrennie

Single quotes.

brenniebrennie

This is still LocalSite-specific. You're going to need a version for new users registered on the site. We have a …

chipx86chipx86

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

chipx86chipx86

This is still referring to Local Sites.

chipx86chipx86

I suspect this function will fail to do the right thing if you do: user.local_sites.add(...)

chipx86chipx86

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

chipx86chipx86

No blank line here.

chipx86chipx86

Two blank lines here.

chipx86chipx86

Docstrings should be of the form: """Single line summary. Multi-line description. """ The summary should be written in the imperative …

brenniebrennie

This should be in the imperative mood, e.g.: "Handle a many-to-many relationship changing between LocalSites and Users."

brenniebrennie

'_on_local_site_users_changed' imported but unused

reviewbotreviewbot

local variable 'group' is assigned to but never used

reviewbotreviewbot

local variable 'group' is assigned to but never used

reviewbotreviewbot

This needs to all fit on the first line.

brenniebrennie

How about "triggers" instead of "reaches" ?

brenniebrennie

The first sentence of a docstring must fit on the first line. How about: """Handle the m2m_changed event for LocalSite …

brenniebrennie

I think this wording needs a bit of work. How about: "This function handles both the case where users are …

brenniebrennie

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

brenniebrennie

This should probably only choose default groups where local_site is None. Otherwise all new users will additionally be added to …

daviddavid
YY
reviewbot
  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. 
      
YY
reviewbot
  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. 
      
chipx86
  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. 
      
brennie
  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. 
      
YY
reviewbot
  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. 
      
chipx86
  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. 
      
YY
reviewbot
  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. 
      
brennie
  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. 
      
YY
YY
reviewbot
  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. 
      
YY
reviewbot
  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. 
      
brennie
  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. 
      
YY
reviewbot
  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. 
      
brennie
  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. 
      
YY
reviewbot
  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
  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. 
      
YY
reviewbot
  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
  1. I'm going to make some slight changes to the documentation and push this. Thanks!

  2. 
      
YY
Review request changed

Status: Closed (submitted)

Change Summary:

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