-
Notifications
You must be signed in to change notification settings - Fork 0
feat: cancelation finalization task #40
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
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.
Pull request overview
This pull request implements a new finalization email task for subscription cancellations. The key changes include:
Changes:
- Removed fallback cancellation email logic from
subscription.updatedevent handler that triggered on status changes to canceled states - Updated
subscription.deletedhandler to checkSTRIPE_ACTIVE_STATUSES(both TRIALING and ACTIVE) instead of just TRIALING, and call the new finalization email task - Added new task
send_finalized_cancelation_email_task()and refactoredsend_trial_cancellation_email_task()to use a shared helper function_send_cancelation_campaign() - Added new Braze campaign setting
BRAZE_SSP_CANCELATION_FINALIZATION_CAMPAIGN - Added comprehensive test coverage for the new finalization email task
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 20 comments.
Show a summary per file
| File | Description |
|---|---|
| enterprise_access/settings/base.py | Added new Braze campaign setting for cancellation finalization |
| enterprise_access/apps/customer_billing/constants.py | Added STRIPE_ACTIVE_STATUSES constant containing ACTIVE and TRIALING statuses |
| enterprise_access/apps/customer_billing/tasks.py | Added new finalization email task, refactored existing cancellation email task to use shared helper function |
| enterprise_access/apps/customer_billing/stripe_event_handlers.py | Removed fallback cancellation logic from subscription.updated, updated subscription.deleted to use new finalization task and check STRIPE_ACTIVE_STATUSES |
| enterprise_access/apps/customer_billing/tests/test_tasks.py | Added comprehensive tests for new finalization email task |
| enterprise_access/apps/customer_billing/tests/test_stripe_event_handlers.py | Removed tests for deleted fallback logic, updated and added tests for subscription.deleted handler changes |
Critical Issue:
There is a pervasive spelling inconsistency throughout the PR. The new code uses "cancelation" (single "l") while the existing codebase consistently uses "cancellation" (double "l"). This affects:
- Setting name:
BRAZE_SSP_CANCELATION_FINALIZATION_CAMPAIGN - Function names:
send_finalized_cancelation_email_task,_send_cancelation_campaign - Docstrings, log messages, and test names
- All references to these identifiers across multiple files
This needs to be corrected to maintain consistency with the existing codebase convention of using "cancellation" with double "l".
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def send_trial_cancellation_email_task( | ||
| checkout_intent_id, trial_end_timestamp | ||
| ): | ||
| def send_finalized_cancelation_email_task(checkout_intent_id, trial_end_timestamp): |
Copilot
AI
Jan 12, 2026
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.
The spelling "cancelation" is inconsistent with the existing codebase convention. The existing code uses "cancellation" (with double "l") consistently. Please update the function name to send_finalized_cancellation_email_task for consistency.
| Send Braze email notification when a trial subscription cancelation is scheduled. | ||
| This task handles sending a scheduled cancelation confirmation email to enterprise |
Copilot
AI
Jan 12, 2026
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.
The spelling "cancelation" is inconsistent with the existing codebase convention. The existing code uses "cancellation" (with double "l") consistently. Please update to use "cancellation" in the docstring.
| campaign_identifier, | ||
| recipients=recipients, | ||
| trigger_properties=braze_trigger_properties, | ||
| organization_name=checkout_intent.enterprise_name, |
Copilot
AI
Jan 12, 2026
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.
The spelling "cancelation" is inconsistent with the existing codebase convention. The existing code uses "cancellation" (with double "l") consistently. Please update to use "cancellation" in the email description parameter.
| from enterprise_access.apps.customer_billing.stripe_event_types import StripeEventType | ||
| from enterprise_access.apps.customer_billing.tasks import ( | ||
| send_billing_error_email_task, | ||
| send_finalized_cancelation_email_task, |
Copilot
AI
Jan 12, 2026
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.
The spelling "cancelation" is inconsistent with the existing codebase convention. The existing code uses "cancellation" (with double "l") consistently. Please update the import and all references to use "cancellation".
| send_finalized_cancelation_email_task, | |
| send_finalized_cancellation_email_task, |
| "enterprise_access.apps.customer_billing.stripe_event_handlers.send_finalized_cancelation_email_task" | ||
| ) | ||
| def test_subscription_deleted_sends_email_for_active_subscription( | ||
| self, mock_send_cancelation_email, mock_cancel, |
Copilot
AI
Jan 12, 2026
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.
The spelling "cancelation" is inconsistent with the existing codebase convention. The existing code uses "cancellation" (with double "l") consistently. Please update the parameter name to use "cancellation".
| checkout_intent_id, | ||
| ) | ||
| send_trial_cancellation_email_task.delay( | ||
| send_finalized_cancelation_email_task.delay( |
Copilot
AI
Jan 12, 2026
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.
The spelling "cancelation" is inconsistent with the existing codebase convention. The existing code uses "cancellation" (with double "l") consistently. Please update the function call to use "cancellation".
| "enterprise_access.apps.customer_billing.tasks.BrazeApiClient" | ||
| ) | ||
| @mock.patch("enterprise_access.apps.customer_billing.tasks.LmsApiClient") | ||
| def test_send_finalized_cancelation_email_success( |
Copilot
AI
Jan 12, 2026
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.
The spelling "cancelation" is inconsistent with the existing codebase convention. The existing code uses "cancellation" (with double "l") consistently. Please update the test method name to use "cancellation".
| def test_send_finalized_cancelation_email_success( | |
| def test_send_finalized_cancellation_email_success( |
|
|
||
| # Run the task and expect exception to be raised | ||
| with self.assertRaises(Exception) as context: | ||
| send_finalized_cancelation_email_task( |
Copilot
AI
Jan 12, 2026
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.
The spelling "cancelation" is inconsistent with the existing codebase convention. The existing code uses "cancellation" (with double "l") consistently. Please update the function call to use "cancellation".
| ] | ||
|
|
||
| # Run the task | ||
| send_finalized_cancelation_email_task( |
Copilot
AI
Jan 12, 2026
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.
The spelling "cancelation" is inconsistent with the existing codebase convention. The existing code uses "cancellation" (with double "l") consistently. Please update the function call to use "cancellation".
| "enterprise_access.apps.customer_billing.tasks.BrazeApiClient" | ||
| ) | ||
| @mock.patch("enterprise_access.apps.customer_billing.tasks.LmsApiClient") | ||
| def test_send_finalized_cancelation_email_braze_exception( |
Copilot
AI
Jan 12, 2026
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.
The spelling "cancelation" is inconsistent with the existing codebase convention. The existing code uses "cancellation" (with double "l") consistently. Please update the test method name to use "cancellation".
b65d07d to
ea66191
Compare
brobro10000
left a comment
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.
Idk if its worth converting all over instances of cancellation to cancelation but feel free to defer on it. Its just a lot of AI comments mentioning the same issue.
ea66191 to
f368e8b
Compare
f368e8b to
35293d8
Compare
|
regarding all the AI spelling commentary - I'd rather do the work to make that spelling consistent in an isolated commit |
* Removed fallback cancellation email logic from subscription.updated handler * Updated subscription.deleted handler to check STRIPE_ACTIVE_STATUSES instead of just TRIALING, and to call send_finalized_cancelation_email_task instead of send_trial_cancellation_email_task * Added new task send_finalized_cancelation_email_task() * Added new Braze campaign setting BRAZE_SSP_CANCELATION_FINALIZATION_CAMPAIGN ENT-11289
35293d8 to
e257050
Compare
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.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """ | ||
| Send Braze email notification when a paid subscription status becomes ``canceled``. | ||
| This task handles sending a final cancelation confirmation email to enterprise | ||
| admins. The email includes the date on which the subscription become canceled. | ||
| Args: | ||
| checkout_intent_id (int): ID of the CheckoutIntent record | ||
| ended_at_timestamp (int): Unix timestamp of when the subscription became canceled. | ||
| Raises: | ||
| BrazeClientError: If there's an error communicating with Braze | ||
| Exception: For any other unexpected errors during email sending | ||
| """ |
Copilot
AI
Jan 13, 2026
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.
Inconsistent spelling: "cancelation" should be "cancellation" to maintain consistency with the existing codebase conventions. Multiple instances in the docstring and function body use "cancelation" when "cancellation" is the standard spelling used elsewhere.
|
|
||
| # Run the task | ||
| send_finalized_cancelation_email_task( | ||
| checkout_intent_id=str(self.checkout_intent.id), |
Copilot
AI
Jan 13, 2026
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.
Inconsistent parameter type: checkout_intent_id is passed as str(self.checkout_intent.id) here, but as self.checkout_intent.id (int) in test_send_finalized_cancelation_email_braze_exception (line 222). According to the function's docstring, checkout_intent_id should be an int. This should be self.checkout_intent.id without str() conversion for consistency.
| checkout_intent_id=str(self.checkout_intent.id), | |
| checkout_intent_id=self.checkout_intent.id, |
| trial_end_value = mock_send_cancelation_email.delay.call_args_list[0].kwargs['trial_end_timestamp'] | ||
| trial_end_value = mock_send_cancelation_email.delay.call_args_list[0].kwargs['ended_at_timestamp'] | ||
| # Test that we use a default trial end of now if no value can be found in the event. | ||
| # The different between these two integer timestamps should be small, |
Copilot
AI
Jan 13, 2026
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.
Spelling error: "different" should be "difference" in the comment.
| # The different between these two integer timestamps should be small, | |
| # The difference between these two integer timestamps should be small, |
| trial_end_value = mock_send_cancelation_email.delay.call_args_list[0].kwargs['ended_at_timestamp'] | ||
| # Test that we use a default trial end of now if no value can be found in the event. | ||
| # The different between these two integer timestamps should be small, | ||
| # certainly less than one second. | ||
| self.assertLess(timezone.now().timestamp() - trial_end_value, 1) |
Copilot
AI
Jan 13, 2026
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.
Misleading variable name: The variable is named "trial_end_value" but it actually holds the "ended_at_timestamp" value for a finalized cancellation, not a trial end timestamp. This should be renamed to "ended_at_value" or "ended_at_timestamp_value" for clarity.
| trial_end_value = mock_send_cancelation_email.delay.call_args_list[0].kwargs['ended_at_timestamp'] | |
| # Test that we use a default trial end of now if no value can be found in the event. | |
| # The different between these two integer timestamps should be small, | |
| # certainly less than one second. | |
| self.assertLess(timezone.now().timestamp() - trial_end_value, 1) | |
| ended_at_timestamp_value = mock_send_cancelation_email.delay.call_args_list[0].kwargs['ended_at_timestamp'] | |
| # Test that we use a default ended_at timestamp of now if no value can be found in the event. | |
| # The difference between these two integer timestamps should be small, | |
| # certainly less than one second. | |
| self.assertLess(timezone.now().timestamp() - ended_at_timestamp_value, 1) |
| """ | ||
| Send Braze email notification when a trial subscription is canceled. | ||
| Send Braze email notification when a trial subscription cancelation is scheduled. | ||
| This task handles sending a cancellation confirmation email to enterprise | ||
| admins when their trial subscription has been canceled. The email includes | ||
| the trial end date and a link to restart their subscription via the Stripe | ||
| billing portal. | ||
| This task handles sending a scheduled cancelation confirmation email to enterprise | ||
| admins. The email includes the (future) trial end date and a link to | ||
| restart their subscription via the Stripe billing portal. |
Copilot
AI
Jan 13, 2026
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.
Inconsistent spelling: "cancelation" should be "cancellation" to maintain consistency with the existing codebase conventions. This appears in the docstring describing the trial subscription cancelation.
| ended_at = subscription.get("ended_at") or timezone.now().timestamp() | ||
| logger.info( | ||
| "Queuing trial cancelation (deletion) email for checkout_intent_id=%s", | ||
| "Queuing cancelation finalization email for checkout_intent_id=%s", |
Copilot
AI
Jan 13, 2026
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.
Inconsistent spelling: "cancelation" should be "cancellation" in the log message to maintain consistency with the existing codebase conventions.
| "Queuing cancelation finalization email for checkout_intent_id=%s", | |
| "Queuing cancellation finalization email for checkout_intent_id=%s", |
| previous_summary = checkout_intent.previous_summary(event, stripe_object_type='subscription') | ||
| if previous_summary.subscription_status == StripeSubscriptionStatus.TRIALING: | ||
| trial_end = subscription.get("trial_end") or timezone.now().timestamp() | ||
| if previous_summary.subscription_status == StripeSubscriptionStatus.ACTIVE: |
Copilot
AI
Jan 13, 2026
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.
The PR description states this should check STRIPE_ACTIVE_STATUSES instead of just TRIALING, but the code only checks for StripeSubscriptionStatus.ACTIVE. This means subscriptions that were in TRIALING status when deleted won't trigger the finalization email. Based on the PR intent and the fact that STRIPE_ACTIVE_STATUSES was added (which includes both ACTIVE and TRIALING), this check should be: if previous_summary.subscription_status in STRIPE_ACTIVE_STATUSES: to properly handle both active paid subscriptions and trial subscriptions that get deleted.
| def test_subscription_deleted_sends_email_for_active_subscription( | ||
| self, mock_send_cancelation_email, mock_cancel, | ||
| ): | ||
| """Subscription deleted event sends finalized cancellation email for ACTIVE subscriptions too.""" | ||
| subscription_id = "sub_test_active_deleted_123" | ||
| subscription_data = { | ||
| "id": subscription_id, | ||
| "status": "canceled", | ||
| "trial_end": 987654321, | ||
| "ended_at": 1234567890, | ||
| "metadata": self._create_mock_stripe_subscription(self.checkout_intent.id), | ||
| } | ||
|
|
||
| # Create prior event with ACTIVE status (not TRIALING) | ||
| self._create_existing_event_data_records( | ||
| subscription_id, | ||
| subscription_status=StripeSubscriptionStatus.ACTIVE, | ||
| ) | ||
|
|
||
| # Ensure enterprise_uuid is present so handler proceeds with cancellation | ||
| self.checkout_intent.enterprise_uuid = uuid.uuid4() | ||
| self.checkout_intent.save(update_fields=["enterprise_uuid"]) | ||
|
|
||
| mock_event = self._create_mock_stripe_event( | ||
| "customer.subscription.deleted", subscription_data | ||
| ) | ||
|
|
||
| StripeEventHandler.dispatch(mock_event) | ||
|
|
||
| mock_cancel.assert_called_once_with(self.checkout_intent) | ||
| mock_send_cancelation_email.delay.assert_called_once_with( | ||
| checkout_intent_id=self.checkout_intent.id, | ||
| ended_at_timestamp=1234567890, | ||
| ) |
Copilot
AI
Jan 13, 2026
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.
Missing test coverage: There's no test that verifies the finalized cancellation email is sent when a subscription in TRIALING status is deleted. The existing test only covers ACTIVE status (test_subscription_deleted_sends_email_for_active_subscription). A test should be added to verify this behavior for TRIALING subscriptions as well, especially given that the PR description mentions checking STRIPE_ACTIVE_STATUSES which includes both ACTIVE and TRIALING.
| from enterprise_access.apps.customer_billing.constants import ( | ||
| INVOICE_PAID_PARENT_TYPE_IDENTIFIER, | ||
| STRIPE_CANCELED_STATUSES, | ||
| StripeSubscriptionStatus |
Copilot
AI
Jan 13, 2026
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.
Missing import: STRIPE_ACTIVE_STATUSES is defined in constants.py but not imported here. If the subscription_deleted handler is updated to check for STRIPE_ACTIVE_STATUSES (as the PR description suggests), this constant needs to be added to the imports from enterprise_access.apps.customer_billing.constants.
| StripeSubscriptionStatus | |
| STRIPE_ACTIVE_STATUSES, | |
| StripeSubscriptionStatus, |
| # Test that we use a default trial end of now if no value can be found in the event. | ||
| # The different between these two integer timestamps should be small, |
Copilot
AI
Jan 13, 2026
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.
Outdated comment: The comment refers to "trial end" but the variable name and parameter have been changed to "ended_at_timestamp" to reflect that this is for finalized cancellations, not just trial endings. The comment should be updated to say "ended_at" instead of "trial end" for accuracy.
| # Test that we use a default trial end of now if no value can be found in the event. | |
| # The different between these two integer timestamps should be small, | |
| # Test that we use a default ended_at timestamp of now if no value can be found in the event. | |
| # The difference between these two integer timestamps should be small, |
ENT-11289
Merge checklist:
./manage.py makemigrationshas been runPost merge: