- Groups:
Automatically add new users to default groups
Review Request #7165 — Created April 4, 2015 and submitted
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:
- make new local site
- make new group, addign local the local site to it, and make it a default group
- make new group and addign local the local site to it (do not make it default group)
- add a user to the local site
- see if the user is added to group created in #2
- see if the user is not added to group created in #3
Unit Tests done:
- check if _add_default_groups signal handler is called properly when a user is registered
- check if _add_default_groups signal handler is called properly when local_site.users.add(user)
- check if _add_default_groups signal handler is called properly when user.local_site.add(local_site)
- check if _add_default_groups adds global default review_groups when a user is registered
- 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:
- there's no default group
- there's no non-default group
| Description | From | Last Updated |
|---|---|---|
|
"to a LocalSite" |
 chipx86
|
|
|
"to the default groups" |
chipx86
|
|
|
The start of this file must be: from __future__ import unicode_literals <other imports here> |
chipx86
|
|
|
No hyphen in "local site" |
 brennie
|
|
|
No comma. |
brennie
|
|
|
This also needs from __future__ import unicode_literals. |
brennie
|
|
|
Single quotes. |
brennie
|
|
|
This is still LocalSite-specific. You're going to need a version for new users registered on the site. We have a … |
chipx86
|
|
|
You don't need to re-save a model when adding to a ManyToManyField relation. |
chipx86
|
|
|
This is still referring to Local Sites. |
chipx86
|
|
|
I suspect this function will fail to do the right thing if you do: user.local_sites.add(...) |
chipx86
|
|
|
Is there a reason we need to do this, and can't simply trust pk_set in a post_add call? |
chipx86
|
|
|
No blank line here. |
chipx86
|
|
|
Two blank lines here. |
chipx86
|
|
|
Docstrings should be of the form: """Single line summary. Multi-line description. """ The summary should be written in the imperative … |
brennie
|
|
|
This should be in the imperative mood, e.g.: "Handle a many-to-many relationship changing between LocalSites and Users." |
brennie
|
|
|
'_on_local_site_users_changed' imported but unused |
 reviewbot
|
|
|
local variable 'group' is assigned to but never used |
reviewbot
|
|
|
local variable 'group' is assigned to but never used |
reviewbot
|
|
|
This needs to all fit on the first line. |
brennie
|
|
|
How about "triggers" instead of "reaches" ? |
brennie
|
|
|
The first sentence of a docstring must fit on the first line. How about: """Handle the m2m_changed event for LocalSite … |
brennie
|
|
|
I think this wording needs a bit of work. How about: "This function handles both the case where users are … |
brennie
|
|
|
Either the second sentence should begin on the previous line, or put a blank line between these. |
brennie
|
|
|
This should probably only choose default groups where local_site is None. Otherwise all new users will additionally be added to … |
 david
|
YY
YY
- Change Summary:
-
documented codes.
- Description:
-
This change is to make a user to be a member of default groups of a local-site, when the user is added to the local-site.
Works done are:
- catch m2m_changed (many-to-many relation changed) signal invoked when a LocalSite-User relation changed
- calculate a diff of LocalSite-User relation
- invoke local_site_user_added for newly added user
- make a receiver of local_site_user_added, to add this newly added user to the LocalSite's default Group
- - Notes:
- - When a change of many_to_many occurs, it seems Django 1) wipes out old records of the relation, and 2) makes new records of the relation.
- At least Django admin is doing it. So, I needed to catch the m2m_changed signal and do #2~4 above. - Commit:
-
c9f2f4ee54f64364437d420e2f3faf49204b749f2d0d301c4bd05a03c46ebcbe93704fa198b8e75e
-
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
-
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.
-
-
-
-
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
-
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.
-
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. -
-
-
-
-
-
YY
- Change Summary:
-
fixed issues. will upload unit tests soon.
- Summary:
-
Automatically add new local-site users to default groups(WIP) Automatically add new users to default groups
- Description:
-
~ This change is to make a user to be a member of default groups of a local-site, when the user is added to the local-site.
~ 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:
~ ~ - catch m2m_changed (many-to-many relation changed) signal invoked when a LocalSite-User relation changed
~ - calculate a diff of LocalSite-User relation
~ - invoke local_site_user_added for newly added user
~ - make a receiver of local_site_user_added, to add this newly added user to the LocalSite's default Group
~ 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 - Commit:
-
1ec28709f182989bded8b8295cac162aa69dcd10b165f216afeaf15cabcc5b6571586cb89a7a572b
-
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
YY
- Change Summary:
-
added unit tests
- Testing Done:
-
Browser tests done:
- make new local site
- make new group, addign local the local site to it, and make it a default group
- make new group and addign local the local site to it (do not make it default group)
- add a user to the local site
- see if the user is added to group created in #2
- see if the user is not added to group created in #3
+ Unit Tests done:
+ + - check if _add_default_groups signal handler is called properly when a user is registered
+ - check if _add_default_groups signal handler is called properly when local_site.users.add(user)
+ - check if _add_default_groups signal handler is called properly when user.local_site.add(local_site)
+ - check if _add_default_groups adds global default review_groups when a user is registered
+ - 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:
- there's no default group
- there's no non-default group
- Commit:
-
b165f216afeaf15cabcc5b6571586cb89a7a572bd87088416dbcb4096094e707f1e036c66a03c4a5
- Diff:
-
Revision 5 (+173 -6)
-
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 -
-
-
YY
- Change Summary:
-
fixed lint issues
- Commit:
-
d87088416dbcb4096094e707f1e036c66a03c4a5be6c2281e2b82268eb9492fa07e51d54037da939
- Diff:
-
Revision 6 (+171 -5)
-
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
-
-
-
-
The first sentence of a docstring must fit on the first line.
How about:
"""Handle the m2m_changed event for LocalSite and User.""" -
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_addedsignal is dispatched.
YY
- Commit:
-
be6c2281e2b82268eb9492fa07e51d54037da939e7808511f09278f54a86fafdc36e5677d5d22731
- Diff:
-
Revision 7 (+170 -5)
-
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
YY
- Commit:
-
e7808511f09278f54a86fafdc36e5677d5d2273158a084d9d7b36a92c2898b26a1bb268ca9fa9402
- Diff:
-
Revision 8 (+171 -5)
-
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
YY
- Commit:
-
58a084d9d7b36a92c2898b26a1bb268ca9fa940253662338f2539bcefd9cb419a7d2ceb124ded7d1
- Diff:
-
Revision 9 (+172 -5)
-
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
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