From 37d4ff3c570d2954f3b175c162ef331737acbf72 Mon Sep 17 00:00:00 2001
From: geistling <34081638+geistling@users.noreply.github.com>
Date: Tue, 21 May 2024 15:37:50 -0700
Subject: [PATCH 01/13] replace action imports with signals

---
 coldfront/core/project/signals.py |  7 +++
 coldfront/core/project/views.py   | 92 ++++++++++++++-----------------
 coldfront/plugins/ldap/utils.py   | 24 ++++++--
 3 files changed, 67 insertions(+), 56 deletions(-)

diff --git a/coldfront/core/project/signals.py b/coldfront/core/project/signals.py
index e0563ba5d..fbb1255ce 100644
--- a/coldfront/core/project/signals.py
+++ b/coldfront/core/project/signals.py
@@ -4,6 +4,13 @@
 project_create = django.dispatch.Signal()
     #providing_args=["project_title"]
 project_post_create = django.dispatch.Signal()
+    #providing_args=["project_obj"]
+
+project_add_projectuser = django.dispatch.Signal()
+    #providing_args=["user_name", "group_name"]
+
+project_preremove_projectuser = django.dispatch.Signal()
+    #providing_args=["user_name", "group_name"]
 
 project_activate_user = django.dispatch.Signal()
     #providing_args=["project_user_pk"]
diff --git a/coldfront/core/project/views.py b/coldfront/core/project/views.py
index c95b996cd..3e179bb15 100644
--- a/coldfront/core/project/views.py
+++ b/coldfront/core/project/views.py
@@ -29,7 +29,11 @@
     allocation_remove_user,
     allocation_activate_user,
 )
-from coldfront.core.project.signals import project_create, project_post_create
+from coldfront.core.project.signals import (
+    project_make_projectuser,
+    project_create,
+    project_post_create
+)
 from coldfront.core.grant.models import Grant
 from coldfront.core.project.forms import (
     ProjectReviewForm,
@@ -611,8 +615,6 @@ def post(self, request, *args, **kwargs):
         )
 
         added_users_count = 0
-        if 'coldfront.plugins.ldap' in settings.INSTALLED_APPS:
-            ldap_conn = LDAPConn()
 
         if formset.is_valid() and allocation_form.is_valid():
             projuserstatus_active = ProjectUserStatusChoice.objects.get(name='Active')
@@ -649,28 +651,19 @@ def post(self, request, *args, **kwargs):
 
                     role_choice = user_form_data.get('role')
 
-                    if 'coldfront.plugins.ldap' in settings.INSTALLED_APPS:
-                        try:
-                            ldap_conn.add_user_to_group(
-                                user_obj.username, project_obj.title,
-                            )
-                            logger.info(
-                                "P678: Coldfront user %s added AD User for %s to AD Group %s",
-                                self.request.user,
-                                user_obj.username,
-                                project_obj.title,
-                            )
-                        except Exception as e:
-                            error = f"Could not add user {user_obj} to AD Group for {project_obj.title}: {e}\nPlease contact Coldfront administration for further assistance."
-                            logger.error(
-                                "P685: user %s could not add AD user of %s to AD Group of %s: %s",
-                                self.request.user, user_obj, project_obj.title, e
-                            )
-                            errors.append(error)
-                            continue
-                        success_msg = f"User {user_obj} added by {request.user} to AD Group for {project_obj.title}"
-                        logger.info(success_msg)
-                        successes.append(success_msg)
+                    try:
+                        project_make_projectuser.send(
+                            sender=self.__class__,
+                            user_name=user_obj.username, group_name=project_obj.title
+                        )
+                    except Exception as e:
+                        error = f"Could not add user {user_obj} to AD Group for {project_obj.title}: {e}\nPlease contact Coldfront administration for further assistance."
+                        logger.exception('P646: %s', e)
+                        errors.append(error)
+                        continue
+                    success_msg = f"User {user_obj} added by {request.user} to AD Group for {project_obj.title}"
+                    logger.info(success_msg)
+                    successes.append(success_msg)
 
                     # Is the user already in the project?
                     project_obj.projectuser_set.update_or_create(
@@ -798,7 +791,6 @@ def post(self, request, *args, **kwargs):
             ingroup = lambda u: u['username'] in users_main_group
             users_no_removal, users_to_remove = sort_by(users_to_remove, ingroup, how="condition")
 
-
         formset = formset_factory(ProjectRemoveUserForm, max_num=len(users_to_remove))
         formset = formset(request.POST, initial=users_to_remove, prefix='userform')
 
@@ -822,30 +814,30 @@ def post(self, request, *args, **kwargs):
 
                     project_user_obj = project_obj.projectuser_set.get(user=user_obj)
 
-                    if 'coldfront.plugins.ldap' in settings.INSTALLED_APPS:
-                        try:
-                            ldap_conn.remove_member_from_group(
-                                user_obj.username, project_obj.title,
-                            )
-                            logger.info(
-                                "P835: Coldfront user %s removed AD User for %s from AD Group for %s",
-                                self.request.user,
-                                user_obj.username,
-                                project_obj.title,
-                            )
-                        except Exception as e:
-                            messages.error(
-                                request,
-                                f"could not remove user {user_obj}: {e}"
-                            )
-                            logger.error(
-                                "P846: Coldfront user %s could NOT remove AD User for %s from AD Group for %s: %s",
-                                self.request.user,
-                                user_obj.username,
-                                project_obj.title,
-                                e
-                            )
-                            continue
+                    try:
+                        project_preremove_projectuser.send(
+                            sender=self.__class__,
+                            user_name=user_obj.username, group_name=project_obj.title
+                        )
+                        logger.info(
+                            "P802: Coldfront user %s removed AD User for %s from AD Group for %s",
+                            self.request.user,
+                            user_obj.username,
+                            project_obj.title,
+                        )
+                    except Exception as e:
+                        messages.error(
+                            request,
+                            f"could not remove user {user_obj}: {e}"
+                        )
+                        logger.error(
+                            "P802: Coldfront user %s could NOT remove AD User for %s from AD Group for %s: %s",
+                            self.request.user,
+                            user_obj.username,
+                            project_obj.title,
+                            e
+                        )
+                        continue
 
                     project_user_obj.status = projectuser_status_removed
                     project_user_obj.save()
diff --git a/coldfront/plugins/ldap/utils.py b/coldfront/plugins/ldap/utils.py
index 0e7eeb9ae..b1c8b7bd5 100644
--- a/coldfront/plugins/ldap/utils.py
+++ b/coldfront/plugins/ldap/utils.py
@@ -14,7 +14,12 @@
 from ldap3.extend.microsoft.addMembersToGroups import ad_add_members_to_groups
 from ldap3.extend.microsoft.removeMembersFromGroups import ad_remove_members_from_groups
 
-from coldfront.core.project.signals import project_create, project_post_create
+from coldfront.core.project.signals import (
+    project_preremove_projectuser,
+    project_make_projectuser,
+    project_create,
+    project_post_create,
+)
 from coldfront.core.utils.common import (
     import_from_settings, uniques_and_intersection
 )
@@ -179,11 +184,6 @@ def return_group_by_name(self, groupname, return_as='dict', attributes=ALL_ATTRI
             raise ValueError("no groups returned")
         return group[0]
 
-    def add_user_to_group(self, user_name, group_name):
-        group = self.return_group_by_name(group_name)
-        user = self.return_user_by_name(user_name)
-        self.add_member_to_group(user, group)
-
     def add_group_to_group(self, group_name, parent_group_name):
         group = self.return_group_by_name(group_name)
         parent_group = self.return_group_by_name(parent_group_name)
@@ -714,3 +714,15 @@ def update_new_project(sender, **kwargs):
             role=ProjectUserRoleChoice.objects.get(name=role_name),
             status=ProjectUserStatusChoice.objects.get(name='Active'),
         )
+
+@receiver(project_make_projectuser)
+def add_user_to_group(sender, **kwargs):
+    ldap_conn = LDAPConn()
+    group = ldap_conn.return_group_by_name(kwargs['group_name'])
+    user = ldap_conn.return_user_by_name(kwargs['user_name'])
+    ldap_conn.add_member_to_group(user, group)
+
+@receiver(project_preremove_projectuser)
+def remove_member_from_group(sender, **kwargs):
+    ldap_conn = LDAPConn()
+    ldap_conn.remove_member_from_group(kwargs['user_name'], kwargs['group_name'])

From 209acc05d3b690aa7eb23c0aa8acf191b4e99fab Mon Sep 17 00:00:00 2001
From: geistling <34081638+geistling@users.noreply.github.com>
Date: Tue, 21 May 2024 16:12:18 -0700
Subject: [PATCH 02/13] use signals for info grab in project views

---
 coldfront/core/project/signals.py |  4 ++++
 coldfront/core/project/views.py   | 27 ++++++++-------------------
 coldfront/plugins/ldap/utils.py   | 12 +++++++++++-
 3 files changed, 23 insertions(+), 20 deletions(-)

diff --git a/coldfront/core/project/signals.py b/coldfront/core/project/signals.py
index fbb1255ce..04e5f2899 100644
--- a/coldfront/core/project/signals.py
+++ b/coldfront/core/project/signals.py
@@ -12,6 +12,10 @@
 project_preremove_projectuser = django.dispatch.Signal()
     #providing_args=["user_name", "group_name"]
 
+project_filter_users_to_remove = django.dispatch.Signal()
+    #providing_args=["project_user_list"]
+    # return tuple of (no_removal, can_remove)
+
 project_activate_user = django.dispatch.Signal()
     #providing_args=["project_user_pk"]
 project_remove_user = django.dispatch.Signal()
diff --git a/coldfront/core/project/views.py b/coldfront/core/project/views.py
index 3e179bb15..cb2452fd0 100644
--- a/coldfront/core/project/views.py
+++ b/coldfront/core/project/views.py
@@ -30,6 +30,8 @@
     allocation_activate_user,
 )
 from coldfront.core.project.signals import (
+    project_filter_users_to_remove,
+    project_preremove_projectuser,
     project_make_projectuser,
     project_create,
     project_post_create
@@ -72,9 +74,6 @@
 if 'django_q' in settings.INSTALLED_APPS:
     from django_q.tasks import Task
 
-if 'coldfront.plugins.ldap' in settings.INSTALLED_APPS:
-    from coldfront.plugins.ldap.utils import LDAPConn
-
 ALLOCATION_ENABLE_ALLOCATION_RENEWAL = import_from_settings(
     'ALLOCATION_ENABLE_ALLOCATION_RENEWAL', True)
 ALLOCATION_DEFAULT_ALLOCATION_LENGTH = import_from_settings(
@@ -754,14 +753,9 @@ def get(self, request, *args, **kwargs):
         users_no_removal = None
 
         # if ldap is activated, prevent selection of users with project corresponding to primary group
-        if 'coldfront.plugins.ldap' in settings.INSTALLED_APPS:
-
-            usernames = [u['username'] for u in users_to_remove]
-            ldap_conn = LDAPConn()
-            users_main_group = ldap_conn.users_in_primary_group(
-                usernames, project_obj.title)
-            ingroup = lambda u: u['username'] in users_main_group
-            users_no_removal, users_to_remove = sort_by(users_to_remove, ingroup, how="condition")
+        users_no_removal, users_to_remove = project_filter_users_to_remove.send(
+            sender=self.__class__, users=users_to_remove, project=project_obj
+        )
 
         context = {}
 
@@ -782,14 +776,9 @@ def post(self, request, *args, **kwargs):
 
         users_to_remove = self.get_users_to_remove(project_obj)
         # if ldap is activated, prevent selection of users with project corresponding to primary group
-        if 'coldfront.plugins.ldap' in settings.INSTALLED_APPS:
-
-            usernames = [u['username'] for u in users_to_remove]
-            ldap_conn = LDAPConn()
-            users_main_group = ldap_conn.users_in_primary_group(
-                usernames, project_obj.title)
-            ingroup = lambda u: u['username'] in users_main_group
-            users_no_removal, users_to_remove = sort_by(users_to_remove, ingroup, how="condition")
+        users_no_removal, users_to_remove = project_filter_users_to_remove.send(
+            sender=self.__class__, users=users_to_remove, project=project_obj
+        )
 
         formset = formset_factory(ProjectRemoveUserForm, max_num=len(users_to_remove))
         formset = formset(request.POST, initial=users_to_remove, prefix='userform')
diff --git a/coldfront/plugins/ldap/utils.py b/coldfront/plugins/ldap/utils.py
index b1c8b7bd5..84e3afd46 100644
--- a/coldfront/plugins/ldap/utils.py
+++ b/coldfront/plugins/ldap/utils.py
@@ -5,7 +5,6 @@
 import operator
 from functools import reduce
 
-from coldfront.core import field_of_science
 from django.db.models import Q
 from django.dispatch import receiver
 from django.utils import timezone
@@ -15,6 +14,7 @@
 from ldap3.extend.microsoft.removeMembersFromGroups import ad_remove_members_from_groups
 
 from coldfront.core.project.signals import (
+    project_filter_users_to_remove,
     project_preremove_projectuser,
     project_make_projectuser,
     project_create,
@@ -715,6 +715,16 @@ def update_new_project(sender, **kwargs):
             status=ProjectUserStatusChoice.objects.get(name='Active'),
         )
 
+@receiver(project_filter_users_to_remove)
+def filter_project_users_to_remove(sender, **kwargs):
+    users_to_remove = kwargs['users_to_remove']
+    usernames = [u['username'] for u in users_to_remove]
+    ldap_conn = LDAPConn()
+    users_main_group = ldap_conn.users_in_primary_group(usernames, kwargs['project'].title)
+    ingroup = lambda u: u['username'] in users_main_group
+    users_no_removal, users_to_remove = sort_by(users_to_remove, ingroup, how="condition")
+    return (users_no_removal, users_to_remove)
+
 @receiver(project_make_projectuser)
 def add_user_to_group(sender, **kwargs):
     ldap_conn = LDAPConn()

From 3ecbb682fff4e286342d1328877aafa666a8dbc3 Mon Sep 17 00:00:00 2001
From: geistling <34081638+geistling@users.noreply.github.com>
Date: Tue, 21 May 2024 16:35:29 -0700
Subject: [PATCH 03/13] fix signal name

---
 coldfront/core/project/signals.py | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/coldfront/core/project/signals.py b/coldfront/core/project/signals.py
index 04e5f2899..c7982c272 100644
--- a/coldfront/core/project/signals.py
+++ b/coldfront/core/project/signals.py
@@ -1,12 +1,11 @@
 import django.dispatch
 
-
 project_create = django.dispatch.Signal()
     #providing_args=["project_title"]
 project_post_create = django.dispatch.Signal()
     #providing_args=["project_obj"]
 
-project_add_projectuser = django.dispatch.Signal()
+project_make_projectuser = django.dispatch.Signal()
     #providing_args=["user_name", "group_name"]
 
 project_preremove_projectuser = django.dispatch.Signal()
@@ -15,8 +14,3 @@
 project_filter_users_to_remove = django.dispatch.Signal()
     #providing_args=["project_user_list"]
     # return tuple of (no_removal, can_remove)
-
-project_activate_user = django.dispatch.Signal()
-    #providing_args=["project_user_pk"]
-project_remove_user = django.dispatch.Signal()
-    #providing_args=["project_user_pk"]

From ada5fba699cc4a73c04746ba25ced2a4ce9117d5 Mon Sep 17 00:00:00 2001
From: geistling <34081638+geistling@users.noreply.github.com>
Date: Wed, 4 Sep 2024 15:56:33 -0400
Subject: [PATCH 04/13] add holylfs06

---
 .../core/resource/management/commands/add_resource_defaults.py   | 1 +
 1 file changed, 1 insertion(+)

diff --git a/coldfront/core/resource/management/commands/add_resource_defaults.py b/coldfront/core/resource/management/commands/add_resource_defaults.py
index ee0cf09af..a49297868 100644
--- a/coldfront/core/resource/management/commands/add_resource_defaults.py
+++ b/coldfront/core/resource/management/commands/add_resource_defaults.py
@@ -83,6 +83,7 @@ def handle(self, *args, **options):
             ('Tier 3', 'Attic Storage - Tape', True, storage_tier, None, 20, True, True),
             ('holylfs04/tier0', 'Holyoke data center lustre storage', True, storage, 'Tier 0', 1, True, True),
             ('holylfs05/tier0', 'Holyoke data center lustre storage', True, storage, 'Tier 0', 1, True, True),
+            ('holylfs06/tier0', 'Holyoke data center lustre storage', True, storage, 'Tier 0', 1, True, True),
             ('nesetape/tier3', 'Cold storage for past projects', True, storage, 'Tier 3', 20, True, True),
             ('holy-isilon/tier1', 'Tier1 storage with snapshots and disaster recovery copy', True, storage, 'Tier 1', 1, True, True),
             ('bos-isilon/tier1', 'Tier1 storage for on-campus storage mounting', True, storage, 'Tier 1', 1, True, True),

From cb3a391bfa5e826238801fb44fa94ec38f996f97 Mon Sep 17 00:00:00 2001
From: geistling <34081638+geistling@users.noreply.github.com>
Date: Wed, 4 Sep 2024 15:58:40 -0400
Subject: [PATCH 05/13] tier 2 gets billed

---
 .../commands/add_resource_defaults.py         | 34 +++++++++----------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/coldfront/core/resource/management/commands/add_resource_defaults.py b/coldfront/core/resource/management/commands/add_resource_defaults.py
index a49297868..d159b9703 100644
--- a/coldfront/core/resource/management/commands/add_resource_defaults.py
+++ b/coldfront/core/resource/management/commands/add_resource_defaults.py
@@ -88,23 +88,23 @@ def handle(self, *args, **options):
             ('holy-isilon/tier1', 'Tier1 storage with snapshots and disaster recovery copy', True, storage, 'Tier 1', 1, True, True),
             ('bos-isilon/tier1', 'Tier1 storage for on-campus storage mounting', True, storage, 'Tier 1', 1, True, True),
             ('holystore01/tier0', 'Luster storage under Tier0', True, storage, 'Tier 0', 1, True, True),
-            ('b-nfs02-p/tier2', 'Tier2 CEPH storage', True, storage, 'Tier 2', 1, False, True),
-            ('b-nfs03-p/tier2', 'Tier2 CEPH storage', True, storage, 'Tier 2', 1, False, True),
-            ('b-nfs04-p/tier2', 'Tier2 CEPH storage', True, storage, 'Tier 2', 1, False, True),
-            ('b-nfs05-p/tier2', 'Tier2 CEPH storage', True, storage, 'Tier 2', 1, False, True),
-            ('b-nfs06-p/tier2', 'Tier2 CEPH storage', True, storage, 'Tier 2', 1, False, True),
-            ('b-nfs07-p/tier2', 'Tier2 CEPH storage', True, storage, 'Tier 2', 1, False, True),
-            ('b-nfs08-p/tier2', 'Tier2 CEPH storage', True, storage, 'Tier 2', 1, False, True),
-            ('b-nfs09-p/tier2', 'Tier2 CEPH storage', True, storage, 'Tier 2', 1, False, True),
-            ('h-nfs11-p/tier2', 'Tier2 CEPH storage', True, storage, 'Tier 2', 1, False, True),
-            ('h-nfs12-p/tier2', 'Tier2 CEPH storage', True, storage, 'Tier 2', 1, False, True),
-            ('h-nfs13-p/tier2', 'Tier2 CEPH storage', True, storage, 'Tier 2', 1, False, True),
-            ('h-nfs14-p/tier2', 'Tier2 CEPH storage', True, storage, 'Tier 2', 1, False, True),
-            ('h-nfs15-p/tier2', 'Tier2 CEPH storage', True, storage, 'Tier 2', 1, False, True),
-            ('h-nfs16-p/tier2', 'Tier2 CEPH storage', True, storage, 'Tier 2', 1, False, True),
-            ('h-nfs17-p/tier2', 'Tier2 CEPH storage', True, storage, 'Tier 2', 1, False, True),
-            ('h-nfs18-p/tier2', 'Tier2 CEPH storage', True, storage, 'Tier 2', 1, False, True),
-            ('h-nfs19-p/tier2', 'Tier2 CEPH storage', True, storage, 'Tier 2', 1, False, True),
+            ('b-nfs02-p/tier2', 'Tier2 CEPH storage', True, storage, 'Tier 2', 1, True, True),
+            ('b-nfs03-p/tier2', 'Tier2 CEPH storage', True, storage, 'Tier 2', 1, True, True),
+            ('b-nfs04-p/tier2', 'Tier2 CEPH storage', True, storage, 'Tier 2', 1, True, True),
+            ('b-nfs05-p/tier2', 'Tier2 CEPH storage', True, storage, 'Tier 2', 1, True, True),
+            ('b-nfs06-p/tier2', 'Tier2 CEPH storage', True, storage, 'Tier 2', 1, True, True),
+            ('b-nfs07-p/tier2', 'Tier2 CEPH storage', True, storage, 'Tier 2', 1, True, True),
+            ('b-nfs08-p/tier2', 'Tier2 CEPH storage', True, storage, 'Tier 2', 1, True, True),
+            ('b-nfs09-p/tier2', 'Tier2 CEPH storage', True, storage, 'Tier 2', 1, True, True),
+            ('h-nfs11-p/tier2', 'Tier2 CEPH storage', True, storage, 'Tier 2', 1, True, True),
+            ('h-nfs12-p/tier2', 'Tier2 CEPH storage', True, storage, 'Tier 2', 1, True, True),
+            ('h-nfs13-p/tier2', 'Tier2 CEPH storage', True, storage, 'Tier 2', 1, True, True),
+            ('h-nfs14-p/tier2', 'Tier2 CEPH storage', True, storage, 'Tier 2', 1, True, True),
+            ('h-nfs15-p/tier2', 'Tier2 CEPH storage', True, storage, 'Tier 2', 1, True, True),
+            ('h-nfs16-p/tier2', 'Tier2 CEPH storage', True, storage, 'Tier 2', 1, True, True),
+            ('h-nfs17-p/tier2', 'Tier2 CEPH storage', True, storage, 'Tier 2', 1, True, True),
+            ('h-nfs18-p/tier2', 'Tier2 CEPH storage', True, storage, 'Tier 2', 1, True, True),
+            ('h-nfs19-p/tier2', 'Tier2 CEPH storage', True, storage, 'Tier 2', 1, True, True),
             ('boslfs02', 'complimentary lab storage', True, storage, 'Tier 0', 1, False, False),
             ('holylabs', 'complimentary lab storage', True, storage, 'Tier 0', 1, False, False),
         ):

From e87ffa61e0a5d9e922578c03136880e413ae268d Mon Sep 17 00:00:00 2001
From: geistling <34081638+geistling@users.noreply.github.com>
Date: Wed, 4 Sep 2024 16:06:16 -0400
Subject: [PATCH 06/13] fix variable name

---
 coldfront/core/project/views.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/coldfront/core/project/views.py b/coldfront/core/project/views.py
index cb2452fd0..b223dc106 100644
--- a/coldfront/core/project/views.py
+++ b/coldfront/core/project/views.py
@@ -754,7 +754,7 @@ def get(self, request, *args, **kwargs):
 
         # if ldap is activated, prevent selection of users with project corresponding to primary group
         users_no_removal, users_to_remove = project_filter_users_to_remove.send(
-            sender=self.__class__, users=users_to_remove, project=project_obj
+            sender=self.__class__, users_to_remove=users_to_remove, project=project_obj
         )
 
         context = {}

From 8103e2b9ccddc973be86f318a25a8c8eb1e82843 Mon Sep 17 00:00:00 2001
From: geistling <34081638+geistling@users.noreply.github.com>
Date: Wed, 4 Sep 2024 16:56:36 -0400
Subject: [PATCH 07/13] update project_filter_users_to_remove signal processing

---
 coldfront/core/project/views.py | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/coldfront/core/project/views.py b/coldfront/core/project/views.py
index b223dc106..f07518994 100644
--- a/coldfront/core/project/views.py
+++ b/coldfront/core/project/views.py
@@ -750,12 +750,14 @@ def get(self, request, *args, **kwargs):
         pk = self.kwargs.get('pk')
         project_obj = get_object_or_404(Project, pk=pk)
         users_to_remove = self.get_users_to_remove(project_obj)
-        users_no_removal = None
 
         # if ldap is activated, prevent selection of users with project corresponding to primary group
-        users_no_removal, users_to_remove = project_filter_users_to_remove.send(
+        signal_response = project_filter_users_to_remove.send(
             sender=self.__class__, users_to_remove=users_to_remove, project=project_obj
         )
+        user_categories = signal_response[0][1]
+        users_no_removal = user_categories[0]
+        users_to_remove = user_categories[1]
 
         context = {}
 
@@ -776,9 +778,12 @@ def post(self, request, *args, **kwargs):
 
         users_to_remove = self.get_users_to_remove(project_obj)
         # if ldap is activated, prevent selection of users with project corresponding to primary group
-        users_no_removal, users_to_remove = project_filter_users_to_remove.send(
-            sender=self.__class__, users=users_to_remove, project=project_obj
+        signal_response = project_filter_users_to_remove.send(
+            sender=self.__class__, users_to_remove=users_to_remove, project=project_obj
         )
+        user_categories = signal_response[0][1]
+        users_no_removal = user_categories[0]
+        users_to_remove = user_categories[1]
 
         formset = formset_factory(ProjectRemoveUserForm, max_num=len(users_to_remove))
         formset = formset(request.POST, initial=users_to_remove, prefix='userform')

From 873125c6f24c387942f64b89fcd023033e4ff561 Mon Sep 17 00:00:00 2001
From: geistling <34081638+geistling@users.noreply.github.com>
Date: Mon, 9 Sep 2024 14:27:58 -0400
Subject: [PATCH 08/13] work on testing

---
 coldfront/core/project/tests/test_views.py | 44 ++++++++++++++++++++--
 coldfront/core/project/views.py            | 23 +++++++----
 coldfront/core/test_helpers/utils.py       |  2 +-
 coldfront/plugins/system_monitor/utils.py  |  2 +-
 4 files changed, 57 insertions(+), 14 deletions(-)

diff --git a/coldfront/core/project/tests/test_views.py b/coldfront/core/project/tests/test_views.py
index 04e0519a3..32c617d75 100644
--- a/coldfront/core/project/tests/test_views.py
+++ b/coldfront/core/project/tests/test_views.py
@@ -1,6 +1,6 @@
 import logging
 
-from django.test import TestCase, tag
+from django.test import TestCase, tag, override_settings
 from django.urls import reverse
 
 from coldfront.core.test_helpers import utils
@@ -12,7 +12,11 @@
     ProjectStatusChoiceFactory,
     ProjectAttributeTypeFactory,
 )
-from coldfront.core.project.models import Project, ProjectUserStatusChoice
+from coldfront.core.project.models import (
+    Project, ProjectUser,
+    ProjectUserRoleChoice,
+    ProjectUserStatusChoice
+)
 
 logging.disable(logging.CRITICAL)
 
@@ -161,7 +165,6 @@ def test_project_attribute_create_post_required_values(self):
 
     def test_project_attribute_create_value_type_match(self):
         """ProjectAttributeCreate correctly flags value-type mismatch"""
-
         self.client.force_login(self.admin_user,
                     backend='django.contrib.auth.backends.ModelBackend')
         # test that value must be numeric if proj_attr_type is string
@@ -351,7 +354,7 @@ def test_projectnotecreateview_access(self):
         self.project_access_tstbase(self.url)
 
 
-class ProjectAddUsersSearchView(ProjectViewTestBase):
+class ProjectAddUsersSearchViewTest(ProjectViewTestBase):
     """Tests for ProjectAddUsersSearchView"""
     def setUp(self):
         """set up users and project for testing"""
@@ -365,6 +368,39 @@ def test_projectadduserssearchview_access(self):
         utils.test_user_cannot_access(self, self.proj_datamanager, self.url)# data manager cannot access
         utils.test_user_cannot_access(self, self.proj_allocation_user, self.url)# user cannot access
 
+class ProjectAddUsersViewTest(ProjectViewTestBase):
+    """Tests for ProjectAddUsersView"""
+    def setUp(self):
+        """set up users and project for testing"""
+        self.url = reverse('project-add-users', kwargs={'pk': self.project.pk})
+
+    @override_settings(PLUGIN_LDAP=True)
+    def test_projectaddusers_ldapsignalfail_messages(self):
+        """Test the messages displayed when the add user signal fails"""
+        self.client.force_login(self.pi_user)
+
+    def test_add_users_form_validation(self):
+        """Test that the formset and allocation form are validated correctly"""
+        self.client.force_login(self.proj_accessmanager)
+        # Prepare form data for adding a user
+        form_data = {
+            'q': 'search_user',
+            'search_by': 'username',
+            'userform-TOTAL_FORMS': '1',
+            'userform-INITIAL_FORMS': '0',
+            'userform-MIN_NUM_FORMS': '0',
+            'userform-MAX_NUM_FORMS': '1',
+            'userform-0-selected': 'on',
+            'userform-0-role': ProjectUserRoleChoice.objects.get(name='User').pk,
+            'userform-0-username': self.nonproj_allocation_user.username,
+            'allocationform-allocation': [self.proj_allocation.pk]
+        }
+        response = self.client.post(self.url, data=form_data)
+        self.assertEqual(response.url, reverse('project-detail', kwargs={'pk': self.project.pk}))
+        self.assertEqual(response.status_code, 302)
+        # Check that user was added
+        self.assertTrue(ProjectUser.objects.filter(project=self.project, user=self.nonproj_allocation_user).exists())
+
 
 class ProjectUserDetailViewTest(ProjectViewTestBase):
     """Tests for ProjectUserDetailView"""
diff --git a/coldfront/core/project/views.py b/coldfront/core/project/views.py
index f07518994..7c7ed6dba 100644
--- a/coldfront/core/project/views.py
+++ b/coldfront/core/project/views.py
@@ -749,15 +749,20 @@ def get_users_to_remove(self, project_obj):
     def get(self, request, *args, **kwargs):
         pk = self.kwargs.get('pk')
         project_obj = get_object_or_404(Project, pk=pk)
-        users_to_remove = self.get_users_to_remove(project_obj)
+        users_list = self.get_users_to_remove(project_obj)
 
         # if ldap is activated, prevent selection of users with project corresponding to primary group
         signal_response = project_filter_users_to_remove.send(
-            sender=self.__class__, users_to_remove=users_to_remove, project=project_obj
+            sender=self.__class__, users_to_remove=users_list, project=project_obj
         )
-        user_categories = signal_response[0][1]
-        users_no_removal = user_categories[0]
-        users_to_remove = user_categories[1]
+        print('signal_response', signal_response)
+        if signal_response:
+            user_categories = signal_response[0][1]
+            users_no_removal = user_categories[0]
+            users_to_remove = user_categories[1]
+        else:
+            users_no_removal = users_list
+            users_to_remove = []
 
         context = {}
 
@@ -781,9 +786,11 @@ def post(self, request, *args, **kwargs):
         signal_response = project_filter_users_to_remove.send(
             sender=self.__class__, users_to_remove=users_to_remove, project=project_obj
         )
-        user_categories = signal_response[0][1]
-        users_no_removal = user_categories[0]
-        users_to_remove = user_categories[1]
+        if signal_response:
+            user_categories = signal_response[0][1]
+            users_to_remove = user_categories[1]
+        else:
+            users_to_remove = users_to_remove
 
         formset = formset_factory(ProjectRemoveUserForm, max_num=len(users_to_remove))
         formset = formset(request.POST, initial=users_to_remove, prefix='userform')
diff --git a/coldfront/core/test_helpers/utils.py b/coldfront/core/test_helpers/utils.py
index fe1c858c0..8ce58c733 100644
--- a/coldfront/core/test_helpers/utils.py
+++ b/coldfront/core/test_helpers/utils.py
@@ -1,5 +1,5 @@
 """utility functions for unit and integration testing"""
-from bs4 import BeautifulSoup
+from beautifulsoup4 import BeautifulSoup
 
 def page_contains_for_user(test_case, user, url, text):
     """Check that page contains text for user"""
diff --git a/coldfront/plugins/system_monitor/utils.py b/coldfront/plugins/system_monitor/utils.py
index f07003b98..c4d7a50c9 100644
--- a/coldfront/plugins/system_monitor/utils.py
+++ b/coldfront/plugins/system_monitor/utils.py
@@ -1,7 +1,7 @@
 import re
 
 import requests
-from bs4 import BeautifulSoup
+from beautifulsoup4 import BeautifulSoup
 
 from coldfront.core.utils.common import import_from_settings
 

From f6481820ca32c81ce742031f5d71469e4291ced3 Mon Sep 17 00:00:00 2001
From: geistling <34081638+geistling@users.noreply.github.com>
Date: Fri, 13 Sep 2024 20:35:44 -0700
Subject: [PATCH 09/13] fix addusers test

---
 coldfront/core/project/tests/test_views.py | 5 ++---
 coldfront/core/project/views.py            | 7 ++-----
 coldfront/core/test_helpers/utils.py       | 2 +-
 3 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/coldfront/core/project/tests/test_views.py b/coldfront/core/project/tests/test_views.py
index 32c617d75..bd094bbed 100644
--- a/coldfront/core/project/tests/test_views.py
+++ b/coldfront/core/project/tests/test_views.py
@@ -384,15 +384,14 @@ def test_add_users_form_validation(self):
         self.client.force_login(self.proj_accessmanager)
         # Prepare form data for adding a user
         form_data = {
-            'q': 'search_user',
-            'search_by': 'username',
+            'q': self.nonproj_allocation_user.username,
+            'search_by': 'username_only',
             'userform-TOTAL_FORMS': '1',
             'userform-INITIAL_FORMS': '0',
             'userform-MIN_NUM_FORMS': '0',
             'userform-MAX_NUM_FORMS': '1',
             'userform-0-selected': 'on',
             'userform-0-role': ProjectUserRoleChoice.objects.get(name='User').pk,
-            'userform-0-username': self.nonproj_allocation_user.username,
             'allocationform-allocation': [self.proj_allocation.pk]
         }
         response = self.client.post(self.url, data=form_data)
diff --git a/coldfront/core/project/views.py b/coldfront/core/project/views.py
index 7c7ed6dba..9d54d2ca0 100644
--- a/coldfront/core/project/views.py
+++ b/coldfront/core/project/views.py
@@ -612,7 +612,6 @@ def post(self, request, *args, **kwargs):
         allocation_form = ProjectAddUsersToAllocationForm(
             request.user, project_obj.pk, request.POST, prefix='allocationform'
         )
-
         added_users_count = 0
 
         if formset.is_valid() and allocation_form.is_valid():
@@ -672,7 +671,6 @@ def post(self, request, *args, **kwargs):
                             'status': projuserstatus_active,
                         }
                     )
-
                     added_users_count += 1
                     for allocation in Allocation.objects.filter(
                         pk__in=allocation_form_data
@@ -755,14 +753,13 @@ def get(self, request, *args, **kwargs):
         signal_response = project_filter_users_to_remove.send(
             sender=self.__class__, users_to_remove=users_list, project=project_obj
         )
-        print('signal_response', signal_response)
         if signal_response:
             user_categories = signal_response[0][1]
             users_no_removal = user_categories[0]
             users_to_remove = user_categories[1]
         else:
-            users_no_removal = users_list
-            users_to_remove = []
+            users_no_removal = []
+            users_to_remove = users_list
 
         context = {}
 
diff --git a/coldfront/core/test_helpers/utils.py b/coldfront/core/test_helpers/utils.py
index 8ce58c733..fe1c858c0 100644
--- a/coldfront/core/test_helpers/utils.py
+++ b/coldfront/core/test_helpers/utils.py
@@ -1,5 +1,5 @@
 """utility functions for unit and integration testing"""
-from beautifulsoup4 import BeautifulSoup
+from bs4 import BeautifulSoup
 
 def page_contains_for_user(test_case, user, url, text):
     """Check that page contains text for user"""

From 0e6c6c00b0761845364956d38137289777b4312d Mon Sep 17 00:00:00 2001
From: geistling <34081638+geistling@users.noreply.github.com>
Date: Tue, 17 Sep 2024 09:10:02 -0700
Subject: [PATCH 10/13] add removeuser signal test

---
 coldfront/core/project/tests/test_views.py | 45 ++++++++++++++++------
 1 file changed, 33 insertions(+), 12 deletions(-)

diff --git a/coldfront/core/project/tests/test_views.py b/coldfront/core/project/tests/test_views.py
index bd094bbed..fa65911c1 100644
--- a/coldfront/core/project/tests/test_views.py
+++ b/coldfront/core/project/tests/test_views.py
@@ -2,6 +2,7 @@
 
 from django.test import TestCase, tag, override_settings
 from django.urls import reverse
+from unittest.mock import patch
 
 from coldfront.core.test_helpers import utils
 from coldfront.core.test_helpers.factories import (
@@ -309,6 +310,16 @@ def test_pi_user_cannot_be_removed(self):
         users_to_remove = context['formset'].initial
         self.assertNotIn(self.pi_user.username, [u['username'] for u in users_to_remove])
 
+    @patch('coldfront.core.project.signals.project_preremove_projectuser.send')
+    def test_projectremove_users_signal_fail(self, mock_signal):
+        """Test that the add users form fails when the signal sent to LDAP fails"""
+        self.client.force_login(self.proj_accessmanager)
+        mock_signal.side_effect = Exception("LDAP error occurred")
+        # Prepare form data for adding a user
+        response = self.client.post(self.url, data=self.form_data, follow=True)
+        self.assertContains(response, 'LDAP error occurred')
+        self.assertContains(response, 'Could not remove user')
+
 
 class ProjectUpdateViewTest(ProjectViewTestBase):
     """Tests for ProjectUpdateView"""
@@ -373,17 +384,7 @@ class ProjectAddUsersViewTest(ProjectViewTestBase):
     def setUp(self):
         """set up users and project for testing"""
         self.url = reverse('project-add-users', kwargs={'pk': self.project.pk})
-
-    @override_settings(PLUGIN_LDAP=True)
-    def test_projectaddusers_ldapsignalfail_messages(self):
-        """Test the messages displayed when the add user signal fails"""
-        self.client.force_login(self.pi_user)
-
-    def test_add_users_form_validation(self):
-        """Test that the formset and allocation form are validated correctly"""
-        self.client.force_login(self.proj_accessmanager)
-        # Prepare form data for adding a user
-        form_data = {
+        self.form_data = {
             'q': self.nonproj_allocation_user.username,
             'search_by': 'username_only',
             'userform-TOTAL_FORMS': '1',
@@ -394,12 +395,32 @@ def test_add_users_form_validation(self):
             'userform-0-role': ProjectUserRoleChoice.objects.get(name='User').pk,
             'allocationform-allocation': [self.proj_allocation.pk]
         }
-        response = self.client.post(self.url, data=form_data)
+
+    @override_settings(PLUGIN_LDAP=True)
+    def test_projectaddusers_ldapsignalfail_messages(self):
+        """Test the messages displayed when the add user signal fails"""
+        self.client.force_login(self.pi_user)
+
+    def test_projectaddusers_form_validation(self):
+        """Test that the formset and allocation form are validated correctly"""
+        self.client.force_login(self.proj_accessmanager)
+        # Prepare form data for adding a user
+        response = self.client.post(self.url, data=self.form_data)
         self.assertEqual(response.url, reverse('project-detail', kwargs={'pk': self.project.pk}))
         self.assertEqual(response.status_code, 302)
         # Check that user was added
         self.assertTrue(ProjectUser.objects.filter(project=self.project, user=self.nonproj_allocation_user).exists())
 
+    @patch('coldfront.core.project.signals.project_make_projectuser.send')
+    def test_projectaddusers_signal_fail(self, mock_signal):
+        """Test that the add users form fails when the signal sent to LDAP fails"""
+        self.client.force_login(self.proj_accessmanager)
+        mock_signal.side_effect = Exception("LDAP error occurred")
+        # Prepare form data for adding a user
+        response = self.client.post(self.url, data=self.form_data, follow=True)
+        self.assertContains(response, 'LDAP error occurred')
+        self.assertContains(response, 'Added 0 users')
+
 
 class ProjectUserDetailViewTest(ProjectViewTestBase):
     """Tests for ProjectUserDetailView"""

From 835037623e37341c5b719bf4acbf0da10ec5f88c Mon Sep 17 00:00:00 2001
From: geistling <34081638+geistling@users.noreply.github.com>
Date: Tue, 17 Sep 2024 15:59:06 -0700
Subject: [PATCH 11/13] remove test

---
 coldfront/core/project/tests/test_views.py | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/coldfront/core/project/tests/test_views.py b/coldfront/core/project/tests/test_views.py
index fa65911c1..9f3b29325 100644
--- a/coldfront/core/project/tests/test_views.py
+++ b/coldfront/core/project/tests/test_views.py
@@ -310,16 +310,6 @@ def test_pi_user_cannot_be_removed(self):
         users_to_remove = context['formset'].initial
         self.assertNotIn(self.pi_user.username, [u['username'] for u in users_to_remove])
 
-    @patch('coldfront.core.project.signals.project_preremove_projectuser.send')
-    def test_projectremove_users_signal_fail(self, mock_signal):
-        """Test that the add users form fails when the signal sent to LDAP fails"""
-        self.client.force_login(self.proj_accessmanager)
-        mock_signal.side_effect = Exception("LDAP error occurred")
-        # Prepare form data for adding a user
-        response = self.client.post(self.url, data=self.form_data, follow=True)
-        self.assertContains(response, 'LDAP error occurred')
-        self.assertContains(response, 'Could not remove user')
-
 
 class ProjectUpdateViewTest(ProjectViewTestBase):
     """Tests for ProjectUpdateView"""

From 6bbbe6b6cc915a59d89ea4bbf9826460b9037918 Mon Sep 17 00:00:00 2001
From: geistling <34081638+geistling@users.noreply.github.com>
Date: Tue, 17 Sep 2024 17:41:51 -0700
Subject: [PATCH 12/13] update test

---
 coldfront/core/project/tests/test_views.py | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/coldfront/core/project/tests/test_views.py b/coldfront/core/project/tests/test_views.py
index 9f3b29325..b8873dacd 100644
--- a/coldfront/core/project/tests/test_views.py
+++ b/coldfront/core/project/tests/test_views.py
@@ -391,9 +391,11 @@ def test_projectaddusers_ldapsignalfail_messages(self):
         """Test the messages displayed when the add user signal fails"""
         self.client.force_login(self.pi_user)
 
-    def test_projectaddusers_form_validation(self):
+    @patch('coldfront.core.project.signals.project_make_projectuser.send')
+    def test_projectaddusers_form_validation(self, mock_signal):
         """Test that the formset and allocation form are validated correctly"""
         self.client.force_login(self.proj_accessmanager)
+        mock_signal.return_value = None
         # Prepare form data for adding a user
         response = self.client.post(self.url, data=self.form_data)
         self.assertEqual(response.url, reverse('project-detail', kwargs={'pk': self.project.pk}))

From d6838990d2e9cca9b91961c57dc39b762b37df1f Mon Sep 17 00:00:00 2001
From: claire-peters <clairepeters@g.harvard.edu>
Date: Wed, 18 Sep 2024 01:31:20 -0400
Subject: [PATCH 13/13] update logging

---
 coldfront/core/project/views.py | 2 +-
 coldfront/plugins/ldap/utils.py | 6 ++++--
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/coldfront/core/project/views.py b/coldfront/core/project/views.py
index 9d54d2ca0..0b742687c 100644
--- a/coldfront/core/project/views.py
+++ b/coldfront/core/project/views.py
@@ -828,7 +828,7 @@ def post(self, request, *args, **kwargs):
                             request,
                             f"could not remove user {user_obj}: {e}"
                         )
-                        logger.error(
+                        logger.exception(
                             "P802: Coldfront user %s could NOT remove AD User for %s from AD Group for %s: %s",
                             self.request.user,
                             user_obj.username,
diff --git a/coldfront/plugins/ldap/utils.py b/coldfront/plugins/ldap/utils.py
index 84e3afd46..2afa305e2 100644
--- a/coldfront/plugins/ldap/utils.py
+++ b/coldfront/plugins/ldap/utils.py
@@ -198,8 +198,9 @@ def add_member_to_group(self, member, group):
         except Exception as e:
             logger.exception("Error encountered while adding user to group: %s", e)
             raise LDAPUserAdditionError("Error adding user to group.")
-        if not self.member_in_group(member_dn, group_dn):
+        if not self.member_in_group(member_dn, group_dn) or not result:
             raise LDAPUserAdditionError("Member not successfully added to group.")
+        logger.info('user %s added to AD group %s', member_dn, group_dn)
         return result
 
     def remove_member_from_group(self, user_name, group_name):
@@ -219,8 +220,9 @@ def remove_member_from_group(self, user_name, group_name):
         except Exception as e:
             logger.exception("Error encountered while removing user from group: %s", e)
             raise LDAPUserRemovalError("Error removing user from group.")
-        if self.member_in_group(user_dn, group_dn):
+        if self.member_in_group(user_dn, group_dn) or not result:
             raise LDAPUserRemovalError("Member not successfully removed from group.")
+        logger.info('user %s removed from AD group %s', user_dn, group_dn)
         return result
 
     def users_in_primary_group(self, usernames, groupname):