support: Set discounted price instead percentage for customers. #29957
Conversation
|
Nice! Just to check, since I don't see it in the screenshots -- is there a confirmation banner? |
No, I can add that as a separate PR since we don't have confirmation banners on this page. |
|
Hm, I just meant these banners, which we currently have:
|
amanagr
force-pushed
the
review_discount_price
branch
from
a0c3dd0
to
517fea1
Compare
|
Updated the PR as discussed here 
|
amanagr
force-pushed
the
review_discount_price
branch
from
517fea1
to
dfda1e9
Compare
alya
added
the
integration review
|
Looks great to me! |
| def estimate_annual_recurring_revenue_by_realm() -> Dict[str, int]: # nocoverage | ||
| def get_estimated_arr_and_rate_by_realm() -> Tuple[Dict[str, int], Dict[str, str]]: # nocoverage | ||
| # NOTE: Customers without a plan might still have a discount attached to them which | ||
| # are not included in `plan_rate`. |
There was a problem hiding this comment.
This would be basically a stale discount percent display value? I guess we could do a migration to clear those if it was a priority.
There was a problem hiding this comment.
No, we don't want to do anything here. I just wanted to note that even if there is no discount displayed for the customer in the support table, that doesn't mean there is no discount applied to them. I am not sure where to best reflect that.
There was a problem hiding this comment.
Okay, I guess that won't be valid after #29957 (comment)
| ) | ||
| self.assert_in_success_response( | ||
| ["Discount for realm-name-4 changed to 50% from 0%."], result | ||
| [ | ||
| "Monthly price for realm-name-4 changed to 50 from 0. Annual price changed to 500 from 0." |
There was a problem hiding this comment.
The "from 0" feels odd here; probably that suggests the success message code should be saying "from the default price" for this case.
| row["plan_type"] == Realm.PLAN_TYPE_LIMITED | ||
| and string_id in realms_with_default_discount | ||
| ): | ||
| row["effective_rate"] = 100 - int(realms_with_default_discount[string_id]) |
There was a problem hiding this comment.
Would it make sense to still display an effective rate for this case based on their required_plan_tier configuration?
Could be a follow-up, but that seems to make sense to me.
| } | ||
| } | ||
| }, | ||
| ); |
There was a problem hiding this comment.
@laurynmm can you also review the support panel code as its maintainer?
| plan.discount = format_discount_percentage( | ||
| Decimal((original_plan_price - plan.price_per_license) / original_plan_price * 100) | ||
| ) | ||
| plan.save(update_fields=["price_per_license", "discount"]) |
There was a problem hiding this comment.
Would it make sense to rework plan.discount to be a property, rather than a database field? Just thinking about how if we change the base pricing, we'll need to run a migration to update these cached values.
There was a problem hiding this comment.
Yeah, especially since annual and monthy discounted prices could be different and hence the plan.discount.
| @@ -1276,49 +1278,65 @@ def create_card_update_session(self) -> Dict[str, Any]: | |||
| def apply_discount_to_plan( | |||
| self, | |||
| plan: CustomerPlan, | |||
| discount: Decimal, | |||
| customer: Customer, | |||
There was a problem hiding this comment.
Isn't this just self.get_customer()? I think we generally try to avoid passing that as a parameter.
There was a problem hiding this comment.
Yeah, I can just modify it to use plan.customer.
timabbott
force-pushed
the
review_discount_price
branch
from
dfda1e9
to
acbd3f4
Compare
|
This looks good to me, merged with the below minor changes. Several notes above about potential follow-ups may be well worth pursuing, but I think they all are mostly invisible technical tweaks and would be easier to review as follow-ups anyway, so there's no reason to block on finishing them. Thanks for doing this @amanagr! diff --git a/corporate/migrations/0043_remove_customer_default_discount_and_more.py b/corporate/migrations/0043_remove_customer_default_discount_and_more.py
index 9e0b402144..21e7d2c4af 100644
--- a/corporate/migrations/0043_remove_customer_default_discount_and_more.py
+++ b/corporate/migrations/0043_remove_customer_default_discount_and_more.py
@@ -6,6 +6,10 @@ from django.db import migrations, models
from django.db.backends.base.schema import BaseDatabaseSchemaEditor
from django.db.migrations.state import StateApps
+# It's generally unsafe to import product code from migrations,
+# because the migration will be run with the **current** version of
+# that code, not the version at the time of the migration. But because
+# this migration will only be run for Zulip Cloud, this is OK.
from corporate.lib.stripe import get_price_per_license
@@ -17,6 +21,12 @@ def calculate_discounted_price_per_license(
def calculate_discounted_price(apps: StateApps, schema_editor: BaseDatabaseSchemaEditor) -> None:
+ """Migrates existing customers with a configured default discount to
+ instead have discounted prices for their configured required_plan_tier.
+
+ Does not operate on CustomerPlan fields, since those are already
+ resolved into a price.
+ """
BILLING_SCHEDULE_ANNUAL = 1
BILLING_SCHEDULE_MONTHLY = 2
@@ -39,6 +49,9 @@ def calculate_discounted_price(apps: StateApps, schema_editor: BaseDatabaseSchem
monthly_price_per_license, customer.default_discount
)
customers_to_update.append(customer)
+ print(
+ f"\nChanging price for {customer.id}: {customer.default_discount} => {customer.annual_discounted_price}/{customer.monthly_discounted_price}"
+ )
Customer.objects.bulk_update(
customers_to_update, ["annual_discounted_price", "monthly_discounted_price"] |
This allows us to set the price of a plan exactly as discussed with the customer.
|
Ci failed due to this, fixed in 1d52d08 |
amanagr
force-pushed
the
review_discount_price
branch
from
acbd3f4
to
4568d37
Compare
First we need to run #29890 for exisitng customers (not required for testing this PR though)
cloud discount
basic free trial discount
business discount
next plan discount