-
Notifications
You must be signed in to change notification settings - Fork 17
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
LG-13880: Add confirmation success message (fallback method) #1434
base: main
Are you sure you want to change the base?
Conversation
b85b7d1
to
70d50f1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, with suggestions 👍 Tested and works as expected
<!-- Note: Temp hardcode env var !--> | ||
<input type="hidden" name="retURL" value="https://federalist-17bd62cc-77b7-4687-9c62-39b462ce6fd5.sites.pages.cloud.gov/preview/gsa-tts/identity-site/LG-13880-add-confirmation-fallback{{ page.permalink | append: 'case-submitted/#success' }}" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we make these configurable values in _config.yml
so that we can override them in the Cloud.gov Pages backend without risking human error of temporary code changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're expecting consistent keys across locales for this data, I think it'd be good to enhance our existing spec to check for consistency in the new file. Right now it only checks data/*/settings.yml
.
identity-site/spec/data_spec.rb
Line 8 in efe225e
keys = Dir['_data/*/settings.yml'].map { |path| [path, hash_keys(YAML.load_file(path))] } |
Same with translations checker:
yaml = YAML.safe_load(File.read("#{folder}/en/settings.yml")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: I might suggest this be named to align to the page it's actually loaded on. Specifically, it's not loaded on the "Contact Us" page, but it is loaded on the case-submitted
page.
@@ -0,0 +1,15 @@ | |||
--- | |||
permalink: /contact/case-submitted/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we align the permalink with the filename? One is case-submitted
and the other is contact-submitted
describe 'English' do | ||
let(:lang) { 'en' } | ||
|
||
it 'includes permalink' do | ||
expect(settings['permalink']).to eq('/contact/case-submitted/') | ||
end | ||
end | ||
|
||
describe 'Spanish' do | ||
let(:lang) { 'es' } | ||
|
||
it 'includes permalink' do | ||
expect(settings['permalink']).to eq('/es/contact/case-submitted/') | ||
end | ||
end | ||
|
||
describe 'French' do | ||
let(:lang) { 'fr' } | ||
|
||
it 'includes permalink' do | ||
expect(settings['permalink']).to eq('/fr/contact/case-submitted/') | ||
end | ||
end | ||
|
||
describe 'Chinese' do | ||
let(:lang) { 'zh' } | ||
|
||
it 'includes permalink' do | ||
expect(settings['permalink']).to eq('/zh/contact/case-submitted/') | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make this more maintainable to possible future language additions, maybe we should loop over the canonical source of truth for available locales rather than manually listing them out?
We do this elsewhere, e.g.
identity-site/spec/data_spec.rb
Lines 21 to 22 in efe225e
it 'has directories for each supported language' do | |
SITE_CONFIG['languages'].each do |locale| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice helpers 🙌
🎫 Ticket
LG-13880
🛠 Summary of changes
Adds a success message to the contact form found at:
The above form does not use the same technique for submission found on a similar form at https://login.gov/partners/business-inquiries/ due to a lack over control over the setting
Access-Control-Allow-Origin
on Salesforce servers.Note: Some vars are temporarily overridden for staging.