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

[CAPT-2030] Fix EY started_at timestamp #3437

Merged
merged 1 commit into from
Nov 28, 2024
Merged

Conversation

asmega
Copy link
Contributor

@asmega asmega commented Nov 27, 2024

Context

  • https://dfedigital.atlassian.net/browse/CAPT-2030
  • Fix EY claim started at timestamp which is being overwritten when the practitioner starts their half of the journey
  • Once deployed will need to retro populate incorrect data which I'll do by hand on the box

@asmega asmega added the deploy Deploy a review app for this PR label Nov 27, 2024
@asmega asmega marked this pull request as ready for review November 27, 2024 09:54
Copy link
Contributor

@rjlynch rjlynch left a 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
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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?

Copy link
Contributor Author

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

Copy link
Contributor Author

@asmega asmega Nov 28, 2024

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

@asmega asmega force-pushed the fix-ey-meta-timestamps branch from 5cb441b to d525be5 Compare November 28, 2024 16:34
@asmega asmega merged commit f06e6ae into master Nov 28, 2024
15 checks passed
@asmega asmega deleted the fix-ey-meta-timestamps branch November 28, 2024 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
business reviewed deploy Deploy a review app for this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants