Skip to content

LG-13355: filter piv upsell#10976

Merged
mdiarra3 merged 54 commits intomainfrom
LG-13355-filter-piv-upsell
Aug 28, 2024
Merged

LG-13355: filter piv upsell#10976
mdiarra3 merged 54 commits intomainfrom
LG-13355-filter-piv-upsell

Conversation

@mdiarra3
Copy link
Contributor

@mdiarra3 mdiarra3 commented Jul 23, 2024

🎫 Ticket

Link to the relevant ticket:
LG-13355

🛠 Summary of changes

This leverages using the Fed domain list to ensure we are only prompting users with Fed emails to add PIV or .mil users.

📜 Testing Plan

Provide a checklist of steps to confirm the changes.

  • Locally you can either download the full fed domain path by running ruby lib/fed_email_domain_downloader.rb to download the file locally. Move that file from tmp folder to fed_domain_emails folder. Or use the sample fed_email_domain sample file in the folder.
  • Set fed_domain_file_path to the fed_email_domain.txt and set use_fed_domain_file to true
  • Create an account with .mil or a valid .gov email domain,
  • Be directed to add a piv for valid domains or .mil. Ensure that when using an invalid domain you are not prompted for Emails.

@mdiarra3 mdiarra3 marked this pull request as ready for review July 25, 2024 20:42
Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Can you rebase to resolve merge conflicts? I'll plan to give this a closer look shortly.

@@ -0,0 +1,6 @@
module FederalEmailDomainHelper
def default_federal_domains
Copy link
Contributor

Choose a reason for hiding this comment

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

WDYT of naming this to indicate it has side effects?

Suggested change
def default_federal_domains
def set_up_default_federal_domains!

Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Works well in testing. Left a few non-blocking comments, but LGTM otherwise 👍

Comment on lines +11 to +14
csv = CSV.parse(response.body, col_sep: ',', headers: true)
csv.each do |row|
FederalEmailDomain.find_or_create_by(name: row['Domain name'])
end
Copy link
Contributor

Choose a reason for hiding this comment

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

This is probably fine since we're only dealing with about 1400 entries, but I'd wonder if this should work a little more like the disposable emails task using insert_all. I think upsert_all could be an option too if we wanted to gracefully handle duplicates. I think the "find" element here is kinda wasted since we don't care to do anything with the result, so we could also just insert and rescue and discard on a ActiveRecord::RecordNotUnique exception.

If this was a bigger file, I'd also suggest using CSV.foreach instead of CSV.parse + Array#each, since it'd process row-by-row instead of loading the whole thing into memory first.

end
end
end
# rake "fed_email_domains:load_to_db"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# rake "fed_email_domains:load_to_db"
# rake "federal_email_domains:load_to_db"

skip_encryption_allowed_list: '["urn:gov:gsa:SAML:2.0.profiles:sp:sso:dev", "urn:gov:gsa:SAML:2.0.profiles:sp:sso:int"]'
state_tracking_enabled: false
telephony_adapter: pinpoint
use_fed_domain_class: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Noting that this will cause an error if #11082 is merged before this. We could address the issue by only overriding where it's different from default.

@mdiarra3 mdiarra3 merged commit 4684179 into main Aug 28, 2024
@mdiarra3 mdiarra3 deleted the LG-13355-filter-piv-upsell branch August 28, 2024 13:13
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.

4 participants