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.

Sign up for GitHub

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

Jump to bottom

4195 beneficiary endpoint fix #12846

Merged
merged 0 commits into from
Jun 1, 2023
Merged

4195 beneficiary endpoint fix #12846

merged 0 commits into from
Jun 1, 2023

Conversation

cadibemma
Copy link
Contributor

Summary

  • snake/camel case fix for beneficiary letters endpoint

Related issue(s)

Testing done

  • Specs

What areas of the site does it impact?

  • None

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 or Grafana (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

@cadibemma cadibemma requested review from a team as code owners May 31, 2023 17:33
@va-vfs-bot va-vfs-bot temporarily deployed to 4195-beneficiary-endpoint-fix/main/main May 31, 2023 17:37 Inactive
Tonksthebear
Tonksthebear previously approved these changes May 31, 2023
View reviewed changes
Copy link
Contributor

@Tonksthebear Tonksthebear left a comment

Choose a reason for hiding this comment

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

LGTM

aherzberg
aherzberg suggested changes May 31, 2023
View reviewed changes
Copy link
Contributor

@aherzberg aherzberg 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. just a question about field orders

modules/mobile/app/models/mobile/v0/adapters/letter_info.rb
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the functional difference is the military service field but there's also some reordering of fields. was this intentional? our docs have the fields ordered has they were originally. There's shouldn't be any behavioral difference but rather keep it consistent with docs than not.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is good for my knowledge. I saw the reordering but thought we didn't care about that. Would like to know as well

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 intentionally changed the order to match the order thats returned from EVSS. Do you suggest that I change it back to match docs? or update the docs?

Copy link
Contributor

Choose a reason for hiding this comment

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

Either way could work. I'm seeing a schema a little different from what EVSS was returning in spec/support/vcr_cassettes/evss/letters/beneficiary_veteran.yml but it's also different from our original ordering so ones not better than the other. I think it may be more simple to change it back to match the docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, its now fixed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good. thanks!

kpethtel
kpethtel previously approved these changes May 31, 2023
View reviewed changes
Copy link
Contributor

@kpethtel kpethtel left a comment

Choose a reason for hiding this comment

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

LGTM.

@cadibemma cadibemma dismissed stale reviews from kpethtel and Tonksthebear via b123ae6 May 31, 2023 23:40
@va-vsp-bot va-vsp-bot requested a deployment to 4195-beneficiary-endpoint-fix/main/main May 31, 2023 23:41 In progress
@va-vfs-bot va-vfs-bot temporarily deployed to 4195-beneficiary-endpoint-fix/main/main May 31, 2023 23:41 Inactive
@cadibemma cadibemma requested a review from aherzberg May 31, 2023 23:42
kpethtel
kpethtel approved these changes Jun 1, 2023
View reviewed changes
Copy link
Contributor

@kpethtel kpethtel left a comment

Choose a reason for hiding this comment

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

LGTM and seems to be in the same order as in the docs. 👍

@va-vsp-bot va-vsp-bot requested a deployment to 4195-beneficiary-endpoint-fix/main/main June 1, 2023 14:39 In progress
@va-vfs-bot va-vfs-bot temporarily deployed to 4195-beneficiary-endpoint-fix/main/main June 1, 2023 14:40 Inactive
aherzberg
aherzberg approved these changes Jun 1, 2023
View reviewed changes
Copy link
Contributor

@aherzberg aherzberg left a comment

Choose a reason for hiding this comment

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

LGTM

@cadibemma cadibemma merged commit e9abb7c into master Jun 1, 2023
@cadibemma cadibemma deleted the 4195-beneficiary-endpoint-fix branch June 1, 2023 16:01
ryan-mcneil pushed a commit that referenced this pull request Dec 11, 2023
@cadibemma
4195 beneficiary endpoint fix (#12846)
180fb86 
* fixed naming in beneficiary to match old schema

* updated specs

* fixed order changes to match docs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Tonksthebear Tonksthebear Tonksthebear left review comments

@kpethtel kpethtel kpethtel approved these changes

@aherzberg aherzberg aherzberg approved these changes

Assignees
No one assigned
Labels
mobile
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@cadibemma @aherzberg @Tonksthebear @kpethtel @va-vfs-bot @va-vsp-bot