-
Notifications
You must be signed in to change notification settings - Fork 0
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
CRM457-2303: Caseworker part 1 fixes #867
Conversation
Just a heads up it looks like some of these fixes clash with what's on #865 so that will have to be rebased |
Quality Gate passedIssues Measures |
@@ -102,7 +102,7 @@ def data_for_calculation | |||
private | |||
|
|||
def youth_court_fee_claimed | |||
data.fetch('include_youth_court_fee_original', data['include_youth_court_fee']) | |||
data['include_youth_court_fee_original'] || data['include_youth_court_fee'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Be aware that if this is false, it'll check the other value. I don't think it's what you want to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true (which means somewhere also this is untested - will fix this to be more specific, tbh I'm pretty sure rubocop auto-changed this from code where I check specifically if data['include_youth_court_fee_original'] is nil using .nil?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is only a problem if include_youth_court_fee_original
is false but include_youth_court_fee
is true, i.e. if the provider didn't claim the fee but the caseworker awarded it to them anyway. At the moment this can't bite us because I don't think we intend to give caseworkers functionality do that, but it would be a kindness to our future selves to do this properly just in case requirements are added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is being handled in #865 (for our future selves)
Description of change
Link to relevant ticket
Notes for reviewer