Factor group join into separate view #4154
Conversation
| @@ -112,18 +113,12 @@ def _update_group(self, appstruct): | |||
| @view_config(route_name='group_read', | |||
| request_method='GET', | |||
| renderer='h:templates/groups/share.html.jinja2', | |||
| effective_principals=security.Authenticated) | |||
| effective_principals=security.Authenticated, | |||
| has_permission='read') | |||
There was a problem hiding this comment.
We make it so that this is only called if you have read permission, so that JoinGroupController below will be called if you don't
| # page. | ||
| if not request.has_permission('read'): | ||
| request.override_renderer = 'h:templates/groups/join.html.jinja2' | ||
| return {'group': group} |
There was a problem hiding this comment.
JoinGroupController handles this now
Current coverage is 83.86% (diff: 92.59%)@@ master #4154 diff @@
==========================================
Files 158 159 +1
Lines 6228 6342 +114
Methods 0 0
Messages 0 0
Branches 706 737 +31
==========================================
+ Hits 5207 5319 +112
Misses 947 947
- Partials 74 76 +2
|
|
h.views.groups could do with a fair bit more cleaning up. I think it would probably be better if all the views for the 'group_read' route were together in one class, and the 'group_leave' route may not be necessary (just use the 'group_read' route with a 'leave' request param). But in this PR I'm doing only the tidy up that I need for the changes that this series of PRs is driving at - I need the group read page and the group join page to be separate view callables so that I can change their view config arguments separately |
Factor the group join page (the view for the 'group_read' route when the user is logged-in but isn't a member of the group) into a separate view callable. Previously h.views.groups.read() was called for the 'group_read' route when the user was logged-in, whether they're a member of the group or not. In the future the classic group page that this read() view provides is going to be used only when the 'search_page' feature flag is off, and a completely separate view (h.views.activity.GroupSearchController) is going to be used when the flag is on. We'll add a view config predicate to read() so that it's not called if the flag is on. But we still want the join group page to be called even if the feature flag is on, so we can't have this read() function (which is not going to be called when the flag is on anymore) also being responsible for the join page. Additionally there was a separate view function, join(), which was called when the user is logged-in but isn't a member of the group and POSTs to the group_read route. It's a little odd that GET requests in the "join group" scenario (group_read route, logged-in, not a member) are handled by read() (which also handles the read group scenario) whereas POST requests in the join group scenario are handled by a separate join() function. So refactor things so that: * read() is only for the group read page that you see when you're a member of the group * The new JoinController is for the join page that you see when you aren't a member, and has separate methods for GET and POST to this page Note that there's still one more separate view function for this route as well, which this commit doesn't touch: read_unauthenticated() (for when you're not logged in).
seanh
force-pushed
the
factor-group-join-into-separate-view
branch
from
bb14f46
to
95b85f7
Compare
| @view_config(route_name='group_read_noslug', request_method='GET') | ||
| def read_noslug(group, request): | ||
| _check_slug(group, request) | ||
| @view_defaults(route_name='group_read', |
There was a problem hiding this comment.
So, there are three different group_read cases here, each guarded by a set of predicates that determines whether they apply:
- User logged in, has permission to read annotations in the group: Redirect to search page if that feature is enabled, show legacy group overview page otherwise
- User logged in, does not have permissions to read annotations in the group: Show group join page
- User logged out: Show group join page. The user is invited to log in first, after which they are redirected back to the join page
There was a problem hiding this comment.
Yep. I like to use the view config as much as possible to make Pyramid choose the right view, instead of having branching logic in Python. Makes the view functions simpler
|
LGTM |
Factor the group join page (the view for the 'group_read' route when the
user is logged-in but isn't a member of the group) into a separate view
callable.
Previously h.views.groups.read() was called for the 'group_read' route
when the user was logged-in, whether they're a member of the group or
not.
In the future the classic group page that this read() view provides is
going to be used only when the 'search_page' feature flag is off, and a
completely separate view (h.views.activity.GroupSearchController) is
going to be used when the flag is on. We'll add a view config predicate
to read() so that it's not called if the flag is on.
But we still want the join group page to be called even if the feature
flag is on, so we can't have this read() function (which is not going to
be called when the flag is on anymore) also being responsible for the
join page.
Additionally there was a separate view function, join(), which was
called when the user is logged-in but isn't a member of the group and
POSTs to the group_read route. It's a little odd that GET requests in
the "join group" scenario (group_read route, logged-in, not a member)
are handled by read() (which also handles the read group scenario)
whereas POST requests in the join group scenario are handled by a
separate join() function.
So refactor things so that:
read() is only for the group read page that you see when you're a
member of the group
The new JoinController is for the join page that you see when you
aren't a member, and has separate methods for GET and POST to this page
Note that there's still one more separate view function for this route as
well, which this commit doesn't touch: read_unauthenticated() (for when you're
not logged in).