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

[ARP - lower camel responses - 1] set_key_transform :camel_lower #20621

Merged
merged 3 commits into from
Feb 11, 2025

Conversation

nihil2501
Copy link
Contributor

@nihil2501 nihil2501 commented Feb 5, 2025

This is the head of a PR stack that switches all existing ARP endpoints to input and output camel cased request and response payloads explicitly and in particular bypasses olive_branch middleware. The PRs sequence the work like 1 <- 2 <- 3 <- 4 <- 5 and can each be approved one at a time in that order. Then that whole chain can get merged into this head PR and platform can re-approve a final time to go into master.

⚠️ Please feel free to visit the 5 minimal diffs in quick succession! ⚠️

The PR titles and minimal diffs are self explanatory:

It is worth reiterating the code comment that exists in the final PR of the PR stack that bypasses olive_branch for ARP endpoints:

olive_branch transforms (a) request and (b) response payloads.

(a) It deeply transforms request and query param keys.
At times it is convenient for params to act as snake-cased setters at a Ruby interface, but not always. Form resources are a possible example where not.

For now, let's wait to encounter the cases where we really want this convenience. If we do encounter some, we may discover that we want a more explicit and collocated way to opt in.

(b) It reloads the response from JSON, deeply transforms keys, and dumps back to JSON.
This is superfluous because our serialization layer jsonapi-serializer already has a configuration option for key casing. This realizes our desired casing the one and only time it is visiting an object during serialization.

pixiitech
pixiitech previously approved these changes Feb 5, 2025
Copy link
Contributor

@pixiitech pixiitech 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

Base automatically changed from art/100197/new_org_lookup to master February 5, 2025 20:06
@chumpy chumpy dismissed pixiitech’s stale review February 5, 2025 20:06

The base branch was changed.

@va-vfs-bot va-vfs-bot temporarily deployed to art/lower-camel-responses/1/main/main February 5, 2025 20:07 Inactive
@nihil2501 nihil2501 force-pushed the art/lower-camel-responses/1 branch from a767000 to 7f5c32c Compare February 6, 2025 00:54
@va-vfs-bot va-vfs-bot temporarily deployed to art/lower-camel-responses/1/main/main February 6, 2025 00:55 Inactive
chumpy
chumpy previously approved these changes Feb 6, 2025
@nihil2501 nihil2501 marked this pull request as ready for review February 6, 2025 16:48
@nihil2501 nihil2501 requested review from a team as code owners February 6, 2025 16:48
rmtolmach
rmtolmach previously approved these changes Feb 7, 2025
…0622)

* [ARP] camel case POA form persistence

* lint fixes

* [ARP - lower camel responses - 3] handle camel params in ARP IPF controller (#20623)

* [ARP] handle camel params in ARP IPF controller

* [ARP - lower camel responses - 4] camel response in ARP users controller (#20624)

* [ARP] camel response in ARP users controller

* [ARP - lower camel responses - 5] bypass olive branch (#20625)

* [ARP] bypass olive branch

* lint fixes
@nihil2501 nihil2501 dismissed stale reviews from rmtolmach and chumpy via 16d1459 February 7, 2025 22:37
Copy link

github-actions bot commented Feb 7, 2025

1 Warning
⚠️ This PR changes 413 LoC (not counting whitespace/newlines).

In order to ensure each PR receives the proper attention it deserves, we recommend not exceeding
200. Expect some delays getting reviews.

File Summary

Files

  • modules/accredited_representative_portal/app/controllers/accredited_representative_portal/v0/in_progress_forms_controller.rb (+1/-1)

  • modules/accredited_representative_portal/app/controllers/accredited_representative_portal/v0/representative_users_controller.rb (+7/-7)

  • modules/accredited_representative_portal/app/models/accredited_representative_portal/power_of_attorney_form.rb (+38/-38)

  • modules/accredited_representative_portal/app/serializers/accredited_representative_portal/application_serializer.rb (+1/-0)

  • modules/accredited_representative_portal/config/initializers/bypass_olive_branch.rb (+17/-0)

  • modules/accredited_representative_portal/spec/factories/power_of_attorney_form.rb (+42/-42)

  • modules/accredited_representative_portal/spec/factories/representative_user.rb (+5/-1)

  • modules/accredited_representative_portal/spec/requests/accredited_representative_portal/bypass_olive_branch_spec.rb (+38/-0)

  • modules/accredited_representative_portal/spec/requests/accredited_representative_portal/v0/in_progress_forms_spec.rb (+11/-14)

  • modules/accredited_representative_portal/spec/requests/accredited_representative_portal/v0/power_of_attorney_requests_spec.rb (+52/-59)

  • modules/accredited_representative_portal/spec/requests/accredited_representative_portal/v0/user_spec.rb (+8/-15)

  • modules/accredited_representative_portal/spec/serializers/accredited_representative_portal/power_of_attorney_request_serializer_spec.rb (+8/-8)

    Note: We exclude files matching the following when considering PR size:

    *.csv, *.json, *.tsv, *.txt, *.md, Gemfile.lock, app/swagger, modules/mobile/docs, spec/fixtures/, spec/support/vcr_cassettes/, modules/mobile/spec/support/vcr_cassettes/, db/seeds, modules/vaos/app/docs, modules/meb_api/app/docs, modules/appeals_api/app/swagger/, *.bru, *.pdf
    

Big PRs are difficult to review, often become stale, and cause delays.

Generated by 🚫 Danger

@va-vfs-bot va-vfs-bot temporarily deployed to art/lower-camel-responses/1/main/main February 7, 2025 22:38 Inactive
@nihil2501 nihil2501 merged commit adf715c into master Feb 11, 2025
34 checks passed
@nihil2501 nihil2501 deleted the art/lower-camel-responses/1 branch February 11, 2025 14:39
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.

5 participants