[PM-25721] Retain leading spaces in imported fields.
- Added cleanupCipher method to preserve leading/trailing spaces in notes and set them to null if they contain only whitespace or are empty.
- Updated the BaseImporter class to remove trimming of notes to maintain original formatting.
- Added unit tests for cleanupCipher to verify behavior for various note and name scenarios.
đī¸ Tracking
GitHub Issue (Clients): https://github.com/bitwarden/clients/issues/9159
đ Objective
đ¸ Screenshots
â° Reminders before review
- Contributor guidelines followed
- All formatters and local linters executed and passed
- Written new unit and / or integration tests where applicable
- Protected functional changes with optionality (feature flags)
- Used internationalization (i18n) for all UI strings
- CI builds passed
- Communicated to DevOps any deployment requirements
- Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team
đĻŽ Reviewer guidelines
- đ (
:+1:) or similar for great changes - đ (
:memo:) or âšī¸ (:information_source:) for notes or general info - â (
:question:) for questions - đ¤ (
:thinking:) or đ (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion - đ¨ (
:art:) for suggestions / improvements - â (
:x:) or â ī¸ (:warning:) for more significant problems or concerns needing attention - đą (
:seedling:) or âģī¸ (:recycle:) for future improvements or indications of technical debt - â (
:pick:) for minor or nitpick changes
Fix for https://github.com/bitwarden/clients/issues/9159
Thank you for your contribution! We've added this to our internal tracking system for review. ID: PM-25721 Link: https://bitwarden.atlassian.net/browse/PM-25721
Details on our contribution process can be found here: https://contributing.bitwarden.com/contributing/pull-requests/community-pr-process.
Resolved code comments
Resolved code comments
@JaredScar Thanks for coming back and updating the PR. The comment remarked for removal in https://github.com/bitwarden/clients/pull/16411/files#r2348956071 is still present.
Resolved code comments
@JaredScar Thanks for coming back and updating the PR. The comment remarked for removal in https://github.com/bitwarden/clients/pull/16411/files#r2348956071 is still present.
Resolved! Good catch!
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.
:white_check_mark: JaredScar
:x: mcamirault
You have signed the CLA already but the status is still pending? Let us recheck it.
Checkmarx One â Scan Summary & Details â 33b9d6b5-3cac-48e7-897e-03bde9c59827
Great job! No new security vulnerabilities introduced in this pull request
@JaredScar looks like this broke some importer tests where the notes field now has \n or other control characters at the end of it. Should be a quick fix though.
@JaredScar looks like this broke some importer tests where the notes field now has
\nor other control characters at the end of it. Should be a quick fix though.
Is there a way I can reproduce this to patch it up?
Is there a way I can reproduce this to patch it up?
Execute npm run test in a terminal from the base repo directory to run our full test suite, which will give you a summary at the end of all failing tests. Running all of them can take a couple minutes though, so after the first run I'd recommend running npm run test -- <path/to/testfile> on the individual .spec.ts files as you fix them.
Is there a way I can reproduce this to patch it up?
Execute
npm run testin a terminal from the base repo directory to run our full test suite, which will give you a summary at the end of all failing tests. Running all of them can take a couple minutes though, so after the first run I'd recommend runningnpm run test -- <path/to/testfile>on the individual.spec.tsfiles as you fix them.
All seem to pass for me? @mcamirault
All seem to pass for me? @mcamirault
@JaredScar I believe you're running the command from a subdirectory that doesn't include the failing tests, because there are a lot more tests total than shown in your screenshot. Based on the numbers you may be in the folder apps/web. Make sure your terminal is in the base directory of the repo when you run the test command; you should see ~770 test suites and ~14.5k tests
@JaredScar: Some unrelated changes were introduced with 469cbd9 I assume these were caused by a bad merge from main. Please remove the changes on
sdk.service.tsandapi.service.ts.
Yup agreed... I wasn't sure why those were erroring in the first place. I never touched them on this branch, but then they errored when I had merged from main. Will resolve. Also will resolve @mcamirault when got a second, thanks!
Codecov Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 41.90%. Comparing base (b1591da) to head (3092009).
:warning: Report is 1 commits behind head on main.
:white_check_mark: All tests successful. No failed tests found.
Additional details and impacted files
@@ Coverage Diff @@
## main #16411 +/- ##
==========================================
- Coverage 41.90% 41.90% -0.01%
==========================================
Files 3591 3591
Lines 104159 104160 +1
Branches 15710 15711 +1
==========================================
- Hits 43649 43644 -5
- Misses 58648 58655 +7
+ Partials 1862 1861 -1
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
:rocket: New features to boost your workflow:
- :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.