Skip to content

Commit 5e99098

Browse files
amorask-bitwardenamorask-bitwarden
committed
fix(billing): skip ended schedule phases when updating storage (#7420)
(cherry picked from commit a5052d8)
1 parent 34b6ce9 commit 5e99098

4 files changed

Lines changed: 218 additions & 27 deletions

File tree

‎src/Core/Billing/Organizations/Commands/UpdateOrganizationSubscriptionCommand.cs‎

Lines changed: 35 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -128,24 +128,37 @@ public Task<BillingCommandResult<Subscription>> Run(
128128
CommandName, activeSchedule.Id, subscription.Id);
129129

130130
var phase1 = activeSchedule.Phases[0];
131+
var now = subscription.TestClock?.FrozenTime ?? DateTime.UtcNow;
131132

132133
/* This applies the change set's price IDs (which are Phase 1 / current-plan prices)
133-
* to both phases. This works because storage prices are uniform across the Families
134-
* migration. If storage prices ever differ between phases, both this command and
135-
* UpdatePremiumStorageCommand would need plan-aware price resolution (e.g. matching
134+
* to all active phases. This works because storage prices are uniform across the
135+
* Families migration. If storage prices ever differ between phases, both this command
136+
* and UpdatePremiumStorageCommand would need plan-aware price resolution (e.g. matching
136137
* Phase 2's seat price to determine the correct storage price). */
137-
var phases = new List<SubscriptionSchedulePhaseOptions>
138+
var phases = new List<SubscriptionSchedulePhaseOptions>();
139+
140+
// Stripe rejects schedule updates that include phases whose end_date is in the past.
141+
// A phase ending at exactly `now` has effectively ended (strict > is intentional).
142+
if (phase1.EndDate > now)
138143
{
139-
new()
144+
phases.Add(new SubscriptionSchedulePhaseOptions
140145
{
141146
StartDate = phase1.StartDate,
142147
EndDate = phase1.EndDate,
143148
Items = ApplyChangesToPhaseItems(phase1.Items, changeSet.Changes),
144149
Discounts = phase1.Discounts?.Select(d =>
145150
new SubscriptionSchedulePhaseDiscountOptions { Coupon = d.CouponId }).ToList(),
146151
ProrationBehavior = phase1.ProrationBehavior
147-
}
148-
};
152+
});
153+
}
154+
else
155+
{
156+
_logger.LogWarning(
157+
"{Command}: Phase 1 has already ended (EndDate: {EndDate}), updating only active phase(s)",
158+
CommandName, phase1.EndDate);
159+
}
160+
161+
var phase1Ended = phase1.EndDate <= now;
149162

150163
if (activeSchedule.Phases.Count >= 2)
151164
{
@@ -155,12 +168,24 @@ public Task<BillingCommandResult<Subscription>> Run(
155168
StartDate = phase2.StartDate,
156169
EndDate = phase2.EndDate,
157170
Items = ApplyChangesToPhaseItems(phase2.Items, changeSet.Changes),
158-
Discounts = phase2.Discounts?.Select(d =>
159-
new SubscriptionSchedulePhaseDiscountOptions { Coupon = d.CouponId }).ToList(),
171+
// When Phase 2 is already active, its one-time migration discount has been
172+
// applied and consumed. Re-including it would cause Stripe to re-apply it.
173+
Discounts = phase1Ended
174+
? []
175+
: phase2.Discounts?.Select(d =>
176+
new SubscriptionSchedulePhaseDiscountOptions { Coupon = d.CouponId }).ToList(),
160177
ProrationBehavior = phase2.ProrationBehavior
161178
});
162179
}
163180

181+
if (phases.Count == 0)
182+
{
183+
_logger.LogWarning(
184+
"{Command}: Schedule ({ScheduleId}) has no updatable phases remaining",
185+
CommandName, activeSchedule.Id);
186+
return DefaultConflict;
187+
}
188+
164189
/* Note: the schedule phase API does not support PendingInvoiceItemInterval. For annual
165190
* subscribers, the non-schedule path invoices prorations monthly. When the top-level
166191
* ProrationBehavior is AlwaysInvoice (structural changes), Stripe invoices immediately.
@@ -226,7 +251,7 @@ await stripeAdapter.UpdateSubscriptionScheduleAsync(activeSchedule.Id,
226251
{
227252
return await stripeAdapter.GetSubscriptionAsync(organization.GatewaySubscriptionId, new SubscriptionGetOptions
228253
{
229-
Expand = ["customer"]
254+
Expand = ["customer", "test_clock"]
230255
});
231256
}
232257
catch (StripeException stripeException) when (stripeException.StripeError?.Code == ErrorCodes.ResourceMissing)

‎src/Core/Billing/Premium/Commands/UpdatePremiumStorageCommand.cs‎

Lines changed: 37 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,9 @@ public class UpdatePremiumStorageCommand(
3939
{
4040
private readonly ILogger<UpdatePremiumStorageCommand> _logger = logger;
4141

42+
protected override Conflict DefaultConflict =>
43+
new("We had a problem updating your subscription. Please contact support for assistance.");
44+
4245
public Task<BillingCommandResult<None>> Run(User user, short additionalStorageGb) => HandleAsync<None>(async () =>
4346
{
4447
if (user is not { Premium: true, GatewaySubscriptionId: not null and not "" })
@@ -55,7 +58,7 @@ public Task<BillingCommandResult<None>> Run(User user, short additionalStorageGb
5558
var premiumPlans = await pricingClient.ListPremiumPlans();
5659
var subscription = await stripeAdapter.GetSubscriptionAsync(user.GatewaySubscriptionId, new SubscriptionGetOptions
5760
{
58-
Expand = ["customer"]
61+
Expand = ["customer", "test_clock"]
5962
});
6063

6164
// Find the password manager subscription item (seat, not storage) and match it to a plan
@@ -160,24 +163,37 @@ public Task<BillingCommandResult<None>> Run(User user, short additionalStorageGb
160163
CommandName, activeSchedule.Id, subscription.Id);
161164

162165
var phase1 = activeSchedule.Phases[0];
166+
var now = subscription.TestClock?.FrozenTime ?? DateTime.UtcNow;
163167

164168
// Storage prices are uniform across the Premium migration, so we use the same price
165-
// for both phases. If storage prices ever differ between phases, this would need
166-
// plan-aware price resolution (e.g. matching Phase 2's seat price to a plan).
169+
// for all active phases. If storage prices ever differ between phases, this would
170+
// need plan-aware price resolution (e.g. matching Phase 2's seat price to a plan).
167171
var storagePriceId = premiumPlan.Storage.StripePriceId;
168172

169-
var phases = new List<SubscriptionSchedulePhaseOptions>
173+
var phases = new List<SubscriptionSchedulePhaseOptions>();
174+
175+
// Stripe rejects schedule updates that include phases whose end_date is in the past.
176+
// A phase ending at exactly `now` has effectively ended (strict > is intentional).
177+
if (phase1.EndDate > now)
170178
{
171-
new()
179+
phases.Add(new SubscriptionSchedulePhaseOptions
172180
{
173181
StartDate = phase1.StartDate,
174182
EndDate = phase1.EndDate,
175183
Items = BuildPhaseItemsWithStorage(phase1.Items, storagePriceId, additionalStorageGb),
176184
Discounts = phase1.Discounts?.Select(d =>
177185
new SubscriptionSchedulePhaseDiscountOptions { Coupon = d.CouponId }).ToList(),
178186
ProrationBehavior = phase1.ProrationBehavior
179-
}
180-
};
187+
});
188+
}
189+
else
190+
{
191+
_logger.LogWarning(
192+
"{Command}: Phase 1 has already ended (EndDate: {EndDate}), updating only active phase(s)",
193+
CommandName, phase1.EndDate);
194+
}
195+
196+
var phase1Ended = phase1.EndDate <= now;
181197

182198
if (activeSchedule.Phases.Count >= 2)
183199
{
@@ -187,12 +203,24 @@ public Task<BillingCommandResult<None>> Run(User user, short additionalStorageGb
187203
StartDate = phase2.StartDate,
188204
EndDate = phase2.EndDate,
189205
Items = BuildPhaseItemsWithStorage(phase2.Items, storagePriceId, additionalStorageGb),
190-
Discounts = phase2.Discounts?.Select(d =>
191-
new SubscriptionSchedulePhaseDiscountOptions { Coupon = d.CouponId }).ToList(),
206+
// When Phase 2 is already active, its one-time migration discount has been
207+
// applied and consumed. Re-including it would cause Stripe to re-apply it.
208+
Discounts = phase1Ended
209+
? []
210+
: phase2.Discounts?.Select(d =>
211+
new SubscriptionSchedulePhaseDiscountOptions { Coupon = d.CouponId }).ToList(),
192212
ProrationBehavior = phase2.ProrationBehavior
193213
});
194214
}
195215

216+
if (phases.Count == 0)
217+
{
218+
_logger.LogWarning(
219+
"{Command}: Schedule ({ScheduleId}) has no updatable phases remaining",
220+
CommandName, activeSchedule.Id);
221+
return DefaultConflict;
222+
}
223+
196224
await stripeAdapter.UpdateSubscriptionScheduleAsync(activeSchedule.Id,
197225
new SubscriptionScheduleUpdateOptions
198226
{

‎test/Core.Test/Billing/Organizations/Commands/UpdateOrganizationSubscriptionCommandTests.cs‎

Lines changed: 78 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1134,6 +1134,76 @@ await _stripeAdapter.Received(1).UpdateSubscriptionScheduleAsync(
11341134
await _stripeAdapter.DidNotReceive().UpdateSubscriptionAsync(Arg.Any<string>(), Arg.Any<SubscriptionUpdateOptions>());
11351135
}
11361136

1137+
[Fact]
1138+
public async Task Run_AddItem_WithSchedule_Phase2Active_UpdatesOnlyPhase2()
1139+
{
1140+
var organization = CreateOrganization();
1141+
var subscription = CreateSubscription(items: [("price_seats", "si_1", 5)]);
1142+
1143+
SetupGetSubscription(organization, subscription);
1144+
1145+
var schedule = CreateMockSchedule(
1146+
subscription.Id,
1147+
[("price_seats", 5)],
1148+
[("price_seats_new", 5)],
1149+
phase2Active: true);
1150+
_stripeAdapter.ListSubscriptionSchedulesAsync(Arg.Any<SubscriptionScheduleListOptions>())
1151+
.Returns(new StripeList<SubscriptionSchedule> { Data = [schedule] });
1152+
1153+
var changeSet = new OrganizationSubscriptionChangeSet
1154+
{
1155+
Changes = [new AddItem("price_storage", 3)]
1156+
};
1157+
1158+
var result = await _command.Run(organization, changeSet);
1159+
1160+
Assert.True(result.Success);
1161+
1162+
await _stripeAdapter.Received(1).UpdateSubscriptionScheduleAsync(
1163+
schedule.Id,
1164+
Arg.Is<SubscriptionScheduleUpdateOptions>(opts =>
1165+
opts.Phases.Count == 1 &&
1166+
opts.Phases[0].Items.Any(i => i.Price == "price_seats_new" && i.Quantity == 5) &&
1167+
opts.Phases[0].Items.Any(i => i.Price == "price_storage" && i.Quantity == 3) &&
1168+
opts.Phases[0].Discounts != null && opts.Phases[0].Discounts.Count == 0));
1169+
1170+
await _stripeAdapter.DidNotReceive().UpdateSubscriptionAsync(Arg.Any<string>(), Arg.Any<SubscriptionUpdateOptions>());
1171+
}
1172+
1173+
[Fact]
1174+
public async Task Run_UpdateItemQuantity_Zero_WithSchedule_Phase2Active_RemovesFromPhase2()
1175+
{
1176+
var organization = CreateOrganization();
1177+
var subscription = CreateSubscription(items: [("price_seats", "si_1", 5), ("price_storage", "si_2", 3)]);
1178+
1179+
SetupGetSubscription(organization, subscription);
1180+
1181+
var schedule = CreateMockSchedule(
1182+
subscription.Id,
1183+
[("price_seats", 5), ("price_storage", 3)],
1184+
[("price_seats_new", 5), ("price_storage", 3)],
1185+
phase2Active: true);
1186+
_stripeAdapter.ListSubscriptionSchedulesAsync(Arg.Any<SubscriptionScheduleListOptions>())
1187+
.Returns(new StripeList<SubscriptionSchedule> { Data = [schedule] });
1188+
1189+
var changeSet = new OrganizationSubscriptionChangeSet
1190+
{
1191+
Changes = [new UpdateItemQuantity("price_storage", 0)]
1192+
};
1193+
1194+
var result = await _command.Run(organization, changeSet);
1195+
1196+
Assert.True(result.Success);
1197+
1198+
await _stripeAdapter.Received(1).UpdateSubscriptionScheduleAsync(
1199+
schedule.Id,
1200+
Arg.Is<SubscriptionScheduleUpdateOptions>(opts =>
1201+
opts.Phases.Count == 1 &&
1202+
opts.Phases[0].Items.All(i => i.Price != "price_storage")));
1203+
1204+
await _stripeAdapter.DidNotReceive().UpdateSubscriptionAsync(Arg.Any<string>(), Arg.Any<SubscriptionUpdateOptions>());
1205+
}
1206+
11371207
private static Organization CreateOrganization() => new()
11381208
{
11391209
Id = Guid.NewGuid(),
@@ -1191,14 +1261,18 @@ private void SetupUpdateSubscription(Subscription subscription)
11911261
private static SubscriptionSchedule CreateMockSchedule(
11921262
string subscriptionId,
11931263
(string priceId, long quantity)[] phase1Items,
1194-
(string priceId, long quantity)[]? phase2Items = null)
1264+
(string priceId, long quantity)[]? phase2Items = null,
1265+
bool phase2Active = false)
11951266
{
1267+
var phase1Start = phase2Active ? DateTime.UtcNow.AddYears(-1) : DateTime.UtcNow;
1268+
var phase1End = phase2Active ? DateTime.UtcNow.AddDays(-1) : DateTime.UtcNow.AddYears(1);
1269+
11961270
var phases = new List<SubscriptionSchedulePhase>
11971271
{
11981272
new()
11991273
{
1200-
StartDate = DateTime.UtcNow,
1201-
EndDate = DateTime.UtcNow.AddYears(1),
1274+
StartDate = phase1Start,
1275+
EndDate = phase1End,
12021276
Items = phase1Items.Select(i =>
12031277
new SubscriptionSchedulePhaseItem { PriceId = i.priceId, Quantity = i.quantity }).ToList(),
12041278
ProrationBehavior = ProrationBehavior.None
@@ -1209,7 +1283,7 @@ private static SubscriptionSchedule CreateMockSchedule(
12091283
{
12101284
phases.Add(new SubscriptionSchedulePhase
12111285
{
1212-
StartDate = DateTime.UtcNow.AddYears(1),
1286+
StartDate = phase1End,
12131287
Items = phase2Items.Select(i =>
12141288
new SubscriptionSchedulePhaseItem { PriceId = i.priceId, Quantity = i.quantity }).ToList(),
12151289
ProrationBehavior = ProrationBehavior.None

‎test/Core.Test/Billing/Premium/Commands/UpdatePremiumStorageCommandTests.cs‎

Lines changed: 68 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -740,11 +740,72 @@ await _stripeAdapter.Received(1).UpdateSubscriptionScheduleAsync(
740740
await _userService.Received(1).SaveUserAsync(Arg.Is<User>(u => u.MaxStorageGb == 10));
741741
}
742742

743+
[Theory, BitAutoData]
744+
public async Task Run_IncreaseStorage_WithSchedule_Phase2Active_UpdatesOnlyPhase2(User user)
745+
{
746+
user.Premium = true;
747+
user.MaxStorageGb = 5;
748+
user.Storage = 2L * 1024 * 1024 * 1024;
749+
user.GatewaySubscriptionId = "sub_123";
750+
751+
var subscription = CreateMockSubscription("sub_123", 4);
752+
_stripeAdapter.GetSubscriptionAsync("sub_123", Arg.Any<SubscriptionGetOptions>()).Returns(subscription);
753+
754+
var schedule = CreateMockSchedule("sub_123", hasStorage: true, storageQuantity: 4, phase2Active: true);
755+
_stripeAdapter.ListSubscriptionSchedulesAsync(Arg.Any<SubscriptionScheduleListOptions>())
756+
.Returns(new StripeList<SubscriptionSchedule> { Data = [schedule] });
757+
758+
var result = await _command.Run(user, 9);
759+
760+
Assert.True(result.IsT0);
761+
762+
await _stripeAdapter.Received(1).UpdateSubscriptionScheduleAsync(
763+
schedule.Id,
764+
Arg.Is<SubscriptionScheduleUpdateOptions>(opts =>
765+
opts.Phases.Count == 1 &&
766+
opts.Phases[0].Items.Any(i => i.Price == "price_premium_new" && i.Quantity == 1) &&
767+
opts.Phases[0].Items.Any(i => i.Price == "price_storage" && i.Quantity == 9) &&
768+
opts.Phases[0].Discounts != null && opts.Phases[0].Discounts.Count == 0));
769+
770+
await _stripeAdapter.DidNotReceive().UpdateSubscriptionAsync(Arg.Any<string>(), Arg.Any<SubscriptionUpdateOptions>());
771+
await _userService.Received(1).SaveUserAsync(Arg.Is<User>(u => u.MaxStorageGb == 10));
772+
}
773+
774+
[Theory, BitAutoData]
775+
public async Task Run_RemoveStorage_WithSchedule_Phase2Active_RemovesFromPhase2(User user)
776+
{
777+
user.Premium = true;
778+
user.MaxStorageGb = 10;
779+
user.Storage = 500L * 1024 * 1024;
780+
user.GatewaySubscriptionId = "sub_123";
781+
782+
var subscription = CreateMockSubscription("sub_123", 9);
783+
_stripeAdapter.GetSubscriptionAsync("sub_123", Arg.Any<SubscriptionGetOptions>()).Returns(subscription);
784+
785+
var schedule = CreateMockSchedule("sub_123", hasStorage: true, storageQuantity: 9, phase2Active: true);
786+
_stripeAdapter.ListSubscriptionSchedulesAsync(Arg.Any<SubscriptionScheduleListOptions>())
787+
.Returns(new StripeList<SubscriptionSchedule> { Data = [schedule] });
788+
789+
var result = await _command.Run(user, 0);
790+
791+
Assert.True(result.IsT0);
792+
793+
await _stripeAdapter.Received(1).UpdateSubscriptionScheduleAsync(
794+
schedule.Id,
795+
Arg.Is<SubscriptionScheduleUpdateOptions>(opts =>
796+
opts.Phases.Count == 1 &&
797+
opts.Phases[0].Items.All(i => i.Price != "price_storage")));
798+
799+
await _stripeAdapter.DidNotReceive().UpdateSubscriptionAsync(Arg.Any<string>(), Arg.Any<SubscriptionUpdateOptions>());
800+
await _userService.Received(1).SaveUserAsync(Arg.Is<User>(u => u.MaxStorageGb == 1));
801+
}
802+
743803
private static SubscriptionSchedule CreateMockSchedule(
744804
string subscriptionId,
745805
bool hasStorage,
746806
int storageQuantity = 0,
747-
bool singlePhase = false)
807+
bool singlePhase = false,
808+
bool phase2Active = false)
748809
{
749810
var phase1Items = new List<SubscriptionSchedulePhaseItem>
750811
{
@@ -756,12 +817,15 @@ private static SubscriptionSchedule CreateMockSchedule(
756817
phase1Items.Add(new SubscriptionSchedulePhaseItem { PriceId = "price_storage", Quantity = storageQuantity });
757818
}
758819

820+
var phase1Start = phase2Active ? DateTime.UtcNow.AddYears(-1) : DateTime.UtcNow;
821+
var phase1End = phase2Active ? DateTime.UtcNow.AddDays(-1) : DateTime.UtcNow.AddYears(1);
822+
759823
var phases = new List<SubscriptionSchedulePhase>
760824
{
761825
new()
762826
{
763-
StartDate = DateTime.UtcNow,
764-
EndDate = DateTime.UtcNow.AddYears(1),
827+
StartDate = phase1Start,
828+
EndDate = phase1End,
765829
Items = phase1Items,
766830
ProrationBehavior = ProrationBehavior.None
767831
}
@@ -781,7 +845,7 @@ private static SubscriptionSchedule CreateMockSchedule(
781845

782846
phases.Add(new SubscriptionSchedulePhase
783847
{
784-
StartDate = DateTime.UtcNow.AddYears(1),
848+
StartDate = phase1End,
785849
Items = phase2Items,
786850
Discounts =
787851
[

0 commit comments

Comments
 (0)