Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bulk organization user creation/addition feature #3651

Merged
merged 32 commits into from
Dec 4, 2024
Merged
Show file tree
Hide file tree
Changes from 28 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
4b45cd9
add first draft
teovin Nov 6, 2024
2741dd6
Merge branch 'develop' into bulk-org-user-creation
teovin Nov 7, 2024
125d965
lint fix
teovin Nov 7, 2024
741405b
update language, make names optional
teovin Nov 7, 2024
df20fef
show help text along with errors, validate the header rows
teovin Nov 14, 2024
83f4498
add test
teovin Nov 15, 2024
6c88171
clean up the queries, update success message per clare
teovin Nov 15, 2024
0e60202
add messaging scenario where all users in csv are invalid
teovin Nov 18, 2024
0f09756
move messages into helper functions
teovin Nov 18, 2024
d41aa7e
reorganize emailing pieces
teovin Nov 18, 2024
420ae6a
DRY the messaging helpers
teovin Nov 18, 2024
808e525
DRY the test a bit
teovin Nov 18, 2024
cd4a571
minor text update
teovin Nov 18, 2024
0f4251c
rename vars
teovin Nov 20, 2024
2431e07
put feature behind flag
teovin Nov 20, 2024
d9ef26e
Merge remote-tracking branch 'origin' into bulk-org-user-creation
teovin Nov 20, 2024
6488bfe
rebase and readd the migration
teovin Nov 20, 2024
18e1fa2
pass user raw_email for when sending emails to updated users
teovin Nov 21, 2024
4e76c75
validate form for no data and duplicate emails cases as well
teovin Nov 21, 2024
eace4cb
DRY the validation
teovin Nov 22, 2024
41d7dcc
Merge branch 'develop' into bulk-org-user-creation
teovin Nov 22, 2024
afb6a4d
tweak to language
teovin Nov 22, 2024
eb97ff2
update test lang as well
teovin Nov 22, 2024
2d59d57
further dry the validation
teovin Nov 22, 2024
3bed51f
review changes
teovin Nov 25, 2024
1cbee33
lint
teovin Nov 25, 2024
c5483e2
test fix
teovin Dec 2, 2024
2b4e104
lowercase email to prevent creating new user with same email
teovin Dec 2, 2024
4ce15de
use django EmailValidator class to validate emails
teovin Dec 2, 2024
acd0457
show duplicate user in error message
teovin Dec 2, 2024
92e06de
rename var
teovin Dec 2, 2024
cf5882d
prevent downcasing raw email
teovin Dec 3, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
158 changes: 158 additions & 0 deletions perma_web/perma/forms.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
import logging
import secrets
import csv
import re
from io import TextIOWrapper
import string
from typing import Any, Mapping

Expand Down Expand Up @@ -437,6 +440,161 @@ def save(self, commit=True):
return instance


class MultipleUsersFormWithOrganization(ModelForm):
"""
Create multiple organization users via CSV file
"""
organizations = forms.ModelChoiceField(label='Organization', queryset=Organization.objects.order_by('name'))
indefinite_affiliation = forms.BooleanField(
label="Permanent affiliation",
required=False,
initial=True
)
expires_at = forms.DateTimeField(
label="Affiliation expiration date",
widget=forms.DateTimeInput(attrs={"type": "date"}),
required=False
)
csv_file = forms.FileField(label='* User information',
help_text=mark_safe("<br>* When creating your CSV, please include the following fields: first_name, last_name, email. "
"First and last name columns may be left blank.<br><br>"
"If there is already a Perma.cc account associated with an "
"email, we will add an Organization affiliation. If there is not, "
"an account will be created and automatically affiliated with this "
"Organization."))

def __init__(self, request, data=None, files=None, **kwargs):
super(MultipleUsersFormWithOrganization, self).__init__(data, files, **kwargs)
self.request = request
self.user_data = {}
self.created_users = {}
self.updated_users = {}
self.ineligible_users = {}

# Filter available organizations based on the current user
query = self.fields['organizations'].queryset
if request.user.is_registrar_user():
query = query.filter(registrar_id=request.user.registrar_id)
elif request.user.is_organization_user:
query = query.filter(users=request.user.pk)
self.fields['organizations'].queryset = query

class Meta:
model = LinkUser
fields = ["organizations", "indefinite_affiliation", "expires_at", "csv_file"]

def clean_csv_file(self):
file = self.cleaned_data['csv_file']

# check if file is CSV
if not file.name.endswith('.csv'):
raise forms.ValidationError("The file must be a CSV.")

file = TextIOWrapper(file, encoding='utf-8')
reader = csv.DictReader(file)

# validate the headers
headers = reader.fieldnames
if not all(item in headers for item in ['first_name', 'last_name', 'email']):
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed something fun. If you want, you actually don't need the seen set or the row_count integer! if email in self.user_data is the same as if email in seen, and row_count is the same as len(self.user_data) 🙂 But if you prefer the aesthetics of seen and row_count, no objections here!


for row in reader:
row_count += 1
email = row.get('email')
email = email.strip().lower() if email else None
rebeccacremona marked this conversation as resolved.
Show resolved Hide resolved

if not email:
raise forms.ValidationError("Each row in the CSV file must contain email.")
if not re.match(r'^[^\s@]+@[^\s@]+\.[^\s@]+$', email):
raise forms.ValidationError("CSV file must contain valid email addresses.")
rebeccacremona marked this conversation as resolved.
Show resolved Hide resolved

if email in seen:
raise forms.ValidationError("CSV file cannot contain duplicate users.")
rebeccacremona marked this conversation as resolved.
Show resolved Hide resolved
else:
seen.add(email)
self.user_data[email] = {
'first_name': row.get('first_name', '').strip(),
'last_name': row.get('last_name', '').strip()
}

if row_count == 0:
raise forms.ValidationError("CSV file must contain at least one user.")

file.seek(0)
self.cleaned_data['csv_file'] = file
return file

def save(self, commit=True):
expires_at = self.cleaned_data['expires_at']
organization = self.cleaned_data['organizations']

emails = set(self.user_data.keys())
existing_users = LinkUser.objects.filter(email__in=emails)
rebeccacremona marked this conversation as resolved.
Show resolved Hide resolved
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 = emails - set(self.ineligible_users.keys()) - set(self.updated_users.keys())
created_user_affiliations = []

if new_user_emails and commit:
for user in new_user_emails:
rebeccacremona marked this conversation as resolved.
Show resolved Hide resolved
new_user = LinkUser(
email=user,
first_name=self.user_data[user]['first_name'],
last_name=self.user_data[user]['last_name']
)
new_user.save()
self.created_users[user] = new_user

created_user_affiliations.append(
UserOrganizationAffiliation(
user=new_user,
organization=organization,
expires_at=expires_at
)
)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I regret am having a little trouble reading this section. I think it is partly because of variable names.

created_user_affiliations is a list of affiliation objects (which makes sense to me) but, the similarly named updated_user_affiliations is a list of user objects.

similarly with preexisting_affiliations_set and all_user_affiliations; both are sets, and both are sets of user objects.

I am confused whether updated_user_affiliations and all_user_affiliations are in fact needed. Is this true? updated_user_affiliations is only used to make all_user_affiliations, and all_user_affiliations is the same as set(self.updated_users.values())? If that's true, I think this would be easier to read if you used that directly.

if commit:
# 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=updated_user_affiliations,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just a thought about readability. Do you think it would be easier to read if, right here after you get the existing affiliation objects, you did the update? Then, you could do the calculations to figure out if any new affiliation objects need to be created, and then create them, in its own section with its own comment. I think that might be clearer than doing them together, but you may disagree 🙂

organization=organization))

preexisting_affiliations_set = set(affiliation.user for affiliation in preexisting_affiliations)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, here's a good Django + database thing to know about! Since you are iterating through the UserOrganizationAffiliation queryset and fetching each object's related user object, this will make one database query per object. Django includes a utility for situations like this, select_related: https://docs.djangoproject.com/en/5.1/ref/models/querysets/#select-related. If you call UserOrganizationAffiliation.objects.filter(...).select_related('user'), then Django will get all those user objects in one single query!

all_user_affiliations = set(updated_user_affiliations)
# new affiliations
new_affiliations = all_user_affiliations - 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


### USER EDIT FORMS ###

class UserAddRegistrarForm(UserFormWithRegistrar):
Expand Down
29 changes: 29 additions & 0 deletions perma_web/perma/migrations/0057_auto_20241120_1837.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
# Generated by Django 4.2.16 on 2024-11-20 18:37

from django.db import migrations

FLAG_NAME = "bulk-organization-user-creation"

def create_bulk_organization_user_creation_feature_flag(apps, schema_editor):
Flag = apps.get_model("waffle", "Flag")
flag = Flag(
name=FLAG_NAME,
testing=True
)
flag.save()

def delete_bulk_organization_user_creation_feature_flag(apps, schema_editor):
Flag = apps.get_model("waffle", "Flag")
flags = Flag.objects.filter(name=FLAG_NAME)
flags.delete()

class Migration(migrations.Migration):

dependencies = [
('perma', '0056_delete_historicallink'),
('waffle', '0004_update_everyone_nullbooleanfield'),
]

operations = [
migrations.RunPython(create_bulk_organization_user_creation_feature_flag, delete_bulk_organization_user_creation_feature_flag),
]
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
TITLE: A Perma.cc account has been created for you
{% with form.cleaned_data.organizations.0 as org %}
{% with form.cleaned_data.organizations as org %}
A Perma.cc account has been created for you by {{ request.user.get_full_name }} on behalf of {{ org }}.{% endwith %}

{% include 'email/includes/activation.txt' %}
3 changes: 2 additions & 1 deletion perma_web/perma/templates/includes/fieldset.html
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@
{% for error in field.errors %}
<span class="field-error">{{ error }}</span>
{% endfor %}
{% elif field.help_text %}
{% endif %}
{% if field.help_text %}
<span class="help-inline">{{ field.help_text }}</span>
{% endif %}
</div>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
{% extends "admin-layout.html" %}

{% block title %} | Add users to organization{% endblock %}

{% block adminContent %}
<h3 class="body-bh">Add multiple users to organization via CSV</h3>

{% if not form %}
<p>{{ error_message }}</p>
{% else %}

<form class="add-user" method="post" enctype="multipart/form-data">
{% csrf_token %}
{% include "includes/fieldset.html" %}
<button type="submit" class="btn">Add organization users</button>
<a href="{% url 'user_management_manage_organization_user' %}" class="btn cancel">Cancel</a>
</form>

<script>
const checkbox = document.getElementById("id_a-indefinite_affiliation");
const datetimeField = document.getElementById("id_a-expires_at");
const datetimeFieldLabel = document.querySelector('label[for="id_a-expires_at"]');

const toggleExpirationDateField = () => {
const displayStyle = checkbox.checked ? "none" : "block";
datetimeField.style.display = displayStyle;
datetimeFieldLabel.style.display = displayStyle;
};

document.addEventListener("DOMContentLoaded", toggleExpirationDateField);
checkbox.addEventListener("change", toggleExpirationDateField);
</script>

{% endif %}
{% endblock %}
3 changes: 3 additions & 0 deletions perma_web/perma/templates/user_management/manage_users.html
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@ <h2 class="body-ah">{{ pretty_group_name_plural|title }}</h2>

<a class="action-heading" data-toggle="collapse" href="#add-member" aria-expanded="false" aria-controls="#add-member"><i class="icon-plus-sign" aria-hidden="true"></i> add<span class="_verbose"> {{ pretty_group_name|lower }}</span></a>

{% if group_name == 'organization_user' and bulk_org_user_creation_feature_flag %}
<p class="bulk-users">or <a href="{% url 'user_management_organization_user_add_multiple_users' %}">add multiple users</a></p>
{% endif %}
{% if messages %}
{% for message in messages %}
<div class="alert-{{ message.level_tag }} alert-block" >{% if 'safe' in message.tags %}{{ message|safe }}{% else %}{{ message }}{% endif %}</div>
Expand Down
1 change: 1 addition & 0 deletions perma_web/perma/tests/test_permissions.py
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ def test_permissions(client, admin_user, registrar_user, org_user, link_user_fac
['user_management_manage_organization_user'],
['user_management_manage_organization'],
['user_management_organization_user_add_user'],
['user_management_organization_user_add_multiple_users']
],
'allowed': {admin_user, registrar_user, org_user_registrar_user, sponsored_user_registrar_user, org_user},
},
Expand Down
77 changes: 77 additions & 0 deletions perma_web/perma/tests/test_views_user_management.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,15 @@
from django.http import HttpResponse, JsonResponse
from django.urls import reverse
from django.core import mail
from django.core.files.uploadedfile import SimpleUploadedFile
from django.conf import settings
from django.db import IntegrityError
from django.test import override_settings
from django.test.client import RequestFactory

from perma.models import LinkUser, Organization, Registrar, Sponsorship, UserOrganizationAffiliation
from perma.tests.utils import PermaTestCase
from perma.forms import MultipleUsersFormWithOrganization


class UserManagementViewsTestCase(PermaTestCase):
Expand Down Expand Up @@ -641,6 +644,80 @@ def add_org_user(self):
organizations=self.organization
).exists())

def test_add_multiple_org_users_via_csv(self):
def create_csv_file(filename, content):
return SimpleUploadedFile(filename, content.encode('utf-8'), content_type='text/csv')

def initialize_form(csv_file, data=None):
data = {'organizations': selected_organization.pk, 'indefinite_affiliation': True}
return MultipleUsersFormWithOrganization(request=request, data=data, files={'csv_file': csv_file})

# --- initialize data ---
csv_data = 'first_name,last_name,email\nJohn,Doe,[email protected]\nJane,Smith,[email protected]'
another_csv_data = 'first_name,last_name,email\nJohn2,Doe,[email protected]\nJane2,Smith,[email protected]'
invalid_csv_data = 'name\nJohn Doe'
another_invalid_csv_data = 'first_name,last_name,email\nJohn,Doe,\nJane,Smith,[email protected]'

valid_csv_file = create_csv_file('users.csv', csv_data)
another_valid_csv_file = create_csv_file('another_valid_users.csv', another_csv_data)
one_more_valid_csv_file = create_csv_file('one_more_valid_users.csv', csv_data)
invalid_csv_file = create_csv_file('invalid_users.csv', invalid_csv_data)
another_invalid_csv_file = create_csv_file('another_invalid_users.csv', another_invalid_csv_data)

request = RequestFactory().get('/')
request.user = self.registrar_user
selected_organization = self.another_organization

# --- test form initialization ---
form = MultipleUsersFormWithOrganization(request=request)
# the registrar user has 3 organizations tied to it as verified in the users.json sample data
self.assertEqual(form.fields['organizations'].queryset.count(), 3)
# confirm that the first item in organization selection field matches the first organization of the registrar
self.assertEqual(form.fields['organizations'].queryset.first(), request.user.registrar.organizations
.order_by('name').first())

# --- test csv validation ---
# valid csv
form1 = initialize_form(valid_csv_file)
self.assertTrue(form1.is_valid())

# invalid csv - missing headers
form2 = initialize_form(invalid_csv_file)
self.assertFalse(form2.is_valid())
self.assertTrue("CSV file must contain a header row with first_name, last_name and email columns."
in form2.errors['csv_file'])

# invalid csv - missing email field
form3 = initialize_form(another_invalid_csv_file)
self.assertFalse(form3.is_valid())
self.assertTrue("Each row in the CSV file must contain email."
in form3.errors['csv_file'])

# --- test user creation ---
self.assertTrue(form1.is_valid())
form1.save(commit=True)
created_user_ids = [user.id for user in form1.created_users.values()]
self.assertEqual(len(created_user_ids), 2)
self.assertEqual(UserOrganizationAffiliation.objects.filter(user_id__in=created_user_ids).count(), 2)

# --- test user update ---
existing_user = LinkUser.objects.create(email="[email protected]", first_name="John2", last_name="Doe")
form4 = initialize_form(another_valid_csv_file)
self.assertTrue(form4.is_valid())
form4.save(commit=True)
self.assertEqual(len(form4.updated_users), 1)
self.assertTrue(existing_user in form4.updated_users.values())
self.assertEqual(len(form4.created_users), 1)
self.assertEqual(next(iter(form4.updated_users)), "[email protected]")

# --- test validation errors ---
LinkUser.objects.filter(raw_email="[email protected]").update(is_staff=True)
form5 = initialize_form(one_more_valid_csv_file)
self.assertTrue(form5.is_valid())
form5.save(commit=True)
self.assertEqual(len(form5.ineligible_users), 1)
self.assertEqual("[email protected]", next(iter(form5.ineligible_users)))

def test_admin_user_can_add_new_user_to_org(self):
self.log_in_user(self.admin_user)
self.add_org_user()
Expand Down
3 changes: 2 additions & 1 deletion perma_web/perma/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
from django.urls import re_path, reverse_lazy
from django.views.generic import RedirectView

from perma.views.user_management import AddUserToOrganization, AddUserToRegistrar, AddSponsoredUserToRegistrar, AddUserToAdmin, AddRegularUser
from perma.views.user_management import AddUserToOrganization, AddMultipleUsersToOrganization, AddUserToRegistrar, AddSponsoredUserToRegistrar, AddUserToAdmin, AddRegularUser
from .views.common import DirectTemplateView
from .views import (
admin_stats,
Expand Down Expand Up @@ -148,6 +148,7 @@

re_path(r'^manage/organization-users/?$', user_management.manage_organization_user, name='user_management_manage_organization_user'),
re_path(r'^manage/organization-users/add-user/?$', AddUserToOrganization.as_view(), name='user_management_organization_user_add_user'),
re_path(r'^manage/organization-users/add-multiple-users/?$', AddMultipleUsersToOrganization.as_view(), name='user_management_organization_user_add_multiple_users'),
re_path(r'^manage/organization-users/(?P<user_id>\d+)/?$', user_management.manage_single_organization_user, name='user_management_manage_single_organization_user'),
re_path(r'^manage/organization-users/(?P<user_id>\d+)/delete/?$', user_management.manage_single_organization_user_delete, name='user_management_manage_single_organization_user_delete'),
re_path(r'^manage/organization-users/(?P<user_id>\d+)/reactivate/?$', user_management.manage_single_organization_user_reactivate, name='user_management_manage_single_organization_user_reactivate'),
Expand Down
Loading