Skip to content

Fix overrides bug #2071

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.

Sign up for GitHub

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

Jump to bottom
Merged
reshmabidikar merged 4 commits into from
Dec 20, 2024
Merged

Fix overrides bug #2071

reshmabidikar merged 4 commits into from
Dec 20, 2024
+110 −13

Conversation

@reshmabidikar
Copy link
Contributor

@reshmabidikar reshmabidikar commented Dec 16, 2024 •
edited
Loading

There is a bug in the code due to which it is currently not possible to override a usage price by itself, it is necessary to update either a fixed or a recurring price too. This PR attempts to address this bug.

See this comment.

@reshmabidikar
Copy link
Contributor Author

reshmabidikar commented Dec 16, 2024

If you agree that this is a bug, I can write a test for this fix.

sbrossie
sbrossie reviewed Dec 16, 2024
View reviewed changes
Copy link
Member

@sbrossie sbrossie left a comment •
edited
Loading

Choose a reason for hiding this comment

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

OK with the fix.

  1. I would suggest to even throw if we end up with null currency as it will break down the line
  2. Is there a way add a small test (if easy)

@reshmabidikar
Copy link
Contributor Author

reshmabidikar commented Dec 18, 2024

For 2/- above, I've added a test See c29ce39.
For 1/ above, I was not sure where to throw an exception. So I've added a validation to check that there is at least one overridden price and a test for this. With this change, there is now no need for this check. See e941bba. If this is not what you meant please let me know and I can make changes accordingly.

@reshmabidikar reshmabidikar marked this pull request as ready for review December 18, 2024 09:48
sbrossie
sbrossie approved these changes Dec 19, 2024
View reviewed changes
profiles/killbill/src/test/java/org/killbill/billing/jaxrs/TestEntitlement.java
requestOptions);
callbackServlet.assertListenerStatus();
final Subscription subscription = subscriptionApi.getSubscription(entitlementJson.getSubscriptionId(), requestOptions);
Assert.assertEquals(subscription.getState(), EntitlementState.ACTIVE);
Copy link
Member

@sbrossie sbrossie Dec 19, 2024

Choose a reason for hiding this comment

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

Any way we could check that price was correctly overridden - would subscription#getLastActivePlan() provide such info?

Copy link
Contributor Author

@reshmabidikar reshmabidikar Dec 20, 2024

Choose a reason for hiding this comment

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

Addressed in 984a82d

jaxrs/src/main/java/org/killbill/billing/jaxrs/resources/SubscriptionResourceHelpers.java Outdated
for (final PhasePriceJson input : priceOverrides) {
Preconditions.checkNotNull(input);

Preconditions.checkArgument(input.getFixedPrice() != null || input.getRecurringPrice() != null || input.getUsagePrices() != null || (input.getUsagePrices() != null && !input.getUsagePrices().isEmpty()),
Copy link
Member

@sbrossie sbrossie Dec 19, 2024

Choose a reason for hiding this comment

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

I think we can remove the input.getUsagePrices() != null and simply keep the (input.getUsagePrices() != null && !input.getUsagePrices().isEmpty()

Copy link
Contributor Author

@reshmabidikar reshmabidikar Dec 20, 2024

Choose a reason for hiding this comment

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

Addressed in 984a82d

jaxrs/src/main/java/org/killbill/billing/jaxrs/resources/SubscriptionResourceHelpers.java Outdated
final PlanPhaseSpecifier planPhaseSpecifier = spec.getPlanName() != null ?
new PlanPhaseSpecifier(spec.getPlanName(), phaseType) :
new PlanPhaseSpecifier(spec.getProductName(), spec.getBillingPeriod(), spec.getPriceListName(), phaseType);
final Currency resolvedCurrency = input.getFixedPrice() != null || input.getRecurringPrice() != null || (input.getUsagePrices() != null && !input.getUsagePrices().isEmpty()) ? currency : null;
Copy link
Member

@sbrossie sbrossie Dec 19, 2024

Choose a reason for hiding this comment

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

Good point, I don't think this is needed.

@reshmabidikar reshmabidikar merged commit bbe8e95 into killbill:master Dec 20, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sbrossie sbrossie sbrossie approved these changes

@pierre pierre Awaiting requested review from pierre

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@reshmabidikar @sbrossie