Bug/10354 - In-App Feedback: Phone number, SSN, Email are not properly recognized if they end in punctuation #10395
Conversation
…information that ends in punctuation.
matt-guest-wilcore
requested review from
timwright12,
alexandec,
theodur,
dumathane,
cadibemma,
Sparowhawk and
DonMcCaugheyUSDS
There was a problem hiding this comment.
The checkStringForPII() function likely still has the inverse problem when there is leading punctuation (e.g. My SS:123-45-6789 or even call me415-321-1234). Looking at EMAIL_REGEX_EXP and its mates, they're anchored to the start (^) and end ($). Did you consider alternate regex patterns that might find the forbidden patterns without requiring the string to be split on whitespace?
Also, this seems like an easy function to unit test, but I didn't see any tests for it. If I didn't miss something, would you add unit tests?
…. Updated checkStringForPII to rely on regex patterns instead of splitting by whitespace. Preserved original phone and mailto regex patterns for linkify by creating separate constants, resolving issues in getLinkifiedText tests.
… detection, and have not had issues. However, I couldn't do the same for phone numbers. I opted to keep separate patterns for linking and PII as phone number detection is a lot more complex than SSN, and emails.
… the PII function and the other for linking and other uses. I think mixing the two into a unified pattern creates a lot of complexity as PII relies less on anchoring and is more flexible while linking is more strict. I also had issues with mailto links not working with past changes, so maintaining two different sets of patterns solved this.
There was a problem hiding this comment.
Approved, thanks Matt for improving this and adding tests! Please fix the linting error and merge as it.
I am requesting some follow-on work. You added tests for the InAppFeedbackScreen component, which is a correct and important to test, but you use this component to drive all the test cases for checkStringForPII().
I strongly prefer that tests focus on the smallest unit of code possible, which is the checkStringForPII() function in this case. Testing InAppFeedbackScreen is important too, but is more of an integration test than a unit test.
Small, focused tests are easier to understand and maintain and usually run significantly faster. In this case, InAppFeedbackScreen.test.tsx is 177 lines long, requires loading heavyweight React libraries and creating a new InAppFeedbackScreen component instance for each test case and waiting asynchronously for the component to respond to events. I bet that unit tests for just checkStringForPII() run 10 to 100 times faster and significantly smaller code than InAppFeedbackScreen.test.tsx. And from personal experience, focused tests will make extending checkStringForPII() much easier (and I can envision us discovering new PII patterns to filter for).
Please keep a couple representative test cases for InAppFeedbackScreen and look into adding unit tests for checkStringForPII(). Let me know if there's anything challenging about writing smaller, focused tests like this.
…file and caused a linting issue.
…IfPunctuationIsPresent
…IfPunctuationIsPresent
|
@DonMcCaugheyUSDS I was making the updates for the follow on work you mentioned and I found an issue with the The fix shouldn't be too drastic but I'd wait on merging this until I get that fix in. My apologies for the delay. |
…allows any instance of a pattern to be identified in a string. This also fixes the issue where checkStringForPII was returning only the censored portion of the message, now we return the whole message with all sensitive info replaced with placeholder data. I also wrote unit tests for checkStringForPII function.
… wanted to remove a few test cases from the integration tests I wrote earlier to improve test runtime and cutback on redundancy.
…IfPunctuationIsPresent
Description of Change
Ticket
The in-app feedback screen doesn't allow for personal information such as phone numbers, SSN and emails, if they are detected then we must show the user a warning.
As described in the ticket, if someone input an email but ended it with punctuation then we wouldn't show the user the warning.
This PR fixes that issue, we can now detect sensitive information even if it has punctuation.
Screenshots/Video
Testing
Navigate to the developer screen to find the in-app feedback screen
Reviewer Validations
Verify that sensitive information like emails, phone numbers and SSNs that end in punctuation trigger a warning alert.
EX:
PR Checklist
Reviewer: Confirm the items below as you review
For QA
Run a build for this branch