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

Don't return a nil duty station when creating a new service member #1823

Merged
merged 1 commit into from Mar 6, 2019

Conversation

2 participants
@chrisgilmerproj
Copy link
Contributor

chrisgilmerproj commented Mar 5, 2019

Description

If you try to create a service member right now you'll get this:

{
  "created_at": "2019-03-04T23:59:45.870Z",
  "current_station": {
    "address": {
      "city": "",
      "id": "00000000-0000-0000-0000-000000000000",
      "postal_code": "",
      "state": "",
      "street_address_1": ""
    },
    "affiliation": "",
    "created_at": null,
    "id": "00000000-0000-0000-0000-000000000000",
    "name": "",
    "updated_at": null
  },
  "has_social_security_number": false,
  "id": "9907f6de-9292-4c71-81eb-ed2ea78ad4a3",
  "is_profile_complete": false,
  "orders": [],
  "updated_at": "2019-03-04T23:59:45.870Z",
  "user_id": "c3ddf806-87bc-4933-ac47-5db81524f4b1"
}

The current_station is nil because the user is just starting to fill out their profile and nothing is known about them yet. Returning this structure causes issues because its not OpenAPI Spec compliant with our internal.yaml which says some of those fields are required (like updated_at and created_at). This breaks Swagger Clients attempting to use our api. Instead we should be returning this:

{
  "created_at": "2019-03-04T23:59:45.870Z",
  "has_social_security_number": false,
  "id": "9907f6de-9292-4c71-81eb-ed2ea78ad4a3",
  "is_profile_complete": false,
  "orders": [],
  "updated_at": "2019-03-04T23:59:45.870Z",
  "user_id": "c3ddf806-87bc-4933-ac47-5db81524f4b1"
}

Reviewer Notes

Honestly the real fix here is to make models.ServiceMember.CurrentStation a pointer instead of a value because we allow it to be NULL in the DB. But modifying this breaks a lot of Eager loading in our app that rely on Orders.ServiceMember.CurrentStation.Address and that fight wasn't worth fighting here. Instead I've opted to return nothing if the ID (or ForeignKey reference) is empty. I'm pretty certain this is ok for the app and I don't see tests failing.

Setup

  1. Create a new local user
  2. Find the local user's service member ID, go to the DB and delete the row from the service_members table.
  3. Use the swagger docs and try out http://milmovelocal:3000/internal/docs#/service_members/createServiceMember with the payload {} (an empty map)
  4. A user should be created.

Code Review Verification Steps

  • Request review from a member of a different team.
  • Have the Pivotal acceptance criteria been met for this change?

References

@chrisgilmerproj chrisgilmerproj self-assigned this Mar 5, 2019

@chrisgilmerproj chrisgilmerproj requested review from stangah and rdhariwal Mar 5, 2019

@codecov

This comment has been minimized.

Copy link

codecov bot commented Mar 6, 2019

Codecov Report

Merging #1823 into master will decrease coverage by 0.17%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1823      +/-   ##
==========================================
- Coverage   49.42%   49.24%   -0.18%     
==========================================
  Files         427      425       -2     
  Lines       18328    18138     -190     
  Branches     1631     1630       -1     
==========================================
- Hits         9058     8932     -126     
+ Misses       8473     8426      -47     
+ Partials      797      780      -17
@rdhariwal
Copy link
Contributor

rdhariwal left a comment

:shipit:

@chrisgilmerproj chrisgilmerproj merged commit 63cf342 into master Mar 6, 2019

19 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
ci/circleci: acceptance_tests_experimental Your tests passed on CircleCI!
Details
ci/circleci: acceptance_tests_local Your tests passed on CircleCI!
Details
ci/circleci: acceptance_tests_staging Your tests passed on CircleCI!
Details
ci/circleci: build_app Your tests passed on CircleCI!
Details
ci/circleci: build_migrations Your tests passed on CircleCI!
Details
ci/circleci: build_tools Your tests passed on CircleCI!
Details
ci/circleci: client_test Your tests passed on CircleCI!
Details
ci/circleci: client_test_coverage Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_mymove Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_office Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_tsp Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_golang Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_yarn Your tests passed on CircleCI!
Details
ci/circleci: pre_test Your tests passed on CircleCI!
Details
ci/circleci: server_test Your tests passed on CircleCI!
Details
ci/circleci: server_test_coverage Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 49.42%)
Details
codecov/project Absolute coverage decreased by -0.17% but relative coverage increased by +50.57% compared to 0a60bb0
Details

@chrisgilmerproj chrisgilmerproj deleted the cg_164418251_remove_nil_dutystation branch Mar 6, 2019

@chrisgilmerproj chrisgilmerproj referenced this pull request Mar 22, 2019

Open

WIP - Try out load testing framework locust.io #1597

0 of 2 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.