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

Remove extra logging and fix order of events #859

Merged
merged 2 commits into from
Jun 10, 2020
Merged

Conversation

cintamani
Copy link
Contributor

This removes some unnecessary error raised, fix an issue with the order of events and adds an extra check before deciding if a registration can be activated or not.

This removes some unnecessary error raised, fix an issue with the order of events and adds an extra check before deciding if a registration can be activated or not.
@cintamani cintamani added the enhancement New feature or request label Jun 5, 2020
@cintamani cintamani self-assigned this Jun 5, 2020
Copy link
Member

@Cruikshanks Cruikshanks left a comment

Choose a reason for hiding this comment

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

No issue with the tweaks to the order of things. Plus agree we can drop throwing the exceptions when payment is outstanding or a conviction check is required.

But I think it would still be useful to at least log when we have tried to activate a registration and it has failed. How would you feel about instead of raising an exception we record something in the log instead?

@cintamani
Copy link
Contributor Author

If can_be_completed? is false we return rather than raise an error, and this is fine because any finance operation (and not only finance operation) will run that code without having a concern if the registration can actually be activated. That is the unnecessary logging I am removing.

If can_be_completed? is true, then we are saying "this registration is into a state we can activate". Given that an error. with sending an email is already custom logged, we are not worried about that. Other things that can go wrong are going to raise an error that will end up in airbrake for us to see as we are using ! methods to activate the registration. Hence, I don't think we need more logging.

Unless I didn't get what you meant, of course, as always XD

Copy link
Member

@Cruikshanks Cruikshanks left a comment

Choose a reason for hiding this comment

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

I was on the fence. I have been persuaded. Hit the button! 😁

@cintamani cintamani merged commit 9893811 into master Jun 10, 2020
@cintamani cintamani deleted the remove-extra-logging branch June 10, 2020 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
2 participants