From 217ab9a260af9c9571a15868c08e1e981a9fddbd Mon Sep 17 00:00:00 2001 From: Rebecca Cremona Date: Wed, 4 Dec 2024 17:05:46 -0500 Subject: [PATCH 01/13] Remove redundant variables. --- perma_web/perma/forms.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/perma_web/perma/forms.py b/perma_web/perma/forms.py index 24347024a..d8d86fe67 100755 --- a/perma_web/perma/forms.py +++ b/perma_web/perma/forms.py @@ -499,11 +499,7 @@ def clean_csv_file(self): raise forms.ValidationError("CSV file must contain a header row with first_name, last_name and email columns.") # validate the rows - seen = set() - row_count = 0 - for row in reader: - row_count += 1 email = row.get('email') email = email.strip() if email else None @@ -516,7 +512,7 @@ def clean_csv_file(self): except ValidationError: raise forms.ValidationError(f"CSV file contains invalid email address: {email}") - if email in seen: + if email in self.user_data: raise forms.ValidationError(f"CSV file cannot contain duplicate users: {email}") else: seen.add(email) @@ -525,7 +521,7 @@ def clean_csv_file(self): 'last_name': row.get('last_name', '').strip() } - if row_count == 0: + if not len(self.user_data): raise forms.ValidationError("CSV file must contain at least one user.") file.seek(0) From 669f56a911190656cf6aba47511153edc9018a40 Mon Sep 17 00:00:00 2001 From: Rebecca Cremona Date: Wed, 4 Dec 2024 17:33:03 -0500 Subject: [PATCH 02/13] Store raw email in user_data; use lowercased version as key. --- perma_web/perma/forms.py | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/perma_web/perma/forms.py b/perma_web/perma/forms.py index d8d86fe67..c1d38efae 100755 --- a/perma_web/perma/forms.py +++ b/perma_web/perma/forms.py @@ -512,11 +512,11 @@ def clean_csv_file(self): except ValidationError: raise forms.ValidationError(f"CSV file contains invalid email address: {email}") - if email in self.user_data: + if email.lower() in self.user_data: raise forms.ValidationError(f"CSV file cannot contain duplicate users: {email}") else: - seen.add(email) - self.user_data[email] = { + self.user_data[email.lower()] = { + 'raw_email': email, 'first_name': row.get('first_name', '').strip(), 'last_name': row.get('last_name', '').strip() } @@ -532,10 +532,8 @@ def save(self, commit=True): expires_at = self.cleaned_data['expires_at'] organization = self.cleaned_data['organizations'] - raw_emails = set(self.user_data.keys()) - # lower casing the emails to feed into the filter query in order to prevent duplicate user creation - lower_case_emails = {email.lower() for email in self.user_data.keys()} - existing_users = LinkUser.objects.filter(email__in=lower_case_emails) + all_emails = set(self.user_data.keys()) + existing_users = LinkUser.objects.filter(email__in=all_emails) updated_user_affiliations = [] for user in existing_users: @@ -546,17 +544,16 @@ def save(self, commit=True): updated_user_affiliations.append(user) self.updated_users[user.email] = user - new_user_emails = lower_case_emails - set(self.ineligible_users.keys()) - set(self.updated_users.keys()) + new_user_emails = all_emails - set(self.ineligible_users.keys()) - set(self.updated_users.keys()) created_user_affiliations = [] if new_user_emails and commit: for email in new_user_emails: - raw_email = next((raw_email for raw_email in raw_emails if raw_email.lower() == email.lower()), None) new_user = LinkUser( - email=raw_email, - first_name=self.user_data[raw_email]['first_name'], - last_name=self.user_data[raw_email]['last_name'] + email=self.user_data[email]['raw_email'], + first_name=self.user_data[email]['first_name'], + last_name=self.user_data[email]['last_name'] ) new_user.save() self.created_users[email] = new_user From 4380427433c3dd1beb70c1a75978562470ac8e2d Mon Sep 17 00:00:00 2001 From: Rebecca Cremona Date: Wed, 4 Dec 2024 17:51:06 -0500 Subject: [PATCH 03/13] Store collection of updated users in one spot. --- perma_web/perma/forms.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/perma_web/perma/forms.py b/perma_web/perma/forms.py index c1d38efae..9c454df0d 100755 --- a/perma_web/perma/forms.py +++ b/perma_web/perma/forms.py @@ -534,14 +534,12 @@ def save(self, commit=True): all_emails = set(self.user_data.keys()) existing_users = LinkUser.objects.filter(email__in=all_emails) - updated_user_affiliations = [] for user in existing_users: if commit: if user.is_staff or user.is_registrar_user(): self.ineligible_users[user.email] = user else: - updated_user_affiliations.append(user) self.updated_users[user.email] = user new_user_emails = all_emails - set(self.ineligible_users.keys()) - set(self.updated_users.keys()) @@ -572,13 +570,12 @@ def save(self, commit=True): # create or update the affiliations of existing users # affiliations that already exist - preexisting_affiliations = (UserOrganizationAffiliation.objects.filter(user__in=updated_user_affiliations, + preexisting_affiliations = (UserOrganizationAffiliation.objects.filter(user__in=self.updated_users.values(), organization=organization)) preexisting_affiliations_set = set(affiliation.user for affiliation in preexisting_affiliations) - all_user_affiliations = set(updated_user_affiliations) # new affiliations - new_affiliations = all_user_affiliations - preexisting_affiliations_set + new_affiliations = set(self.updated_users.values())- preexisting_affiliations_set new_affiliation_objs = [] for item in new_affiliations: From 08b7e30f7d42d2d54075e5a9b06a2518596c80fa Mon Sep 17 00:00:00 2001 From: Rebecca Cremona Date: Wed, 4 Dec 2024 17:59:03 -0500 Subject: [PATCH 04/13] Handle existing users in a single, uninterrupted block of logic. --- perma_web/perma/forms.py | 45 ++++++++++++++++++++-------------------- 1 file changed, 23 insertions(+), 22 deletions(-) diff --git a/perma_web/perma/forms.py b/perma_web/perma/forms.py index 9c454df0d..7cd6454aa 100755 --- a/perma_web/perma/forms.py +++ b/perma_web/perma/forms.py @@ -542,6 +542,29 @@ def save(self, commit=True): else: self.updated_users[user.email] = user + # update the expiration date of any affiliations that already exist + preexisting_affiliations = UserOrganizationAffiliation.objects.filter( + user__in=self.updated_users.values(), + organization=organization + ) + if preexisting_affiliations and commit: + preexisting_affiliations.update(expires_at=expires_at) + + # build affiliation objects for existing users that need them + users_with_existing_affiliations = set(affiliation.user for affiliation in preexisting_affiliations) + users_without_existing_affiliations = set(self.updated_users.values())- users_with_existing_affiliations + new_affiliation_objs = [] + for user in users_without_existing_affiliations: + new_affiliation_objs.append(UserOrganizationAffiliation( + user=user, + organization=organization, + expires_at=expires_at + )) + + if new_affiliation_objs and commit: + UserOrganizationAffiliation.objects.bulk_create(new_affiliation_objs) + + new_user_emails = all_emails - set(self.ineligible_users.keys()) - set(self.updated_users.keys()) created_user_affiliations = [] @@ -568,28 +591,6 @@ def save(self, commit=True): # create the affiliations for new users UserOrganizationAffiliation.objects.bulk_create(created_user_affiliations) - # create or update the affiliations of existing users - # affiliations that already exist - preexisting_affiliations = (UserOrganizationAffiliation.objects.filter(user__in=self.updated_users.values(), - organization=organization)) - - preexisting_affiliations_set = set(affiliation.user for affiliation in preexisting_affiliations) - # new affiliations - new_affiliations = set(self.updated_users.values())- preexisting_affiliations_set - new_affiliation_objs = [] - - for item in new_affiliations: - new_affiliation_objs.append(UserOrganizationAffiliation( - user=item, - organization=organization, - expires_at=expires_at - )) - - if preexisting_affiliations: - preexisting_affiliations.update(expires_at=expires_at) - if new_affiliation_objs: - UserOrganizationAffiliation.objects.bulk_create(new_affiliation_objs) - return self From b200386d548b399466f20931f0bbb9381dac8128 Mon Sep 17 00:00:00 2001 From: Rebecca Cremona Date: Wed, 4 Dec 2024 18:05:28 -0500 Subject: [PATCH 05/13] Create all new affilation objects in one batch, instead of splittin by new/existing users. --- perma_web/perma/forms.py | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/perma_web/perma/forms.py b/perma_web/perma/forms.py index 7cd6454aa..28b341ace 100755 --- a/perma_web/perma/forms.py +++ b/perma_web/perma/forms.py @@ -531,6 +531,7 @@ def clean_csv_file(self): def save(self, commit=True): expires_at = self.cleaned_data['expires_at'] organization = self.cleaned_data['organizations'] + affiliations_to_create = [] all_emails = set(self.user_data.keys()) existing_users = LinkUser.objects.filter(email__in=all_emails) @@ -553,22 +554,15 @@ def save(self, commit=True): # build affiliation objects for existing users that need them users_with_existing_affiliations = set(affiliation.user for affiliation in preexisting_affiliations) users_without_existing_affiliations = set(self.updated_users.values())- users_with_existing_affiliations - new_affiliation_objs = [] for user in users_without_existing_affiliations: - new_affiliation_objs.append(UserOrganizationAffiliation( + affiliations_to_create.append(UserOrganizationAffiliation( user=user, organization=organization, expires_at=expires_at )) - if new_affiliation_objs and commit: - UserOrganizationAffiliation.objects.bulk_create(new_affiliation_objs) - - new_user_emails = all_emails - set(self.ineligible_users.keys()) - set(self.updated_users.keys()) - created_user_affiliations = [] - if new_user_emails and commit: for email in new_user_emails: new_user = LinkUser( @@ -579,7 +573,7 @@ def save(self, commit=True): new_user.save() self.created_users[email] = new_user - created_user_affiliations.append( + affiliations_to_create.append( UserOrganizationAffiliation( user=new_user, organization=organization, @@ -587,9 +581,9 @@ def save(self, commit=True): ) ) - if commit: - # create the affiliations for new users - UserOrganizationAffiliation.objects.bulk_create(created_user_affiliations) + # create new affiliation objects + if affiliations_to_create and commit: + UserOrganizationAffiliation.objects.bulk_create(affiliations_to_create) return self From d83eebd799df8c5fa85abf4a27021e9dcf9c423f Mon Sep 17 00:00:00 2001 From: Rebecca Cremona Date: Wed, 4 Dec 2024 18:06:39 -0500 Subject: [PATCH 06/13] Perform check even if commit is False. --- perma_web/perma/forms.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/perma_web/perma/forms.py b/perma_web/perma/forms.py index 28b341ace..55ec3142c 100755 --- a/perma_web/perma/forms.py +++ b/perma_web/perma/forms.py @@ -537,11 +537,10 @@ def save(self, commit=True): existing_users = LinkUser.objects.filter(email__in=all_emails) for user in existing_users: - if commit: - if user.is_staff or user.is_registrar_user(): - self.ineligible_users[user.email] = user - else: - self.updated_users[user.email] = user + if user.is_staff or user.is_registrar_user(): + self.ineligible_users[user.email] = user + else: + self.updated_users[user.email] = user # update the expiration date of any affiliations that already exist preexisting_affiliations = UserOrganizationAffiliation.objects.filter( From e4a68d78d0aa76ea52a103def37c51c8400b497d Mon Sep 17 00:00:00 2001 From: Rebecca Cremona Date: Wed, 4 Dec 2024 18:09:21 -0500 Subject: [PATCH 07/13] Move commit check to save operation. --- perma_web/perma/forms.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/perma_web/perma/forms.py b/perma_web/perma/forms.py index 55ec3142c..56247bbc7 100755 --- a/perma_web/perma/forms.py +++ b/perma_web/perma/forms.py @@ -560,16 +560,17 @@ def save(self, commit=True): expires_at=expires_at )) + # create new users and their affiliation objects new_user_emails = all_emails - set(self.ineligible_users.keys()) - set(self.updated_users.keys()) - - if new_user_emails and commit: + if new_user_emails: for email in new_user_emails: new_user = LinkUser( email=self.user_data[email]['raw_email'], first_name=self.user_data[email]['first_name'], last_name=self.user_data[email]['last_name'] ) - new_user.save() + if commit: + new_user.save() self.created_users[email] = new_user affiliations_to_create.append( From 0eb74d04501b2abf1fbdab88f44f5c5821de886d Mon Sep 17 00:00:00 2001 From: Rebecca Cremona Date: Wed, 4 Dec 2024 18:11:19 -0500 Subject: [PATCH 08/13] Remove redundant existence check. --- perma_web/perma/forms.py | 33 ++++++++++++++++----------------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/perma_web/perma/forms.py b/perma_web/perma/forms.py index 56247bbc7..6a6928953 100755 --- a/perma_web/perma/forms.py +++ b/perma_web/perma/forms.py @@ -562,24 +562,23 @@ def save(self, commit=True): # create new users and their affiliation objects new_user_emails = all_emails - set(self.ineligible_users.keys()) - set(self.updated_users.keys()) - if new_user_emails: - for email in new_user_emails: - new_user = LinkUser( - email=self.user_data[email]['raw_email'], - first_name=self.user_data[email]['first_name'], - last_name=self.user_data[email]['last_name'] - ) - if commit: - new_user.save() - self.created_users[email] = new_user - - affiliations_to_create.append( - UserOrganizationAffiliation( - user=new_user, - organization=organization, - expires_at=expires_at - ) + for email in new_user_emails: + new_user = LinkUser( + email=self.user_data[email]['raw_email'], + first_name=self.user_data[email]['first_name'], + last_name=self.user_data[email]['last_name'] + ) + if commit: + new_user.save() + self.created_users[email] = new_user + + affiliations_to_create.append( + UserOrganizationAffiliation( + user=new_user, + organization=organization, + expires_at=expires_at ) + ) # create new affiliation objects if affiliations_to_create and commit: From 69251397c873e88c9d6782de287073f1134addd3 Mon Sep 17 00:00:00 2001 From: Rebecca Cremona Date: Wed, 4 Dec 2024 18:13:07 -0500 Subject: [PATCH 09/13] Use select_related to reduce calls to the database from n to 1. --- perma_web/perma/forms.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/perma_web/perma/forms.py b/perma_web/perma/forms.py index 6a6928953..cde11af79 100755 --- a/perma_web/perma/forms.py +++ b/perma_web/perma/forms.py @@ -546,7 +546,7 @@ def save(self, commit=True): preexisting_affiliations = UserOrganizationAffiliation.objects.filter( user__in=self.updated_users.values(), organization=organization - ) + ).select_related('user') if preexisting_affiliations and commit: preexisting_affiliations.update(expires_at=expires_at) From 92e032fd2cd381f741c9f2d7db2899a7d6dc2f2d Mon Sep 17 00:00:00 2001 From: Rebecca Cremona Date: Wed, 4 Dec 2024 18:17:48 -0500 Subject: [PATCH 10/13] Whitespace, comment. --- perma_web/perma/forms.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/perma_web/perma/forms.py b/perma_web/perma/forms.py index cde11af79..b7fb664fb 100755 --- a/perma_web/perma/forms.py +++ b/perma_web/perma/forms.py @@ -531,18 +531,19 @@ def clean_csv_file(self): def save(self, commit=True): expires_at = self.cleaned_data['expires_at'] organization = self.cleaned_data['organizations'] - affiliations_to_create = [] all_emails = set(self.user_data.keys()) - existing_users = LinkUser.objects.filter(email__in=all_emails) + affiliations_to_create = [] + # find any existing users, and exclude any that are ineligible to become org users + existing_users = LinkUser.objects.filter(email__in=all_emails) for user in existing_users: if user.is_staff or user.is_registrar_user(): self.ineligible_users[user.email] = user else: self.updated_users[user.email] = user - # update the expiration date of any affiliations that already exist + # update the affiliation expiration date for any already-affiliated users preexisting_affiliations = UserOrganizationAffiliation.objects.filter( user__in=self.updated_users.values(), organization=organization From d6479f5a5bf3664aeb8b577d4e3c697cb2bab978 Mon Sep 17 00:00:00 2001 From: Rebecca Lynn Cremona Date: Thu, 5 Dec 2024 12:51:20 -0500 Subject: [PATCH 11/13] Update perma_web/perma/forms.py Co-authored-by: cmsetzer <37311998+cmsetzer@users.noreply.github.com> --- perma_web/perma/forms.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/perma_web/perma/forms.py b/perma_web/perma/forms.py index b7fb664fb..735c89517 100755 --- a/perma_web/perma/forms.py +++ b/perma_web/perma/forms.py @@ -521,7 +521,7 @@ def clean_csv_file(self): 'last_name': row.get('last_name', '').strip() } - if not len(self.user_data): + if not self.user_data: raise forms.ValidationError("CSV file must contain at least one user.") file.seek(0) From 09c88c8a3a4cc27c937ff57f0c1cbb69b986a663 Mon Sep 17 00:00:00 2001 From: Rebecca Lynn Cremona Date: Thu, 5 Dec 2024 12:51:42 -0500 Subject: [PATCH 12/13] Update perma_web/perma/forms.py Co-authored-by: cmsetzer <37311998+cmsetzer@users.noreply.github.com> --- perma_web/perma/forms.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/perma_web/perma/forms.py b/perma_web/perma/forms.py index 735c89517..d1ef892de 100755 --- a/perma_web/perma/forms.py +++ b/perma_web/perma/forms.py @@ -553,7 +553,7 @@ def save(self, commit=True): # build affiliation objects for existing users that need them users_with_existing_affiliations = set(affiliation.user for affiliation in preexisting_affiliations) - users_without_existing_affiliations = set(self.updated_users.values())- users_with_existing_affiliations + users_without_existing_affiliations = set(self.updated_users.values()) - users_with_existing_affiliations for user in users_without_existing_affiliations: affiliations_to_create.append(UserOrganizationAffiliation( user=user, From 547864960685c6f0de6844c6de357c56e37a6b68 Mon Sep 17 00:00:00 2001 From: Rebecca Lynn Cremona Date: Thu, 5 Dec 2024 12:55:09 -0500 Subject: [PATCH 13/13] Calling set on dict implicitly makes a set of the keys. Co-authored-by: cmsetzer <37311998+cmsetzer@users.noreply.github.com> --- perma_web/perma/forms.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/perma_web/perma/forms.py b/perma_web/perma/forms.py index d1ef892de..cd2f629b3 100755 --- a/perma_web/perma/forms.py +++ b/perma_web/perma/forms.py @@ -532,7 +532,7 @@ def save(self, commit=True): expires_at = self.cleaned_data['expires_at'] organization = self.cleaned_data['organizations'] - all_emails = set(self.user_data.keys()) + all_emails = set(self.user_data) affiliations_to_create = [] # find any existing users, and exclude any that are ineligible to become org users @@ -562,7 +562,7 @@ def save(self, commit=True): )) # create new users and their affiliation objects - new_user_emails = all_emails - set(self.ineligible_users.keys()) - set(self.updated_users.keys()) + new_user_emails = all_emails - set(self.ineligible_users) - set(self.updated_users) for email in new_user_emails: new_user = LinkUser( email=self.user_data[email]['raw_email'],