Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -168,8 +168,10 @@ run-https: tmp/$(HOST)-$(PORT).key tmp/$(HOST)-$(PORT).crt ## Runs the developme

normalize_yaml: ## Normalizes YAML files (alphabetizes keys, fixes line length, smart quotes)
yarn normalize-yaml .rubocop.yml --disable-sort-keys --disable-smart-punctuation
find ./config/locales/transliterate -type f -name '*.yml' -exec yarn normalize-yaml --disable-sort-keys --disable-smart-punctuation {} \;
find ./config/locales/telephony -type f -name '*.yml' | xargs yarn normalize-yaml --disable-smart-punctuation
find ./config/locales -not -path "./config/locales/telephony*" -type f -name '*.yml' | xargs yarn normalize-yaml \
find ./config/locales -not \( -path "./config/locales/telephony*" -o -path "./config/locales/transliterate/*" \) -type f -name '*.yml' | \
xargs yarn normalize-yaml \
config/pinpoint_supported_countries.yml \
config/pinpoint_overrides.yml \
config/country_dialing_codes.yml
Expand Down
23 changes: 19 additions & 4 deletions app/services/usps_in_person_proofing/enrollment_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,10 @@ def create_usps_enrollment(enrollment, pii)
applicant = UspsInPersonProofing::Applicant.new(
{
unique_id: unique_id,
first_name: pii['first_name'],
last_name: pii['last_name'],
address: address,
city: pii['city'],
first_name: transliterate(pii['first_name']),
last_name: transliterate(pii['last_name']),
address: transliterate(address),
city: transliterate(pii['city']),
state: pii['state'],
zip_code: pii['zipcode'],
email: 'no-reply@login.gov',
Expand Down Expand Up @@ -98,6 +98,21 @@ def handle_standard_error(err, enrollment)
def analytics(user: AnonymousUser.new)
Analytics.new(user: user, request: nil, session: {}, sp: nil)
end

def transliterate(value)
return value unless IdentityConfig.store.usps_ipp_transliteration_enabled

result = transliterator.transliterate(value)
if result.unsupported_chars.present?
result.original
else
result.transliterated
end
end

def transliterator
Transliterator.new
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this is stateless, so we could micro-optimize and save a few allocations by memoizing

or, we could probably just make transliterate a static method?

Suggested change
Transliterator.new
@transliterator ||= Transliterator.new

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is exactly what I tried before, and it caused the mock to be preserved between tests (and therefore the tests failed). I also don't think this micro-optimization has a high payoff.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

oh I just saw that there's a class << self around all of this, no wonder there was test pollution. It's probably time to maybe split this class up and maybe maybe it stateful (ex InPersonEnroller.new(user).create_enrollment vs generic Helper), but that's definitely outside the scope of this PR

end
end
end
end
45 changes: 45 additions & 0 deletions app/services/usps_in_person_proofing/transliterator.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
module UspsInPersonProofing
class Transliterator
# This is the default. May not be able to override this in current version.
REPLACEMENT = '?'.freeze

# Container to hold the results of transliteration
TransliterationResult = Struct.new(
# Was the value different after transliteration?
:changed?,
# Original value submtted for transliteration
:original,
# Transliterated value
:transliterated,
# Characters from the original that could not be transliterated
:unsupported_chars,
keyword_init: true,
)

# Transliterate values for usage in the USPS API. This will additionally strip/reduce
# excess whitespace and check for special characters that are unsupported by transliteration.
# Additional validation may be necessary depending on the specific field being transliterated.
#
# @param [String,#to_s] value The value to transliterate for USPS
# @return [TransliterationResult] The transliterated value
def transliterate(value)
stripped = value.to_s.gsub(/\s+/, ' ').strip
transliterated = I18n.transliterate(stripped, locale: :en)

unsupported_chars = []
unless stripped.count(REPLACEMENT) == transliterated.count(REPLACEMENT)
transliterated.chars.zip(stripped.chars).each do |val, stripped|
unsupported_chars.append(stripped) if val == REPLACEMENT && stripped != REPLACEMENT
end
end

# Using struct instead of exception here to reduce likelihood of logging PII
TransliterationResult.new(
changed?: value != transliterated,
original: value,
transliterated: transliterated,
unsupported_chars: unsupported_chars,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does it make more sense for unsupported_chars to be a boolean? Are we going to refer to the array values to update the characters in the transliterate/en.yml file?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Does it make more sense for unsupported_chars to be a boolean?

No, we need to the list of characters to generate the error message required by the story.

Are we going to refer to the array values to update the characters in the transliterate/en.yml file?

No. Monitoring transliteration in-detail is out-of-scope, and the characters could in some cases amount to additional recorded PII.

)
end
end
end
2 changes: 2 additions & 0 deletions config/application.yml.default
Original file line number Diff line number Diff line change
Expand Up @@ -329,6 +329,7 @@ usps_ipp_request_timeout: 10
usps_ipp_sponsor_id: ''
usps_ipp_username: ''
usps_mock_fallback: true
usps_ipp_transliteration_enabled: false
get_usps_proofing_results_job_cron: '0/10 * * * *'
get_usps_proofing_results_job_reprocess_delay_minutes: 5
get_usps_proofing_results_job_request_delay_milliseconds: 1000
Expand Down Expand Up @@ -398,6 +399,7 @@ development:
state_tracking_enabled: true
telephony_adapter: test
use_dashboard_service_providers: true
usps_ipp_transliteration_enabled: true
usps_upload_sftp_directory: "/gsa_order"
usps_upload_sftp_host: localhost
usps_upload_sftp_password: test
Expand Down
3 changes: 2 additions & 1 deletion config/i18n-tasks.yml
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,8 @@ ignore_unused:
# - common.brand

## Ignore these keys completely:
# ignore:
ignore:
- 'i18n.transliterate.rule.*'
# - kaminari.*

## Sometimes, it isn't possible for i18n-tasks to match the key correctly,
Expand Down
23 changes: 23 additions & 0 deletions config/locales/transliterate/en.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
en:
i18n:
transliterate:
rule:
# Convert okina to apostrophe
ʻ: "'"
# Convert quotation marks
’: "'"
‘: "'"
‛: "'"
“: '"'
‟: '"'
”: '"'
# Convert hyphens
‐: '-'
‑: '-'
‒: '-'
–: '-'
—: '-'
﹘: '-'
# Convert number signs
﹟: '#'
#: '#'
1 change: 1 addition & 0 deletions lib/identity_config.rb
Original file line number Diff line number Diff line change
Expand Up @@ -427,6 +427,7 @@ def self.build_store(config_map)
config.add(:usps_ipp_username, type: :string)
config.add(:usps_ipp_request_timeout, type: :integer)
config.add(:usps_upload_enabled, type: :boolean)
config.add(:usps_ipp_transliteration_enabled, type: :boolean)
config.add(:get_usps_proofing_results_job_cron, type: :string)
config.add(:get_usps_proofing_results_job_reprocess_delay_minutes, type: :integer)
config.add(:get_usps_proofing_results_job_request_delay_milliseconds, type: :integer)
Expand Down
26 changes: 16 additions & 10 deletions spec/i18n_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ class BaseTask
{ key: 'account.navigation.menu', locales: %i[fr] }, # "Menu" is "Menu" in French
{ key: 'doc_auth.headings.photo', locales: %i[fr] }, # "Photo" is "Photo" in French
{ key: /^i18n\.locale\./ }, # Show locale options translated as that language
{ key: /^i18n\.transliterate\./ }, # Approximate non-ASCII characters in ASCII
{ key: /^countries/ }, # Some countries have the same name across languages
{ key: 'links.contact', locales: %i[fr] }, # "Contact" is "Contact" in French
{ key: 'simple_form.no', locales: %i[es] }, # "No" is "No" in Spanish
Expand Down Expand Up @@ -89,7 +90,9 @@ def allowed_untranslated_key?(locale, key)
missing_interpolation_argument_keys = []

i18n.data[i18n.base_locale].select_keys do |key, _node|
next if i18n.t(key).is_a?(Array) || i18n.t(key).nil?
if key.start_with?('i18n.transliterate.rule.') || i18n.t(key).is_a?(Array) || i18n.t(key).nil?
next
end

interpolation_arguments = i18n.locales.map do |locale|
extract_interpolation_arguments i18n.t(key, locale)
Expand All @@ -109,20 +112,23 @@ def allowed_untranslated_key?(locale, key)
i18n_file = full_path.sub("#{root_dir}/", '')

describe i18n_file do
it 'has only lower_snake_case keys' do
keys = flatten_hash(YAML.load_file(full_path)).keys
# Transliteration includes special characters by definition, so it could fail the below checks
if !full_path.match?(%(/config/locales/transliterate/))
it 'has only lower_snake_case keys' do
keys = flatten_hash(YAML.load_file(full_path)).keys

bad_keys = keys.reject { |key| key =~ /^[a-z0-9_.]+$/ }
bad_keys = keys.reject { |key| key =~ /^[a-z0-9_.]+$/ }

expect(bad_keys).to be_empty
end
expect(bad_keys).to be_empty
end

it 'has only has XML-safe identifiers (keys start with a letter)' do
keys = flatten_hash(YAML.load_file(full_path)).keys
it 'has only has XML-safe identifiers (keys start with a letter)' do
keys = flatten_hash(YAML.load_file(full_path)).keys

bad_keys = keys.select { |key| key.split('.').any? { |part| part =~ /^[0-9]/ } }
bad_keys = keys.select { |key| key.split('.').any? { |part| part =~ /^[0-9]/ } }

expect(bad_keys).to be_empty
expect(bad_keys).to be_empty
end
end

it 'has correctly-formatted interpolation values' do
Expand Down
123 changes: 100 additions & 23 deletions spec/services/usps_in_person_proofing/enrollment_helper_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,24 @@
merge(same_address_as_id: current_address_matches_id).
transform_keys(&:to_s)
end
let(:subject) { described_class }
subject(:subject) { described_class }
let(:subject_analytics) { FakeAnalytics.new }
let(:transliterator) { UspsInPersonProofing::Transliterator.new }
let(:service_provider) { nil }
let(:usps_ipp_transliteration_enabled) { true }

before(:each) do
stub_request_token
stub_request_enroll
allow(IdentityConfig.store).to receive(:usps_mock_fallback).and_return(usps_mock_fallback)
allow(subject).to receive(:transliterator).and_return(transliterator)
allow(transliterator).to receive(:transliterate).
with(anything) do |val|
transliterated_without_change(val)
end
allow(subject).to receive(:analytics).and_return(subject_analytics)
allow(IdentityConfig.store).to receive(:usps_ipp_transliteration_enabled).
and_return(usps_ipp_transliteration_enabled)
end

describe '#schedule_in_person_enrollment' do
Expand Down Expand Up @@ -58,38 +67,79 @@
expect(enrollment.current_address_matches_id).to eq(current_address_matches_id)
end

it 'creates usps enrollment' do
proofer = UspsInPersonProofing::Mock::Proofer.new
mock = double

expect(UspsInPersonProofing::Proofer).to receive(:new).and_return(mock)
expect(mock).to receive(:request_enroll) do |applicant|
expect(applicant.first_name).to eq(Idp::Constants::MOCK_IDV_APPLICANT[:first_name])
expect(applicant.last_name).to eq(Idp::Constants::MOCK_IDV_APPLICANT[:last_name])
expect(applicant.address).to eq(Idp::Constants::MOCK_IDV_APPLICANT[:address1])
expect(applicant.city).to eq(Idp::Constants::MOCK_IDV_APPLICANT[:city])
expect(applicant.state).to eq(Idp::Constants::MOCK_IDV_APPLICANT[:state])
expect(applicant.zip_code).to eq(Idp::Constants::MOCK_IDV_APPLICANT[:zipcode])
expect(applicant.email).to eq('no-reply@login.gov')
expect(applicant.unique_id).to eq(enrollment.unique_id)

proofer.request_enroll(applicant)
context 'transliteration disabled' do
let(:usps_ipp_transliteration_enabled) { false }

it 'creates usps enrollment without using transliteration' do
mock_proofer = double(UspsInPersonProofing::Mock::Proofer)
expect(subject).to receive(:usps_proofer).and_return(mock_proofer)

expect(transliterator).not_to receive(:transliterate)
expect(mock_proofer).to receive(:request_enroll) do |applicant|
expect(applicant.first_name).to eq(Idp::Constants::MOCK_IDV_APPLICANT[:first_name])
expect(applicant.last_name).to eq(Idp::Constants::MOCK_IDV_APPLICANT[:last_name])
expect(applicant.address).to eq(Idp::Constants::MOCK_IDV_APPLICANT[:address1])
expect(applicant.city).to eq(Idp::Constants::MOCK_IDV_APPLICANT[:city])
expect(applicant.state).to eq(Idp::Constants::MOCK_IDV_APPLICANT[:state])
expect(applicant.zip_code).to eq(Idp::Constants::MOCK_IDV_APPLICANT[:zipcode])
expect(applicant.email).to eq('no-reply@login.gov')
expect(applicant.unique_id).to eq(enrollment.unique_id)

UspsInPersonProofing::Mock::Proofer.new.request_enroll(applicant)
end

subject.schedule_in_person_enrollment(user, pii)
end
end

subject.schedule_in_person_enrollment(user, pii)
context 'transliteration enabled' do
let(:usps_ipp_transliteration_enabled) { true }

it 'creates usps enrollment while using transliteration' do
mock_proofer = double(UspsInPersonProofing::Mock::Proofer)
expect(subject).to receive(:usps_proofer).and_return(mock_proofer)

first_name = Idp::Constants::MOCK_IDV_APPLICANT[:first_name]
last_name = Idp::Constants::MOCK_IDV_APPLICANT[:last_name]
address = Idp::Constants::MOCK_IDV_APPLICANT[:address1]
city = Idp::Constants::MOCK_IDV_APPLICANT[:city]

expect(transliterator).to receive(:transliterate).
with(first_name).and_return(transliterated_without_change(first_name))
expect(transliterator).to receive(:transliterate).
with(last_name).and_return(transliterated(last_name))
expect(transliterator).to receive(:transliterate).
with(address).and_return(transliterated_with_failure(address))
expect(transliterator).to receive(:transliterate).
with(city).and_return(transliterated(city))

expect(mock_proofer).to receive(:request_enroll) do |applicant|
expect(applicant.first_name).to eq(first_name)
expect(applicant.last_name).to eq("transliterated_#{last_name}")
expect(applicant.address).to eq(address)
expect(applicant.city).to eq("transliterated_#{city}")
expect(applicant.state).to eq(Idp::Constants::MOCK_IDV_APPLICANT[:state])
expect(applicant.zip_code).to eq(Idp::Constants::MOCK_IDV_APPLICANT[:zipcode])
expect(applicant.email).to eq('no-reply@login.gov')
expect(applicant.unique_id).to eq(enrollment.unique_id)

UspsInPersonProofing::Mock::Proofer.new.request_enroll(applicant)
end

subject.schedule_in_person_enrollment(user, pii)
end
end

context 'when the enrollment does not have a unique ID' do
it 'uses the deprecated InPersonEnrollment#usps_unique_id value to create the enrollment' do
enrollment.update(unique_id: nil)
proofer = UspsInPersonProofing::Mock::Proofer.new
mock = double
mock_proofer = double(UspsInPersonProofing::Mock::Proofer)
expect(subject).to receive(:usps_proofer).and_return(mock_proofer)

expect(UspsInPersonProofing::Proofer).to receive(:new).and_return(mock)
expect(mock).to receive(:request_enroll) do |applicant|
expect(mock_proofer).to receive(:request_enroll) do |applicant|
expect(applicant.unique_id).to eq(enrollment.usps_unique_id)

proofer.request_enroll(applicant)
UspsInPersonProofing::Mock::Proofer.new.request_enroll(applicant)
end

subject.schedule_in_person_enrollment(user, pii)
Expand Down Expand Up @@ -146,4 +196,31 @@
end
end
end

def transliterated_without_change(value)
UspsInPersonProofing::Transliterator::TransliterationResult.new(
changed?: false,
original: value,
transliterated: value,
unsupported_chars: [],
)
end

def transliterated(value)
UspsInPersonProofing::Transliterator::TransliterationResult.new(
changed?: true,
original: value,
transliterated: "transliterated_#{value}",
unsupported_chars: [],
)
end

def transliterated_with_failure(value)
UspsInPersonProofing::Transliterator::TransliterationResult.new(
changed?: true,
original: value,
transliterated: "transliterated_failed_#{value}",
unsupported_chars: [':'],
)
end
end
Loading