{"action":"created","issue":{"url":"https://api.github.com/repos/alphagov/smart-answers/issues/1856","labels_url":"https://api.github.com/repos/alphagov/smart-answers/issues/1856/labels{/name}","comments_url":"https://api.github.com/repos/alphagov/smart-answers/issues/1856/comments","events_url":"https://api.github.com/repos/alphagov/smart-answers/issues/1856/events","html_url":"https://github.com/alphagov/smart-answers/pull/1856","id":97938325,"number":1856,"title":"Refactor minimum-wage Smart Answers","user":{"login":"chrisroos","id":6556,"avatar_url":"https://avatars.githubusercontent.com/u/6556?v=3","gravatar_id":"","url":"https://api.github.com/users/chrisroos","html_url":"https://github.com/chrisroos","followers_url":"https://api.github.com/users/chrisroos/followers","following_url":"https://api.github.com/users/chrisroos/following{/other_user}","gists_url":"https://api.github.com/users/chrisroos/gists{/gist_id}","starred_url":"https://api.github.com/users/chrisroos/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/chrisroos/subscriptions","organizations_url":"https://api.github.com/users/chrisroos/orgs","repos_url":"https://api.github.com/users/chrisroos/repos","events_url":"https://api.github.com/users/chrisroos/events{/privacy}","received_events_url":"https://api.github.com/users/chrisroos/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":7,"created_at":"2015-07-29T13:40:46Z","updated_at":"2015-08-06T13:25:40Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/alphagov/smart-answers/pulls/1856","html_url":"https://github.com/alphagov/smart-answers/pull/1856","diff_url":"https://github.com/alphagov/smart-answers/pull/1856.diff","patch_url":"https://github.com/alphagov/smart-answers/pull/1856.patch"},"body":"I've made a number of changes to the minimum-wage shared logic that's used by both am-i-getting-minimum-wage and minimum-wage-calculator-employers.\r\n\r\nThere are quite a few commits but they're all fairly small and I hope the commit message explains the motivation.\r\n\r\nIn broad terms, these are the changes I've made:\r\n\r\n* Remove unused code:\r\n\r\n  * From unused `precalculate` blocks.\r\n  * From unused `save_input_as` values.\r\n  * From unused custom validation where the validation is already provided by the `money_question`.\r\n  * From parsing that's already handled by questions that use (e.g.) the `parse: Float` option.\r\n\r\n* Move formatting to templates, e.g. use `OutcomeHelper#format_money`.\r\n\r\n* Use calculator values in templates and remove no-longer necessary calculate blocks.\r\n\r\n* Use `validate` blocks instead of custom validation in calculate blocks. Add tests where they didn't exist previously.\r\n\r\n* Set calculator attributes in `next_node` blocks rather than `calculate` blocks whose return values aren't used.\r\n\r\n* Consistently use `next_node` blocks rather than the shortcuts available to `multiple_choice` questions.\r\n\r\n* Instantiate the calculator early on in the flow and then add each response to it as they come in, rather than instantiating the object in one go later on in the flow. I had to add accessors to the calculator to make this possible. Setting attributes directly on the calculator means that I don't have to pollute the state object in order to share variables within the flow.\r\n\r\n* Move validation logic out of the flow and into the calculator.\r\n\r\n* Change some tests that were asserting against state variables so that they now assert against values on the calculator. I started out deleting the tests that were asserting against state variables that no longer existed but then realised that it's possible I'd be losing some useful tests. These integration tests that now assert against the calculator should move to the calculator tests if they're deemed useful.\r\n\r\n__NOTE__. All but the first two questions appear twice, once for determining minimum wage for the current period and once for a previous period.\r\n\r\nHaving now gone through the process I can see a slightly better ordering of commits but trying to reorder the commits in this branch would be a bit painful and probably not worth doing.\r\n"},"comment":{"url":"https://api.github.com/repos/alphagov/smart-answers/issues/comments/128363011","html_url":"https://github.com/alphagov/smart-answers/pull/1856#issuecomment-128363011","issue_url":"https://api.github.com/repos/alphagov/smart-answers/issues/1856","id":128363011,"user":{"login":"chrisroos","id":6556,"avatar_url":"https://avatars.githubusercontent.com/u/6556?v=3","gravatar_id":"","url":"https://api.github.com/users/chrisroos","html_url":"https://github.com/chrisroos","followers_url":"https://api.github.com/users/chrisroos/followers","following_url":"https://api.github.com/users/chrisroos/following{/other_user}","gists_url":"https://api.github.com/users/chrisroos/gists{/gist_id}","starred_url":"https://api.github.com/users/chrisroos/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/chrisroos/subscriptions","organizations_url":"https://api.github.com/users/chrisroos/orgs","repos_url":"https://api.github.com/users/chrisroos/repos","events_url":"https://api.github.com/users/chrisroos/events{/privacy}","received_events_url":"https://api.github.com/users/chrisroos/received_events","type":"User","site_admin":false},"created_at":"2015-08-06T13:25:40Z","updated_at":"2015-08-06T13:25:40Z","body":"> I might be wrong, but I think the save_input_as :current_or_past_payments statement in what_would_you_like_to_check? is now redundant.\r\n\r\nI think you're right. I'll get rid of it."}}