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

98249 Implement draft appointment controller method #20137

Merged
merged 14 commits into from
Jan 28, 2025

Conversation

randomsync
Copy link
Member

@randomsync randomsync commented Jan 7, 2025

Summary

Add a create_draft appointment method, which creates a draft appointment with EPS, gets additional data like provider, drive times, and available slots and returns the combined payload.

Related issue(s)

department-of-veterans-affairs/va.gov-team#98249

Testing done

  • New code is covered by unit tests

Acceptance criteria

  • I fixed|updated|added unit tests and integration tests for each feature (if applicable).
  • No error nor warning in the console.

@va-vfs-bot va-vfs-bot temporarily deployed to 98249_appt_draft_controller/main/main January 7, 2025 19:58 Inactive
@va-vfs-bot va-vfs-bot temporarily deployed to 98249_appt_draft_controller/main/main January 17, 2025 00:31 Inactive
@va-vfs-bot va-vfs-bot temporarily deployed to 98249_appt_draft_controller/main/main January 17, 2025 23:54 Inactive
@va-vfs-bot va-vfs-bot temporarily deployed to 98249_appt_draft_controller/main/main January 18, 2025 00:07 Inactive
@randomsync randomsync marked this pull request as ready for review January 18, 2025 00:18
@randomsync randomsync requested review from a team as code owners January 18, 2025 00:18
@randomsync randomsync requested a review from a team January 18, 2025 00:19
@randomsync randomsync changed the title 98249 Implement draft appointment controller method (WIP) 98249 Implement draft appointment controller method Jan 20, 2025
@va-vfs-bot va-vfs-bot temporarily deployed to 98249_appt_draft_controller/main/main January 20, 2025 21:49 Inactive
kanchanasuriya
kanchanasuriya previously approved these changes Jan 20, 2025
Copy link
Contributor

@kanchanasuriya kanchanasuriya left a comment

Choose a reason for hiding this comment

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

LGTM

@stevenjcumming
Copy link
Contributor

@randomsync there's a failing test. Possibly a flakey test, please re-run the Code Check - Test until it passes.

stevenjcumming
stevenjcumming previously approved these changes Jan 21, 2025
Comment on lines +69 to +72
draft_appointment = eps_appointment_service.create_draft_appointment(referral_id: draft_params[:referral_id])

provider = eps_provider_service.get_provider_service(provider_id: draft_params[:provider_id])

Copy link
Contributor

Choose a reason for hiding this comment

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

You could also make these private methods (with or without memoization as needed)

private 

def draft_appointment
  eps_appointment_service.create_draft_appointment(referral_id: draft_params[:referral_id])
end

def provider 
  eps_provider_service.get_provider_service(provider_id: draft_params[:provider_id])
end

@va-vfs-bot va-vfs-bot temporarily deployed to 98249_appt_draft_controller/main/main January 21, 2025 18:16 Inactive
@va-vfs-bot va-vfs-bot temporarily deployed to 98249_appt_draft_controller/main/main January 21, 2025 18:53 Inactive
Copy link
Contributor

@lee-delarm6 lee-delarm6 left a comment

Choose a reason for hiding this comment

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

Looks good, but maybe more tests for nil/empty objects or params.

Will those TODOs at the top be covered in this PR or another one?

@randomsync
Copy link
Member Author

randomsync commented Jan 22, 2025

Looks good, but maybe more tests for nil/empty objects or params.

Will those TODOs at the top be covered in this PR or another one?

The other TODOs also have corresponding tickets that will be worked on separately. One of them especially (referrals validation) cannot be done until referrals part is done.

Copy link
Contributor

@devin-mccurdy devin-mccurdy left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@lee-delarm6 lee-delarm6 left a comment

Choose a reason for hiding this comment

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

More tests will be added in another PR Gaurav has said, so good to go

@va-vfs-bot va-vfs-bot temporarily deployed to 98249_appt_draft_controller/main/main January 24, 2025 21:00 Inactive
@randomsync randomsync requested a review from a team January 28, 2025 19:38
@randomsync randomsync merged commit 8327078 into master Jan 28, 2025
26 checks passed
@randomsync randomsync deleted the 98249_appt_draft_controller branch January 28, 2025 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants