Add a grace window for expired regs to renew #290
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
add-grace-window-expired-regs
branch
from
875a663
to
bc4d527
Compare
Pinpointed where in the exisiting specs to add our unit tests for the 'grace window'.
We need to add config so that the renewals engine understands what the grace window is, and with this in place we can also bring it into our unit tests. The inside grace window still fails, but at least we have a failing test to work with.
Update the `CanChangeRegistrationStatus` concern so that it knows about the grace window and uses this knowledge in its determination as to whether something can be renewed.
Cruikshanks
force-pushed
the
add-grace-window-expired-regs
branch
from
bc4d527
to
9932ac6
Compare
The grace window changes essentilly means simply marking a registration as `EXPIRED` no longer means it is expired. So a number of tests that check you are redirected to a unrenewable page at various stages of the journey started failing because in the setup all they did was call `registration.metaData.expire!`. This change also ensures that the expiry date is set to something that is outside the grace window, so our new code that determines whether something can be renewed returns false.
|
@irisfaraway Fixed my failing tests, and they all needed fixing in the same way. I'm happy I've kept in keeping with what the test was trying to do, but got a niggle in the back of my mind that the way I've solved the problem might not be the best. Let me know if you think there is a better way. |
irisfaraway
requested changes
Nov 2, 2018
app/models/concerns/waste_carriers_engine/can_change_registration_status.rb
Outdated
Show resolved
Hide resolved
app/models/concerns/waste_carriers_engine/can_change_registration_status.rb
Outdated
Show resolved
Hide resolved
spec/requests/waste_carriers_engine/renewal_start_forms_spec.rb
Outdated
Show resolved
Hide resolved
Perfectly valid feedback that we should be splitting the existing logic into 2 lines; work out what the end date of the grace window is, then compare this against the current date.
In my rush to ship I added the check for whether a renewal is in the grace window to the `should_not_be_expired?` method. However as was rightly pointed our in the PR > This method was initially intended to check whether the registration's status should be 'EXPIRED' and would be changed as soon as the scheduled job hit it, thus the concept of "should be expired". If a registration was in the grace window, it should still be expired it just happens it can also be renewed. Thankfully a simply fix, and much betterv place for the grace window check was to move it to the renewal_allowed? method.
Spotted that `CanChangeRegistrationStatus.close_to_expiry_date?` was not using `expiry_time_adjusted_for_daylight_savings.to_date` to determine the expiry date before then using it in the logic. It should be, so corrected under the 'boy scout rule'.
Found a test that was broken first by the order of checks in `renewal_allowed?`, but also when I started debugging found we should also be checking in `in_expiry_grace_window?` that the current day was not just before the last day of the grace window, but that it also came after the expiry date of the registration. Also had a funny with the grace window being 0, so updated tests to account for this.
Took on the suggestion in the PR feedback to simplify how we denote expired registrations by using a one liner that sets the `expires_on` date outside of the grace window.
irisfaraway
approved these changes
Nov 5, 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
Currently when the registration expires there is no mechanism to still start the renewal. Our NCCC team have made a strong case that this is problematic for a number of reasons, and as such they are finding they are having to implement a workaround which involves
The reasons given for still permitting a user to renew are
So this change is about still allowing registrations which have expired to renew, as long as that expiry falls within a predefined "grace window".