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

[PM-13755] Decouple Invite and Edit User Flows #12277

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

JimmyVo16
Copy link
Contributor

@JimmyVo16 JimmyVo16 commented Dec 6, 2024

🎟️ Tracking

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

📔 Objective

Refactor the code to separate the invite and edit user flows, improving readability by reducing conditional complexity and decoupling responsibilities.

For members.component.ts

  1. Create an overload invite method and move all the invite user flow from edit method into it.

For member-dialog.component.ts

  1. Refactor the submit method into smaller methods with more focused responsibilities: handleEditUser, handleInviteUsers, and getUserView.
  2. Remove the toast in the submit method as form validation prevents this code from being executed. Discussion

No new visible behavior was introduced.

📸 Screenshots

Tested by going through invite, edit, revoke, and remove

Regression.testing.webm

⏰ 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

@JimmyVo16 JimmyVo16 requested a review from eliykat December 6, 2024 21:10
@JimmyVo16 JimmyVo16 self-assigned this Dec 6, 2024
@JimmyVo16 JimmyVo16 requested a review from a team as a code owner December 6, 2024 21:10
Copy link
Contributor

github-actions bot commented Dec 6, 2024

Logo
Checkmarx One – Scan Summary & Detailsd02b872a-71d7-4a94-8ca2-7ba270df0807

Great job, no security vulnerabilities found in this Pull Request

Copy link

codecov bot commented Dec 6, 2024

Codecov Report

Attention: Patch coverage is 33.72093% with 57 lines in your changes missing coverage. Please review.

Project coverage is 34.97%. Comparing base (c6a3055) to head (56db3b4).

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...omponents/member-dialog/member-dialog.component.ts 0.00% 34 Missing ⚠️
...console/organizations/members/members.component.ts 0.00% 20 Missing ⚠️
...log/validators/org-seat-limit-reached.validator.ts 88.23% 2 Missing ⚠️
...er-access-report/member-access-report.component.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #12277      +/-   ##
==========================================
+ Coverage   34.96%   34.97%   +0.01%     
==========================================
  Files        2976     2977       +1     
  Lines       90585    90618      +33     
  Branches    16980    16974       -6     
==========================================
+ Hits        31671    31694      +23     
- Misses      56457    56467      +10     
  Partials     2457     2457              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@eliykat eliykat left a comment

Choose a reason for hiding this comment

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

All makes sense to me! Minor feedback only.

Also note your Jira ticket link is wrong.

@JimmyVo16
Copy link
Contributor Author

Sanity test for email validator
test email input validator.webm

private async handleInviteUsers(userView: OrganizationUserAdminView, organization: Organization) {
const emails = [...new Set(this.formGroup.value.emails.trim().split(/\s*,\s*/))];

this.setInputEmailCountValidator(organization, emails.length);
Copy link
Member

Choose a reason for hiding this comment

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

Validators should be attached when the form is first initialized, so that the user gets immediate feedback when filling it out. You should delete setInputEmailCountValidator and attach your validator in the existing setFormValidators method.

@JimmyVo16
Copy link
Contributor Author

Testing
Code review testing 2.webm

Copy link
Member

@eliykat eliykat left a comment

Choose a reason for hiding this comment

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

You also have some failures caused by strict typechecking: https://github.com/bitwarden/clients/actions/runs/12283466303/job/34277004841

It's a bit premature to start solving these, I suggest adding the magic comment to the top of the offending files:

// FIXME: Update this file to be type safe and remove this and next line
// @ts-strict-ignore

@JimmyVo16
Copy link
Contributor Author

You also have some failures caused by strict typechecking: https://github.com/bitwarden/clients/actions/runs/12283466303/job/34277004841

It's a bit premature to start solving these, I suggest adding the magic comment to the top of the offending files:

// FIXME: Update this file to be type safe and remove this and next line
// @ts-strict-ignore

I was looking at this yesterday. The only thing that I'm confused about is that this isn't my change. I guess the type check goes through the whole code repo or code paths?

Either way, I'm going to do what you suggested.

@JimmyVo16
Copy link
Contributor Author

JimmyVo16 commented Dec 12, 2024

Testing Code review 3
Testing 3.webm

Edit: This testing includes both validators; inputEmailValidator and orgSeatLimitReachedValidator.

@eliykat
Copy link
Member

eliykat commented Dec 13, 2024

I was looking at this yesterday. The only thing that I'm confused about is that this isn't my change. I guess the type check goes through the whole code repo or code paths?

You're right - my bad. main was broken, so when you updated your branch you pulled in the failures. main is now fixed so it resolved itself when you last updated your branch. :)

This lgtm!

eliykat
eliykat previously approved these changes Dec 13, 2024
eliykat
eliykat previously approved these changes Dec 19, 2024
ttalty
ttalty previously approved these changes Jan 13, 2025
@JimmyVo16 JimmyVo16 dismissed stale reviews from ttalty and eliykat via 27f1e14 January 20, 2025 16:28
@JimmyVo16
Copy link
Contributor Author

I will do some manual testing after the pipeline succeeds.

@JimmyVo16
Copy link
Contributor Author

I did some regression tests for the Free and the family tiers to make sure my changes and Brandon's changes are both working.

Family.tier.testing.mov
Free.tier.testing.mov

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