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

Kma 162678897 experiment set gex handler #1530

Merged
merged 45 commits into from Jan 11, 2019

Conversation

3 participants
@kimallen
Copy link
Contributor

kimallen commented Dec 29, 2018

Description

This creates a new environment variable that determines if a gex request should actually be sent or not. By default, the gex request will not be sent in local development, but will be sent in all deployed environments. The gex request now is found in an interface struct's function SendRequest. The actual request is stubbed by a call to a local server and utilizes that url instead of the gex url. Gex gex url is moved to a config var as GEX_URL.

This also now moves the environment variables accessed within the function that sends the gex request to parameters and passes them through a struct in the webserver/main.go, with acceptance tests.

Reviewer Notes

*I should probably set the gex url as a constant, though not sure where to do that. done
*Are there other tests I should write?

Code Review Verification Steps

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

References

@kimallen kimallen changed the title Kma 162678897 experiment set gex handler WIP-Kma 162678897 experiment set gex handler Dec 29, 2018

@kimallen

This comment has been minimized.

Copy link
Contributor

kimallen commented Dec 29, 2018

Tests were passing locally, but now are not, so I'm moving this to WIP. Feel free to take a look though and comment.

@mikena-truss
Copy link
Contributor

mikena-truss left a comment

As far as testing goes, it's a hard question to answer in this situation. With the method params/signature, we'd want to test the various scenarios of what we can send to gex and what they would return. From my understanding though, they really just give us a 200 🤷‍♀️

It does feel kind of empty though, like we're missing something. Maybe someone else has thoughts.

.envrc Outdated
@@ -141,7 +141,7 @@ export HTTP_SDDC_PROTOCOL="http"
export HTTP_SDDC_PORT="8080"
export DPS_REDIRECT_URL="https://dpstest.sddc.army.mil/cust"
export DPS_COOKIE_NAME="DPSIVV"

export REALLY_SEND_GEX_REQUEST=false

This comment has been minimized.

@mikena-truss

mikena-truss Dec 31, 2018

Contributor

Can we just call this SEND_TO_GEX or SEND_INVOICE_TO_GEX?

This comment has been minimized.

@kimallen

kimallen Jan 3, 2019

Contributor

Removed this altogether in exchange for a GEX_URL constant, as suggested

@@ -679,6 +682,18 @@ func main() {
}
handlerContext.SetFileStorer(storer)

var gexRequester gex.SenderToGex
if v.GetBool("really-send-gex-request") {
gexRequester = gex.SendGex{URL: "https://gexweba.daas.dla.mil/msg_data/submit/"}

This comment has been minimized.

@mikena-truss

mikena-truss Dec 31, 2018

Contributor

Agree that this should be in a config file rather than hard coded.

This comment has been minimized.

@mikena-truss

mikena-truss Dec 31, 2018

Contributor

In fact, we may not need the SEND_TO_GEX boolean if we put this in a config. Just have the url be empty outside of prod and rather than checking the boolean, check if the url is empty. If the url is empty, start a local web server.

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Dec 31, 2018

Contributor

We are now using a pattern where we check the configs ahead of using variables. Look at checkEmail or checkCSRF for examples. So if we move this to some kind of config variable or file we ought to have a corresponding method. Also, we put tests for these in cmd/main_test.go which is where I'd add in the test for the check method.

This comment has been minimized.

@kimallen

kimallen Jan 3, 2019

Contributor

Created a GEX_URL constant and added function and test

}

// SendGex represents a struct to contain an actual gex request function
type SendGex struct{ URL string }

This comment has been minimized.

@mikena-truss

mikena-truss Dec 31, 2018

Contributor

I generally prefer that the type name for something that's meant to use an interface should be a descriptor on the implementation. Also following the same structure/tense. So in this case, what is specific about this implementation? Maybe SendToGexHTTP or something similar?

This comment has been minimized.

@kimallen

kimallen Jan 9, 2019

Contributor

Adjusted

@@ -38,8 +52,8 @@ func SendInvoiceToGex(edi string, transactionName string) (resp *http.Response,
if err != nil {

This comment has been minimized.

@mikena-truss

mikena-truss Dec 31, 2018

Contributor

While you are here can you change GetTLSConfig to getTLSConfig? It is not used outside of this file.

This comment has been minimized.

@kimallen

kimallen Jan 2, 2019

Contributor

yes

This comment has been minimized.

@kimallen

kimallen Jan 9, 2019

Contributor

This ended up getting removed and the TLS configs are collected in initDODCertificates in webserver/main.go

// Ensure that the transaction body ends with a newline, otherwise the GEX
// EDI parser will fail silently
// SenderToGex is an interface for sending and receiving a request
type SenderToGex interface {

This comment has been minimized.

@mikena-truss

mikena-truss Dec 31, 2018

Contributor

GexSender sounds a little cleaner to me. Or just follow the service object pattern with this, so it would be SendToGex for the struct and Call for the method. This avoids the need to name them both something unique, since it's a single use type.

I also think the interface itself should generally live in a different file, possibly even package, since the reason for creating it is that there can be multiple implementations.

This comment has been minimized.

@kimallen

kimallen Jan 2, 2019

Contributor

If the Interface is in a different package, that would help with naming (it didn't like an interface inside the gex pkg being called with gex.Gex. . .) . What package would you recommend?

This comment has been minimized.

@kimallen

kimallen Jan 9, 2019

Contributor

Didn't move this. Wondering if we can leave it there with a note to move it when we reuse it.

This comment has been minimized.

@kimallen

kimallen Jan 11, 2019

Contributor

Put moving of the interface into a new Pivotal story

edi = strings.TrimSpace(edi) + "\n"

URL := s.URL
hasSlash, _ := regexp.MatchString("(.*/$)", URL)

This comment has been minimized.

@mikena-truss

mikena-truss Dec 31, 2018

Contributor

Rather then messing around with the trailing slash, it's probably a little safer here to use the url.Path/path.Join functions to append the path.

This comment has been minimized.

@kimallen

kimallen Jan 11, 2019

Contributor

used net/url's .Path

@@ -38,8 +52,8 @@ func SendInvoiceToGex(edi string, transactionName string) (resp *http.Response,
if err != nil {

This comment has been minimized.

@mikena-truss

mikena-truss Dec 31, 2018

Contributor

Also above (and not from this PR), but I'm curious if we want to be referencing env variables this far down in the call stack. It seems like those would be better parsed earlier on and passed through params. This separates configs from implementation a little better. Just making a note. Not something to fix here.

This comment has been minimized.

@kimallen

kimallen Jan 11, 2019

Contributor

Moved the env vars

suite.Run(t, hs)
}

func (suite *GexSuite) TestGexSend_SendRequest_OK() {

This comment has been minimized.

@mikena-truss

mikena-truss Dec 31, 2018

Contributor

OK/Bad might be better as subtests rather than their own tests.

This comment has been minimized.

@kimallen

kimallen Jan 3, 2019

Contributor

Combined them into one

@kimallen kimallen changed the title WIP-Kma 162678897 experiment set gex handler Kma 162678897 experiment set gex handler Jan 9, 2019

@kimallen

This comment has been minimized.

Copy link
Contributor

kimallen commented Jan 9, 2019

The test is passing! Removing from WIP, up for re-review

Bat team :)

@@ -27,7 +26,7 @@ func (h SendGexRequestHandler) Handle(params gexop.SendGexRequestParams) middlew
// EDI parser will fail silently
transactionBody = strings.TrimSpace(transactionBody) + "\n"

resp, err := gex.SendInvoiceToGex(transactionBody, transactionName)
resp, err := h.GexSender().Call(transactionBody, transactionName)

This comment has been minimized.

@mikena-truss

mikena-truss Jan 9, 2019

Contributor

Seeing this as a SO dependency makes me so happy :)

ediWriter := edi.NewWriter(os.Stdout)
err = ediWriter.WriteAll(invoice858C.Segments())
return nil, err
}

//TODO: Infra will work to refactor and reduce duplication (also found in webserver/main.go)

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Jan 9, 2019

Contributor

Quick not to @pjdufour-truss - we should figure out how to make this more available. It's now copied in three places (and probably outside the scope of this PR to refactor).

@chrisgilmerproj
Copy link
Contributor

chrisgilmerproj left a comment

🚀 - nice job on this!

flag.Bool("gex", false, "Choose to send the file to gex")
flag.String("transactionName", "test", "The required name sent in the url of the gex api request")

flag := pflag.CommandLine

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Jan 9, 2019

Contributor

I think we discussed, definitely move this up above your other flags.

This comment has been minimized.

@kimallen

kimallen Jan 10, 2019

Contributor

yup

TLSConfig: tlsConfig,
GEXBasicAuthUsername: v.GetString("gex-basic-auth-username"),
GEXBasicAuthPassword: v.GetString("gex-basic-auth-password"),
}.Call(ediString, v.GetString("transaction-name"))

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Jan 10, 2019

Contributor

Check the output of resp, err here to check for null, like we talked about in Slack.

This comment has been minimized.

@kimallen

kimallen Jan 11, 2019

Contributor

Done

@@ -101,10 +148,75 @@ func processInvoice(db *pop.Connection, shipment models.Shipment, invoiceModel m
if err != nil {
return nil, err
}
return gex.SendInvoiceToGex(invoice858CString, *transactionName)
resp, err := gexSender.Call(invoice858CString, *transactionName)

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Jan 10, 2019

Contributor

check the resp, err here too

This comment has been minimized.

@kimallen

kimallen Jan 11, 2019

Contributor

Done

@@ -56,7 +92,18 @@ func main() {
log.Fatal(verrs)
}

resp, err := processInvoice(db, shipment, invoiceModel, sendToGex, transactionName)
certificates, rootCAs, err := initDODCertificates(v, logger)

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Jan 10, 2019

Contributor

What happens if these come back as nil, nil, nil?

This comment has been minimized.

@kimallen

kimallen Jan 11, 2019

Contributor

I added some checks for nil values for certificates and rootCAs in the error handling, which will cause it to fail.

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Jan 11, 2019

Contributor

Thanks!

}
resp, err := gexSender.Call(invoice858CString, *transactionName)
if resp == nil || err != nil {
log.Fatal("cannot be nil or have error", err)

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Jan 11, 2019

Contributor

I think you'll want this to be more explicit because we'll be reading the error in CloudWatch. So perhaps:

log.Fatal("Gex Sender had no response", err)

This comment has been minimized.

@kimallen

kimallen Jan 11, 2019

Contributor

Done

GEXBasicAuthPassword: v.GetString("gex-basic-auth-password"),
}.Call(ediString, v.GetString("transaction-name"))
if resp == nil || err != nil {
log.Fatal("Cannot be nil or Error sending POST request", err)

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Jan 11, 2019

Contributor

Same here:

log.Fatal("POST request to GEX had no response", err)

This comment has been minimized.

@kimallen

kimallen Jan 11, 2019

Contributor

Done

@kimallen kimallen merged commit 560f496 into master Jan 11, 2019

12 checks passed

ci/circleci: acceptance_tests 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: 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

@kimallen kimallen deleted the kma-162678897-experiment-set-gex-handler branch Jan 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment