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

EDM-567/cache lcpe #20674

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

EDM-567/cache lcpe #20674

wants to merge 27 commits into from

Conversation

jefftmarks
Copy link
Contributor

@jefftmarks jefftmarks commented Feb 7, 2025

Summary

  • *This work is behind a feature toggle (flipper): NO
  • (Summarize the changes that have been made to the platform): EDM frontend team is integrating license, certs, prep course, and exams (LCPE) into the GI Comparison tool. Vets-api facilitates the request upstream to GIDS. Previously existed as a simple pass through service fronted by GIDS Redis facade. LCPE will implement a different pattern with http caching (If-None-Match and Etag headers) to handle relatively large license and certs file.
  • (If bug, how to reproduce)
  • (What is the solution, why is this the solution?) GIDS will implement versioning with preloaded datasets and verify that the client has the most recent version of data. Vets-api's job is to compare client versions with the currently cached redis version, and then verify that most recent version with GIDS. Vets-api will cache after making a request to GIDS if there's a new version returned (as opposed to existing pattern for other GIDS endpoints which check the cache first before sending requests upstream to GIDS). Diagrams here
  • (Which team do you work for, does your team own the maintenance of this component?) Education Data Migration (EDM) yes except for raise_custom_error.rb, which needed to be modified to allow 304s to be returned to client
  • (If introducing a flipper, what is the success criteria being targeted?)

Related issue(s)

Testing done

  • New code is covered by unit tests Fixed failing tests. PR already large, so will add new tests in separate PR to cover new files created
  • Describe what the old behavior was prior to the change
  • Describe the steps required to verify your changes are working as expected. Exclusively stating 'Specs run' is NOT acceptable as appropriate testing Run GIDS and vets-api locally and test http caching using browser
  • If this work is behind a flipper:
    • Tests need to be written for both the flipper on and flipper off scenarios. Docs.
    • What is the testing plan for rolling out the feature?

Screenshots

Note: Optional

What areas of the site does it impact?

(Describe what parts of the site are impacted andifcode touched other areas)
GI Bill Comparison tool (once new LCPE integration is launched)

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

Had to add new option into raise_custom_error.rb middleware. Otherwise 304 responses were getting caught and transformed into backend errors and we need them to make it back to client. Is this allowed/preferred solution? Other option would be to duplicate raise_custom_error middleaware and rename it but that seemed repetitive if most of the logic is the same otherwise.

@va-vfs-bot va-vfs-bot temporarily deployed to EDM-567/cache-lcpe/main/main February 7, 2025 19:34 Inactive
@va-vfs-bot va-vfs-bot temporarily deployed to EDM-567/cache-lcpe/main/main February 7, 2025 19:42 Inactive
@va-vfs-bot va-vfs-bot temporarily deployed to EDM-567/cache-lcpe/main/main February 10, 2025 19:29 Inactive
@va-vfs-bot va-vfs-bot temporarily deployed to EDM-567/cache-lcpe/main/main February 10, 2025 19:51 Inactive
@va-vfs-bot va-vfs-bot temporarily deployed to EDM-567/cache-lcpe/main/main February 19, 2025 23:13 Inactive
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.

2 participants