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.

Sign up for GitHub

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

Jump to bottom

Add rake task to set the status of expired registration exemptions #255

Merged
merged 6 commits into from
Jul 5, 2019
Merged

Add rake task to set the status of expired registration exemptions #255

merged 6 commits into from
Jul 5, 2019

Conversation

cintamani
Copy link
Contributor

From https://eaflood.atlassian.net/browse/RUBY-413

This adds a rake task and a service object to deal with setting the status of a registration exemption to expired when it is in an active status and the expiry date is in the past.
After talking about expiring on the day of expiring or the day before, we have decided that the registration is considered expired after the date has passed, hence this include a fix on our logic for "not_expired" registrations too.
One more thing part of this PR is the definition of the RegistrationExemption table name which allows us to use the AASM default defined scopes.

@cintamani
Add rake task to set the status of expired registration exemptions
2f09975 
From https://eaflood.atlassian.net/browse/RUBY-413

This adds a rake task and a service object to deal with setting the status of a registration exemption to expired when it is in an active status and the expire date is in the past.
After talking about expiring on the day of expire or the day before, we have decided that the registration is considered expired after the date have passed, hence this include a fix on our logic for "not_expired" registrations too.
One more thing part of this PR is the definition of the RegistrationExemption table name which allow us to use the AASM default defined scopes.
@cintamani cintamani added the enhancement New feature or request label Jul 2, 2019
@cintamani cintamani self-assigned this Jul 2, 2019
@cintamani cintamani mentioned this pull request Jul 2, 2019
Merged
irisfaraway
irisfaraway previously requested changes Jul 2, 2019
View reviewed changes
app/services/expired_registrations_service.rb Outdated Show resolved Hide resolved
app/models/waste_exemptions_engine/registration_exemption.rb Outdated Show resolved Hide resolved
@Cruikshanks
Copy link
Member

Cruikshanks commented Jul 2, 2019 •
edited

I would like to see https://eaflood.atlassian.net/browse/RUBY-455 get decided on as a priority before we approve and merge this.

Essentially I disagree with the current thinking that an exemption is still ACTIVE on its expiry date, as this

  • contradicts what we seem to say to users in our content
  • would mean Waste exemptions definition of expires_on differs from Waste Carriers, and I think they should be the same

So I'd like to see this bottomed out before add more code that depends on this decision.

@cintamani
Copy link
Contributor Author

It totally makes sense. Will park this for now until the next session with the team.

@cintamani cintamani changed the title Add rake task to set the status of expired registration exemptions [BLOCKED] Add rake task to set the status of expired registration exemptions Jul 2, 2019
@cintamani cintamani changed the title [BLOCKED] Add rake task to set the status of expired registration exemptions Add rake task to set the status of expired registration exemptions Jul 4, 2019
@cintamani
Copy link
Contributor Author

We discussed and agreed that the rules for "expired" and "not_expired" in this PR are good to go. We will work around the issue of EPR export by scheduling the job at the correct time for the live release. Open issue is around the overriding of the expired scope.

@cintamani cintamani dismissed irisfaraway’s stale review July 5, 2019 10:07

This is ready for review :) I didn't move the scope in the AASM file after all, I thought is better to keep it in the model

irisfaraway
irisfaraway approved these changes Jul 5, 2019
View reviewed changes
Copy link
Member

@irisfaraway irisfaraway left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Maybe we should hold off on merging until all the release prep is done though.

@cintamani
Copy link
Contributor Author

Merging this as the version that will go out with the release have already been tagged :)

@cintamani cintamani merged commit 63d6982 into master Jul 5, 2019
@cintamani cintamani deleted the 413-expire-registration branch July 5, 2019 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@irisfaraway irisfaraway irisfaraway approved these changes

@Cruikshanks Cruikshanks Awaiting requested review from Cruikshanks

Assignees

@cintamani cintamani

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
3 participants
@cintamani @Cruikshanks @irisfaraway