{"action":"created","issue":{"url":"https://api.github.com/repos/hypothesis/h/issues/4139","repository_url":"https://api.github.com/repos/hypothesis/h","labels_url":"https://api.github.com/repos/hypothesis/h/issues/4139/labels{/name}","comments_url":"https://api.github.com/repos/hypothesis/h/issues/4139/comments","events_url":"https://api.github.com/repos/hypothesis/h/issues/4139/events","html_url":"https://github.com/hypothesis/h/pull/4139","id":192282199,"number":4139,"title":"Change group route","user":{"login":"seanh","id":22498,"avatar_url":"https://avatars.githubusercontent.com/u/22498?v=3","gravatar_id":"","url":"https://api.github.com/users/seanh","html_url":"https://github.com/seanh","followers_url":"https://api.github.com/users/seanh/followers","following_url":"https://api.github.com/users/seanh/following{/other_user}","gists_url":"https://api.github.com/users/seanh/gists{/gist_id}","starred_url":"https://api.github.com/users/seanh/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/seanh/subscriptions","organizations_url":"https://api.github.com/users/seanh/orgs","repos_url":"https://api.github.com/users/seanh/repos","events_url":"https://api.github.com/users/seanh/events{/privacy}","received_events_url":"https://api.github.com/users/seanh/received_events","type":"User","site_admin":false},"labels":[{"id":427068869,"url":"https://api.github.com/repos/hypothesis/h/labels/Feature:%20Activity%20Pages","name":"Feature: Activity Pages","color":"5319e7","default":false},{"id":460185446,"url":"https://api.github.com/repos/hypothesis/h/labels/Needs%20Review","name":"Needs Review","color":"0e8a16","default":false}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":1,"created_at":"2016-11-29T13:30:34Z","updated_at":"2016-11-30T10:42:41Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/hypothesis/h/pulls/4139","html_url":"https://github.com/hypothesis/h/pull/4139","diff_url":"https://github.com/hypothesis/h/pull/4139.diff","patch_url":"https://github.com/hypothesis/h/pull/4139.patch"},"body":"Fixes https://github.com/hypothesis/h/issues/4004\r\n\r\n<del>This is marked as WIP because it depends on <https://github.com/hypothesis/h/pull/4136>.</del>\r\n\r\nThis pull request changes the `/groups/{pubid}/search` URL to `/groups/{pubid}/{slug}`.\r\n\r\nThis is not nearly as simple as it may sound because `/groups/{pubid}/{slug}` is already a heavily overloaded URL (with different behaviours depending on whether you're logged in or not, a member of the group or not, whether the activity_pages or search_page feature flags are on or not, redirects when the slug is missing or incorrect...).  The `/groups/{pubid}/search` also has many different behaviours (for just getting the page and for submitting the search form and clicking various buttons in the sidebar) and these are now all piled onto the `/groups/{pubid}/{slug}` URL as well.  \r\n\r\nIn addition, the existing  `/groups/{pubid}/{slug}` views are in `h.views.groups` whereas the `/groups/{pubid}/search` are in `h.views.activity`, and the groups ones use traversal whereas the activity ones use URL dispatch.\r\n\r\nMerging these two URLs into one has consequences for the structure and factoring of the codebase where the views for the two URLs (`h.views.groups` and `h.views.activity`) are concerned.\r\n\r\n## Behaviour\r\n\r\nThe new behaviour that this pull request implements is:\r\n\r\n* If you aren't logged in then `/groups/{pubid}/{slug}` shows the \"Login to join group\" page. Either the classic or the new version of this page, depending on the activity_pages feature flag.  \r\n\r\n* If you are logged in, but aren't a member of the group, then `/groups/{pubid}/{slug}` shows the \"Join group\" page. Either the classic or the new version of this page, depending on the activity_pages feature flag.\r\n\r\n* If you're already a member of the group then `/groups/{pubid}/{slug}` shows the group's activity search page if the search_page feature flag is on or the classic group page if it's not.\r\n\r\n* `/groups/{pubid}`, `/groups/{pubid}/` and `/groups/{pubid}/{something_else}` all redirect to `/groups/{pubid}/{slug}` in all cases\r\n\r\n* `/groups/{pubid}[/[{anything}]]` 404s if no group with the given pubid exists, in all cases (previously `/groups/{does_not_exist}/search` would show an empty search page, _not_ a 404, but `/groups/{does_not_exist}[/[{anything_else}]]` would 404)\r\n\r\n* Each of these different \"versions\" of the page is implemented as a separate view function or method, and we use Pyramid routing to select the correct view depending on the situation. We don't have Python logic in the view functions saying \"if the user is a member then X else Y\" etc.\r\n\r\n* The `/search` page no longer redirects to the group's page if you search for a group that doesn't exist. This is because we don't want to send users to a 404 page.\r\n\r\n## Pyramid routing\r\n\r\nThe `/groups/{pubid}/search` page was implemented by the `\"activity.group_search\"` route and the views in `h.views.activity.GroupSearchController` and `h.views.activity.GroupUserSearchController` .\r\n\r\nThe `/groups/{pubid}/{slug}` page, which is already used on master for the \"Login to join group\", \"Join group\", and classic group home pages, was implemented by the `\"group_read\"` route and the views in `h.views.groups`.\r\n\r\nIt's not possible for both the `\"activity.group_search\"` and the `\"group_read\"` route to use the `/groups/{pubid}/{slug}` URL without overriding each other in unwanted ways. (Even if it is possible, this would be very brittle and not easily testable routing.)\r\n\r\nIn this pull request this has been reconciled by having the group activity search page views in `h.views.activity` share the `\"group_read\"` route with the other group views in `h.views.groups`.  The `\"activity.group_search\"` route is no longer used. The view configurations on the various views that use the `\"group_read\"` route have been tightened up to correctly and more explicitly declare which views should be called under which circumstances:\r\n\r\n* `h.views.activity.GroupSearchController` (the new group activity search page) is only called when the user is a member of the group and the `'search_page'` feature is on.\r\n\r\n  Once activity pages is released and the `'search_page'` feature flag is removed we can remove the `'search_page'` predicate from this.\r\n\r\n* `h.views.groups.read()` (the classic group home page) is only called when the user is a member of the group and the `search_page` feature is _not_ on.\r\n\r\n  Once activity pages is released and the `'search_page'` feature flag is removed we can delete this view function.\r\n\r\n* `h.views.groups.GroupJoinController()` is only called when the user is logged in and is _not_ a member of the group\r\n\r\n* `h.views.groups.read_unauthenticated()` is only called when the user is _not_ logged in.\r\n\r\n## Traversal\r\n\r\nThe `\"activity.group_search\"` route (which has been deleted) used [Pyramid's URL dispatch mechanism](http://docs.pylonsproject.org/projects/pyramid//en/latest/narr/urldispatch.html) to look up and call the view for a request.\r\n\r\nThe `\"group_read\"` route, which is now shared by the previous `\"group_read\"` views and the views that used to use the `\"activity.group_search\"` route, uses [Pyramid's traversal mechanism](http://docs.pylonsproject.org/projects/pyramid//en/latest/narr/hellotraversal.html).\r\n\r\nSo the group activity search page views in `h.views.activity.GroupSearchController` now use traversal not URL dispatch, to match with the existing `\"group_read\"` views.\r\n\r\nThis means that the views no longer need to look up the group in the database, nor deal with the case when the group doesn't exist, because traversal handles this for them.\r\n\r\n## Known issues\r\n\r\n* The user activity page's route is called `\"activity.user_search\"` whereas the group activity page's route is `\"group_read\"`.\r\n\r\n* The views for the `\"group_read\"` route are spread across two modules `h.views.activity` and `h.views.groups`.\r\n\r\n  `h.views.activity.GroupSearchController` shares the `\"group_read\"` route with the other group views in `h.views.groups`. On the other hand, it shares much code with the other activity search views in `h.views.activity`.\r\n\r\nIn a follow-up pull request I think these issues could be resolved by:\r\n\r\n* Moving all the group views from `h.views.groups` into `h.views.activity` and renaming the `\"group_read\"` route to `\"activity.group_search\"` to match with `\"activity.user_search\"`. Or\r\n\r\n* Move `h.views.activity.GroupSearchController` into `h.views.groups` so that all of the group views are together in one place.  Also move `h.views.activity.UserSearchController` into a new `h.views.users` module to mirror this organisation.\r\n\r\n  `GroupSearchController` and `UserSearchController` would both still inherit from `h.views.activity.SearchController`, which is implemented in a different module from either of the inheriting classes, but maybe that's okay.\r\n\r\n  Several shared private helper functions in `h.views.activity` would also need to be made available to `GroupSearchController` and `UserSearchController` which are now in different files. Making these helper functions into methods of `SearchController` might work, or move them into a helpers module.\r\n"},"comment":{"url":"https://api.github.com/repos/hypothesis/h/issues/comments/263841372","html_url":"https://github.com/hypothesis/h/pull/4139#issuecomment-263841372","issue_url":"https://api.github.com/repos/hypothesis/h/issues/4139","id":263841372,"user":{"login":"seanh","id":22498,"avatar_url":"https://avatars.githubusercontent.com/u/22498?v=3","gravatar_id":"","url":"https://api.github.com/users/seanh","html_url":"https://github.com/seanh","followers_url":"https://api.github.com/users/seanh/followers","following_url":"https://api.github.com/users/seanh/following{/other_user}","gists_url":"https://api.github.com/users/seanh/gists{/gist_id}","starred_url":"https://api.github.com/users/seanh/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/seanh/subscriptions","organizations_url":"https://api.github.com/users/seanh/orgs","repos_url":"https://api.github.com/users/seanh/repos","events_url":"https://api.github.com/users/seanh/events{/privacy}","received_events_url":"https://api.github.com/users/seanh/received_events","type":"User","site_admin":false},"created_at":"2016-11-30T10:42:41Z","updated_at":"2016-11-30T10:42:41Z","body":"Rebased and ready for review"}}
{"actor":{"display_login":"seanh"}}