Skip to content
This repository has been archived by the owner on Dec 15, 2018. It is now read-only.

Commit

Permalink
Updated sync_grouper to not needlessly query the database when given …
Browse files Browse the repository at this point in the history
…an empty set of emails

Summary: This is in response to warnings like https://forge-sentry.pp.dropbox.com/sentry/changes/issues/79/. When we are given an empty set of admin emails, or an empty dictionary of project admins, we still do a query of (~User.email.in_(the empty list)), which is trivially true and expensive. This diff avoids doing that.

Test Plan: unit tests

Reviewers: kylec, anupc

Reviewed By: anupc

Subscribers: changesbot, herb, kylec

Differential Revision: https://tails.corp.dropbox.com/D223113
  • Loading branch information
Naphat Sanguansin committed Aug 24, 2016
1 parent 46ba9a5 commit f342e3c
Showing 1 changed file with 6 additions and 5 deletions.
11 changes: 6 additions & 5 deletions changes/jobs/sync_grouper.py
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ def _get_admin_emails_from_grouper():


def _sync_admin_users(admin_emails):
# type: (Iterable[str]) -> None
# type: (Set[str]) -> None
"""Take a look at the Changes user database. Every user with email in
`admin_emails` should become a Changes admin, and every user already
an admin whose email is not in `admin_emails` will have their
Expand All @@ -76,6 +76,7 @@ def _sync_admin_users(admin_emails):
people who should be admin.
"""
# revoke access for people who should not have admin access
assert len(admin_emails) > 0
User.query.filter(
~User.email.in_(admin_emails),
User.is_admin.is_(True),
Expand Down Expand Up @@ -148,10 +149,10 @@ def _sync_project_admin_users(project_admin_mapping):
project_admin_mapping (Dict[str, Set[str]]): The mapping from
user emails to project patterns
"""
User.query.filter(
~User.email.in_(project_admin_mapping.keys()),
~User.project_permissions.is_(None),
).update({
args = [~User.project_permissions.is_(None)]
if len(project_admin_mapping) > 0:
args.append(~User.email.in_(project_admin_mapping.keys()))
User.query.filter(*args).update({
'project_permissions': None
}, synchronize_session=False)
for email, project_permissions in project_admin_mapping.iteritems():
Expand Down

0 comments on commit f342e3c

Please sign in to comment.