Skip to content

LG-10074: Make usps enrollment email address configurable#9115

Merged
sheldon-b merged 7 commits intomainfrom
sbachstein/lg-10074-make-usps-enrollment-email-address-configurable
Aug 30, 2023
Merged

LG-10074: Make usps enrollment email address configurable#9115
sheldon-b merged 7 commits intomainfrom
sbachstein/lg-10074-make-usps-enrollment-email-address-configurable

Conversation

@sheldon-b
Copy link
Contributor

🎫 Ticket

LG-10074

🛠 Summary of changes

  • Adds a new configuration value for the email address that we send to USPS when we create USPS enrollments
  • Begins using the new configuration value, including falling back to 'no-reply@login.gov' if the configuration value is set to empty string

@@ -19,11 +19,17 @@
let(:service_provider) { nil }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This spec file has gotten pretty loose; I'm planning to submit a follow-up PR that refactors this file. Just an FYI

@sheldon-b sheldon-b marked this pull request as ready for review August 29, 2023 19:16
@sheldon-b sheldon-b requested review from a team and allthesignals August 29, 2023 19:16
Copy link
Contributor

@NavaTim NavaTim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, LGTM.

Comment on lines +66 to +67
email: IdentityConfig.store.usps_ipp_enrollment_status_update_email_address.presence ||
'no-reply@login.gov',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fallback isn't necessary with the fallback configuration in place (though you'll need to update the test if you accept this change).

Suggested change
email: IdentityConfig.store.usps_ipp_enrollment_status_update_email_address.presence ||
'no-reply@login.gov',
email: IdentityConfig.store.usps_ipp_enrollment_status_update_email_address,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was on the fence about it myself -- it's only useful if someone accidentally edits the config and specifies an empty string for the email address. And even then it will be a kind of sneaky intervention. Yet it's still a pretty common pattern (example) for there to be a hard-coded default on top of the configuration default.

I'll go ahead with your suggestion. Also going to start a thread in slack to hear other folks' thoughts on if there's a good reason to have the hard-coded default

@sheldon-b sheldon-b merged commit fa8c991 into main Aug 30, 2023
@sheldon-b sheldon-b deleted the sbachstein/lg-10074-make-usps-enrollment-email-address-configurable branch August 30, 2023 03:18
@mdiarra3 mdiarra3 mentioned this pull request Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants