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

Fix link domain name in PPM Info Sheet url in email to SM #2366

Merged
merged 5 commits into from Jul 16, 2019

Conversation

2 participants
@kimallen
Copy link
Contributor

commented Jul 9, 2019

Description

The office domain name was being applied to a url sent to the SM after approval of their PPM by the office user. This passes appnames through handlerContext to apply it as the Hostname when sending an email notification to the SM.

Setup

Not sure how to test locally since we do not send an email locally.
FWIW, as an office user, go to NOADVC ppm (in new moves queue). Approve Basics and the Approve PPM. This is when the email is sent.

Code Review Verification Steps

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

References

ppmInfoSheetURL := url.URL{
Scheme: "https",
Host: m.session.Hostname,
Host: m.milServername,

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 9, 2019

Contributor

I think you can get this from the ctx variable. Is that possible?

This comment has been minimized.

Copy link
@kimallen

kimallen Jul 9, 2019

Author Contributor

I tried ctx first, but it wasn't in there (that I could find). If there's a way to get it through that, I'm happy to poke around more.

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 9, 2019

Contributor

I did a little digging and you're right. The context coming from the handler is from the http request and doesn't contain the information from the handler context. You could potentially add it by using https://godoc.org/context#WithValue but I'm not sure that's the way you want to pass this information. So your method is the best one.

@chrisgilmerproj
Copy link
Contributor

left a comment

I think we might be able to make the change smaller. Can you see if the appnames are in the context variable ctx?

moveID: move.ID,
db: suite.DB(),
logger: suite.logger,
milServername: "milmovedomain",

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 9, 2019

Contributor

I'd prefer if even the test cases used the development name of milmovelocal.

This comment has been minimized.

Copy link
@kimallen

kimallen Jul 15, 2019

Author Contributor

edited

session *auth.Session // TODO - remove this when we move permissions up to handlers and out of models
db *pop.Connection
logger Logger
milServername string

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jul 9, 2019

Contributor

This might be clearer if the data was stored under domain or host since that's what we're doing. At some point we may be using this for other domains so we don't really want to hard code state into the object.

This comment has been minimized.

Copy link
@kimallen

kimallen Jul 15, 2019

Author Contributor

modified

@kimallen kimallen force-pushed the kim-166583211-ppm-info-sheet-domain branch from c25bbb6 to 7dfce92 Jul 15, 2019

@codecov

This comment has been minimized.

Copy link

commented Jul 15, 2019

Codecov Report

Merging #2366 into master will decrease coverage by 0.1%.
The diff coverage is 28.6%.

@@           Coverage Diff            @@
##           master   #2366     +/-   ##
========================================
- Coverage    59.7%   59.6%   -<.1%     
========================================
  Files         265     267      +2     
  Lines       14868   14893     +25     
========================================
+ Hits         8868    8872      +4     
- Misses       4967    4987     +20     
- Partials     1033    1034      +1
Impacted Files Coverage Δ
pkg/cli/webserver.go 38.5% <ø> (ø)
pkg/handlers/contexts.go 0% <0%> (ø) ⬆️
pkg/handlers/internalapi/office.go 58.5% <100%> (ø) ⬆️
pkg/notifications/move_approved.go 63.5% <50%> (-1.5%) ⬇️
pkg/services/accesscode/validate_access_code.go 75% <0%> (-25%) ⬇️
pkg/services/accesscode/fetch_access_code.go 100% <0%> (ø)
pkg/handlers/converters_strfmt.go 0% <0%> (ø)
@chrisgilmerproj
Copy link
Contributor

left a comment

🚀 - Nice job!

@kimallen kimallen merged commit 150e958 into master Jul 16, 2019

17 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_api 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

@kimallen kimallen deleted the kim-166583211-ppm-info-sheet-domain branch Jul 16, 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.