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

[10-10CG] Add paginated facilities endpoint to CG controller #18560

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

coope93
Copy link
Contributor

@coope93 coope93 commented Sep 20, 2024

Summary

  • This work is behind a feature toggle (flipper): NO
  • The front end work that uses this endpoint is behind a flipper toggle, so the backend does not have the toggle.
  • The lighthouse api returns pagination information, but the existing Facilities/V1/Client did not return the full response object. This PR adds a method to the client to return the full response, as well as a new caregivers route that returns the facilities list with pagination metadata.
  • Update client to handle no data key being returned by the lighthouse api. This is technically possible if you request a page that is outside of the page range. My forthcoming vets-website changes will eliminate this issue from the front end, but the endpoint could technically be called with page 5000 for a result set of 1.

Related issue(s)

Testing done

  • New code is covered by unit tests
  • Added spec for Facilities/V1/Client

What areas of the site does it impact?

10-10CG

Acceptance criteria

  • I fixed|updated|added unit tests and integration tests for each feature (if applicable).
  • No error nor warning in the console.
  • Events are being sent to the appropriate logging solution
  • Documentation has been updated (link to documentation)
  • No sensitive information (i.e. PII/credentials/internal URLs/etc.) is captured in logging, hardcoded, or specs
  • Feature/bug has a monitor built into Datadog (if applicable)
  • If app impacted requires authentication, did you login to a local build and verify all authenticated routes work as expected
  • I added a screenshot of the developed feature

Requested Feedback

I updated our existing controller spec for this route. I would eventually like to replace that spec with a request spec, but at this moment I didn't want to take on the scope of that work unless someone really wants me to.

@@ -11,7 +11,7 @@ class CaregiversAssistanceClaimsController < ApplicationController
before_action :load_user, only: :create

before_action :record_submission_attempt, only: :create
before_action :initialize_claim
before_action :initialize_claim, only: [:create, :download_pdf]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The new facilities method does not require initializing a claim. These are the only other two exposed routes in this controller. I think this also makes this initialize_claim logic more obvious.


resources :caregivers_assistance_claims, only: :create do
collection do
get(:facilities)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added new facilities route, and moved the post declaration here. This is functionally the same as before, just easier to read.

@@ -21,7 +21,7 @@ def initialize(body, status)
self.body = body
self.status = status
parsed_body = JSON.parse(body)
self.data = parsed_body['data']
self.data = parsed_body['data'] || []
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If no facilities are returned, the lighthouse api does not return a data key or object. This handles that scenario by setting it to an empty array.

Copy link
Contributor

Choose a reason for hiding this comment

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

💅 This wasn't an issue when we were pulling Facilities for specific States, but definitely comes in to play now. Nice!

@@ -244,4 +245,55 @@
).to eq(false)
end
end

describe '#facilities' do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As mentioned above, I'd like to eventually move these to request specs.

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 deleted these and moved them to the request spec.

Mitch-A6
Mitch-A6 previously approved these changes Sep 24, 2024
@ryan-mcneil
Copy link
Contributor

@coope93 Please pull master to clear up the CVE

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.

4 participants