Merge pull request #483 from UW-GAC/feature/do-not-retain-group-membe…

…rship-records-for-inactive-accounts

Do not retain group membership records for inactive accounts

.github/workflows/ci.yml

@@ -112,9 +112,17 @@ jobs:
           ls -la .coverage*
 
       - name: Combine coverage data
-        run: hatch run cov-combine:combine
+        run: |
+          hatch run cov:combine
+          ls -la .coverage*
+
+      - name: Create coverage xml file
+        run: |
+          hatch run cov:xml
+          ls -la .
 
       - name: Upload coverage to Codecov
         uses: codecov/codecov-action@v4
         with:
+          fail_ci_if_error: true # optional (default = false)
           token: ${{ secrets.CODECOV_TOKEN }}

CHANGELOG.md

@@ -1,5 +1,10 @@
 # Change log
 
+## 0.23.0 (devel)
+
+* Do not track previous groups for inactive accounts.
+* Add an audit error if any group membership records exist for inactive accounts.
+
 ## 0.22.1 (2024-05-22)
 
 * Maintenance: specify different numpy version requirement for python 3.12.

anvil_consortium_manager/__init__.py

@@ -1 +1 @@
-__version__ = "0.22.1"
+__version__ = "0.23.0.dev0"

anvil_consortium_manager/audit/audit.py

@@ -165,7 +165,7 @@ class AccountAudit(AnVILAudit):
 
     def run_audit(self):
         # Only checks active accounts.
-        for account in models.Account.objects.filter(status=models.Account.ACTIVE_STATUS).all():
+        for account in models.Account.objects.active():
             model_instance_result = ModelInstanceResult(account)
             if not account.anvil_exists():
                 model_instance_result.add_error(self.ERROR_NOT_IN_ANVIL)
@@ -246,11 +246,8 @@ class ManagedGroupMembershipAudit(AnVILAudit):
     ERROR_ACCOUNT_MEMBER_NOT_IN_ANVIL = "Account not a member in AnVIL"
     """Error when an Account is a member of a ManagedGroup on the app, but not in AnVIL."""
 
-    ERROR_DEACTIVATED_ACCOUNT_IS_ADMIN_IN_ANVIL = "Account is deactivated but is an admin in AnVIL."
-    """Error when a deactivated Account is an admin of a ManagedGroup in AnVIL."""
-
-    ERROR_DEACTIVATED_ACCOUNT_IS_MEMBER_IN_ANVIL = "Account is deactivated but is a member in AnVIL."
-    """Error when a deactivated Account is a member of a ManagedGroup in AnVIL."""
+    ERROR_DEACTIVATED_ACCOUNT = "Account is deactivated but still has membership records in the app."
+    """Error when a deactivated Account still has membership records in the app."""
 
     ERROR_GROUP_ADMIN_NOT_IN_ANVIL = "Group not an admin in AnVIL"
     """Error when a ManagedGroup is an admin of another ManagedGroup on the app, but not in AnVIL."""
@@ -293,24 +290,22 @@ def run_audit(self):
         for membership in self.managed_group.groupaccountmembership_set.all():
             # Create an audit result instance for this model.
             model_instance_result = ModelInstanceResult(membership)
+            # Check for deactivated account memberships.
+            if membership.account.status == models.Account.INACTIVE_STATUS:
+                model_instance_result.add_error(self.ERROR_DEACTIVATED_ACCOUNT)
+            # Check membership status on AnVIL.
             if membership.role == models.GroupAccountMembership.ADMIN:
                 try:
                     admins_in_anvil.remove(membership.account.email.lower())
-                    if membership.account.status == models.Account.INACTIVE_STATUS:
-                        model_instance_result.add_error(self.ERROR_DEACTIVATED_ACCOUNT_IS_ADMIN_IN_ANVIL)
                 except ValueError:
-                    # For active accounts, this is an error - the email is not in the list of members.
-                    if membership.account.status == models.Account.ACTIVE_STATUS:
-                        model_instance_result.add_error(self.ERROR_ACCOUNT_ADMIN_NOT_IN_ANVIL)
+                    # This is an error - the email is not in the list of admins.
+                    model_instance_result.add_error(self.ERROR_ACCOUNT_ADMIN_NOT_IN_ANVIL)
             elif membership.role == models.GroupAccountMembership.MEMBER:
                 try:
                     members_in_anvil.remove(membership.account.email.lower())
-                    if membership.account.status == models.Account.INACTIVE_STATUS:
-                        model_instance_result.add_error(self.ERROR_DEACTIVATED_ACCOUNT_IS_MEMBER_IN_ANVIL)
                 except ValueError:
-                    # For active accounts, this is an error - the email is not in the list of members.
-                    if membership.account.status == models.Account.ACTIVE_STATUS:
-                        model_instance_result.add_error(self.ERROR_ACCOUNT_MEMBER_NOT_IN_ANVIL)
+                    # This is an error - the email is not in the list of members.
+                    model_instance_result.add_error(self.ERROR_ACCOUNT_MEMBER_NOT_IN_ANVIL)
             self.add_result(model_instance_result)
 
         # Check group-group membership.

anvil_consortium_manager/migrations/0018_alter_historicalworkspace_is_requester_pays_and_more.py

@@ -0,0 +1,23 @@
+# Generated by Django 5.0 on 2024-05-30 18:01
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('anvil_consortium_manager', '0017_workspace_is_requester_pays'),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name='historicalworkspace',
+            name='is_requester_pays',
+            field=models.BooleanField(default=False, help_text='Indicator of whether the workspace is set to requester pays.', verbose_name='Requester pays'),
+        ),
+        migrations.AlterField(
+            model_name='workspace',
+            name='is_requester_pays',
+            field=models.BooleanField(default=False, help_text='Indicator of whether the workspace is set to requester pays.', verbose_name='Requester pays'),
+        ),
+    ]

anvil_consortium_manager/models.py

@@ -277,12 +277,9 @@ def deactivate(self):
         self.save()
 
     def reactivate(self):
-        """Set status to reactivated and add to any AnVIL groups."""
+        """Set status to reactivated."""
         self.status = self.ACTIVE_STATUS
         self.save()
-        group_memberships = self.groupaccountmembership_set.all()
-        for membership in group_memberships:
-            membership.anvil_create()
 
     def anvil_exists(self):
         """Check if this account exists on AnVIL.
@@ -302,10 +299,11 @@ def anvil_exists(self):
         return True
 
     def anvil_remove_from_groups(self):
-        """Remove this account from all groups on AnVIL."""
+        """Remove this account from all groups on AnVIL and delete membership records from the app."""
         group_memberships = self.groupaccountmembership_set.all()
         for membership in group_memberships:
             membership.anvil_delete()
+            membership.delete()
 
     def get_accessible_workspaces(self):
         """Get a list of workspaces an Account has access to.

anvil_consortium_manager/templates/anvil_consortium_manager/account_confirm_deactivate.html

@@ -17,7 +17,7 @@ <h2>Deactivate Account</h2>
       <h2>{{ object }}</h2>
 
       <p>
-        Deactivating an account will remove it from all groups on AnVIL, but will still store the record of its previous groups in this app.
+        Deactivating an account will remove it from all groups on AnVIL and in the app.
       </p>
 
       <div class="my-3">

anvil_consortium_manager/templates/anvil_consortium_manager/account_detail.html

@@ -22,7 +22,7 @@
   {% endif %}
   {% if is_inactive %}
     <div class="my-3 alert alert-dark" role="alert">
-    This account is inactive. It has been removed from all groups on AnVIL, but a record of previous group membership is still stored in this app.
+    This account is inactive
     </div>
   {% endif %}
 {% endblock pills %}

anvil_consortium_manager/templates/anvil_consortium_manager/managedgroup_detail.html

@@ -136,35 +136,16 @@ <h2 class="accordion-header" id="headingMembersOne">
         <h2 class="accordion-header" id="headingMembersTwo">
           <button class="accordion-button collapsed" type="button" data-bs-toggle="collapse" data-bs-target="#collapseMembersTwo" aria-expanded="false" aria-controls="collapseMembersTwo">
             <span class="fa-solid fa-user-group mx-2"></span>
-            View active accounts in this group
-            <span class="badge mx-2 bg-secondary pill"> {{ active_account_table.rows|length }}</span>
+            View accounts in this group
+            <span class="badge mx-2 bg-secondary pill"> {{ account_table.rows|length }}</span>
           </button>
         </h2>
         <div id="collapseMembersTwo" class="accordion-collapse collapse" aria-labelledby="headingMembersTwo">
           <div class="accordion-body">
             <p>
-              This table shows active Accounts that in this group, either as an admin or a member.
+              This table shows Accounts that in this group, either as an admin or a member.
             </p>
-            {% render_table active_account_table %}
-          </div>
-        </div>
-      </div>
-
-      <div class="accordion-item">
-        <h2 class="accordion-header" id="headingMembersThree">
-          <button class="accordion-button collapsed" type="button" data-bs-toggle="collapse" data-bs-target="#collapseMembersThree" aria-expanded="false" aria-controls="collapseMembersTwo">
-            <span class="fa-solid fa-users-slash mx-2"></span>
-            View inactive accounts in this group
-            <span class="badge mx-2 bg-secondary pill"> {{ inactive_account_table.rows|length }}</span>
-          </button>
-        </h2>
-        <div id="collapseMembersThree" class="accordion-collapse collapse" aria-labelledby="headingMembersThree">
-          <div class="accordion-body">
-            <p>
-              This table shows a record of the inactive Accounts that were in this group, either as an admin or a member.
-              Note that these Accounts are not part of the group on AnVIL.
-            </p>
-            {% render_table inactive_account_table %}
+            {% render_table account_table %}
           </div>
         </div>
       </div>

anvil_consortium_manager/tests/test_audit.py

@@ -2520,10 +2520,12 @@ def test_group_is_both_admin_and_member(self):
         self.assertTrue(record_result.ok())
 
     def test_deactivated_account_not_member_in_anvil(self):
-        """Audit is ok if a deactivated account is not in the group on AnVIL."""
+        """Audit fails if a deactivated account is not in the group on AnVIL."""
         group = factories.ManagedGroupFactory.create()
         # Create an inactive account that is a member of this group.
-        factories.GroupAccountMembershipFactory.create(group=group, account__status=models.Account.INACTIVE_STATUS)
+        membership = factories.GroupAccountMembershipFactory.create(
+            group=group, account__status=models.Account.INACTIVE_STATUS
+        )
         # The Account is not a member in AnVIL
         api_url_members = self.get_api_url_members(group.name)
         self.anvil_response_mock.add(
@@ -2541,10 +2543,16 @@ def test_deactivated_account_not_member_in_anvil(self):
         )
         audit_results = audit.ManagedGroupMembershipAudit(group)
         audit_results.run_audit()
-        self.assertTrue(audit_results.ok())
-        self.assertEqual(len(audit_results.get_verified_results()), 1)
-        self.assertEqual(len(audit_results.get_error_results()), 0)
+        self.assertFalse(audit_results.ok())
+        self.assertEqual(len(audit_results.get_verified_results()), 0)
+        self.assertEqual(len(audit_results.get_error_results()), 1)
         self.assertEqual(len(audit_results.get_not_in_app_results()), 0)
+        model_result = audit_results.get_result_for_model_instance(membership)
+        self.assertFalse(model_result.ok())
+        self.assertEqual(
+            model_result.errors,
+            set([audit_results.ERROR_ACCOUNT_MEMBER_NOT_IN_ANVIL, audit_results.ERROR_DEACTIVATED_ACCOUNT]),
+        )
 
     def test_deactivated_account_member_in_anvil(self):
         """Audit is not ok if a deactivated account is in the group on AnVIL."""
@@ -2577,14 +2585,14 @@ def test_deactivated_account_member_in_anvil(self):
         record_result = audit_results.get_result_for_model_instance(membership)
         self.assertEqual(
             record_result.errors,
-            set([audit_results.ERROR_DEACTIVATED_ACCOUNT_IS_MEMBER_IN_ANVIL]),
+            set([audit_results.ERROR_DEACTIVATED_ACCOUNT]),
         )
 
     def test_deactivated_account_not_admin_in_anvil(self):
-        """Audit is ok if a deactivated account is not in the group on AnVIL."""
+        """Audit is not ok if a deactivated account is not in the group on AnVIL."""
         group = factories.ManagedGroupFactory.create()
         # Create an inactive account that is a member of this group.
-        factories.GroupAccountMembershipFactory.create(
+        membership = factories.GroupAccountMembershipFactory.create(
             group=group,
             account__status=models.Account.INACTIVE_STATUS,
             role=models.GroupAccountMembership.ADMIN,
@@ -2606,10 +2614,16 @@ def test_deactivated_account_not_admin_in_anvil(self):
         )
         audit_results = audit.ManagedGroupMembershipAudit(group)
         audit_results.run_audit()
-        self.assertTrue(audit_results.ok())
-        self.assertEqual(len(audit_results.get_verified_results()), 1)
-        self.assertEqual(len(audit_results.get_error_results()), 0)
+        self.assertFalse(audit_results.ok())
+        self.assertEqual(len(audit_results.get_verified_results()), 0)
+        self.assertEqual(len(audit_results.get_error_results()), 1)
         self.assertEqual(len(audit_results.get_not_in_app_results()), 0)
+        model_result = audit_results.get_result_for_model_instance(membership)
+        self.assertFalse(model_result.ok())
+        self.assertEqual(
+            model_result.errors,
+            set([audit_results.ERROR_ACCOUNT_ADMIN_NOT_IN_ANVIL, audit_results.ERROR_DEACTIVATED_ACCOUNT]),
+        )
 
     def test_deactivated_account_admin_in_anvil(self):
         """Audit is not ok if a deactivated account is in the group on AnVIL."""
@@ -2644,7 +2658,7 @@ def test_deactivated_account_admin_in_anvil(self):
         record_result = audit_results.get_result_for_model_instance(membership)
         self.assertEqual(
             record_result.errors,
-            set([audit_results.ERROR_DEACTIVATED_ACCOUNT_IS_ADMIN_IN_ANVIL]),
+            set([audit_results.ERROR_DEACTIVATED_ACCOUNT]),
         )
 
 

anvil_consortium_manager/tests/test_models_anvil_api_unit.py

@@ -200,7 +200,7 @@ def test_anvil_remove_from_groups_in_one_group(self):
         self.anvil_response_mock.add(responses.DELETE, remove_from_group_url, status=204)
         self.object.anvil_remove_from_groups()
         # The membership was removed.
-        self.assertEqual(models.GroupAccountMembership.objects.count(), 1)
+        self.assertEqual(models.GroupAccountMembership.objects.count(), 0)
 
     def test_anvil_remove_from_groups_in_two_groups(self):
         """anvil_remove_from_groups succeeds if the account is in two groups."""
@@ -213,7 +213,7 @@ def test_anvil_remove_from_groups_in_two_groups(self):
         self.anvil_response_mock.add(responses.DELETE, remove_from_group_url_2, status=204)
         self.object.anvil_remove_from_groups()
         # The membership was removed.
-        self.assertEqual(models.GroupAccountMembership.objects.count(), 2)
+        self.assertEqual(models.GroupAccountMembership.objects.count(), 0)
 
     def test_anvil_remove_from_groups_api_failure(self):
         """anvil_remove_from_groups does not remove group memberships if any API call failed."""
@@ -231,8 +231,8 @@ def test_anvil_remove_from_groups_api_failure(self):
         )
         with self.assertRaises(anvil_api.AnVILAPIError):
             self.object.anvil_remove_from_groups()
-        # No memberships were removed.
-        self.assertEqual(models.GroupAccountMembership.objects.count(), 2)
+        # Only the successful membership was removed.
+        self.assertEqual(models.GroupAccountMembership.objects.count(), 1)
 
     def test_deactivate_no_groups(self):
         """deactivate properly sets the status field if the account is not in any groups."""
@@ -250,8 +250,8 @@ def test_deactivate_one_group(self):
         self.object.deactivate()
         self.object.refresh_from_db()
         self.assertEqual(self.object.status, self.object.INACTIVE_STATUS)
-        # The membership was not removed from the app.
-        self.assertEqual(models.GroupAccountMembership.objects.count(), 1)
+        # The membership was removed from the app.
+        self.assertEqual(models.GroupAccountMembership.objects.count(), 0)
 
     def test_deactivate_two_groups(self):
         """deactivate succeeds if the account is in two groups."""
@@ -265,48 +265,38 @@ def test_deactivate_two_groups(self):
         self.object.deactivate()
         self.object.refresh_from_db()
         self.assertEqual(self.object.status, self.object.INACTIVE_STATUS)
-        # The memberships were not removed.
-        self.assertEqual(models.GroupAccountMembership.objects.count(), 2)
-
-    def test_reactivate_no_groups(self):
-        """reactivate properly sets the status field if the account is not in any groups."""
-        # Make sure it doesn't fail and that there are no API calls.
-        self.object.status = self.object.INACTIVE_STATUS
-        self.object.save()
-        self.object.reactivate()
-        self.object.refresh_from_db()
-        self.assertEqual(self.object.status, self.object.ACTIVE_STATUS)
+        # The memberships were removed.
+        self.assertEqual(models.GroupAccountMembership.objects.count(), 0)
 
-    def test_reactivate_one_group(self):
-        """reactivate succeeds if the account is in one group."""
-        self.object.status = self.object.INACTIVE_STATUS
-        self.object.save()
-        membership = factories.GroupAccountMembershipFactory.create(account=self.object)
-        group = membership.group
-        add_to_group_url = self.get_api_add_to_group_url(group.name)
-        self.anvil_response_mock.add(responses.PUT, add_to_group_url, status=204)
-        self.object.reactivate()
+    def test_deactivate_api_failure(self):
+        """deactivate does not remove from any groups or set the status if an API call failed."""
+        memberships = factories.GroupAccountMembershipFactory.create_batch(2, account=self.object)
+        group_1 = memberships[0].group
+        group_2 = memberships[1].group
+        remove_from_group_url_1 = self.get_api_remove_from_group_url(group_1.name)
+        remove_from_group_url_2 = self.get_api_remove_from_group_url(group_2.name)
+        self.anvil_response_mock.add(responses.DELETE, remove_from_group_url_1, status=204)
+        self.anvil_response_mock.add(
+            responses.DELETE,
+            remove_from_group_url_2,
+            status=409,
+            json={"message": "api error"},
+        )
+        with self.assertRaises(anvil_api.AnVILAPIError):
+            self.object.deactivate()
         self.object.refresh_from_db()
         self.assertEqual(self.object.status, self.object.ACTIVE_STATUS)
-        # The membership was not removed from the app.
+        # Only the one membership was removed.
         self.assertEqual(models.GroupAccountMembership.objects.count(), 1)
 
-    def test_reactivate_two_groups(self):
-        """reactivate succeeds if the account is in two groups."""
+    def test_reactivate_no_groups(self):
+        """reactivate properly sets the status field if the account is not in any groups."""
+        # Make sure it doesn't fail and that there are no API calls.
         self.object.status = self.object.INACTIVE_STATUS
         self.object.save()
-        memberships = factories.GroupAccountMembershipFactory.create_batch(2, account=self.object)
-        group_1 = memberships[0].group
-        group_2 = memberships[1].group
-        add_to_group_url_1 = self.get_api_add_to_group_url(group_1.name)
-        add_to_group_url_2 = self.get_api_add_to_group_url(group_2.name)
-        self.anvil_response_mock.add(responses.PUT, add_to_group_url_1, status=204)
-        self.anvil_response_mock.add(responses.PUT, add_to_group_url_2, status=204)
         self.object.reactivate()
         self.object.refresh_from_db()
         self.assertEqual(self.object.status, self.object.ACTIVE_STATUS)
-        # The memberships were not removed.
-        self.assertEqual(models.GroupAccountMembership.objects.count(), 2)
 
 
 class ManagedGroupAnVILAPIMockTest(AnVILAPIMockTestMixin, TestCase):