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

Orders Gateway #1396

Merged
merged 85 commits into from Mar 15, 2019

Conversation

4 participants
@jamesatheyDDS
Copy link
Contributor

jamesatheyDDS commented Nov 29, 2018

Description

Fleshes out the Orders API.

  • Adds a new electronic_orders table which just stores the EDIPI, orders number, issuer
  • Adds a new electronic_orders_revisions table for all orders amendments, stores all other orders data, and has a foreign key relationship with electronic_orders
  • Re-introduces the distinction between the lower commissioned officer ranks and the warrant officer ranks. The only reason these were co-mingled before was that they happen to share an entitlement. Commissioned officers and warrant officers are distinct in the eyes of the services and the members, and the personnel systems interacting with the Orders API distinguish between them
  • Adds read and write permissions for electronic Orders submitted by the various branches
  • Bumps the version number of the Orders API to 1.0.0. The diff is large mostly because the online Swagger editor started applying auto-formatting and reorganized all of the documentation blocks
  • Introduces two new GET endpoints for Orders - one to fetch all Orders by EDIPI, and another to fetch Orders by Issuer and Orders number. These two endpoints are more useful and REST-ful than the previous GET which took all of these as query parameters, and would have been quite a pain to correctly and safely implement

Reviewer Notes

After consulting with several other engineers, we agreed that Electronic Orders should live in its own tables for now instead of grafting them onto the existing orders table. As such, this PR does not expose or surface electronic orders in any Web UI. That will be work for the future.

The only user-visible changes in this PR are to separate the Warrant Officer ranks from the commissioned officer ranks.

Setup

Code Review Verification Steps

  • Code follows the guidelines for Logging
  • The requirements listed in Querying the Database Safely have been satisfied.
  • Tested in the Experimental environment (for changes to containers, app startup, or connection to data stores)
  • Request review from a member of a different team.

jamesatheyDDS added some commits Nov 29, 2018

Electronic Orders: Update documentation strings to remove references …
…to MyMove - it's not called that anymore

correct some typos in enums
separate warrant officer ranks from lower commissioned officer ranks …
…- these are very different positions in the eyes of members and the branches, and besides, the electronic orders require it. Besides, the co-mingling only happened in the first place because the entitlements happen to be shared, but that's not guaranteed to be permanent

@jamesatheyDDS jamesatheyDDS added the wip label Nov 29, 2018

@jamesatheyDDS jamesatheyDDS self-assigned this Nov 29, 2018

jamesatheyDDS added some commits Nov 29, 2018

Electronic Orders: give losing_unit_id and gaining_unit_id differentl…
…y named constraints, since they point to the same foreign key
First draft of implementation of GetOrders endpoint. Many changes req…
…uired:

- add missing Orders types to the internal API.
- revise Orders API:
  - to use an enum for issuing organization
  - add "rfo" status, so that Army HRC can send Request For Orders data instead of real orders
  - add issuer paramater to PostRevision so that the Orders structure can be fully specified instead of inferred
  - clarify a bunch of documentation
  - grudgingly accept a bunch of formatting changes to long strings imposed by the online Swagger editor
- split the four different API handlers into their own files, because they are starting to get long enough to justify it
- add conversion maps that translate between the DB values (internal API) and the external Orders API
store service member and unit info directly in order row, so that fut…
…ure changes to the member's info or to the unit don't change the original order
@codecov

This comment has been minimized.

Copy link

codecov bot commented Feb 13, 2019

Codecov Report

Merging #1396 into master will increase coverage by 0.75%.
The diff coverage is 77.81%.

@@            Coverage Diff             @@
##           master    #1396      +/-   ##
==========================================
+ Coverage   49.55%   50.31%   +0.75%     
==========================================
  Files         432      440       +8     
  Lines       18510    19069     +559     
  Branches     1636     1636              
==========================================
+ Hits         9173     9594     +421     
- Misses       8533     8634     +101     
- Partials      804      841      +37

jamesatheyDDS added some commits Feb 13, 2019

Put electronic orders into their own table for now; postponing the pa…
…in of hosting manual and electronic orders in the same universe (and handling amendments in the UX)

orders and revisions in separate tables, like the API expects
sort revisions in ascending sequence number order, to make appending …
…new revisions easier

check to see whether the various accounting blocks are actually supplied before dereferencing them

jamesatheyDDS added some commits Mar 13, 2019

Add indexes to electronic_orders table for orders_number+issuer and f…
…or edipi; make it explicit in the docs that Orders are considered unique by orders_number+issuer
Create an ElectronicOrder and its first ElectronicOrdersRevision in a…
… single transcation; make Revision.seqNum in the OrdersAPI nullable again, because otherwise a seqNum of 0 is considered null instead of the number 0
Show resolved Hide resolved pkg/handlers/ordersapi/get_orders.go

id, err := uuid.FromString(params.UUID.String())
if err != nil {
h.Logger().Error(fmt.Sprintf("Not a valid UUID: %s; why didn't the generated Swagger code catch this?", params.UUID))

This comment has been minimized.

@mikena-truss

mikena-truss Mar 14, 2019

Contributor

With this, and other error messages, should we be adding some more flavor from the request? Unless there's some wrapping happening that I'm not in tune to via the logger, it seems like these messages could get easily lost.

This comment has been minimized.

@jamesatheyDDS

jamesatheyDDS Mar 14, 2019

Author Contributor

I was wondering about this. These logs do not make their way back to the client, and I would like them to. Do any other API handlers in the codebase write output to the client on error? Do I need to make my own middleware.Responder implementers for this?

This comment has been minimized.

@jamesatheyDDS

jamesatheyDDS Mar 14, 2019

Author Contributor

Oh wait, I see handlers.ResponseForError. I take it that's what I should use?

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Mar 14, 2019

Contributor

Yeah, using handlers.ResponseForError would be good. You could go further and use handlers.RespondAndTranceError and that would give us insights into orders using Honeycomb.

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Mar 14, 2019

Contributor

You're also right that the errors don't show up in the client, just in the server logs which go to CloudWatch.

This comment has been minimized.

@jamesatheyDDS

jamesatheyDDS Mar 15, 2019

Author Contributor

@mikena-truss @chrisgilmerproj handlers.ResponseForError does not follow the logging guidelines - it logs at the Debug level, where the guidelines say to use Info for responses like 401, 403, and 404. It also doesn't handle Forbidden cases from attempts to POST or PUT, i.e., there's ErrFetchForbidden but no ErrWriteForbidden. That's fine, I can add support for that, but I can't use handlers.ResponseForCustomErrors as a stopgap, since that logs at Error.

handlers.ResponseForConflictErrors also logs at the Error level (!), which seems extreme.

Addressing the logging levels of these functions is beyond the scope of this PR, right? I can make do with what's there, particularly since if I start changing yet another part of the code, that will lead to more PR comments...

This comment has been minimized.

@jamesatheyDDS

jamesatheyDDS Mar 15, 2019

Author Contributor

While we're talking about the behavior of handlers.ResponseForError, its logs are redundant and include newlines for some reason(?):

2019-03-15T14:14:38.674Z DEBUG ordersapi/index_orders_for_member.go:30 forbidden {"error": "Not permitted to read any Orders: FETCH_FORBIDDEN", "errorVerbose": "FETCH_FORBIDDEN\nNot permitted to read any Orders"}

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Mar 15, 2019

Contributor

Yeah, DEBUG is special in development. I can look into how it behaves but it's not really anything you need to fix. I'll also do another pass at what those functions are doing. But if you're using them then I think we're in a good place for having everything be the same.

Show resolved Hide resolved pkg/handlers/ordersapi/get_orders_by_issuer_and_orders_num.go Outdated

ordersPayload, err := payloadForElectronicOrderModel(orders)
if err != nil {
return handlers.ResponseForError(h.Logger(), err)

This comment has been minimized.

@mikena-truss

mikena-truss Mar 14, 2019

Contributor

err here could have no information depending on what went wrong. When we fail generating payloads, it might make sense to wrap the error with some flavor, so ResponseForError has something good to log.

Show resolved Hide resolved pkg/handlers/ordersapi/api.go Outdated
Show resolved Hide resolved pkg/handlers/ordersapi/get_orders_by_issuer_and_orders_num_test.go Outdated
Show resolved Hide resolved pkg/handlers/ordersapi/api.go
Show resolved Hide resolved pkg/handlers/ordersapi/index_orders_for_member.go Outdated

jamesatheyDDS added some commits Mar 14, 2019

use handlers.FmtDate{Time}Ptr() in payload creation function, like ot…
…her APIs in the codebase do, instead of doing the typecast directly
Have the DB filter ElectronicOrders by allowed Issuers instead of doi…
…ng that in code; generate slice of allowed Issuers from the ClientCert in a separate member function
Use handlers.ResponseForError() wherever possible in the Orders API i…
…nstead of returning generated Swagger response types
Add testdatagen.MakeDefaultElectronicOrder; refactor testdatagen.Make…
…ElectronicOrder to use Assertions struct; refactor ElectronicOrder and Orders API tests to use these
@chrisgilmerproj
Copy link
Contributor

chrisgilmerproj left a comment

⭐️ - This is some great work. Thanks for working with us to get this to the finish line.

@mikena-truss
Copy link
Contributor

mikena-truss left a comment

Thanks for making some of those changes. The GET handlers I took a look at, look good 👍

@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor

chrisgilmerproj commented Mar 15, 2019

You can ignore the "Codacy/PR Quality Review" errors, they won't affect you being able to merge this.

@jamesatheyDDS jamesatheyDDS merged commit 614abbc into master Mar 15, 2019

18 of 19 checks passed

Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
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 77.81% of diff hit (target 49.55%)
Details
codecov/project 50.31% (+0.75%) compared to 2a74fed
Details

@jamesatheyDDS jamesatheyDDS deleted the electronic-orders branch Mar 18, 2019

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.