[PM-26690] Wire VNextSavePolicyCommand behind PolicyValidatorsRefactor feature flag #6483
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6483 +/- ##
==========================================
+ Coverage 52.27% 52.31% +0.03%
==========================================
Files 1909 1909
Lines 84598 84664 +66
Branches 7558 7562 +4
==========================================
+ Hits 44223 44289 +66
Misses 38659 38659
Partials 1716 1716 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
New Issues (3)Checkmarx found the following issues in this Pull Request
|
There was a problem hiding this comment.
Pull Request Overview
This PR introduces wiring for VNextSavePolicyCommand behind the PolicyValidatorsRefactor feature flag across policy save endpoints. The implementation allows gradual migration from the legacy ISavePolicyCommand to the new IVNextSavePolicyCommand interface.
Key Changes
- Added
IVNextSavePolicyCommanddependency injection alongside existingISavePolicyCommandacross services and controllers - Implemented feature flag checks that route policy saves to either the new VNext command or legacy command based on the
PolicyValidatorsRefactorflag - Added comprehensive test coverage for both enabled and disabled feature flag scenarios
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/Core/Constants.cs | Added PolicyValidatorsRefactor feature flag constant |
| src/Core/Auth/Services/Implementations/SsoConfigService.cs | Integrated VNext command with conditional routing based on feature flag for TDE policy saves |
| src/Core/AdminConsole/OrganizationFeatures/OrganizationDomains/VerifyOrganizationDomainCommand.cs | Added feature flag check to route SingleOrg policy saves through VNext command |
| src/Api/AdminConsole/Public/Models/Request/PolicyUpdateRequestModel.cs | Added ToSavePolicyModel method to support VNext command interface |
| src/Api/AdminConsole/Public/Controllers/PoliciesController.cs | Implemented feature flag routing for public API policy updates |
| src/Api/AdminConsole/Controllers/PoliciesController.cs | Implemented feature flag routing for private API PutVNext endpoint |
| test/Core.Test/Auth/Services/SsoConfigServiceTests.cs | Added test verifying VNext command usage when feature flag enabled |
| test/Core.Test/AdminConsole/OrganizationFeatures/OrganizationDomains/VerifyOrganizationDomainCommandTests.cs | Added test verifying VNext command usage when feature flag enabled |
| test/Api.Test/Controllers/PoliciesControllerTests.cs | Added tests for both enabled and disabled feature flag scenarios in PutVNext |
| test/Api.Test/AdminConsole/Public/Controllers/PoliciesControllerTests.cs | Added tests for both enabled and disabled feature flag scenarios in public API Put |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
r-tome
changed the title
JimmyVo16
left a comment
JimmyVo16
left a comment
There was a problem hiding this comment.
Just one small suggestion.
| { | ||
| var performedBy = new SystemUser(EventSystemUser.Unknown); | ||
| await _vNextSavePolicyCommand.SaveAsync(new SavePolicyModel(singleOrgPolicy, performedBy, new EmptyMetadataModel())); | ||
| await _vNextSavePolicyCommand.SaveAsync(new SavePolicyModel(resetPasswordPolicy, performedBy, new EmptyMetadataModel())); |
There was a problem hiding this comment.
Consider adding a different constructor for SavePolicyModel, so the client can simply pass in what it has, and the SavePolicyModel object will fill in the default values.
Currently, the client needs to be aware that EmptyMetadataModel is the default value. We're also calling this logic in multiple places, so centralizing it would be nice, even if the value is low.
So something like this.
public record SavePolicyModel(PolicyUpdate PolicyUpdate, IActingUser? PerformedBy, IPolicyMetadataModel Metadata)
{
public SavePolicyModel(PolicyUpdate PolicyUpdate)
: this(PolicyUpdate, null, new EmptyMetadataModel())
{
}
public SavePolicyModel(PolicyUpdate PolicyUpdate, IActingUser performedBy)
: this(PolicyUpdate, performedBy, new EmptyMetadataModel())
{
}
public SavePolicyModel(PolicyUpdate PolicyUpdate, EmptyMetadataModel metadata)
: this(PolicyUpdate, null, metadata)
{
}
}
|
r-tome
dismissed stale reviews from Patrick-Pimentel-Bitwarden and JimmyVo16
The base branch was changed.
r-tome
force-pushed
the
ac/pm-26690/use-vnextsavepolicycommand
branch
from
cf6d0d1 to
5cbadc1
Compare
…on feature flag - Added IFeatureService and IVNextSavePolicyCommand dependencies to PoliciesController. - Updated PutVNext method to conditionally use VNextSavePolicyCommand or SavePolicyCommand based on the PolicyValidatorsRefactor feature flag. - Enhanced unit tests to verify behavior for both enabled and disabled states of the feature flag.
…d based on feature flag - Introduced IFeatureService and IVNextSavePolicyCommand to manage policy saving based on the PolicyValidatorsRefactor feature flag. - Updated the Put method to conditionally use the new VNextSavePolicyCommand or the legacy SavePolicyCommand. - Added unit tests to validate the behavior of the Put method for both enabled and disabled states of the feature flag.
…ommand based on feature flag - Added IFeatureService and IVNextSavePolicyCommand dependencies to VerifyOrganizationDomainCommand. - Updated EnableSingleOrganizationPolicyAsync method to conditionally use VNextSavePolicyCommand or SavePolicyCommand based on the PolicyValidatorsRefactor feature flag. - Enhanced unit tests to validate the behavior when the feature flag is enabled.
…feature flag - Added IFeatureService and IVNextSavePolicyCommand dependencies to SsoConfigService. - Updated SaveAsync method to conditionally use VNextSavePolicyCommand or SavePolicyCommand based on the PolicyValidatorsRefactor feature flag. - Added unit tests to validate the behavior when the feature flag is enabled.
…ptyMetadataModel parameter. Update related usages across the codebase to reflect the new constructor overloads.
…r improved null safety
r-tome
force-pushed
the
ac/pm-26690/use-vnextsavepolicycommand
branch
from
5cbadc1 to
fe9e8a7
Compare
|
Claude finished @r-tome's task —— View job Code Review CompleteSummaryThis PR successfully implements the Critical IssuesNone identified - the implementation is solid and follows good practices. Suggested Improvements1. Missing null-safety directive in Constants.csLocation: Issue: The new feature flag constant was added to a file that has Recommendation: While this aligns with the existing file pattern, consider creating a follow-up task to migrate Impact: Low - cosmetic/technical debt 2. Inconsistent feature flag checking patternLocations:
Issue: Two different patterns are used for feature flag branching: Pattern 1 (ternary operator in PoliciesController.cs:230): var policy = _featureService.IsEnabled(FeatureFlagKeys.PolicyValidatorsRefactor) ?
await _vNextSavePolicyCommand.SaveAsync(savePolicyRequest) :
await _savePolicyCommand.VNextSaveAsync(savePolicyRequest);Pattern 2 (if-else in Public/PoliciesController.cs:101): Policy policy;
if (_featureService.IsEnabled(FeatureFlagKeys.PolicyValidatorsRefactor))
{
var savePolicyModel = model.ToSavePolicyModel(_currentContext.OrganizationId!.Value, type);
policy = await _vNextSavePolicyCommand.SaveAsync(savePolicyModel);
}
else
{
var policyUpdate = model.ToPolicyUpdate(_currentContext.OrganizationId!.Value, type);
policy = await _savePolicyCommand.SaveAsync(policyUpdate);
}Recommendation: Standardize on the if-else pattern for better readability and consistency, especially when the branches involve different parameter transformations. The ternary is fine for simple value selections but becomes harder to read with async calls. Impact: Low - readability/maintainability 3. Test naming could be more specificLocation: Test files Issue: Test method names like Current: Put_WhenPolicyValidatorsRefactorEnabled_UsesVNextSavePolicyCommandSuggestion: Put_WhenPolicyValidatorsRefactorEnabled_CallsVNextSavePolicyCommandAndNotLegacyThis makes it clearer that the test verifies both the positive case (calling VNext) and negative case (not calling legacy). Impact: Very Low - test clarity 4. Potential for code duplication during feature flag cleanupIssue: When the feature flag is eventually removed, there will be cleanup work across 4+ files to remove the branching logic and delete the legacy code paths. Recommendation: Add a TODO comment at the feature flag constant definition to track the cleanup task: // TODO: PM-XXXXX - Remove this feature flag after successful rollout
// Files to clean up: PoliciesController.cs, Public/PoliciesController.cs,
// SsoConfigService.cs, VerifyOrganizationDomainCommand.cs
public const string PolicyValidatorsRefactor = "pm-26423-refactor-policy-side-effects";Impact: Low - future maintainability Good Practices Observed
Action ItemsRequired (Blocking)None Recommended (Non-blocking)
Security & Performance Notes
ConclusionThis is a well-implemented feature flag integration. The code is production-ready with excellent test coverage. The suggested improvements are minor refinements that don't block merging. Great work on addressing the previous review feedback! |
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-26690
📔 Objective
Wire up
VNextSavePolicyCommandbehind thePolicyValidatorsRefactorfeature flag across all policy save endpoints: private APIPutVNext, public API Put, domain verification, and SSO configuration⏰ Reminders before review
🦮 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