Skip to content
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

Refactor landlord-immigration-check #2550

Merged
merged 21 commits into from Jun 2, 2016

Conversation

@floehopper
Copy link
Contributor

commented May 25, 2016

Description

I started work on this with the aim of reducing the duplication in the postcode lookups in LandlordImmigrationCheckCalculator and BenefitCapCalculatorConfiguration as per this Trello card. However, as a pre-cursor to that I've ended up refactoring the code towards the new-style and simplifying the LandlordImmigrationCheckCalculator considerably.

External changes

None. This is just an internal refactoring.

@chrisroos chrisroos self-assigned this Jun 2, 2016

@chrisroos

This comment has been minimized.

Copy link
Contributor

commented Jun 2, 2016

Looks good to me, @floehopper.

floehopper added 21 commits May 25, 2016
Test LandlordImmigrationCheckCalculator#areas_for_postcode with inval…
…id postcode

Previously this test was just testing `GdsApi::Imminence#areas_for_postcode`.
While the test was useful from an illustrative point of view, I don't think it
belongs in this test case which is supposed to be testing the
`LandlordImmigrationCheckCalculator` class.

I think it's more useful to test
`LandlordImmigrationCheckCalculator#areas_for_postcode` and so I've made that
method public and converted the test to do just that.
Use GdsApi::Response#to_ostruct vs #to_hash
The former is invoked implicitly via `GdsApi::Response#method_missing` when
calling `#results`. The `OpenStruct` form of the results is more convenient than
the `Hash` form and makes this code more consistent with the code in
`BenefitCapCalculatorConfiguration#area`.
Simplify LandlordImmigrationCheckCalculator#areas_for_postcode
Even when the underlying HTTP request responds with a 404,
`GdsApi::Response#code` seems to return 200, so the ternary condition is always
true.

As far as I can see, the `GdsApi::Response` returned by
`GdsApi::Imminence#areas_for_postcode` always has a `results` entry containing
an array of areas. In the case of an unknown postcode, even though the
underlying HTTP request responds with a 404, the `results` entry contains an
empty array.

Thus there is no need for the ternary expression and the code can be simplified
accordingly.

The following terminal output demonstrates what happens when using
`GdsApi::Imminence#areas_for_postcode` against the production instance of
the Imminence app, firstly where the postcode is known and secondly where it is
not know:

    $ PLEK_SERVICE_IMMINENCE_URI='https://imminence.publishing.service.gov.uk' rails c
    Loading development environment (Rails 4.2.6)

    irb(main):001:0> Services.imminence_api.areas_for_postcode('B1 1PW').to_ostruct
    => #<OpenStruct _response_info=#<OpenStruct status="ok", links=[]>, total=4, start_index=1, page_size=4, current_page=1, pages=1, results=[#<OpenStruct slug="birmingham-borough-council", name="Birmingham Borough Council", country_name="England", type="MTD", codes=#<OpenStruct gss="E08000025">>, #<OpenStruct slug="west-midlands", name="West Midlands", country_name="England", type="EUR", codes=#<OpenStruct gss="E15000005">>, #<OpenStruct slug="ladywood", name="Ladywood", country_name="England", type="MTW", codes=#<OpenStruct gss="E05001193">>, #<OpenStruct slug="birmingham-ladywood", name="Birmingham, Ladywood", country_name="England", type="WMC", codes=#<OpenStruct gss="E14000564">>]>

    irb(main):002:0> Services.imminence_api.areas_for_postcode('E15').to_ostruct
    => #<OpenStruct _response_info=#<OpenStruct status=404, links=[]>, total=0, start_index=1, page_size=0, current_page=1, pages=1, results=[]>

 You can see that in both cases the entry for `results` contains an array.
Remove unnecessary invocation of `Object#try`
As far as I can see `GdsApi::Imminence#areas_for_postcode` always returns an
instance of `GdsApi::Response`, and the latter always implements a `#code`
method. So we can simplify the code by removing this call to `Object#try`.
LandlordImmigrationCheckCalculator#areas_for_postcode should return a…
…rray

If the ternary condition is false, then we should return an empty array and not
an empty hash, because the former is what clients of this method are expecting.
e.g. `LandlordImmigrationCheckCalculator#postcode_within?`.

Note that the reason why the unit tests were all passing was because the ternary
condition is never false (even when the postcode is unknown), but I wanted to
fix the code before removing it in a later commit, so that the git history makes
more sense.
Inline local variable in LandlordImmigrationCheckCalculator
The `response` local variable is no longer serving much purpose.
Improve name of tests in LandlordImmigrationCheckCalculatorTest
I think the first of these scenarios is better explained as having an "unknown"
postcode rather than an "invalid" one. It strikes me that "E15" is at least in
some way a "valid" postcode, but it does not exist - hence the 404.

Similarly, I don't think it's particularly useful to describe the other
postcodes as "valid".
Rename LandlordImmigrationCheckCalculator#included_country?
Prior to 1st Dec 2014 the rules only applied in the West Midlands. Since
1st Feb 2016 the rules have only applied in England. As far as the flow is
concerned it only needs to know whether the rules apply for the given postcode.
It doesn't need to know why. I think using `rules_apply?` instead of
`included_country?` better hides the implementation.
Rename LandlordImmigrationCheckCalculator::VALID_COUNTRIES
I don't think the word "valid" is useful here - it's not as if there is a
validation error if the country is not in this list.
Refactor LandlordImmigrationCheckCalculator#rules_apply?
I don't think the `postcode_within?` method was a useful abstraction. I think
`countries_for_postcode` is a more useful abstraction.

Although the rules may at some point apply in other countries within the UK,
while they only apply in England, I think it's better not to worry about
multiple countries and keep the code simpler.

Note that this commit highlighted to me the fact that there may be edge cases
where a postcode is in multiple areas, some of which are in England and some of
which are not. This may be possible, because the areas are of varying sizes. At
the moment the code will consider a postcode to be in England if any of its
areas are in England.

This commit does not change the behaviour at all, but I thought it was worth
mentioning. An obvious way to fix this would be to only consider areas of the
country type ("CTR"), however, it doesn't look as if Imminence returns such
areas.
Simplify LandlordImmigrationCheckCalculator#countries_for_postcode
Using the `country_name` method provided by `OpenStruct` is simpler than the
Hash-lookup approach.
Use `on_response` block in LandlordImmigrationCheckFlow
This brings the flow into line with the new-style approach used in
`PartYearProfitTaxCreditsFlow`.
Build LandlordImmigrationCheckCalculator in test setup
This reduces duplication in the test and improve the signal-to-noise ratio.
Remove unnecessary tests from LandlordImmigrationCheckCalculatorTest
Although at some point, the rules may apply in countries outside England, I'm
pretty sure this isn't imminent and so I'd prefer ignore that possibility for
now and keep the code as simple as possible for the current requirements.
Use contexts in LandlordImmigrationCheckCalculatorTest
I think this makes the test easier to read.
Stub at higher level of abstraction in LandlordImmigrationCheckCalcul…
…atorTest

While it was initially useful to stub at the HTTP request level, I'm now
confident that I understand the behaviour of
`GdsApi::Imminence#areas_for_postcode` well enough that I'm happy to stub at
this higher level of abstraction.

I think this makes the test more readable.
Update regression test artefacts for landlord-immigration-check
I'm happy to do this in a single commit rather than in each commit which makes
changes to the relevant files, because none of the changes affect the behaviour
of the flow. All the regression tests are still passing.
Re-order the tests in LandlordImmigrationCheckCalculatorTest
I think this is a more natural order for the tests.
Improve coverage in LandlordImmigrationCheckCalculatorTest
Typically Imminence returns multiple areas for a given postcode, e.g. an area
for the local borough council and another for the county council. It is
_theoretically_ possible that not all of these areas are in the same country,
because they each have their own `country_name` attribute.

If this does happen, then we consider the postcode to be in England if *any* of
the areas are in England. I think this is probably good enough, but I thought it
was worth highlighting the behaviour in a test.
@floehopper

This comment has been minimized.

Copy link
Contributor Author

commented Jun 2, 2016

@chrisroos: Thanks for reviewing.

Rebasing and force-pushing in preparation for merging.

@floehopper floehopper force-pushed the refactor-landlord-immigration-check branch from 25895c8 to 6650b7b Jun 2, 2016

@floehopper floehopper merged commit af5996d into master Jun 2, 2016

1 check passed

default "Build #5286 succeeded on Jenkins"
Details

@floehopper floehopper deleted the refactor-landlord-immigration-check branch Jun 2, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.