Skip to content

Fyst 1730 strip any values that are not allowed from middle initial field #5478

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Jan 30, 2025
Merged

Fyst 1730 strip any values that are not allowed from middle initial field #5478

merged 4 commits into from
Jan 30, 2025
+41 −9

Conversation

arinchoi03
Copy link
Contributor

@arinchoi03 arinchoi03 commented Jan 29, 2025 •
edited
Loading

Link to pivotal/JIRA issue

Is PM acceptance required? (delete one)

  • Yes - don't merge until JIRA issue is accepted!

Reminder: merge main into this branch and get green tests before merging to main

What was done?

  • Stripping all values that is not allowed -- will regular sanitize (sanitize_for_xml) first to without limiting length to 1 incase an acceptable letter was somehow provided for this field, then after rejecting unacceptable characters, limit to 1
  • Addressed middle initials in all places where it was pulling from direct_file_data (notably: I did not sanitize account_holder_middle_initial b/c this is input through intake & we validate this field.
  • Further thoughts we do not validate any other fields in this manner except zip_code (remove unaccepted characters from first name/last name -- suffix is a drop down I think so there is little reason to think this would contain unaccepted characters). Should we apply something like this for the first/last names? I didn't want to block this PR with that effort since first/last names are probably used more heavily across the app.

How to test?

  • Describe the testing approach taken to verify the changes, including:
    • Unit/manual tests
    • Test data used: none, but there is an example on prod?
  • Risk Assessment
    • XMLS touched:
      • return header
      • az_return_xml:
        • DependentDetails Name Middle Initial
        • QualParentsAncestors Name MiddleInitial
      • md_502b xml:
        • Dependent Name MiddleInitial
      • nc d400 xml:
        • MFSSposueName MiddleInitial
        • PaymentContact PersonName MiddleInitial
      • nj1040:
        • MarriedCuPartFilingSeparate SpouseName MiddleInitial

Screenshots (for visual changes)

  • Before
  • After

@arinchoi03
Sanitize middle initial in return header

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
GPG key ID: 4AEE18F83AFDEB23
Expired
Verified
Learn about vigilant mode
e3b7206
@arinchoi03
Update all other places where we pull middle initial directly from di…
Loading
Loading status checks…
89cbedd 
…rect_file_data to sanitize the initial
@github-actions GitHub Actions
Copy link

Heroku app: https://gyr-review-app-5478-39c36265b2b8.herokuapp.com/
View logs: heroku logs --app gyr-review-app-5478 (optionally add --tail)

anisharamnani
anisharamnani reviewed Jan 29, 2025
View reviewed changes
app/lib/submission_builder/ty2024/states/md/documents/md502b.rb
@@ -19,7 +19,7 @@ def document
xml.Dependent do
xml.Name do
xml.FirstName sanitize_for_xml(dependent.first_name, 16)
xml.MiddleInitial sanitize_for_xml(dependent.middle_initial, 1) if dependent.middle_initial.present?
xml.MiddleInitial sanitize_middle_initial(dependent.middle_initial) if sanitize_middle_initial(dependent.middle_initial).present?
Copy link
Contributor

Choose a reason for hiding this comment

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

lines 185 and 189 also have xml.MiddleInitial

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 did not sanitize account_holder_middle_initial b/c this is input through intake & we validate this field.

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry for missing the coment!

anisharamnani
anisharamnani reviewed Jan 29, 2025
View reviewed changes
app/lib/submission_builder/ty2024/states/nj/documents/nj1040.rb
@@ -31,7 +31,7 @@ def document
xml.SpouseSSN intake.spouse.ssn
xml.SpouseName do
xml.FirstName sanitize_for_xml(intake.spouse.first_name)
xml.MiddleInitial sanitize_for_xml(intake.spouse.middle_initial) if intake.spouse.middle_initial.present?
xml.MiddleInitial sanitize_middle_initial(intake.spouse.middle_initial) if sanitize_middle_initial(intake.spouse.middle_initial).present?
Copy link
Contributor

Choose a reason for hiding this comment

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

line 100 (the dependents block) also has xml.MiddleInitial

@arinchoi03
One more middle initial to sanitize
Loading
Loading status checks…
2b3c4f8
@arinchoi03
Merge remote-tracking branch 'origin' into FYST-1730-strip-any-values…
Loading
Loading status checks…
7c726a6 
…-that-are-not-allowed-from-middle-initial-field
@arinchoi03 arinchoi03 merged commit fbb491d into main Jan 30, 2025
7 checks passed
@arinchoi03 arinchoi03 deleted the FYST-1730-strip-any-values-that-are-not-allowed-from-middle-initial-field branch January 30, 2025 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anisharamnani anisharamnani

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@arinchoi03 @anisharamnani