-
Notifications
You must be signed in to change notification settings - Fork 427
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix admin pages when group has no organization #9010
Conversation
org_labels = ["-- None --"] | ||
org_pubids = [""] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a -- None --
option to the organization dropdown (<select>
) on the admin page for creating or editing an open or restricted group.
@@ -153,6 +157,7 @@ def __init__(self, *args): | |||
title=_("Organization"), | |||
description=_("Organization which this group belongs to"), | |||
widget=group_organization_select_widget, | |||
missing=None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Allow organization
to be missing from create/edit group admin form submissions (this is what happens if the user chooses -- None --
)
@@ -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"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Allow GroupCreateService.create_*_group()
to be called with an organization=None
kwarg and this will have the same effect as if it were called with no organization
kwarg at all. This is convenient from the caller's point of view (the caller is the admin pages view/schema code).
@@ -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"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix the create group admin page to not assume that appstruct
(i.e. the submitted form fields) contains an organization whose value matches one of the keys in self.organizations
. If the user selects -- None --
for the organization then appstruct["organization"]
will be None
and the local variable organization
will end up being None
organization.authority | ||
if organization | ||
else self.request.default_authority |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the user selected -- None --
for the organization then organization
will be None
. In that case create a group belonging to the default authority.
@@ -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"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is when updating an existing group as opposed to creating a new one. If the group has no organization then appstruct["organization"]
will be None
and organization
will also be None
"organization": ( | ||
group.organization.pubid if group.organization else None | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This happens when loading the group edit page. If the group has no organization then group.organization
will be None
in which case we now set appstruct["organization"]
to None
which causes the form to render with -- None --
selected for the organization, rather than crashing during page load. (This was the original crash from #9009, but fixing this page load crash then reveals the various other crashes fixed above that happen when trying to actually submit the form).
""" | ||
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 | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Repetitive tests, but that's how the existing tests are structured and it's a result of how the code is structured. I prefer the test repetition over getting clever in the tests, and don't want to take the time to restructure the code and rewrite all the tests right now.
): | ||
return on_success( | ||
{ | ||
# No "organization" in these on_success() kwargs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is an "organization" field, but it is None. Is that what you meant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed the comment
ac9252d
to
e4b14bf
Compare
def test_read_renders_form_if_group_has_no_organization( | ||
self, pyramid_request, group | ||
): | ||
group.organization = None | ||
view = GroupEditViews(GroupContext(group), pyramid_request) | ||
|
||
response = view.read() | ||
|
||
assert response["form"]["organization"] is None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the test for the original page load crash in #9009
24f4774
to
e01a883
Compare
h/schemas/forms/admin/group.py
Outdated
|
||
# Get the authority from the list of organisations based on the one | ||
# actually picked in the form | ||
authority = form.bindings["organizations"][value["organization"]].authority | ||
if group := form.bindings.get("group"): | ||
authority = group.authority | ||
elif organization := value.get("organization"): | ||
# If there's no group then fall back on the organization's authority | ||
# (this happens when creating a new group as opposed to editing an | ||
# existing one). | ||
authority = form.bindings.get("organizations")[organization].authority | ||
else: | ||
# If there's no group or organisation then fall back on the default authority | ||
# (this happens when creating a new group and selecting no organization | ||
# for the group to belong to). | ||
authority = form.bindings["request"].default_authority |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The local variable authority
is used below to validate that the group's creator and members (both of which the user can edit in the admin form) all belong to the same authority as the group. But authority
has to be derived differently in different cases. There are 4 cases here:
- When editing an existing group that does have an organization: in this case
group.authority
will be used - When editing an existing group that does not have an organization: again
group.authority
will be used - When creating a new group with an organization:
organization.authority
will be used - When creating a new group without an organization:
request.default_authority
will be used
e01a883
to
e4a02ef
Compare
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.
e4a02ef
to
916de34
Compare
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 forediting 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 couldtherefore 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 openor restricted groups has two problems with this:
option for a group to not have an organization. So when editing
a group with no organization it'll force you to choose an
organization before you can make any other changes
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.
Testing
Use either http://localhost:5000/admin/groups/new or http://localhost:5000/groups/new to create a restricted or open group with no organization and then use
http://localhost:5000/admin/groups/<PUBID>
to edit the group.