-
Notifications
You must be signed in to change notification settings - Fork 16
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
[CAPT-2030] Fix EY started_at timestamp #3437
Conversation
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.
LGTM, left one question but it's not a blocker
def build_claim | ||
existing_or_new_claim.tap do |claim| | ||
claim.eligibility ||= main_eligibility | ||
claim.eligibility.practitioner_claim_started_at = journey_session.answers.practitioner_claim_started_at |
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.
claim.eligibility.practitioner_claim_started_at = journey_session.answers.practitioner_claim_started_at |
Do we need this line? Is this not handled by the set_eligibility_attributes
in the base class?
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.
- I just tried and is needed and evident with failing test
- the provider has already created the claim and eligibility at this time
- unlike the other journeys where claim and eligibility are created once
- therefore it skips
main_eligibility
on the line above (27) - which would have called
set_eligibility_attributes
which you mentioned
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.
tbh the existing code its a little bit spaghetti and a bit of a roundabout way. could probably delete most of the stuff and be explicit on what is needed, i think moving away from inheritance would help this
5cb441b
to
d525be5
Compare
Context