clients icon indicating copy to clipboard operation
clients copied to clipboard

[PM-25721] Retain leading spaces in imported fields.

Open JaredScar opened this issue 4 months ago â€ĸ 14 comments

  • 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

JaredScar avatar Sep 14 '25 02:09 JaredScar

Fix for https://github.com/bitwarden/clients/issues/9159

JaredScar avatar Sep 14 '25 02:09 JaredScar

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.

bitwarden-bot avatar Sep 14 '25 13:09 bitwarden-bot

Resolved code comments

JaredScar avatar Nov 23 '25 03:11 JaredScar

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.

djsmith85 avatar Nov 24 '25 13:11 djsmith85

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!

JaredScar avatar Nov 24 '25 16:11 JaredScar

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Dec 01 '25 16:12 CLAassistant

CLA assistant check
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.

CLAassistant avatar Dec 01 '25 16:12 CLAassistant

Logo Checkmarx One – Scan Summary & Details – 33b9d6b5-3cac-48e7-897e-03bde9c59827

Great job! No new security vulnerabilities introduced in this pull request

github-actions[bot] avatar Dec 01 '25 16:12 github-actions[bot]

@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.

mcamirault avatar Dec 01 '25 19:12 mcamirault

@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.

Is there a way I can reproduce this to patch it up?

JaredScar avatar Dec 02 '25 20:12 JaredScar

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.

mcamirault avatar Dec 03 '25 16:12 mcamirault

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.

image

All seem to pass for me? @mcamirault

JaredScar avatar Dec 04 '25 02:12 JaredScar

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

mcamirault avatar Dec 04 '25 15:12 mcamirault

@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.ts and api.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!

JaredScar avatar Dec 09 '25 00:12 JaredScar

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.

codecov[bot] avatar Dec 16 '25 15:12 codecov[bot]