Skip to content
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

fix(#9341): strip invisible characters #9364

Merged
merged 7 commits into from
Aug 26, 2024

Conversation

aloundoye
Copy link
Member

Description

fix #9341

Code review checklist

  • Readable: Concise, well named, follows the style guide, documented if necessary.
  • Documented: Configuration and user documentation on cht-docs
  • Tested: Unit and/or e2e where appropriate
  • Internationalised: All user facing text
  • Backwards compatible: Works with existing data and configuration or includes a migration. Any breaking changes documented in the release notes.

License

The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.

@aloundoye
Copy link
Member Author

@garethbowen, can you look at this one?

Copy link
Member

@garethbowen garethbowen left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good to me!

Can you please also add few unit tests to prove that this works as expected. One technique I like to use is red/green testing which means you write the test which is failing, and then write the code to solve it, and see that the test goes green. Because you've already written the code I recommend commenting it out, and writing the test, and then uncommenting the code again. This gives me a lot of confidence that I've actually solved the issue.

The best place to put these unit tests is smsparser.spec.js

@aloundoye aloundoye requested a review from garethbowen August 23, 2024 16:04
Copy link
Member

@garethbowen garethbowen left a comment

Choose a reason for hiding this comment

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

I didn't realise this function wasn't tested at all! Thank you for filling that hole in the coverage.

Can you add one more test which tests that invisible characters are stripped? For example, copy one of the tests you added and add an invisible character to the input string?

@aloundoye aloundoye requested a review from garethbowen August 26, 2024 10:30
Copy link
Member

@garethbowen garethbowen left a comment

Choose a reason for hiding this comment

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

Excellent! Thanks for the quick turnaround.

I'll merge this as soon as the build passes.

@aloundoye
Copy link
Member Author

I'll merge this as soon as the build passes.

Thanks @garethbowen

@garethbowen garethbowen merged commit 3736569 into medic:master Aug 26, 2024
21 checks passed
@aloundoye aloundoye deleted the 9341-strip-invisible-characters branch August 26, 2024 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Strip invisible characters from the fields bsYear, bsMonth, bsDay
2 participants