Skip to content

[PM-18234] Add SendPolicyRequirement#5409

Merged
eliykat merged 11 commits intomainfrom
ac/pm-18234/server-add-sendpolicyrequirement
Feb 23, 2025
Merged

[PM-18234] Add SendPolicyRequirement#5409
eliykat merged 11 commits intomainfrom
ac/pm-18234/server-add-sendpolicyrequirement

Conversation

@eliykat
Copy link
Copy Markdown
Member

@eliykat eliykat commented Feb 17, 2025

🎟️ Tracking

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

📔 Objective

  • Implement a SendPolicyRequirement, which summarizes the business requirements of our two Send-related policy types: DisableSend and SendOptions. This makes it easier to consume for calling code, which doesn't have to concern itself with handling Policy and data objects directly.
  • Implement a factory function for the new requirement, which is responsible for filtering out policies that won't be enforced, and combining multiple policies into a single requirement.
  • Connect the factory function to DI so it can be accessed via IPolicyRequirementQuery (already implemented).
  • Update the calling code to use the new interface (feature flagged).

This is a new pattern that we are just starting to roll out, so if you're reviewing please let me know if you have any questions.

Note that generally there will be 1 PolicyType per requirement, however in this case they were closely related enough I thought it was a nice bonus to group them.

📸 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

@codecov
Copy link
Copy Markdown

codecov bot commented Feb 17, 2025

Codecov Report

Attention: Patch coverage is 91.30435% with 4 lines in your changes missing coverage. Please review.

Project coverage is 44.52%. Comparing base (5241e09) to head (76ee5d9).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...Core/Tools/Services/Implementations/SendService.cs 85.71% 2 Missing and 1 partial ⚠️
...licies/PolicyRequirements/SendPolicyRequirement.cs 95.83% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5409      +/-   ##
==========================================
+ Coverage   44.47%   44.52%   +0.04%     
==========================================
  Files        1512     1513       +1     
  Lines       70282    70325      +43     
  Branches     6341     6347       +6     
==========================================
+ Hits        31261    31314      +53     
+ Misses      37675    37663      -12     
- Partials     1346     1348       +2     

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

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Feb 17, 2025

Logo
Checkmarx One – Scan Summary & Details03c59607-f0d4-4e0a-aa13-514a410abf1b

Fixed Issues (4)

Great job! The following issues were fixed in this Pull Request

Severity Issue Source File / Package
MEDIUM CSRF /src/Api/Public/Controllers/CollectionsController.cs: 87
MEDIUM CSRF /src/Api/Controllers/CollectionsController.cs: 171
MEDIUM CSRF /src/Api/Controllers/CollectionsController.cs: 143
LOW Log_Forging /src/Api/Platform/Push/Controllers/PushController.cs: 76

@eliykat eliykat marked this pull request as ready for review February 19, 2025 02:06
@eliykat eliykat requested review from a team as code owners February 19, 2025 02:06
@eliykat eliykat requested a review from BTreston February 19, 2025 02:06
audreyality

This comment was marked as resolved.

@eliykat eliykat requested a review from audreyality February 21, 2025 00:42
Copy link
Copy Markdown
Member

@audreyality audreyality left a comment

Choose a reason for hiding this comment

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

👍🏻 Looks good to me! Thank you for responding to my comments!

Comment on lines +131 to +132
sutProvider.GetDependency<IPolicyService>().AnyPoliciesApplicableToUserAsync(
Arg.Any<Guid>(), Arg.Any<PolicyType>()).ThrowsAsync<Exception>();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍🏻 I love when a test causes a method that shouldn't be called to throw. It's such an elegant way to test the invariant.

@sonarqubecloud
Copy link
Copy Markdown

@eliykat eliykat merged commit b0c6fc9 into main Feb 23, 2025
53 checks passed
@eliykat eliykat deleted the ac/pm-18234/server-add-sendpolicyrequirement branch February 23, 2025 23:19
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