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: #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/<PUBID>` 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/<PUBID>` 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/<PUBID>` 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

@@ -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(

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
  )

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,
  }

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,

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

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,