{"action":"created","issue":{"url":"https://api.github.com/repos/mysociety/alaveteli/issues/2969","labels_url":"https://api.github.com/repos/mysociety/alaveteli/issues/2969/labels{/name}","comments_url":"https://api.github.com/repos/mysociety/alaveteli/issues/2969/comments","events_url":"https://api.github.com/repos/mysociety/alaveteli/issues/2969/events","html_url":"https://github.com/mysociety/alaveteli/pull/2969","id":123658636,"number":2969,"title":"Refactor RequestController#show_response into new controller","user":{"login":"lizconlan","id":27760,"avatar_url":"https://avatars.githubusercontent.com/u/27760?v=3","gravatar_id":"","url":"https://api.github.com/users/lizconlan","html_url":"https://github.com/lizconlan","followers_url":"https://api.github.com/users/lizconlan/followers","following_url":"https://api.github.com/users/lizconlan/following{/other_user}","gists_url":"https://api.github.com/users/lizconlan/gists{/gist_id}","starred_url":"https://api.github.com/users/lizconlan/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/lizconlan/subscriptions","organizations_url":"https://api.github.com/users/lizconlan/orgs","repos_url":"https://api.github.com/users/lizconlan/repos","events_url":"https://api.github.com/users/lizconlan/events{/privacy}","received_events_url":"https://api.github.com/users/lizconlan/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/mysociety/alaveteli/labels/5%20-%20awaiting%20review","name":"5 - awaiting review","color":"eb6420"}],"state":"open","locked":false,"assignee":{"login":"garethrees","id":282788,"avatar_url":"https://avatars.githubusercontent.com/u/282788?v=3","gravatar_id":"","url":"https://api.github.com/users/garethrees","html_url":"https://github.com/garethrees","followers_url":"https://api.github.com/users/garethrees/followers","following_url":"https://api.github.com/users/garethrees/following{/other_user}","gists_url":"https://api.github.com/users/garethrees/gists{/gist_id}","starred_url":"https://api.github.com/users/garethrees/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/garethrees/subscriptions","organizations_url":"https://api.github.com/users/garethrees/orgs","repos_url":"https://api.github.com/users/garethrees/repos","events_url":"https://api.github.com/users/garethrees/events{/privacy}","received_events_url":"https://api.github.com/users/garethrees/received_events","type":"User","site_admin":false},"milestone":null,"comments":2,"created_at":"2015-12-23T13:23:19Z","updated_at":"2016-01-18T16:12:53Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/mysociety/alaveteli/pulls/2969","html_url":"https://github.com/mysociety/alaveteli/pull/2969","diff_url":"https://github.com/mysociety/alaveteli/pull/2969.diff","patch_url":"https://github.com/mysociety/alaveteli/pull/2969.patch"},"body":"Reworks the `RequestController#show_response` action into the `new_followup` (GET), `preview_followup`, and `create_followup` (POST) actions of `ResponseController`. These actions are still known as `show_response` and `preview_response` in the routes file in an effort to minimise the disruption to themes (although as view files have moved and/or been renamed, maybe this isn't as useful as I first thought).\r\n\r\nThere is potential both to refactor more controller actions into `RequestController` to make it more generally useful, or to rename it to `FollowupController` and simplify the existing actions to merely `new`, `preview` and `create`.\r\n\r\nAs a preliminary step to refactoring, I have also rewritten the message/alert sending sections of the `RequestController` specs as `spec/integration/request_mailer_spec.rb` which hopefully removes the need for `ApplicationController.test_code_redirect_by_email_token` although I have left it in place for now.\r\n\r\nCloses #2868 "},"comment":{"url":"https://api.github.com/repos/mysociety/alaveteli/issues/comments/172573536","html_url":"https://github.com/mysociety/alaveteli/pull/2969#issuecomment-172573536","issue_url":"https://api.github.com/repos/mysociety/alaveteli/issues/2969","id":172573536,"user":{"login":"garethrees","id":282788,"avatar_url":"https://avatars.githubusercontent.com/u/282788?v=3","gravatar_id":"","url":"https://api.github.com/users/garethrees","html_url":"https://github.com/garethrees","followers_url":"https://api.github.com/users/garethrees/followers","following_url":"https://api.github.com/users/garethrees/following{/other_user}","gists_url":"https://api.github.com/users/garethrees/gists{/gist_id}","starred_url":"https://api.github.com/users/garethrees/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/garethrees/subscriptions","organizations_url":"https://api.github.com/users/garethrees/orgs","repos_url":"https://api.github.com/users/garethrees/repos","events_url":"https://api.github.com/users/garethrees/events{/privacy}","received_events_url":"https://api.github.com/users/garethrees/received_events","type":"User","site_admin":false},"created_at":"2016-01-18T16:12:53Z","updated_at":"2016-01-18T16:12:53Z","body":"As for some general notes on reviewing a big refactoring PR like this:\r\n\r\n- Would have been easier if big (> 10 LOC) extractions were first extracted as one commit, then changed. The combination of extract and change in one commit is difficult to review, because it's hard to see where behaviour has changed, and whether that behaviour change matches the original.\r\n- Better commit messages are super helpful. For example, I don't really know the context of what was broken in (d894a4f7282), so I don't know why its being \"fixed\". Was it broken before the branch was started, or was the breakage introduced in the branch? What was happening because of the breakage that shouldn't have been happening?\r\n- Some specs have been removed without comment. I don't know if thats a mistake or intentional. If the latter, why do we no longer need them?\r\n- More of a preference point, but for anything more than 3 filter actions, I find it easier to follow if the symbols are listed one per line. Its also nice if the actual method definitions is in the same order as the filters, so that you can read down through the methods and understand the filter chain in one reading.\r\n- Again, more style/preference but I try to avoid calling instance variables in helper methods as much as possible. I find it better to pass them in as method arguments so that you know what dependencies the methods have. This is obviously not possible with filter methods, which is why keeping them simple seems better.\r\n- Specs should pass on each commit, because it makes it easier to verify that the commit's changes are working as intended. Would be really nice for Travis to check this."}}