Add tests to check for matching HTML tags across locales#8847
Merged
Conversation
changelog: Internal, Automated Testing, Add test to check for matching HTML tags across languages
aduth
commented
Jul 24, 2023
Comment on lines
+258
to
+259
| de autenticación, o la clave de seguridad.</li><li><strong>En <a | ||
| href="%{account_url}">la página de su cuenta %{app_name}</a>, solicite |
Contributor
Author
There was a problem hiding this comment.
English version has a third link "your Login.gov account page", which was not reflected in the other locales:
identity-idp/config/locales/user_mailer/en.yml
Lines 242 to 243 in 0f21a2a
Contributor
There was a problem hiding this comment.
this is a good candidate for one translation string that should be broken into 2-3 and we should move the markup into the template IMO
Contributor
Author
There was a problem hiding this comment.
Yeah, the email templates in general could use a lot of love 😬 Not going to try to tackle that here.
zachmargolis
approved these changes
Jul 24, 2023
| heading: Nous n’avons pas pu vous associer à ce numéro | ||
| learn_more_link: 'Apprenez-en plus sur quel numéro de téléphone utiliser' | ||
| next_steps_html: 'Essayez <strtong>un autre<strong> numéro que vous utilisez | ||
| next_steps_html: 'Essayez <strong>un autre</strong> numéro que vous utilisez |
Contributor
There was a problem hiding this comment.
wow yikes glad we're making this linter
Comment on lines
+258
to
+259
| de autenticación, o la clave de seguridad.</li><li><strong>En <a | ||
| href="%{account_url}">la página de su cuenta %{app_name}</a>, solicite |
Contributor
There was a problem hiding this comment.
this is a good candidate for one translation string that should be broken into 2-3 and we should move the markup into the template IMO
Comment on lines
+116
to
+118
| html_unique_tags = i18n.locales.map { |locale| i18n.t(key, locale)&.scan(/<.+?>/) }.uniq | ||
|
|
||
| expect(html_unique_tags.size).to eq(1), "HTML tag mismatch for key #{key}" |
Contributor
There was a problem hiding this comment.
maybe we can change the structure of this or the message to indicate what the tags are?
Suggested change
| html_unique_tags = i18n.locales.map { |locale| i18n.t(key, locale)&.scan(/<.+?>/) }.uniq | |
| expect(html_unique_tags.size).to eq(1), "HTML tag mismatch for key #{key}" | |
| html_unique_tags = i18n.locales.map { |locale| i18n.t(key, locale)&.scan(/<.+?>/) }.uniq | |
| expect(html_unique_tags.size).to eq(1), "HTML tag mismatch for key: #{key}, tags: #{html_unique_tags}" |
Contributor
Author
There was a problem hiding this comment.
Yeah, I actually added that locally to help troubleshoot the existing issues, so probably good to include something similar, though hopefully moving forward it should be obvious enough to the person adding the new locale strings that there might be a problem with their tags.
As proposed, it wasn't especially helpful to debugging some of the more gnarly strings like the one you mentioned, since there's so many tags that it's hard to parse and make use of the output.
Contributor
Author
There was a problem hiding this comment.
Since I happen to have the history in my terminal, here's what it ended up looking like for that one 🙈 (this was before I uniq'd it, but not a whole lot better even after)
1) I18n has matching HTML tags
Failure/Error: expect(html_tags.size).to eq(1), "HTML tag mismatch for key #{key}: #{html_tags}"
HTML tag mismatch for key user_mailer.personal_key_sign_in.help_html: [["<p>", "</p>", "<p>", "</p>", "<p>", "<ol>", "<li>", "<strong>", "<a href=\"%{reset_password_url}\">", "</a>", "</strong>", "</li>", "<li>", "<strong>", "<a href=\"%{account_url}\">", "</a>", "</strong>", "</li>", "<li>", "<strong>", "<a href=\"%{account_url}\">", "</a>", "</strong>", "</li>", "</ol>", "</p>", "<br>", "<br>"], ["<p>", "</p>", "<p>", "</p>", "<p>", "<ol>", "<li>", "<strong>", "<a href=\"%{reset_password_url}\">", "</a>", "</strong>", "</li>", "<li>", "<strong>", "<a href=\"%{account_url}\">", "</a>", "</strong>", "</li>", "<li>", "<strong>", "</strong>", "</li>", "</ol>", "</p>", "<br>", "<br>"]]
Contributor
There was a problem hiding this comment.
ah I see your point. I guess it's fine as-is and having the key name is enough to debug
Merged
amirbey
added a commit
that referenced
this pull request
Jul 25, 2023
* LG-10288: Remove duplication of doc_auth_* threshold variables from application.yml (#8803) * LG-10288: remove duplicated entries. * changelog: Internal, Document authentication, Clean up duplicated config entries (LG-10288) * Remove enabled rewrite_oidc_request_prompt feature flag (#8754) changelog: Internal, Maintenance, Remove enabled rewrite_oidc_request_prompt feature flag * LG-10255: Display inline error messages when Acuant SDK recognizes a document is not an ID card type (#8789) * display error messages * changelog: Bug Fixes, Docauth, block submission of non-ids * fix build * add testing for unsupported doc error message and log * refactor in response to feedback * clean up variable names in CaptureAttemptMetadata * LG-10405: Add new placeholder manual address entry page behind a flag (#8808) * Add new blank manual entry page behind a flag changelog: User-Facing Improvements, In-person full address entry, Add scaffolding for manual address entry * Remove unnecessary code * Remove unused effect hook * changelog: User-Facing Improvements, In-person proofing, This changes the number of attempts using the acuant SDK before the fallback to the native camera (#8793) * LG-7355 new Getting Started page (#8794) * New GettingStartedController copied from WelcomeController changelog: User-facing Improvements, Identity Verification, New Getting Started experience behind A/B test * Remove StepIndicator from GettingStartedController * Add GettingStartedAbTestConcern and before action in WelcomeController * Specs for ABTest behavior * Add config flag and ABTestBucket. * Update getting_started_controller_spec and analytics * Add agreement checkbox and specs * Change the setup in getting_started_ab_test_concern_spec * Remove unneeded components from Getting Started template and add feature spec * Add new translation keys * Add new translation text and page formatting * Register both welcome and agreement steps in DocAuthLog * Lint * Test agreement checkbox in getting started feature spec * Remove unneeded locals when rendering show in WelcomeController * Add Agreement feature specs and make them pass * I18n lint * Clean up Funnel::DocAuth::RegisterStep code layout * Replace no_sp_name with APP_NAME when there's no SP * Add show template spec and external_redirect analytics spec Fix help center link by adding entry to MarketingSite * Update spec to match analytics arg change * Change A/B test buckets to :welcome and :getting_started instead of :default and :new * Use FakeABTestBucket to avoid any_instance stub * Only redirect if bucket is :getting_started, stay on Welcome for all other values --------- Co-authored-by: John Maxwell <john.maxwell@gsa.gov> Co-authored-by: Jessica Dembe <jessica.dembe@gsa.gov> Co-authored-by: Alexander Bradley <alexander.bradley@gsa.gov> * Clear `fraud_pending_reason` when activating fraud pending profiles when TMx is disabled (#8809) When ThreatMetrix decisioning is not enabled we do not put users through the fraud review process after they enter their GPO code. This requires clearing out the fraud review columns on their profile. Previously, this was done by explicitly overwriting the fraud review columns on their profile. We are trying to move away from this pattern and move these methods into the profile. While we were explicitly overwriting the columns we missed that we need to also overwrite `fraud_pending_reason` which was added later than the other columns. This commit adds a method to the profile for activating the profile when fraud review is not necessary. That method is used in the verify form under these conditions instead of explicitly modifying the profile. [skip changelog] * LG-10167: Delete notification phone no when associated enrollment canceled (#8805) * LG-10167: Delete phone no when enrollment is canceled changelog: Internal, In-person proofing, Delete phone number when enrollment cancelled * Add ECR reporting to gitlab (#8781) * changelog: Improvements, Scanning, Add ECR scans * Refactor Acuant document detection to use more enum features (#8811) * Use ?? to simplify label function * Make enum uppercase, use String.toLowerCase() * Remove AcuantDocumentTypeLabel because now the strings are lowercase --------- Co-authored-by: Andrew Duthie <aduth@users.noreply.github.com> * Only let users who are fraud review pending visit the please call controller (#8812) The please call controller is used to instruct users who are pending fraud review to contact Login.gov to finish verification. This controller should only be accessible to users who are fraud review. This commit adds a before action to redirect users in the following scenarios: 1. The user is not fraud review pending 2. The user has been fraud rejected (these users get a different message rendered by a different controller) This commit also fixes a latent 500 error on this controller. A user who signs in but is not fraud pending would encounter a `NoMethodError` at this line: ```ruby pending_at = current_user.fraud_review_pending_profile.fraud_review_pending_at ``` [skip changelog] * LG-9968 Read from `fraud_pending_reason` to find fraud pending profiles (#8814) This commit makes the first change to switch over to the new `fraud_pending_reason` column. It reads from the column instead of the `fraud_review_pending_at` timestamp to determine if a profile is fraud review pending. This will allow us to stop writing to the `fraud_review_pending_at` timestamp until the user actually enters the fraud review workflow and go back and make the value nil for users who have not entered fraud review. Eventually we will need to consider the presence of `fraud_pending_reason` and absence of `fraud_review_pending_at` as a new state (i.e. the user has not started fraud review but is eligible to start fraud review once they verify their address). That is out of scope for this PR which is intended to switch the reads to make that new state possible in a future release. changelog: Internal, Profile State, The code that computes whether a user is fraud review pending was changed to compute the fraud review pending status based off of the fraud_pending_reason column and fraud_rejection_at column to remove the fraud_review_pending_at column from the computation and allow the way that column is written to be changed in a future deploy. * Add EmailNormalizer (LG-10313) (#8818) - Helper class we can use to block associated emails changelog: Internal, User suspension, Add code to help normalize emails * Fix crasher in Telephony::AlertSender (#8823) Missing `event:` named arg results in `wrong number of arguments (given 1, expected 0; required keyword: event)` error. (The spec was stubbing but not calling the original implementation, which is why the tests were green before.) changelog: Bug Fixes, Telephony, Add missing named argument to telephony logging call. * LG-10405: Add support for using HTML select elements in document capture React page (#8820) * LG-10405: Add support for using HTML select elements in document capture React page [skip changelog] * LG-10405: Fix type for registerField * LG-10405: Fix lint issue * LG-10390: Remove IRS reproofing from IdV welcome / getting started (#8817) * LG-10390: Remove irs reproofing from welcome Remove references to reproofing for IRS from the welcome / getting started pages changelog: Internal, IRS cleanup, Remove references to IRS reproofing from welcome/getting started pages. * Remove .usa-alert--info-important style No longer used. * Remove unused image * LG-10224: Doc auth error page shows styling tag hypertext to users (#8828) * LG-10224: use same style for ipp and non-ipp. * changelog: User-facing Improvements, Doc Authentication, remaining attempts text issue * LG-10224: lint * changelog: User-Facing Improvements, Doc authentication, Remaining attempts text issue (LG-10224) * LG-10224: address comment. * Render TOTP help text in verification view (#8826) changelog: Internal, MFA Verification, Refactor verification presenters to simplify fallback options * LG-10405: Implement full address PO search page behind feature flag (#8829) * Sloppy working search prototype * Fix labels and error messages * Don't search if fields are missing * Basic feature test * Check for whitespace, not empty strings * Reuse exports from address-search component * Refactor feature flag to use in person context * Fix type errors * Add spec for full address search component * Add additional specs for full address entry page * LG-10405: Implement state select in manual address entry form (#8825) * use select component on page w/sample options --------- Co-authored-by: Timothy Bradley <timothy.bradley@gsa.gov> * changelog: User-Facing Improvements, In-person full address entry, Add full address PO search form --------- Co-authored-by: Tomas Apodaca <thomas.apodaca@gsa.gov> Co-authored-by: Timothy Bradley <timothy.bradley@gsa.gov> * Lg 10190 remove reproof at from profiles 1 of 2 (#8831) * Ignore `reproof_at` in `Profile` class changelog: Internal, Maintenance, Remove unused database column * Remove `reproof_at` test in profile maker spec * Generalize MFA learn more locale string key (#8827) changelog: Internal, MFA Verification, Generalize MFA learn more locale string key * LG-7355 GettingStarted A/B analytics (#8822) * GettingStarted A/B test uses user uuid, not session id * Prepare to remove StepUtilitiesConcern Include in IdvSession concern and remove inclusion everywhere else * Move StepUtilitiesConcern methods into IdvSession concern Because two of the methods reference document_capture_session which is related to flow_session and may move into idv_session And irs_reproofing? will probably be deleted soon * Replace acuant_sdk_ab_test_analytics_args with ab_test_analytics_args Currently defined identically in Idv::AbTestAnalyticsConcern * Remove unneeded ** before merge of ab_test_analytics_args * Remove unneeded native_camera_ab_testing_variables method from hybrid_mobile document capture controller * Add specs for AbTestAnalyticsConcern and then mock in controller specs This allows adding and removing A/B tests without editing all these specs Also add GettingStarted A/B test to ab_test_analytics_args * Update analytics_spec with getting started analytics bucket This spec will still need to be updated by hand whenever A/B test analytics are added or removed. * Rename getting_started_a_b_test_bucket to getting_started_ab_test_bucket for consistency changelog: Improvements, Logging, Generalize analytics logging for A/B tests in Identity Verification and add logging for Getting Started A/B test * Rename ab_test_analytics_args to ab_test_analytics_buckets * Get uuid from document_capture_user in hybrid flow Rename getting_started_ab_test_analytics_args to getting_started_ab_test_analytics_bucket Add navigation to Welcome page in hybrid_mobile_spec to ensure that it doesn't 500 in A/B test before_action * Add a/b test bucket info to analytics for GettingStarted, Welcome, Agreement For completeness for future A/B tests, and for possible ease of queries * Add A/B test buckets to verify proofing results analytics * Clean up getting_started_ab_test_concern_spec * Extract method to choose document_capture_user or current_user to make more testable, and add specs * Add ab test analytics to review_controller visited and submitted events * Fix analytics_spec for review events --------- Co-authored-by: Matt Hinz <matt.hinz@gsa.gov> Co-authored-by: Jonathan Hooper <jonathan.hooper@gsa.gov> Co-authored-by: Zach Margolis <zach.margolis@gsa.gov> * Allow setting remembered device preference when logging in with backup codes (LG-10434) (#8835) * Allow setting remembered device preference when logging in with backup codes changelog: Improvements, Remembered Device, Allow setting remembered device preference when logging in with backup codes Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov> * fix spacing Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov> * Update spec/controllers/two_factor_authentication/backup_code_verification_controller_spec.rb Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com> * remove stubbing of remember device cookie * fix spec --------- Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov> Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com> * Increase code coverage for offline mode of EmailNormalizer (#8832) [skip changelog] * Log when user is logged out after partially authenticating (#8838) changelog: Internal, Logging, Log when user is logged out after partially authenticating * changelog: User-Facing Improvements, In-person proofing, Change the number of -submission- attempts using the Acuant SDK. (#8807) * LG-9968 Only write to fraud_review_pending_at when a user starts the fraud review process (#8821) * LG-9968 Only write to fraud_review_pending when a user starts the fraud review process In #8814 we started the process of changing how we manage fraud review pending profiles. The fraud review workflow takes place after the user has completed all of the other identity verification workflow steps. Functionally this means is comes immediatly after the IdV flow for users who verify with a phone and after GPO code entry for users who verify by mail. In the old model, users marked for fraud review are considered in "fraud review" from the moment their profile is created up until they complete the fraud review workflow. This is problematic because functionally we do not make the decision about whether a user is in fraud review until the moment they are about to enter the workflow. For the out-of-band verify-by-mail flow this means several months can pass and business logic can change before we make a decision about whether the user should enter the fraud review workflow. To address the above we need to change the logic around the fraud review process: 1. For users who verify by phone, we need to mark them fraud review pending as soon as they finish all prior verification steps. This means at the time they enter their password to secure their account. 2. For users who verify by mail, we need to wait until after they enter their GPO code to mark them fraud review pending. These users should not be fraud review pending until such time. To enable the above, we have added a `fraud_pending_reason` column. This column tracks the reason a user would be fraud pending. The `fraud_review_pending_at` column will always mark the time the user began the fraud review workflow and we will only set it immediatly before the user begins that workflow. This means that GPO users will not have a `fraud_review_pending_at` value when their profile is created; they will have a `fraud_pending_reason` value. To enable the migration to this new pattern, we have started using the presence of `fraud_pending_reason` to determine if a user is fraud pending. This allows us to maintain the legacy behavior of considering all users marked for fraud review as in fraud review from the moment their profile is created. This profile changes the write behavior of `fraud_review_pending_at` to only be set after the user is entered the fraud review workflow. This commit also includes a backfill job to square up old records. Once this has been deployed and the backfill job has been run we can move the logic to determine if a user is fraud review pending to inspect the `fraud_review_pending_at` column. [skip changelog] * remove unused method * break if a profile does not have a fraud pending reason * Prompt users to accept rules of use when account was set up prior to rules of use change (#8836) changelog: Bug Fixes, Rules of Use, Prompt users to accept rules of use when account was set up prior to rules of use change Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov> * Fix Rubocop namespace deprecation (#8839) [skip changelog] * LG-10248: Move SMS job to appropriate queue; remove redundant update (#8841) [skip changelog] * Hide doc capture tips from users (#8842) * increase doc_auth_max_capture_attempts_before_tips to hide capture tips * changelog: Bug Fixes, Docauth, don't display doc capture tips * change spacing * Add tests to check for matching HTML tags across locales (#8847) changelog: Internal, Automated Testing, Add test to check for matching HTML tags across languages --------- Co-authored-by: dawei-nava <130466753+dawei-nava@users.noreply.github.com> Co-authored-by: Mitchell Henke <mitchell.henke@gsa.gov> Co-authored-by: eileen-nava <80347702+eileen-nava@users.noreply.github.com> Co-authored-by: Sheldon Bachstein <sheldon.bachstein@gsa.gov> Co-authored-by: Charley Ferguson <charleyferguson@navapbc.com> Co-authored-by: Sonia Connolly <sonia.connolly@gsa.gov> Co-authored-by: John Maxwell <john.maxwell@gsa.gov> Co-authored-by: Jessica Dembe <jessica.dembe@gsa.gov> Co-authored-by: Alexander Bradley <alexander.bradley@gsa.gov> Co-authored-by: Jonathan Hooper <jonathan.hooper@gsa.gov> Co-authored-by: Brittany Greaner <35475380+night-jellyfish@users.noreply.github.com> Co-authored-by: Stephen Shelton <stephen.shelton@gsa.gov> Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com> Co-authored-by: Andrew Duthie <aduth@users.noreply.github.com> Co-authored-by: Matt Hinz <matt.hinz@gsa.gov> Co-authored-by: Tim Bradley <90272033+NavaTim@users.noreply.github.com> Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov> Co-authored-by: Tomas Apodaca <thomas.apodaca@gsa.gov> Co-authored-by: Timothy Bradley <timothy.bradley@gsa.gov> Co-authored-by: Kimball Bighorse <kimball.bighorse@gsa.gov> Co-authored-by: Zach Margolis <zach.margolis@gsa.gov>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🛠 Summary of changes
Adds a test case which checks for matching sets of HTML tags across all locales for a given string key, and resolves current issues.
See discussion: #8837 (comment)
📜 Testing Plan
Confirm no regressions in affected string usage.