Skip to content

Conversation

abergs
Copy link
Member

@abergs abergs commented Aug 29, 2025

๐ŸŽŸ๏ธ Tracking

https://bitwarden.atlassian.net/browse/PM-23661

๐Ÿ“” Objective

This PR maps BW Identity to one or multiple CXF Credentials

โฐ 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

@abergs abergs requested a review from a team as a code owner August 29, 2025 11:05
@abergs abergs requested review from Hinton and coroiu August 29, 2025 11:05
Copy link
Contributor

github-actions bot commented Aug 29, 2025

Logo
Checkmarx One โ€“ Scan Summary & Details โ€“ 5c5cb080-e888-4f2f-9092-f6836e266ee0

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

Copy link

codecov bot commented Aug 29, 2025

Codecov Report

โŒ Patch coverage is 96.68508% with 12 lines in your changes missing coverage. Please review.
โœ… Project coverage is 76.71%. Comparing base (e05ba6e) to head (bcbb3fd).
โš ๏ธ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
crates/bitwarden-exporters/src/cxf/identity.rs 96.95% 11 Missing โš ๏ธ
crates/bitwarden-exporters/src/cxf/export.rs 0.00% 1 Missing โš ๏ธ
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #413      +/-   ##
==========================================
+ Coverage   76.41%   76.71%   +0.29%     
==========================================
  Files         265      265              
  Lines       24433    24794     +361     
==========================================
+ Hits        18670    19020     +350     
- Misses       5763     5774      +11     

โ˜” View full report in Codecov by Sentry.
๐Ÿ“ข Have feedback on the report? Share it here.

๐Ÿš€ New features to boost your workflow:
  • โ„๏ธ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • ๐Ÿ“ฆ JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Member

@Hinton Hinton left a comment

Choose a reason for hiding this comment

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

Can we move this into identity.rs?

coroiu
coroiu previously approved these changes Sep 1, 2025
Copy link

sonarqubecloud bot commented Sep 2, 2025

Comment on lines +425 to +430
identity.email.as_ref().map(|email| EditableField {
id: None,
label: Some("Email".to_string()),
value: EditableFieldString(email.clone()),
extensions: None,
}),
Copy link
Member

Choose a reason for hiding this comment

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

nit: But should you bring in the create_editable_field from #414 here and simplify slightly?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes but let's do a cleanup one once they are all in.

@abergs abergs enabled auto-merge (squash) September 2, 2025 09:28
@abergs abergs requested a review from coroiu September 2, 2025 09:28
Copy link
Contributor

@coroiu coroiu left a comment

Choose a reason for hiding this comment

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

Approving (without review) since this is owned by Architecture and Oscar has already reviewed it

@abergs abergs merged commit 7b5d9db into main Sep 3, 2025
50 checks passed
@abergs abergs deleted the cxf/export-identity branch September 3, 2025 07:26
dani-garcia added a commit that referenced this pull request Sep 3, 2025
## ๐ŸŽŸ๏ธ Tracking

<!-- Paste the link to the Jira or GitHub issue or otherwise describe /
point to where this change is coming from. -->

## ๐Ÿ“” Objective

The merge of #414 and
#413 has created some
conflicts leading to the main branch not building anymore. This PR
should fix that.

## โฐ 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

<!-- Suggested interactions but feel free to use (or not) as you desire!
-->

- ๐Ÿ‘ (`:+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
abergs added a commit that referenced this pull request Sep 7, 2025
## ๐Ÿ“” Objective

This PR adresses a nit in the review from #413

<!-- Describe what the purpose of this PR is, for example what bug
you're fixing or new feature you're adding. -->

## โฐ 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

<!-- Suggested interactions but feel free to use (or not) as you desire!
-->

- ๐Ÿ‘ (`:+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
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.

3 participants