-
Notifications
You must be signed in to change notification settings - Fork 0
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
[AC-1449] Secrets Manager subscription Stripe Integration #72
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
30 file(s) reviewed, 62 comment(s)
Edit PR Review Bot Settings | Greptile
var result = !_featureService.IsEnabled(FeatureFlagKeys.SecretManagerGaBilling, _currentContext) && | ||
!model.UseSecretsManager | ||
? await _organizationService.SignUpAsync(organizationSignup) | ||
: await _organizationSignUpCommand.SignUpAsync(organizationSignup); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Consider extracting this condition into a separate method for better readability and reusability
var result = !_featureService.IsEnabled(FeatureFlagKeys.SecretManagerGaBilling, _currentContext) && | ||
!model.UseSecretsManager | ||
? await _organizationService.UpgradePlanAsync(orgIdGuid, model.ToOrganizationUpgrade()) | ||
: await _organizationUpgradePlanCommand.UpgradePlanAsync(orgIdGuid, model.ToOrganizationUpgrade()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: This condition is identical to the one in the Post method. Consider extracting it to avoid duplication
public ListResponseModel<PlanResponseModel> Get() | ||
{ | ||
var data = StaticStore.PasswordManagerPlans; | ||
var responses = data.Select(plan => new PlanResponseModel(plan)); | ||
return new ListResponseModel<PlanResponseModel>(responses); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: The default Get() method now returns only PasswordManagerPlans instead of all Plans. This change might affect existing clients expecting all plans. Consider adding a comment explaining this change or updating documentation.
[AllowAnonymous] | ||
public ListResponseModel<PlanResponseModel> GetSecretsManagerPlans() | ||
{ | ||
var data = StaticStore.SecretManagerPlans; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
syntax: SecretManagerPlans is misspelled. It should be SecretsManagerPlans to maintain consistency with the method name.
AdditionalSmSeats = AdditionalSmSeats.GetValueOrDefault(0), | ||
AdditionalServiceAccount = AdditionalServiceAccount.GetValueOrDefault(0), | ||
UseSecretsManager = UseSecretsManager, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Use null-coalescing operator (??) instead of GetValueOrDefault for consistency with other properties
var requiredSponsoredProductType = StaticStore.GetSponsoredPlan(sponsorship.PlanSponsorshipType.Value)?.SponsoredProductType; | ||
if (requiredSponsoredProductType == null || | ||
sponsoredOrganization == null || | ||
StaticStore.GetPlan(sponsoredOrganization.PlanType).Product != requiredSponsoredProductType.Value) | ||
StaticStore.GetPasswordManagerPlan(sponsoredOrganization.PlanType).Product != requiredSponsoredProductType.Value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: The change from GetPlan to GetPasswordManagerPlan might affect how plan types are validated. Ensure this doesn't unintentionally exclude valid plan types.
@@ -51,7 +51,7 @@ public async Task SetUpSponsorshipAsync(OrganizationSponsorship sponsorship, | |||
var requiredSponsoredProductType = StaticStore.GetSponsoredPlan(sponsorship.PlanSponsorshipType.Value)?.SponsoredProductType; | |||
if (requiredSponsoredProductType == null || | |||
sponsoredOrganization == null || | |||
StaticStore.GetPlan(sponsoredOrganization.PlanType).Product != requiredSponsoredProductType.Value) | |||
StaticStore.GetPasswordManagerPlan(sponsoredOrganization.PlanType).Product != requiredSponsoredProductType.Value) | |||
{ | |||
throw new BadRequestException("Can only redeem sponsorship offer on families organizations."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: The error message 'Can only redeem sponsorship offer on families organizations' may need to be updated if Secrets Manager plans are now included.
var requiredSponsoringProductType = StaticStore.GetSponsoredPlan(sponsorshipType)?.SponsoringProductType; | ||
if (requiredSponsoringProductType == null || | ||
sponsoringOrg == null || | ||
StaticStore.GetPlan(sponsoringOrg.PlanType).Product != requiredSponsoringProductType.Value) | ||
StaticStore.GetPasswordManagerPlan(sponsoringOrg.PlanType).Product != requiredSponsoringProductType.Value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: The change from GetPlan
to GetPasswordManagerPlan
suggests a separation of Password Manager and Secrets Manager plans. Ensure this change is consistent across the codebase and that it doesn't unintentionally exclude Secrets Manager plans from sponsorship eligibility.
@@ -40,4 +40,6 @@ Task<OrganizationUserOrganizationDetails> GetDetailsByUserAsync(Guid userId, Gui | |||
Task RevokeAsync(Guid id); | |||
Task RestoreAsync(Guid id, OrganizationUserStatusType status); | |||
Task<IEnumerable<OrganizationUserPolicyDetails>> GetByUserIdWithPolicyDetailsAsync(Guid userId, PolicyType policyType); | |||
Task<int> GetOccupiedSmSeatCountByOrganizationIdAsync(Guid organizationId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Consider adding XML documentation for this new method to explain its purpose and usage
@@ -40,4 +40,6 @@ Task<OrganizationUserOrganizationDetails> GetDetailsByUserAsync(Guid userId, Gui | |||
Task RevokeAsync(Guid id); | |||
Task RestoreAsync(Guid id, OrganizationUserStatusType status); | |||
Task<IEnumerable<OrganizationUserPolicyDetails>> GetByUserIdWithPolicyDetailsAsync(Guid userId, PolicyType policyType); | |||
Task<int> GetOccupiedSmSeatCountByOrganizationIdAsync(Guid organizationId); | |||
Task<int> GetOccupiedServiceAccountCountByOrganizationIdAsync(Guid organizationId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Consider adding XML documentation for this new method to explain its purpose and usage
Type of change
Objective
Adding Secrets Manager price during the subscription if an organization wants to add a secrets manager.
Code changes
Before you submit
dotnet format --verify-no-changes
) (required)Greptile Summary
Integrated Secrets Manager pricing into subscription process, focusing on organization management and billing.