diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1724c3a9..d872fed5 100644 --- a/.github/workflows/ci.yml +++ b/.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 }} diff --git a/CHANGELOG.md b/CHANGELOG.md index b48f437e..cc0d79f7 100644 --- a/CHANGELOG.md +++ b/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. diff --git a/anvil_consortium_manager/__init__.py b/anvil_consortium_manager/__init__.py index d74a4749..03b05414 100644 --- a/anvil_consortium_manager/__init__.py +++ b/anvil_consortium_manager/__init__.py @@ -1 +1 @@ -__version__ = "0.22.1" +__version__ = "0.23.0.dev0" diff --git a/anvil_consortium_manager/audit/audit.py b/anvil_consortium_manager/audit/audit.py index 51711c5f..6f1db29e 100644 --- a/anvil_consortium_manager/audit/audit.py +++ b/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. diff --git a/anvil_consortium_manager/migrations/0018_alter_historicalworkspace_is_requester_pays_and_more.py b/anvil_consortium_manager/migrations/0018_alter_historicalworkspace_is_requester_pays_and_more.py new file mode 100644 index 00000000..f05f1aa5 --- /dev/null +++ b/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'), + ), + ] diff --git a/anvil_consortium_manager/models.py b/anvil_consortium_manager/models.py index 394d4b1d..24c12022 100644 --- a/anvil_consortium_manager/models.py +++ b/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. diff --git a/anvil_consortium_manager/templates/anvil_consortium_manager/account_confirm_deactivate.html b/anvil_consortium_manager/templates/anvil_consortium_manager/account_confirm_deactivate.html index 5c272624..0ce402da 100644 --- a/anvil_consortium_manager/templates/anvil_consortium_manager/account_confirm_deactivate.html +++ b/anvil_consortium_manager/templates/anvil_consortium_manager/account_confirm_deactivate.html @@ -17,7 +17,7 @@

Deactivate Account

{{ object }}

- 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.

diff --git a/anvil_consortium_manager/templates/anvil_consortium_manager/account_detail.html b/anvil_consortium_manager/templates/anvil_consortium_manager/account_detail.html index 17ad8ea2..4ad62e74 100644 --- a/anvil_consortium_manager/templates/anvil_consortium_manager/account_detail.html +++ b/anvil_consortium_manager/templates/anvil_consortium_manager/account_detail.html @@ -22,7 +22,7 @@ {% endif %} {% if is_inactive %} {% endif %} {% endblock pills %} diff --git a/anvil_consortium_manager/templates/anvil_consortium_manager/managedgroup_detail.html b/anvil_consortium_manager/templates/anvil_consortium_manager/managedgroup_detail.html index eb83ea74..7f96a27e 100644 --- a/anvil_consortium_manager/templates/anvil_consortium_manager/managedgroup_detail.html +++ b/anvil_consortium_manager/templates/anvil_consortium_manager/managedgroup_detail.html @@ -136,35 +136,16 @@

- 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.

- {% render_table active_account_table %} -
-
-
- -
-

- -

-
-
-

- 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. -

- {% render_table inactive_account_table %} + {% render_table account_table %}
diff --git a/anvil_consortium_manager/tests/test_audit.py b/anvil_consortium_manager/tests/test_audit.py index d0453fbc..d00a0fc4 100644 --- a/anvil_consortium_manager/tests/test_audit.py +++ b/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]), ) diff --git a/anvil_consortium_manager/tests/test_models_anvil_api_unit.py b/anvil_consortium_manager/tests/test_models_anvil_api_unit.py index 5ddf70aa..2b259130 100644 --- a/anvil_consortium_manager/tests/test_models_anvil_api_unit.py +++ b/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): diff --git a/anvil_consortium_manager/tests/test_views.py b/anvil_consortium_manager/tests/test_views.py index 06a8d1e9..9cda1f09 100644 --- a/anvil_consortium_manager/tests/test_views.py +++ b/anvil_consortium_manager/tests/test_views.py @@ -3454,9 +3454,10 @@ def test_api_error_when_removing_account_from_groups(self): self.assertEqual(models.Account.objects.count(), 1) models.Account.objects.get(pk=object.pk) # Does not remove the user from any groups. - self.assertEqual(models.GroupAccountMembership.objects.count(), 2) - models.GroupAccountMembership.objects.get(pk=memberships[0].pk) - models.GroupAccountMembership.objects.get(pk=memberships[1].pk) + # Removes the user from only the groups where the API call succeeded. + self.assertEqual(models.GroupAccountMembership.objects.count(), 1) + self.assertNotIn(memberships[0], models.GroupAccountMembership.objects.all()) + self.assertIn(memberships[1], models.GroupAccountMembership.objects.all()) class AccountDeactivateTest(AnVILAPIMockTestMixin, TestCase): @@ -3621,10 +3622,10 @@ def test_removes_account_from_one_group(self): self.client.force_login(self.user) response = self.client.post(self.get_url(object.uuid), {"submit": ""}) self.assertEqual(response.status_code, 302) - # Memberships are *not* deleted from the app. - self.assertEqual(models.GroupAccountMembership.objects.count(), 1) + # Memberships are deleted from the app. + self.assertEqual(models.GroupAccountMembership.objects.count(), 0) # History for group-account membership is *not* added. - self.assertEqual(models.GroupAccountMembership.history.count(), 1) + self.assertEqual(models.GroupAccountMembership.history.count(), 2) def test_removes_account_from_all_groups(self): """Deactivating an account from the app also removes it from all groups that it is in.""" @@ -3642,8 +3643,8 @@ def test_removes_account_from_all_groups(self): # Status was updated. object.refresh_from_db() self.assertEqual(object.status, object.INACTIVE_STATUS) - # Memberships are *not* deleted from the app. - self.assertEqual(models.GroupAccountMembership.objects.count(), 2) + # Memberships are deleted from the app. + self.assertEqual(models.GroupAccountMembership.objects.count(), 0) def test_api_error_when_removing_account_from_groups(self): """Message when an API error occurred when removing a user from a group.""" @@ -3673,10 +3674,10 @@ def test_api_error_when_removing_account_from_groups(self): # The Account is not marked as inactive. object.refresh_from_db() self.assertEqual(object.status, object.ACTIVE_STATUS) - # Does not remove the user from any groups. - self.assertEqual(models.GroupAccountMembership.objects.count(), 2) - models.GroupAccountMembership.objects.get(pk=memberships[0].pk) - models.GroupAccountMembership.objects.get(pk=memberships[1].pk) + # Removes the user from only the groups where the API call succeeded. + self.assertEqual(models.GroupAccountMembership.objects.count(), 1) + self.assertNotIn(memberships[0], models.GroupAccountMembership.objects.all()) + self.assertIn(memberships[1], models.GroupAccountMembership.objects.all()) def test_account_already_inactive_get(self): """Redirects with a message if account is already deactivated.""" @@ -3887,75 +3888,6 @@ def test_success_url(self): reverse("anvil_consortium_manager:accounts:detail", args=[object.uuid]), ) - def test_adds_account_from_one_group(self): - """Reactivating an account from the app also adds it from one group on AnVIL.""" - object = factories.AccountFactory.create() - membership = factories.GroupAccountMembershipFactory.create(account=object) - group = membership.group - object.status = object.INACTIVE_STATUS - object.save() - add_to_group_url = self.get_api_add_to_group_url(group.name, object.email) - self.anvil_response_mock.add(responses.PUT, add_to_group_url, status=204) - self.client.force_login(self.user) - response = self.client.post(self.get_url(object.uuid), {"submit": ""}) - self.assertEqual(response.status_code, 302) - # History is not added for the GroupAccountMembership. - self.assertEqual(models.GroupAccountMembership.history.count(), 1) - - def test_adds_account_to_all_groups(self): - """Reactivating an account from the app also adds it from all groups that it is in.""" - object = factories.AccountFactory.create() - memberships = factories.GroupAccountMembershipFactory.create_batch(2, account=object) - object.status = object.INACTIVE_STATUS - object.save() - 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, object.email) - self.anvil_response_mock.add(responses.PUT, add_to_group_url_1, status=204) - add_to_group_url_2 = self.get_api_add_to_group_url(group_2.name, object.email) - self.anvil_response_mock.add(responses.PUT, add_to_group_url_2, status=204) - self.client.force_login(self.user) - response = self.client.post(self.get_url(object.uuid), {"submit": ""}) - self.assertEqual(response.status_code, 302) - # Status was updated. - object.refresh_from_db() - self.assertEqual(object.status, object.ACTIVE_STATUS) - - def test_api_error_when_adding_account_to_groups(self): - """Message when an API error occurred when adding a user to a group.""" - object = factories.AccountFactory.create() - memberships = factories.GroupAccountMembershipFactory.create_batch(2, account=object) - object.status = object.INACTIVE_STATUS - object.save() - 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, object.email) - self.anvil_response_mock.add(responses.PUT, add_to_group_url_1, status=204) - add_to_group_url_2 = self.get_api_add_to_group_url(group_2.name, object.email) - self.anvil_response_mock.add( - responses.PUT, - add_to_group_url_2, - status=409, - json={"message": "test error"}, - ) - # Need a client for messages. - self.client.force_login(self.user) - response = self.client.post(self.get_url(object.uuid), {"submit": ""}, follow=True) - self.assertRedirects(response, object.get_absolute_url()) - messages = [m.message for m in get_messages(response.wsgi_request)] - self.assertEqual(len(messages), 1) - self.assertEqual( - views.AccountReactivate.message_error_adding_to_groups.format("test error"), - str(messages[0]), - ) - # The Account is not marked as inactive. - object.refresh_from_db() - self.assertEqual(object.status, object.ACTIVE_STATUS) - # Does not remove the user from any groups. - self.assertEqual(models.GroupAccountMembership.objects.count(), 2) - models.GroupAccountMembership.objects.get(pk=memberships[0].pk) - models.GroupAccountMembership.objects.get(pk=memberships[1].pk) - def test_account_already_active_get(self): """Redirects with a message if account is already active.""" object = factories.AccountFactory.create() @@ -4381,52 +4313,61 @@ def test_shows_workspace_for_only_this_group(self): self.assertIn("workspace_table", response.context_data) self.assertEqual(len(response.context_data["workspace_table"].rows), 0) - def test_active_account_table(self): - """The active account table exists.""" + def test_account_table(self): + """The account table exists.""" obj = factories.ManagedGroupFactory.create() self.client.force_login(self.user) response = self.client.get(self.get_url(obj.name)) - self.assertIn("active_account_table", response.context_data) + self.assertIn("account_table", response.context_data) self.assertIsInstance( - response.context_data["active_account_table"], + response.context_data["account_table"], tables.GroupAccountMembershipStaffTable, ) - def test_active_account_table_none(self): - """No accounts are shown if the group has no active accounts.""" + def test_account_table_none(self): + """No accounts are shown if the group has no accounts.""" group = factories.ManagedGroupFactory.create() self.client.force_login(self.user) response = self.client.get(self.get_url(group.name)) - self.assertIn("active_account_table", response.context_data) - self.assertEqual(len(response.context_data["active_account_table"].rows), 0) + self.assertIn("account_table", response.context_data) + self.assertEqual(len(response.context_data["account_table"].rows), 0) - def test_active_account_table_one(self): - """One accounts is shown if the group has only that active account.""" + def test_account_table_one(self): + """One accounts is shown if the group has only that account.""" group = factories.ManagedGroupFactory.create() factories.GroupAccountMembershipFactory.create(group=group) self.client.force_login(self.user) response = self.client.get(self.get_url(group.name)) - self.assertIn("active_account_table", response.context_data) - self.assertEqual(len(response.context_data["active_account_table"].rows), 1) + self.assertIn("account_table", response.context_data) + self.assertEqual(len(response.context_data["account_table"].rows), 1) - def test_active_account_table_two(self): - """Two accounts are shown if the group has only those active accounts.""" + def test_account_table_two(self): + """Two accounts are shown if the group has only those accounts.""" group = factories.ManagedGroupFactory.create() factories.GroupAccountMembershipFactory.create_batch(2, group=group) self.client.force_login(self.user) response = self.client.get(self.get_url(group.name)) - self.assertIn("active_account_table", response.context_data) - self.assertEqual(len(response.context_data["active_account_table"].rows), 2) + self.assertIn("account_table", response.context_data) + self.assertEqual(len(response.context_data["account_table"].rows), 2) - def test_shows_active_account_for_only_this_group(self): + def test_account_table_shows_account_for_only_this_group(self): """Only shows accounts that are in this group.""" group = factories.ManagedGroupFactory.create(name="group-1") other_group = factories.ManagedGroupFactory.create(name="group-2") factories.GroupAccountMembershipFactory.create(group=other_group) self.client.force_login(self.user) response = self.client.get(self.get_url(group.name)) - self.assertIn("active_account_table", response.context_data) - self.assertEqual(len(response.context_data["active_account_table"].rows), 0) + self.assertIn("account_table", response.context_data) + self.assertEqual(len(response.context_data["account_table"].rows), 0) + + def test_account_table_includes_inactive_accounts(self): + """Shows inactive accounts in the table. Not that this would represent an internal data consistency issue.""" + group = factories.ManagedGroupFactory.create() + factories.GroupAccountMembershipFactory.create(group=group, account__status=models.Account.INACTIVE_STATUS) + self.client.force_login(self.user) + response = self.client.get(self.get_url(group.name)) + self.assertIn("account_table", response.context_data) + self.assertEqual(len(response.context_data["account_table"].rows), 1) def test_group_table(self): """The group table exists.""" diff --git a/anvil_consortium_manager/views.py b/anvil_consortium_manager/views.py index 68b2f906..b493391e 100644 --- a/anvil_consortium_manager/views.py +++ b/anvil_consortium_manager/views.py @@ -630,7 +630,7 @@ def get_selected_result_label(self, item): def get_queryset(self): # Only active accounts. - qs = models.Account.objects.filter(status=models.Account.ACTIVE_STATUS).order_by("email") + qs = models.Account.objects.active().order_by("email") # Use the account adapter to process the query. adapter = get_account_adapter() @@ -685,12 +685,8 @@ def get_context_data(self, **kwargs): context["workspace_table"] = tables.WorkspaceGroupSharingStaffTable( self.object.workspacegroupsharing_set.all(), exclude="group" ) - context["active_account_table"] = tables.GroupAccountMembershipStaffTable( - self.object.groupaccountmembership_set.filter(account__status=models.Account.ACTIVE_STATUS), - exclude="group", - ) - context["inactive_account_table"] = tables.GroupAccountMembershipStaffTable( - self.object.groupaccountmembership_set.filter(account__status=models.Account.INACTIVE_STATUS), + context["account_table"] = tables.GroupAccountMembershipStaffTable( + self.object.groupaccountmembership_set.all(), exclude="group", ) context["group_table"] = tables.GroupGroupMembershipStaffTable( diff --git a/docs/models.rst b/docs/models.rst index 92faa8a7..7f119e40 100644 --- a/docs/models.rst +++ b/docs/models.rst @@ -51,6 +51,21 @@ For a valid account, the ``anvil_exists`` method returns ``True``: By default, the :attr:`~anvil_consortium_manager.models.Account.status` field of an :class:`~anvil_consortium_manager.models.Account` is set to active (:attr:`anvil_consortium_manager.models.Account.STATUS_ACTIVE`). The status can be changed to inactive (:attr:`anvil_consortium_manager.models.Account.STATUS_INACTIVE`) by calling the :meth:`~anvil_consortium_manager.models.Account.deactivate` method on the :class:`~anvil_consortium_manager.models.Account` instance. -This will keep the record of all :class:`~anvil_consortium_manager.models.ManagedGroup`\ s that an :class:`~anvil_consortium_manager.models.Account` is part of, but will remove that Account from all groups -on AnVIL. -If the :class:`~anvil_consortium_manager.models.Account` is reactivated (using the :meth:`~anvil_consortium_manager.models.Account.reactivate` method :class:`~anvil_consortium_manager.models.Account`), it will be added back to all previous Managed Groups on AnVIL. +Deactivating an Account will remove that Account from all groups on AnVIL and delete the :class:`~anvil_consortium_manager.models.GroupAccountMembership`\ records for that Account. + +.. code-block:: pycon + + >>> account = Account(email="foo@bar.com", is_service_account=False) + >>> account.deactivate() + >>> account.get_status_display() + 'Inactive' + +The Account can be reactivated using the :meth:`~anvil_consortium_manager.models.Account.reactivate` method. + +.. code-block:: pycon + + >>> account = Account(email="foo@bar.com", is_service_account=False) + >>> account.deactivate() + >>> account.reactivate() + >>> account.get_status_display() + 'Active' diff --git a/docs/user_guide.rst b/docs/user_guide.rst index 3569f9a0..e5605dd8 100644 --- a/docs/user_guide.rst +++ b/docs/user_guide.rst @@ -241,8 +241,8 @@ If successful, you will be shown a message and information about the account tha Deactivate an account --------------------- -The app provides the ability to deactivate an Account. -When an Account is deactivated, it is removed from all groups on AnVIL but keeps the `GroupAccountMembership` records in the app. +The app provides the ability to deactivate an :class:`~anvil_consortium_manager.models.Account`. +When an Account is deactivated, it is removed from all groups on AnVIL and all :class:`~anvil_consortium_manager.models.GroupAccountMembership`` records for that Account are deleted. After deactivating, it can no longer be added to new groups unless it is reactivated. To deactivate an Account, navigate to the Account detail page and click on the "Deactivate account" button. @@ -251,6 +251,4 @@ Reactivate an account --------------------- Accounts that have been deactivated can be reactivated. -Reactivating a deactivated account adds the account back to all groups on AnVIL that it had been a part of before it was deactivated. - -To deactivate an Account, navigate to the Account detail page and click on the "Reactivate account" button. +To reactivate an Account, navigate to the Account detail page and click on the "Reactivate account" button. diff --git a/pyproject.toml b/pyproject.toml index acb4ba8c..5167a1c9 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -124,8 +124,8 @@ all = [ "rm -f .coverage.*", "hatch run test-sqlite:cov", "hatch run test-mysql:cov", - "hatch run cov-combine:combine", - "hatch run cov-combine:html", + "hatch run cov:combine", + "hatch run cov:html", ] printenv = [ "echo DBBACKEND=$DBBACKEND", @@ -194,7 +194,7 @@ matrix.django.extra-dependencies = [ ] -[tool.hatch.envs.cov-combine] +[tool.hatch.envs.cov] skip-install = true dependencies = [ # Coverage for django templates @@ -202,14 +202,15 @@ dependencies = [ "Django ~= 4.2", "coverage[toml]>=6.5" ] -[tool.hatch.envs.cov-combine.scripts] +[tool.hatch.envs.cov.scripts] combine = [ # Clean up old files first. "rm -f .coverage", - "coverage combine" + "coverage combine", ] report = ["coverage report"] html = ["coverage html"] +xml = ["coverage xml"] [tool.hatch.envs.lint]