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

Send post move survey email #2630

Merged
merged 40 commits into from Sep 5, 2019

Conversation

@ralren
Copy link
Contributor

commented Aug 27, 2019

Description

This deals with making the command line task to send a survey email fifteen days after a move's payment request has been reviewed. Running the command multiple times should not send multiple emails. Another PR will deal with setting this up to be run in AWS. Look here for the contents of the email: https://docs.google.com/document/d/1VLNbT5_cX6P-MX7TgFAXWajp71YG99KZvUZrvQM2qBY/edit#heading=h.cdt9fip3wk8x

Setup

make server_run
make office_client_run
go run ./cmd/send_post_move_survey_email

Also test the integration in local dev with SES by:

  1. make db_dev_e2e_populate to add sample moves
  2. In personally_procured_moves choose a row and set review_date to 15 days minus today.
  3. For the service member associated with the move above update service_members.personal_email to your email.
  4. run go run ./cmd/send_post_move_survey_email --email-backend=ses --aws-ses-domain=devlocal.dp3.us --aws-ses-region=us-west-2 you should receive an email.
  5. Run the above command again. No email should be sent this time.

Code Review Verification Steps

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

References

@ralren ralren added the wip label Aug 27, 2019

@ralren ralren changed the title Ren 168035083 post move survey email Send post move survey email Aug 27, 2019

@ralren ralren added the roci label Aug 27, 2019

@ralren ralren removed the wip label Aug 27, 2019

@codecov

This comment has been minimized.

Copy link

commented Aug 27, 2019

Codecov Report

Merging #2630 into master will increase coverage by 0.1%.
The diff coverage is 63.4%.

@@           Coverage Diff            @@
##           master   #2630     +/-   ##
========================================
+ Coverage    57.2%   57.3%   +0.1%     
========================================
  Files         247     248      +1     
  Lines       12016   12114     +98     
========================================
+ Hits         6871    6934     +63     
- Misses       4446    4475     +29     
- Partials      699     705      +6
Impacted Files Coverage Δ
pkg/notifications/notification_stub.go 0% <0%> (ø) ⬆️
pkg/models/personally_procured_move.go 48.6% <0%> (-0.4%) ⬇️
pkg/notifications/notification_sender.go 0% <0%> (ø) ⬆️
pkg/services/move_documents/ppm_completer.go 60% <100%> (ø) ⬆️
pkg/notifications/move_reviewed.go 71.6% <71.6%> (ø)
pkg/services/accesscode/fetch_access_code.go 100% <0%> (ø) ⬆️
pkg/services/accesscode/validate_access_code.go
pkg/services/accesscode/claim_access_code.go 100% <0%> (ø)
Makefile Outdated Show resolved Hide resolved
@chrisgilmerproj
Copy link
Contributor

left a comment

A few more changes, mostly to Docker and Makefile. And then I think this is ready to go. After merging we can work on how we intend to deploy this to AWS.

ralren added 3 commits Aug 27, 2019

@mkrump mkrump force-pushed the ren-168035083-post-move-survey-email branch from 41ea099 to 0cf2cf1 Aug 29, 2019

@mkrump mkrump force-pushed the ren-168035083-post-move-survey-email branch from 0cf2cf1 to 252497b Aug 29, 2019

@mkrump

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2019

@chrisgilmerproj I think we addressed all of your comments if you want to take another look (check out the updated setup section). There is now a separate notifications table that tracks when / if an email was sent, along with the id returned by SES, which we could use to get additional info from SES if we want to.

@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2019

Looks like this is almost done. There's an issue with the template not being in the right location in the docker file:

make tasks_send_post_move_surveyBuild the docker scheduled tasks container...
docker build -f Dockerfile.tasks_local --tag tasks:latest .
Sending build context to Docker daemon    501MB
Step 1/4 : FROM gcr.io/distroless/base:latest
 ---> 7f04a8d24717
Step 2/4 : COPY bin_linux/save-fuel-price-data /bin/save-fuel-price-data
 ---> Using cache
 ---> ed4b6f5f9a05
Step 3/4 : COPY bin_linux/send-post-move-survey-email /bin/send-post-move-survey-email
 ---> Using cache
 ---> dcad997383dc
Step 4/4 : WORKDIR /bin
 ---> Using cache
 ---> 8467369b9473
Successfully built 8467369b9473
Successfully tagged tasks:latest
sending post move survey with docker command...
DB_NAME=dev_db DB_DOCKER_CONTAINER=milmove-db-dev scripts/wait-for-db-docker
> docker exec milmove-db-dev psql postgres://postgres:mysecretpassword@localhost:5432/dev_db -c 'select 1;' > /dev/null 2>&1
Checking for database connectivity..success!
docker run \
                -t \
                -e DB_HOST="database" \
                -e DB_NAME \
                -e DB_PORT \
                -e DB_USER \
                -e DB_PASSWORD \
                --link="milmove-db-dev:database" \
                --rm \
                tasks:latest \
                send-post-move-survey-email
panic: asset: Asset(pkg/notifications/templates/move_reviewed_template.txt): Asset pkg/notifications/templates/move_reviewed_template.txt not found

goroutine 1 [running]:
github.com/transcom/mymove/pkg/assets.MustAsset(0xfe50f5, 0x36, 0xc000559f58, 0xda9e5d, 0xc000559f68)
        /Users/cgilmer/Projects/transcom/mymove/pkg/assets/assets.go:160 +0x111
github.com/transcom/mymove/pkg/notifications.init.ializers()
        /Users/cgilmer/Projects/transcom/mymove/pkg/notifications/move_reviewed.go:24 +0x3a
make: *** [tasks_send_post_move_survey] Error 2

My thinking is that the command ought to let you pass through the location of the template file like --template /path/to/template/move_reviewed_template.txt so that we can set it in an environment variable. Currently docker is looking for that template at the pkg/notifications/templates/move_reviewed_template.txt location which is specific to our git source tree and would not make sense inside our dockerfile.

@mkrump

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2019

make tasks_send_post_move_survey
Build the docker scheduled tasks container...
docker build -f Dockerfile.tasks_local --tag tasks:latest .
Sending build context to Docker daemon  298.8MB
Step 1/4 : FROM gcr.io/distroless/base:latest
 ---> 7f04a8d24717
Step 2/4 : COPY bin_linux/save-fuel-price-data /bin/save-fuel-price-data
 ---> Using cache
 ---> 9d35e67c00d6
Step 3/4 : COPY bin_linux/send-post-move-survey-email /bin/send-post-move-survey-email
 ---> Using cache
 ---> d75a432e2ad7
Step 4/4 : WORKDIR /bin
 ---> Using cache
 ---> 2e2806984dc5
Successfully built 2e2806984dc5
Successfully tagged tasks:latest
sending post move survey with docker command...
DB_NAME=dev_db DB_DOCKER_CONTAINER=milmove-db-dev scripts/wait-for-db-docker
> docker exec milmove-db-dev psql postgres://postgres:mysecretpassword@localhost:5432/dev_db -c 'select 1;' > /dev/null 2>&1
Checking for database connectivity..success!
docker run \
		-t \
		-e DB_HOST="database" \
		-e DB_NAME \
		-e DB_PORT \
		-e DB_USER \
		-e DB_PASSWORD \
		--link="milmove-db-dev:database" \
		--rm \
		tasks:latest \
		send-post-move-survey-email
2019-09-03T18:33:58.385Z	DEBUG	send_post_move_survey_email/main.go:30	checking config
2019-09-03T18:33:58.385Z	INFO	cli/dbconn.go:272	Connecting to the database	{"url": "postgres://postgres:*****@database:5432/dev_db?sslmode=disable", "db-ssl-root-cert": ""}
2019-09-03T18:33:58.389Z	INFO	notifications/notification_sender.go:79	Using local email backend	{"domain": "milmovelocal"}
2019-09-03T18:33:58.396Z	INFO	notifications/move_reviewed.go:88	no emails to be sent for	{"date": "2019-08-19 18:33:58.3892321 +0000 UTC"}

works on my machine :) just kidding, but seriously. my understanding of how assets pkg works is that it converts all of these forms to binary and when it comes time to load a file it looks in assets.go first. Only if it doesn't find it there does it try to look to the path. assets.go should get generated with the build. if you open that file you should see binary version of the templates.

the assets.go file should get generated as part of make command in

pkg/assets/assets.go: .check_go_version.stamp .check_gopath.stamp
	go-bindata -o pkg/assets/assets.go -pkg assets pkg/paperwork/formtemplates/ pkg/notifications/templates/
@rdhariwal

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2019

image
Ran successfully for me. I ran make clean and then then after I had migrated the db make tasks_send_post_move_survey and it ran without any probs!

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@chrisgilmerproj
Copy link
Contributor

left a comment

I figured out the problem is that the make target in the Makefile wasn't referencing the assets.go file as a prerequisite. I think once that's fixed then we're ready to approve and merge this!

@mkrump

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2019

@chrisgilmerproj most recent series of commits b62b261 fc4d6c4 cover changes you asked for.

@chrisgilmerproj
Copy link
Contributor

left a comment

🚀 - Nice work!

@mkrump mkrump merged commit 373e944 into master Sep 5, 2019

16 checks passed

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_tasks 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 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/go 63.4% of diff hit (target 57%)
Details
codecov/project/go 57% (+0.1%) compared to a945eb7
Details

@mkrump mkrump deleted the ren-168035083-post-move-survey-email branch Sep 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.