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

Add mobile & active status to nearby serializer #3397

Merged
merged 2 commits into from
Oct 7, 2019

Conversation

nfasulo
Copy link
Contributor

@nfasulo nfasulo commented Oct 7, 2019

Description of change

Adds mobile and active_status fields the the JSON response of /nearby. The GeoJSON response for this endpoint and the documentation already include these two new fields.

Reolves department-of-veterans-affairs/vets-contrib/issues/3323.

Testing done

Made request to /nearby on localhost to verify that the two new fields were included.

Testing planned

Will make requests to /nearby in deployed environments to verify that the two new fields were included.

Acceptance Criteria (Definition of Done)

Unique to this PR

  • /nearby JSON response includes mobile and active_status

Applies to all PRs

  • Appropriate logging
  • Swagger docs have been updated, if applicable
  • Provide link to originating GitHub issue, or connected to it via ZenHub
  • Does not contain any sensitive information (i.e. PII/credentials/internal URLs/etc., in logging, hardcoded, or in specs)
  • Provide which alerts would indicate a problem with this functionality (if applicable)

Copy link
Contributor

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

@johnpaulashenfelter johnpaulashenfelter left a comment

Choose a reason for hiding this comment

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

Checked w/ @nfasulo and all the swagger docs and such were already updated and the serializer was what was missed.

This brings the actual behavior up to the specs.

@va-vfs-bot va-vfs-bot temporarily deployed to nf/nearby_facilities_add_new_fields/master October 7, 2019 18:15 Inactive
@edmkitty
Copy link
Contributor

edmkitty commented Oct 7, 2019

Are there any tests for this that need to be updated?

@nfasulo
Copy link
Contributor Author

nfasulo commented Oct 7, 2019

@edmkitty there are no tests that cover all the fields that the serializer returns, which is something that @johnpaulashenfelter brought up too. I'm not sure having another file to add the names of these two fields would have saved us in this case, but I'm open to adding specs for all of the facilities serializers. At this point facilities serializers are covered by the overall request specs (in this case nearby_request_spec.rb), but those don't necessarily get into all the details of each serializer.

Copy link
Contributor

@edmkitty edmkitty left a comment

Choose a reason for hiding this comment

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

yet another approval

@nfasulo
Copy link
Contributor Author

nfasulo commented Oct 7, 2019

I have created a tech debt ticket for the writing tests for this and the rest of the facilities serializers: department-of-veterans-affairs/vets-contrib/issues/3331

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants