From b12ed6d64375912006b7dfc2d1633df5c8fe6f02 Mon Sep 17 00:00:00 2001 From: Adrienne Stilp Date: Wed, 10 Jul 2024 13:17:57 -0700 Subject: [PATCH 1/6] Add a member_group field to StudySites All (or at least most) Study Sites have a members group associated with them, and members's anvil accounts need to be added to that group so that they can get data access on AnVIL. The first step in making this more automated is to link a StudySite with its member group via a OneToOne field. --- .../migrations/0007_studysite_member_group.py | 20 +++++++++++++++++++ .../0008_alter_studysite_member_group.py | 20 +++++++++++++++++++ primed/primed_anvil/models.py | 10 ++++++++++ primed/primed_anvil/tests/test_models.py | 20 +++++++++++++++++++ 4 files changed, 70 insertions(+) create mode 100644 primed/primed_anvil/migrations/0007_studysite_member_group.py create mode 100644 primed/primed_anvil/migrations/0008_alter_studysite_member_group.py diff --git a/primed/primed_anvil/migrations/0007_studysite_member_group.py b/primed/primed_anvil/migrations/0007_studysite_member_group.py new file mode 100644 index 00000000..ff038f61 --- /dev/null +++ b/primed/primed_anvil/migrations/0007_studysite_member_group.py @@ -0,0 +1,20 @@ +# Generated by Django 4.2.13 on 2024-07-10 20:21 + +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + dependencies = [ + ('anvil_consortium_manager', '0019_accountuserarchive'), + ('primed_anvil', '0006_studysite_drupal_node_id'), + ] + + operations = [ + migrations.AddField( + model_name='studysite', + name='member_group', + field=models.ForeignKey(blank=True, help_text='The AnVIL Managed Group associated with this site.', null=True, on_delete=django.db.models.deletion.PROTECT, to='anvil_consortium_manager.managedgroup'), + ), + ] diff --git a/primed/primed_anvil/migrations/0008_alter_studysite_member_group.py b/primed/primed_anvil/migrations/0008_alter_studysite_member_group.py new file mode 100644 index 00000000..e8f20eaa --- /dev/null +++ b/primed/primed_anvil/migrations/0008_alter_studysite_member_group.py @@ -0,0 +1,20 @@ +# Generated by Django 4.2.13 on 2024-07-10 20:24 + +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + dependencies = [ + ('anvil_consortium_manager', '0019_accountuserarchive'), + ('primed_anvil', '0007_studysite_member_group'), + ] + + operations = [ + migrations.AlterField( + model_name='studysite', + name='member_group', + field=models.OneToOneField(blank=True, help_text='The AnVIL Managed Group associated with this site.', null=True, on_delete=django.db.models.deletion.PROTECT, to='anvil_consortium_manager.managedgroup'), + ), + ] diff --git a/primed/primed_anvil/models.py b/primed/primed_anvil/models.py index 87389d1b..ad8727e3 100644 --- a/primed/primed_anvil/models.py +++ b/primed/primed_anvil/models.py @@ -1,3 +1,4 @@ +from anvil_consortium_manager.models import ManagedGroup from django.conf import settings from django.db import models from django.urls import reverse @@ -37,6 +38,15 @@ class StudySite(TimeStampedModel, models.Model): full_name = models.CharField(max_length=255) """The full name of the Study Sites.""" + member_group = models.OneToOneField( + ManagedGroup, + on_delete=models.PROTECT, + help_text="The AnVIL Managed Group associated with this site.", + null=True, + blank=True, + unique=True, + ) + drupal_node_id = models.IntegerField(blank=True, null=True) """Reference node ID for entity in drupal""" diff --git a/primed/primed_anvil/tests/test_models.py b/primed/primed_anvil/tests/test_models.py index b4f249c6..0672b0c0 100644 --- a/primed/primed_anvil/tests/test_models.py +++ b/primed/primed_anvil/tests/test_models.py @@ -1,5 +1,6 @@ """Tests of models in the `primed_anvil` app.""" +from anvil_consortium_manager.tests.factories import ManagedGroupFactory from django.core.exceptions import ValidationError from django.db.utils import IntegrityError from django.test import TestCase @@ -48,6 +49,7 @@ class StudySiteTest(TestCase): def test_model_saving(self): """Creation using the model constructor and .save() works.""" instance = models.StudySite(full_name="Test name", short_name="TEST") + instance.full_clean() instance.save() self.assertIsInstance(instance, models.StudySite) @@ -72,6 +74,24 @@ def test_unique_short_name(self): with self.assertRaises(IntegrityError): instance2.save() + def test_can_set_members_group(self): + member_group = ManagedGroupFactory.create() + instance = models.StudySite(full_name="Test name", short_name="TEST", member_group=member_group) + instance.full_clean() + instance.save() + self.assertIsInstance(instance, models.StudySite) + + def test_same_member_group_different_sites(self): + member_group = ManagedGroupFactory.create() + factories.StudySiteFactory.create(member_group=member_group) + instance = factories.StudySiteFactory.build(member_group=member_group) + with self.assertRaises(ValidationError) as e: + instance.full_clean() + self.assertEqual(len(e.exception.message_dict), 1) + self.assertIn("member_group", e.exception.message_dict) + self.assertEqual(len(e.exception.message_dict["member_group"]), 1) + self.assertIn("Study site with this Member group already exists.", e.exception.message_dict["member_group"][0]) + class AvailableDataTest(TestCase): """Tests for the AvailableData model.""" From 8cceb302d52330e581162c4f988cd52f414ec842 Mon Sep 17 00:00:00 2001 From: Adrienne Stilp Date: Wed, 10 Jul 2024 14:20:11 -0700 Subject: [PATCH 2/6] Show member group information on StudySite detail page Include a link to the member group, if it exists. Also show a table Accoutns that are part of the member group (and therefore should have data access). --- primed/primed_anvil/tests/test_views.py | 51 ++++++++++++++++++- primed/primed_anvil/views.py | 9 +++- .../primed_anvil/studysite_detail.html | 31 ++++++++++- 3 files changed, 86 insertions(+), 5 deletions(-) diff --git a/primed/primed_anvil/tests/test_views.py b/primed/primed_anvil/tests/test_views.py index 22f929b9..188949ef 100644 --- a/primed/primed_anvil/tests/test_views.py +++ b/primed/primed_anvil/tests/test_views.py @@ -3,6 +3,7 @@ from anvil_consortium_manager import models as acm_models from anvil_consortium_manager.tests.factories import ( AccountFactory, + GroupAccountMembershipFactory, ManagedGroupFactory, WorkspaceGroupSharingFactory, ) @@ -17,13 +18,13 @@ from django.test import RequestFactory, TestCase from django.urls import reverse -from primed.cdsa.tables import CDSAWorkspaceStaffTable, CDSAWorkspaceUserTable +from primed.cdsa.tables import CDSAWorkspaceStaffTable, CDSAWorkspaceUserTable, MemberAgreementTable from primed.cdsa.tests.factories import ( CDSAWorkspaceFactory, DataAffiliateAgreementFactory, MemberAgreementFactory, ) -from primed.dbgap.tables import dbGaPWorkspaceStaffTable, dbGaPWorkspaceUserTable +from primed.dbgap.tables import dbGaPApplicationTable, dbGaPWorkspaceStaffTable, dbGaPWorkspaceUserTable from primed.dbgap.tests.factories import ( dbGaPApplicationFactory, dbGaPStudyAccessionFactory, @@ -35,6 +36,7 @@ ) from primed.miscellaneous_workspaces.tests.factories import OpenAccessWorkspaceFactory from primed.primed_anvil.tests.factories import AvailableDataFactory, StudyFactory +from primed.users.tables import UserTable from primed.users.tests.factories import UserFactory from .. import filters, models, tables, views @@ -843,6 +845,22 @@ def test_view_status_code_with_invalid_pk(self): with self.assertRaises(Http404): self.get_view()(request, pk=obj.pk + 1) + def test_table_classes(self): + """Table classes are correct.""" + obj = self.model_factory.create() + self.client.force_login(self.user) + response = self.client.get(self.get_url(obj.pk)) + self.assertIn("tables", response.context_data) + self.assertEqual(len(response.context_data["tables"]), 4) + self.assertIsInstance(response.context_data["tables"][0], UserTable) + self.assertEqual(len(response.context_data["tables"][0].data), 0) + self.assertIsInstance(response.context_data["tables"][1], dbGaPApplicationTable) + self.assertEqual(len(response.context_data["tables"][1].data), 0) + self.assertIsInstance(response.context_data["tables"][2], MemberAgreementTable) + self.assertEqual(len(response.context_data["tables"][2].data), 0) + self.assertIsInstance(response.context_data["tables"][3], tables.AccountTable) + self.assertEqual(len(response.context_data["tables"][3].data), 0) + def test_site_user_table(self): """Contains a table of site users with the correct users.""" obj = self.model_factory.create() @@ -883,6 +901,35 @@ def test_cdsa_table(self): self.assertIn(site_cdsa, table.data) self.assertNotIn(other_cdsa, table.data) + def test_member_group_table(self): + member_group = ManagedGroupFactory.create() + obj = self.model_factory.create(member_group=member_group) + account = AccountFactory.create(verified=True) + GroupAccountMembershipFactory.create(account=account, group=member_group) + other_account = AccountFactory.create(verified=True) + self.client.force_login(self.user) + response = self.client.get(self.get_url(obj.pk)) + table = response.context_data["tables"][3] + self.assertEqual(len(table.rows), 1) + self.assertIn(account, table.data) + self.assertNotIn(other_account, table.data) + + def test_member_table_group_not_set(self): + obj = self.model_factory.create() + AccountFactory.create(verified=True) + self.client.force_login(self.user) + response = self.client.get(self.get_url(obj.pk)) + table = response.context_data["tables"][3] + self.assertEqual(len(table.rows), 0) + + def test_link_to_member_group(self): + """Response includes a link to the member group if it exists.""" + member_group = ManagedGroupFactory.create() + obj = self.model_factory.create(member_group=member_group) + self.client.force_login(self.user) + response = self.client.get(self.get_url(obj.pk)) + self.assertContains(response, member_group.get_absolute_url()) + class StudySiteListTest(TestCase): """Tests for the StudySiteList view.""" diff --git a/primed/primed_anvil/views.py b/primed/primed_anvil/views.py index 0811ec9c..96249814 100644 --- a/primed/primed_anvil/views.py +++ b/primed/primed_anvil/views.py @@ -5,7 +5,7 @@ AnVILConsortiumManagerStaffViewRequired, AnVILConsortiumManagerViewRequired, ) -from anvil_consortium_manager.models import AnVILProjectManagerAccess, Workspace +from anvil_consortium_manager.models import Account, AnVILProjectManagerAccess, Workspace from dal import autocomplete from django.contrib.auth import get_user_model from django.contrib.auth.mixins import LoginRequiredMixin @@ -128,6 +128,7 @@ class StudySiteDetail(AnVILConsortiumManagerStaffViewRequired, MultiTableMixin, UserTable, dbGaPApplicationTable, MemberAgreementTable, + tables.AccountTable, ] # def get_table(self): @@ -136,7 +137,11 @@ def get_tables_data(self): user_qs = User.objects.filter(study_sites=self.object) dbgap_qs = dbGaPApplication.objects.filter(principal_investigator__study_sites=self.object) cdsa_qs = MemberAgreement.objects.filter(study_site=self.object) - return [user_qs, dbgap_qs, cdsa_qs] + if self.object.member_group: + account_qs = Account.objects.filter(groupaccountmembership__group=self.object.member_group) + else: + account_qs = Account.objects.none() + return [user_qs, dbgap_qs, cdsa_qs, account_qs] class StudySiteList(AnVILConsortiumManagerStaffViewRequired, SingleTableView): diff --git a/primed/templates/primed_anvil/studysite_detail.html b/primed/templates/primed_anvil/studysite_detail.html index 04bd8cde..23d742b8 100644 --- a/primed/templates/primed_anvil/studysite_detail.html +++ b/primed/templates/primed_anvil/studysite_detail.html @@ -6,6 +6,13 @@
Full name
{{ object.full_name }}
Short name
{{ object.short_name }}
+
Member group
+ {% if object.member_group %} + {{ object.member_group }} + {% else %} + — + {% endif %} +
{% endblock panel %} @@ -44,8 +51,30 @@

+

Investigators

+ +
+
+
+

+ +

+
+
+

+ This table shows Accounts in the member group for this Study Site. +

+ {% render_table tables.3 %} +
+
+
+
+
-

Study Site Users

Study site user list only contains those site users who have created an account on this website.

{% render_table tables.0 %} From 94ec461961238fcd69934a8f9d214a4b8dcdaeac Mon Sep 17 00:00:00 2001 From: Adrienne Stilp Date: Wed, 10 Jul 2024 14:22:49 -0700 Subject: [PATCH 3/6] Switch the user table to an accordion --- .../primed_anvil/studysite_detail.html | 23 +++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/primed/templates/primed_anvil/studysite_detail.html b/primed/templates/primed_anvil/studysite_detail.html index 23d742b8..5f702061 100644 --- a/primed/templates/primed_anvil/studysite_detail.html +++ b/primed/templates/primed_anvil/studysite_detail.html @@ -75,7 +75,26 @@

-

Study site user list only contains those site users who have created an account on this website.

- {% render_table tables.0 %} +
+
+
+

+ +

+
+
+

+ This table shows users who are associated with this Study Site. +

+ {% render_table tables.0 %} +
+
+
+
+
{% endblock after_panel %} From 6a6b91191278939b9b0fc26da78b093ac342873e Mon Sep 17 00:00:00 2001 From: Adrienne Stilp Date: Wed, 10 Jul 2024 14:37:17 -0700 Subject: [PATCH 4/6] Do not show the number of groups for an Account on StudySite page --- primed/primed_anvil/views.py | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/primed/primed_anvil/views.py b/primed/primed_anvil/views.py index 96249814..da420a8c 100644 --- a/primed/primed_anvil/views.py +++ b/primed/primed_anvil/views.py @@ -124,16 +124,8 @@ class StudySiteDetail(AnVILConsortiumManagerStaffViewRequired, MultiTableMixin, """View to show details about a `StudySite`.""" model = models.StudySite - tables = [ - UserTable, - dbGaPApplicationTable, - MemberAgreementTable, - tables.AccountTable, - ] - # def get_table(self): - # return UserTable(User.objects.filter(study_sites=self.object)) - def get_tables_data(self): + def get_tables(self): user_qs = User.objects.filter(study_sites=self.object) dbgap_qs = dbGaPApplication.objects.filter(principal_investigator__study_sites=self.object) cdsa_qs = MemberAgreement.objects.filter(study_site=self.object) @@ -141,7 +133,12 @@ def get_tables_data(self): account_qs = Account.objects.filter(groupaccountmembership__group=self.object.member_group) else: account_qs = Account.objects.none() - return [user_qs, dbgap_qs, cdsa_qs, account_qs] + return [ + UserTable(user_qs), + dbGaPApplicationTable(dbgap_qs), + MemberAgreementTable(cdsa_qs), + tables.AccountTable(account_qs, exclude=("number_groups",)), + ] class StudySiteList(AnVILConsortiumManagerStaffViewRequired, SingleTableView): From 5e1b03ea03597727178cd17691a79dfdfc9ad4af Mon Sep 17 00:00:00 2001 From: Adrienne Stilp Date: Wed, 10 Jul 2024 15:56:23 -0700 Subject: [PATCH 5/6] Set representatives' sites in CDSA test data --- add_cdsa_example_data.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/add_cdsa_example_data.py b/add_cdsa_example_data.py index 105233ba..cbb34f33 100644 --- a/add_cdsa_example_data.py +++ b/add_cdsa_example_data.py @@ -70,6 +70,7 @@ signed_agreement__version=v10, study_site=cc, ) +cdsa_1001.signed_agreement.representative.study_sites.add(cc) GroupGroupMembershipFactory.create(parent_group=cdsa_group, child_group=cdsa_1001.signed_agreement.anvil_access_group) cdsa_1002 = factories.MemberAgreementFactory.create( @@ -81,6 +82,7 @@ signed_agreement__version=v10, study_site=cardinal, ) +cdsa_1002.signed_agreement.representative.study_sites.add(cardinal) GroupGroupMembershipFactory.create(parent_group=cdsa_group, child_group=cdsa_1002.signed_agreement.anvil_access_group) cdsa_1003 = factories.MemberAgreementFactory.create( From bcf962c3d1aa8911d69211638b628f2c55708f44 Mon Sep 17 00:00:00 2001 From: Adrienne Stilp Date: Wed, 10 Jul 2024 15:56:50 -0700 Subject: [PATCH 6/6] Choose user table depending on whether member_group is set If member_group is set, use the UserAccountSingleGroupMembershipTable; otherwise, use a newly-defined table (UserAccountTable). This lets CC staff see who from the site has a) linked their account and b) been added to the member group (if it is set). --- primed/primed_anvil/tables.py | 16 +++++++++---- primed/primed_anvil/tests/test_tables.py | 29 ++++++++++++++++++++++++ primed/primed_anvil/tests/test_views.py | 13 +++++++++-- primed/primed_anvil/views.py | 9 +++++--- 4 files changed, 58 insertions(+), 9 deletions(-) diff --git a/primed/primed_anvil/tables.py b/primed/primed_anvil/tables.py index 29a79d8d..94b583e3 100644 --- a/primed/primed_anvil/tables.py +++ b/primed/primed_anvil/tables.py @@ -156,8 +156,11 @@ def __init__(self, *args, **kwargs): super().__init__(*args, extra_columns=extra_columns, **kwargs) -class UserAccountSingleGroupMembershipTable(tables.Table): - """A table with users and info about whether they are members of a group.""" +class UserAccountTable(tables.Table): + """A table for `User`s with `Account` information.""" + + name = tables.Column(linkify=True) + account = tables.Column(linkify=True, verbose_name="AnVIL account") class Meta: model = User @@ -167,8 +170,13 @@ class Meta: ) order_by = ("name",) - name = tables.Column(linkify=lambda record: record.get_absolute_url()) - account = tables.Column(verbose_name="AnVIL account") + +class UserAccountSingleGroupMembershipTable(UserAccountTable): + """A table with users and info about whether they are members of a group.""" + + class Meta(UserAccountTable.Meta): + pass + is_group_member = tables.BooleanColumn(verbose_name="Has access?", default=False) def __init__(self, *args, managed_group=None, **kwargs): diff --git a/primed/primed_anvil/tests/test_tables.py b/primed/primed_anvil/tests/test_tables.py index a539ada3..3a892c15 100644 --- a/primed/primed_anvil/tests/test_tables.py +++ b/primed/primed_anvil/tests/test_tables.py @@ -255,6 +255,35 @@ def test_render_not_workspace(self): column.render(None, "foo", None) +class UserAccountTableTest(TestCase): + """Tests for the UserAccountTable class.""" + + def setUp(self): + self.managed_group = ManagedGroupFactory.create() + + def test_row_count_with_no_objects(self): + table = tables.UserAccountTable(User.objects.all()) + self.assertEqual(len(table.rows), 0) + + def test_row_count_with_one_object(self): + UserFactory.create() + table = tables.UserAccountTable(User.objects.all()) + self.assertEqual(len(table.rows), 1) + + def test_row_count_with_two_objects(self): + UserFactory.create_batch(2) + table = tables.UserAccountTable(User.objects.all()) + self.assertEqual(len(table.rows), 2) + + def test_ordering(self): + """Users are ordered alphabetically by name""" + user_foo = UserFactory.create(name="Foo") + user_bar = UserFactory.create(name="Bar") + table = tables.UserAccountTable(User.objects.all()) + self.assertEqual(table.data[0], user_bar) + self.assertEqual(table.data[1], user_foo) + + class UserAccountSingleGroupMembershipTableTest(TestCase): """Tests for the UserAccountSingleGroupMembershipTable class.""" diff --git a/primed/primed_anvil/tests/test_views.py b/primed/primed_anvil/tests/test_views.py index 188949ef..f47e8717 100644 --- a/primed/primed_anvil/tests/test_views.py +++ b/primed/primed_anvil/tests/test_views.py @@ -36,7 +36,6 @@ ) from primed.miscellaneous_workspaces.tests.factories import OpenAccessWorkspaceFactory from primed.primed_anvil.tests.factories import AvailableDataFactory, StudyFactory -from primed.users.tables import UserTable from primed.users.tests.factories import UserFactory from .. import filters, models, tables, views @@ -852,7 +851,7 @@ def test_table_classes(self): response = self.client.get(self.get_url(obj.pk)) self.assertIn("tables", response.context_data) self.assertEqual(len(response.context_data["tables"]), 4) - self.assertIsInstance(response.context_data["tables"][0], UserTable) + self.assertIsInstance(response.context_data["tables"][0], tables.UserAccountTable) self.assertEqual(len(response.context_data["tables"][0].data), 0) self.assertIsInstance(response.context_data["tables"][1], dbGaPApplicationTable) self.assertEqual(len(response.context_data["tables"][1].data), 0) @@ -901,6 +900,16 @@ def test_cdsa_table(self): self.assertIn(site_cdsa, table.data) self.assertNotIn(other_cdsa, table.data) + def test_site_user_table_when_member_group_is_set(self): + """The site user table is the correct class when the member group is set.""" + member_group = ManagedGroupFactory.create() + obj = self.model_factory.create(member_group=member_group) + self.client.force_login(self.user) + response = self.client.get(self.get_url(obj.pk)) + table = response.context_data["tables"][0] + self.assertIsInstance(table, tables.UserAccountSingleGroupMembershipTable) + self.assertEqual(table.managed_group, member_group) + def test_member_group_table(self): member_group = ManagedGroupFactory.create() obj = self.model_factory.create(member_group=member_group) diff --git a/primed/primed_anvil/views.py b/primed/primed_anvil/views.py index da420a8c..7717e667 100644 --- a/primed/primed_anvil/views.py +++ b/primed/primed_anvil/views.py @@ -33,7 +33,6 @@ OpenAccessWorkspaceStaffTable, OpenAccessWorkspaceUserTable, ) -from primed.users.tables import UserTable from . import filters, helpers, models, tables @@ -127,6 +126,10 @@ class StudySiteDetail(AnVILConsortiumManagerStaffViewRequired, MultiTableMixin, def get_tables(self): user_qs = User.objects.filter(study_sites=self.object) + if self.object.member_group: + user_table = tables.UserAccountSingleGroupMembershipTable(user_qs, managed_group=self.object.member_group) + else: + user_table = tables.UserAccountTable(user_qs, exclude="study_sites") dbgap_qs = dbGaPApplication.objects.filter(principal_investigator__study_sites=self.object) cdsa_qs = MemberAgreement.objects.filter(study_site=self.object) if self.object.member_group: @@ -134,10 +137,10 @@ def get_tables(self): else: account_qs = Account.objects.none() return [ - UserTable(user_qs), + user_table, dbGaPApplicationTable(dbgap_qs), MemberAgreementTable(cdsa_qs), - tables.AccountTable(account_qs, exclude=("number_groups",)), + tables.AccountTable(account_qs, exclude=("number_groups")), ]