From 5239c50d9f78c96b7a84d2e813a8e378b55a3e3f Mon Sep 17 00:00:00 2001 From: Sean Hammond Date: Fri, 11 Oct 2024 12:11:40 +0100 Subject: [PATCH] Fix admin pages when group has no organization Fix a crash on the admin page for editing an open or restricted group when the group has no organization. Fixes: https://github.com/hypothesis/h/issues/9009 Problem ------- Open and restricted groups have admin pages that let Hypothesis admins edit the group (type, name, organization, creator, description, scopes, members). You go to https://hypothes.is/admin/groups, search for the open or restricted group that you want, and click on that group's name to go to the `https://hypothes.is/admin/groups/` admin page for editing that group. Private groups do not have admin pages, that's why many of the group names on the the https://hypothes.is/admin/groups admin page aren't linked: those are private groups. Originally the only way to create an open or restricted group was via the https://hypothes.is/admin/groups/new admin page, and that page forces you to choose an organization for the open or restricted group to belong to. The `https://hypothes.is/admin/groups/` page could therefore assume that all open or restricted groups belong to an organization. Recently we've enabled users to create their own open or restricted groups via the https://hypothes.is/groups/new page and groups created by this page do not have organizations. Private groups created by this page have never belonged to organizations. The open and restricted groups that can now also be created don't have organizations either. A user could also create a private group with no organization and then turn it into an open or restricted group. So for the first time it's now possible to have open and restricted groups with no organizations. But the `https://hypothes.is/admin/groups/` admin page for editing open or restricted groups has two problems with this: 1. Loading the page crashes when the group has no organization 2. The form forces groups to belong to an organization, there's no option for a group to not have an organization Solution -------- This commit fixes the page to not crash when a group has no organization and also adds a "-- None --" option to the form's organization dropdown so that it no longer forces you to select an organization when creating or editing a group. As a side effect of these changes admins can now also use the https://hypothes.is/admin/groups/new admin page to create open and restricted groups with no organization, and some further fixes were needed to prevent this from crashing. --- h/schemas/forms/admin/group.py | 15 +++-- h/services/group_create.py | 2 +- h/views/admin/groups.py | 16 ++++-- .../unit/h/schemas/forms/admin/group_test.py | 3 +- tests/unit/h/services/group_create_test.py | 55 ++++++++++++++++-- tests/unit/h/views/admin/groups_test.py | 56 +++++++++++++++++++ 6 files changed, 130 insertions(+), 17 deletions(-) diff --git a/h/schemas/forms/admin/group.py b/h/schemas/forms/admin/group.py index 33a3e623240..1d833e2e4e6 100644 --- a/h/schemas/forms/admin/group.py +++ b/h/schemas/forms/admin/group.py @@ -36,9 +36,13 @@ def username_validator(form, value): user_svc = form.bindings["user_svc"] - # Get the authority from the list of organisations based on the one - # actually picked in the form - authority = form.bindings["organizations"][value["organization"]].authority + organization = value["organization"] + if organization: + # Get the authority from the organization selected in the form. + authority = form.bindings["organizations"][organization].authority + else: + # No organization was selected in the form, use the default authority. + authority = form.bindings["request"].default_authority exc = colander.Invalid(form, None) @@ -117,8 +121,8 @@ def validate(node, value): @colander.deferred def group_organization_select_widget(_node, kwargs): orgs = kwargs["organizations"] - org_labels = [] - org_pubids = [] + org_labels = ["-- None --"] + org_pubids = [""] for org in orgs.values(): org_labels.append(f"{org.name} ({org.authority})") org_pubids.append(org.pubid) @@ -153,6 +157,7 @@ def __init__(self, *args): title=_("Organization"), description=_("Organization which this group belongs to"), widget=group_organization_select_widget, + missing=None, ) creator = colander.SchemaNode( diff --git a/h/services/group_create.py b/h/services/group_create.py index f9d90131a8f..fbf9305ba6f 100644 --- a/h/services/group_create.py +++ b/h/services/group_create.py @@ -105,7 +105,7 @@ def _create(self, name, userid, type_flags, scopes, **kwargs): group_scopes = [GroupScope(scope=s) for s in scopes] - if "organization" in kwargs: + if kwargs.get("organization"): self._validate_authorities_match( creator.authority, kwargs["organization"].authority ) diff --git a/h/views/admin/groups.py b/h/views/admin/groups.py index f8719d94339..4e395917b74 100644 --- a/h/views/admin/groups.py +++ b/h/views/admin/groups.py @@ -71,10 +71,16 @@ def post(self): def on_success(appstruct): """Create a group on successful validation of POSTed form data.""" - organization = self.organizations[appstruct["organization"]] + organization = self.organizations.get(appstruct["organization"]) + # We know this user exists because it is checked during schema validation creator_userid = self.user_svc.fetch( - appstruct["creator"], organization.authority + appstruct["creator"], + ( + organization.authority + if organization + else self.request.default_authority + ), ).userid create_fns = { @@ -175,7 +181,7 @@ def update(self): def on_success(appstruct): """Update the group resource on successful form validation.""" - organization = self.organizations[appstruct["organization"]] + organization = self.organizations.get(appstruct["organization"]) scopes = [GroupScope(scope=o) for o in appstruct["scopes"]] self.group_update_svc.update( @@ -217,7 +223,9 @@ def _update_appstruct(self): "group_type": group.type, "name": group.name, "members": [m.username for m in group.members], - "organization": group.organization.pubid, + "organization": ( + group.organization.pubid if group.organization else None + ), "scopes": [s.scope for s in group.scopes], "enforce_scope": group.enforce_scope, } diff --git a/tests/unit/h/schemas/forms/admin/group_test.py b/tests/unit/h/schemas/forms/admin/group_test.py index 6575d9500a8..5bae9dd2668 100644 --- a/tests/unit/h/schemas/forms/admin/group_test.py +++ b/tests/unit/h/schemas/forms/admin/group_test.py @@ -53,7 +53,7 @@ def test_it_raises_if_required_field_missing( with pytest.raises(colander.Invalid, match=f".*{required_field}.*"): bound_schema.deserialize(group_data) - @pytest.mark.parametrize("optional_field", ("description",)) + @pytest.mark.parametrize("optional_field", ("description", "organization")) def test_it_allows_when_optional_field_missing( self, group_data, bound_schema, optional_field ): @@ -162,6 +162,7 @@ def test_it_lists_organizations(self, bound_schema, org, third_party_org): # pylint:disable=possibly-used-before-assignment assert org_node.widget.values == [ + ("", "-- None --"), (org.pubid, f"{org.name} ({org.authority})"), ( third_party_org.pubid, diff --git a/tests/unit/h/services/group_create_test.py b/tests/unit/h/services/group_create_test.py index 2f5b19e6222..e6e2c5c8733 100644 --- a/tests/unit/h/services/group_create_test.py +++ b/tests/unit/h/services/group_create_test.py @@ -69,8 +69,23 @@ def test_it_sets_access_flags(self, svc, creator, flag, expected_value): assert getattr(group, flag) == expected_value - def test_it_creates_group_with_no_organization_by_default(self, creator, svc): - group = svc.create_private_group("Anteater fans", creator.userid) + @pytest.mark.parametrize("pass_kwarg", [True, False]) + def test_it_creates_group_with_no_organization_by_default( + self, creator, svc, pass_kwarg + ): + """ + If the caller passes no organization it creates a group with no organization. + + This works both if the caller omits the `organization` kwarg entirely + and if the caller passes `organization=None`. It's convenient for the + caller to be able to do it either way. + """ + if pass_kwarg: + kwargs = {"organization": None} + else: + kwargs = {} + + group = svc.create_private_group("Anteater fans", creator.userid, **kwargs) assert group.organization is None @@ -170,10 +185,25 @@ def test_it_sets_access_flags(self, svc, creator, origins, flag, expected_value) assert getattr(group, flag) == expected_value + @pytest.mark.parametrize("pass_kwarg", [True, False]) def test_it_creates_group_with_no_organization_by_default( - self, creator, svc, origins + self, creator, svc, origins, pass_kwarg ): - group = svc.create_open_group("Anteater fans", creator.userid, scopes=origins) + """ + If the caller passes no organization it creates a group with no organization. + + This works both if the caller omits the `organization` kwarg entirely + and if the caller passes `organization=None`. It's convenient for the + caller to be able to do it either way. + """ + if pass_kwarg: + kwargs = {"organization": None} + else: + kwargs = {} + + group = svc.create_open_group( + "Anteater fans", creator.userid, scopes=origins, **kwargs + ) assert group.organization is None @@ -319,11 +349,24 @@ def test_it_sets_access_flags(self, svc, creator, origins, flag, expected_value) assert getattr(group, flag) == expected_value + @pytest.mark.parametrize("pass_kwarg", [True, False]) def test_it_creates_group_with_no_organization_by_default( - self, creator, svc, origins + self, creator, svc, origins, pass_kwarg ): + """ + If the caller passes no organization it creates a group with no organization. + + This works both if the caller omits the `organization` kwarg entirely + and if the caller passes `organization=None`. It's convenient for the + caller to be able to do it either way. + """ + if pass_kwarg: + kwargs = {"organization": None} + else: + kwargs = {} + group = svc.create_restricted_group( - "Anteater fans", creator.userid, scopes=origins + "Anteater fans", creator.userid, scopes=origins, **kwargs ) assert group.organization is None diff --git a/tests/unit/h/views/admin/groups_test.py b/tests/unit/h/views/admin/groups_test.py index d90c3930850..a6601333a3a 100644 --- a/tests/unit/h/views/admin/groups_test.py +++ b/tests/unit/h/views/admin/groups_test.py @@ -185,6 +185,33 @@ def call_on_success( # pylint:disable=unused-argument group_create_service.create_restricted_group.return_value, [user.userid] ) + def test_post_with_no_organization( + self, + base_appstruct, + handle_form_submission, + pyramid_request, + user_service, + group_create_service, + ): + """Test creating a new group with no organization.""" + base_appstruct["organization"] = None + + def call_on_success( # pylint:disable=unused-argument + request, form, on_success, on_failure + ): + return on_success(base_appstruct) + + handle_form_submission.side_effect = call_on_success + view = GroupCreateViews(pyramid_request) + + view.post() + + assert user_service.fetch.call_args[0][1] == pyramid_request.default_authority + assert ( + group_create_service.create_restricted_group.call_args[1]["organization"] + is None + ) + @pytest.fixture def base_appstruct(self, pyramid_request, organization): return { @@ -316,6 +343,35 @@ def call_on_success( # pylint:disable=unused-argument ) assert response["form"] == self._expected_form(group) + def test_update_when_group_has_no_organization( + self, pyramid_request, handle_form_submission, group_update_service, group + ): + group.organization = None + + def call_on_success( # pylint:disable=unused-argument + request, form, on_success, on_failure + ): + return on_success( + { + # No "organization" in these on_success() kwargs. + "creator": "creator", + "description": "description", + "group_type": "open", + "name": "name", + "organization": None, + "scopes": [], + "members": [], + "enforce_scope": False, + } + ) + + handle_form_submission.side_effect = call_on_success + view = GroupEditViews(GroupContext(group), pyramid_request) + + view.update() + + assert group_update_service.update.call_args[1]["organization"] is None + def test_update_updates_group_members_on_success( self, factories,