Fix conviction check to handle grace window #296
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Cruikshanks
force-pushed
the
fix-blocked-actions
branch
from
028ffe8
to
a7681ee
Compare
irisfaraway
requested changes
Nov 6, 2018
app/models/concerns/waste_carriers_engine/can_change_registration_status.rb
Outdated
Show resolved
Hide resolved
| expect(registration.metaData).to_not allow_event :renew | ||
| let(:registration) { build(:registration, :is_active, expires_on: Date.today) } | ||
|
|
||
| it "cannot be renewed in 3 days time" do |
There was a problem hiding this comment.
Maybe this is personal preference, but I think this might be clearer in a separate context – I'd prefer "when the registration expiration date was three days ago, it cannot be renewed today" to "when the registration expiration date is today, it cannot be renewed in 3 days' time".
| close_to_expiry_date? && should_not_be_expired? | ||
| end | ||
|
|
||
| def renew_expired_registration? | ||
| return true if in_expiry_grace_window? | ||
|
|
||
| # The only time an expired registration can be renewed is if the application has previously been submitted, | ||
| # or it is withion the grace window - otherwise expiry is an automatic no | ||
| return false if EXPIRED? |
There was a problem hiding this comment.
Now that this isn't followed by further checks, there's nothing to return true if it's not EXPIRED? – I think we would get nil instead. Which makes me wonder if the tests are covering this.
https://eaflood.atlassian.net/browse/WC-512 During testing of the grace window it was spotted that if a renewal was expired, and it matched during conviction checks, then it could not be continued because the button to access convictions approvals did not display. This was because `TransientRegistration.pending_manual_conviction_check?` had a guard clause that checked the status was `ACTIVE`. This will no longer be the case for registrations which are EXPIRED but in the grace window, hence we change it to use `may_renew?`. We also learnt doing this that `may_renew?` will only work if called on a `metaData` object that is linked to a registration, because of the call to `registration.identifier` in `CanChangeRegistrationStatus.renewal_application_submitted?`. Hence we had to tweak the tests and code to account for this. Finally a test in the transient registration spec flagged that it was possible to for `may_renew?` to return true even if the registration in question was REVOKED. Hence a new guard in `CanChangeRegistrationStatus.renewal_allowed?` to check that the status of the registration is one that can be renewed. 🎉 WIP 🎉
Believe the issue is that locally I have the grace window set, but in travis its set to 0. Hence tests that expect a registration that is expired to not renew work in travis, but not for me locally. These changes should ensure we are consistent across all environments.
Cruikshanks
force-pushed
the
fix-blocked-actions
branch
from
a7681ee
to
45ee7c8
Compare
Now we have rubocop running, we can see what needs to be fixed. This covers minor things and does not add any exceptions.
As was pointed out in the PR, the check this line was doing is covered by the `transitions from:` line in the `:renew event`.
Left a bit of a cliff hanger on one comment, plus reference to the reg expiring is not actually true, it's the fact it cannot be renewed we are interested in. So a bit of tidying up essentially.
Cruikshanks
force-pushed
the
fix-blocked-actions
branch
from
fde7d95
to
33d46ef
Compare
Admittedly `renew_expired_registration?` was added to keep CodeClimate quiet, but the PR feedback rightly spotted that it wasn't returning a result in all cases.
From my read of the code the test that would cause us to miss all the previous logic gates in the method (and therefore return nil) was spec/models/waste_carriers_engine/registration_spec.rb:447 ("when the registration expiration date is less than 6 months away").
Debugging found this did cause nil to be returned, but the tests were still passing because nil == false i.e. we got the same result.
The issue was the name of the method did not really make it clear what it was doing, hence the logic inside didn't really make sense. After struggling to come up with a name and logic that made sense, and didn't break anything I instead came to the conclusion that CodeClimate was wrong in this case and leaving `renewal_allowed?` as it was before was the best solution.
Cruikshanks
force-pushed
the
fix-blocked-actions
branch
from
33d46ef
to
00cca0d
Compare
|
N.B. removing the unnecessary check in |
irisfaraway
approved these changes
Nov 7, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
https://eaflood.atlassian.net/browse/WC-512
During testing of the grace window it was spotted that if a renewal was expired, and it matched during conviction checks, then it could not be continued because the button to access convictions approvals did not display.
This was because
TransientRegistration.pending_manual_conviction_check?had a guard clause that checked the status wasACTIVE. This will no longer be the case for registrations which are EXPIRED but in the grace window, hence we change it to usemay_renew?.We also learnt doing this that
may_renew?will only work if called on ametaDataobject that is linked to a registration, because of the call toregistration.identifierinCanChangeRegistrationStatus.renewal_application_submitted?. Hence we had to tweak the tests and code to account for this.Finally a test in the transient registration spec flagged that it was possible to for
may_renew?to return true even if the registration in question was REVOKED. Hence a new guard inCanChangeRegistrationStatus.renewal_allowed?to check that the status of the registration is one that can be renewed.