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

Support individual exemption deregistration #90

Merged
merged 38 commits into from
Mar 29, 2019
Merged

Support individual exemption deregistration #90

merged 38 commits into from
Mar 29, 2019

Conversation

eminnett
Copy link
Contributor

https://eaflood.atlassian.net/browse/RUBY-63

Add functionality to allow back office users to deregister individual exemptions. The feature includes the following functionality:

  • users can choose to ‘cease’ or ‘revoke’ an exemption
  • users must provide a reason before deregistering
  • only active exemptions can be deregistered
  • only super agents can do this

Support individual exemption deregistration
0a5a872 
https://eaflood.atlassian.net/browse/RUBY-63

Add functionality to allow back office users to deregister individual exemptions. The feature includes the following functionality:

- users can choose to ‘cease’ or ‘revoke’ an exemption
- users must provide a reason before deregistering
- only active exemptions can be deregistered
- only super agents can do this
@eminnett eminnett added the enhancement New feature or request label Mar 15, 2019
@eminnett eminnett self-assigned this Mar 15, 2019
Edward Minnett and others added 19 commits March 18, 2019 16:11
Merge branch 'master' into support-individual-exemption-deregistration
8f10837
Merge branch 'master' into support-individual-exemption-deregistration
b8d2428
Merge branch 'master' into support-individual-exemption-deregistration
d0aaba8
Merge branch 'master' into support-individual-exemption-deregistration
55dd74d
Merge branch 'master' into support-individual-exemption-deregistration
cd7ee78
Merge branch 'master' into support-individual-exemption-deregistration
fffa6a6
Bump dependency versions
e153844
Add input SCSS partial from the front-office app
2d53006
Add CanDeactivateExemption state machine concern
495e387
Add deregistration service
33c046e
Add translation files
25117de
Add validator test shared exampled from the WEX engine
f6674b1
Add form validators
880ee5f
Add test helper modules
a3445d7
Add DeregisterExemptionsForm
0255b39
Update views to support the deregister exemptions journey
0fd4b8f
Add routes, controller, and request tests
fdecb0c
Refactor DeregisterExemptionsController
1d825cf
Fix typo
343453f
@eminnett eminnett mentioned this pull request Mar 25, 2019
Merged
irisfaraway
irisfaraway requested changes Mar 25, 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.

A handful of questions and suggestions - some of these we have talked about already (the CanCanCan stuff) but let me know what you think.

spec/models/registration_exemption_spec.rb Outdated Show resolved Hide resolved
app/services/deregistration_service.rb Outdated Show resolved Hide resolved
config/locales/forms/deregister_exemptions.en.yml Outdated Show resolved Hide resolved
app/forms/deregister_exemptions_form.rb Outdated Show resolved Hide resolved
spec/forms/deregister_exemptions_form_spec.rb Outdated Show resolved Hide resolved
app/controllers/deregister_exemptions_controller.rb Outdated Show resolved Hide resolved
app/controllers/deregister_exemptions_controller.rb Outdated Show resolved Hide resolved
@eminnett
Copy link
Contributor Author

eminnett commented Mar 25, 2019 •
edited

I've realised the old system includes an "exemption_deregister_date". I will add this column to the DB as well as set it via an AASM call back.

@eminnett
Copy link
Contributor Author

@irisfaraway I had to add some code to the User model to be able to call can? on the current user inside the de-registration service (commit ed42b41). Are you happy with how this is done?

irisfaraway
irisfaraway requested changes Mar 29, 2019
View reviewed changes
Copy link
Member

@irisfaraway irisfaraway left a comment •
edited

Choose a reason for hiding this comment

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

Two minor comments. I am happy with the change to User though.

The AWS stuff also seems a bit out of place on this branch – is this to fix the issue from the dependency update? If so I'd rather we did it as a separate PR, merged to master and then updated this branch. I think this will make the history a lot cleaner.

app/views/shared/_resource_exemptions_list.html.erb Outdated Show resolved Hide resolved
app/models/concerns/waste_exemptions_engine/can_deactivate_exemption.rb Outdated Show resolved Hide resolved
@eminnett
Copy link
Contributor Author

The AWS related commits are simply the product of me trying to debug the S3 errors in Travis. Since the errors only showed up in Travis, I had to push the trial and error fixes. None of them worked so reverted the changes.

Next time I'll do that debugging in a separate branch. Hopefully pull/105 will fix the problem as the errors aren't showing up there.

@irisfaraway
Copy link
Member

@eminnett Oops, yes, can see they were getting reverted now (time for more coffee, I think). I'm OK with debugging happening on the same branch but I guess my preference is for removing the commits afterwards. Fingers crossed that 105 solves it though 🤞

@eminnett
Copy link
Contributor Author

No problem at all.

irisfaraway
irisfaraway approved these changes Mar 29, 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.

Provisional approval for when the CI passes! (Give me a nudge if the review is voided and I'll hit the button again.)

@eminnett eminnett merged commit 197ba8f into master Mar 29, 2019
@eminnett eminnett deleted the support-individual-exemption-deregistration branch March 29, 2019 13:23
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

@eminnett eminnett

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
2 participants
@eminnett @irisfaraway