Implement schedule task to collect billing invoice #417
Conversation
Reviewer's Guide by SourceryThis PR implements a monthly billing system for collecting invoice payments using Celery Beat scheduler. The implementation includes a new BillingInvoice model, Stripe integration for payment processing, automated billing collection tasks, and a billing settings UI for organizers to manage their payment information. Sequence diagram for monthly billing collection tasksequenceDiagram
participant CeleryBeat
participant Task as monthly_billing_collect
participant DB as Database
participant Stripe
CeleryBeat->>Task: Trigger task
Task->>DB: Query for events and orders
alt Orders exist
Task->>DB: Create BillingInvoice
Task->>Stripe: Process payment
else No orders
Task->>Task: Log no orders
end
Task->>DB: Save BillingInvoice
Class diagram for new BillingInvoice modelclassDiagram
class BillingInvoice {
+String STATUS_PENDING = "n"
+String STATUS_PAID = "p"
+String STATUS_EXPIRED = "e"
+String STATUS_CANCELED = "c"
+String status
+Decimal amount
+String currency
+Decimal ticket_fee
+String payment_method
+DateTime paid_datetime
+Text note
+Date monthly_bill
+DateTime created_at
+String created_by
+DateTime updated_at
+String updated_by
+DateTime last_reminder_datetime
+DateTime next_reminder_datetime
+Array reminder_schedule
+Boolean reminder_enabled
+String stripe_payment_intent_id
}
Class diagram for OrganizerBillingModelclassDiagram
class OrganizerBillingModel {
+String primary_contact_name
+String primary_contact_email
+String company_or_organization_name
+String address_line_1
+String address_line_2
+String city
+String zip_code
+String country
+String preferred_language
+String tax_id
+String stripe_customer_id
+String stripe_payment_method_id
+String stripe_setup_intent_id
}
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @lcduong - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- The task is subtracting 3 months instead of 1 month when calculating last_month_date (link)
Overall Comments:
- Consider adding a unique constraint or using select_for_update() to prevent race conditions when creating billing invoices for the same event/month combination
- The monthly_billing_collect task is looking back 3 months (relativedelta(months=3)) instead of 1 month - is this intentional? This seems incorrect for monthly billing
Here's what I looked at during the review
- 🔴 General issues: 1 blocking issue, 2 other issues
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/pretix/eventyay_common/tasks.py
Outdated
| first_day_of_current_month = today.replace(day=1) | ||
| logger.info("Start - running task to collect billing on: %s", first_day_of_current_month) | ||
| # Get the last month by subtracting one month from today | ||
| last_month_date = (first_day_of_current_month - relativedelta(months=3)).date() |
There was a problem hiding this comment.
issue (bug_risk): The task is subtracting 3 months instead of 1 month when calculating last_month_date
This will cause billing to be calculated for the wrong month. Should be relativedelta(months=1) to bill for the previous month.
src/pretix/eventyay_common/tasks.py
Outdated
| @@ -117,3 +122,107 @@ | |||
| 'Content-Type': 'application/json', | |||
| } | |||
| return headers | |||
|
|
|||
|
|
|||
| @shared_task(bind=True, max_retries=5, default_retry_delay=60) # Retries up to 5 times with a 60-second delay | |||
There was a problem hiding this comment.
issue (complexity): Consider extracting the event billing logic into a separate function to reduce complexity of the monthly billing collection task.
The monthly_billing_collect function's complexity can be reduced by extracting the per-event billing logic while maintaining the same error handling strategy. Here's a suggested refactor:
def process_event_billing(event, organizer, last_month_date, ticket_rate, today):
billing_invoice = BillingInvoice.objects.filter(
event=event,
monthly_bill=last_month_date,
organizer=organizer
)
if billing_invoice:
logger.debug("Billing invoice already created for event: %s", event.name)
return
total_amount = calculate_total_amount_on_monthly(event, last_month_date)
tickets_fee = calculate_ticket_fee(total_amount, ticket_rate)
billing_invoice = BillingInvoice(
organizer=organizer,
event=event,
amount=total_amount,
currency=event.currency,
ticket_fee=tickets_fee,
monthly_bill=last_month_date,
reminder_schedule=settings.BILLING_REMINDER_SCHEDULE,
created_at=today,
created_by=settings.PRETIX_EMAIL_NONE_VALUE,
updated_at=today,
updated_by=settings.PRETIX_EMAIL_NONE_VALUE
)
billing_invoice.next_reminder_datetime = get_next_reminder_datetime(
settings.BILLING_REMINDER_SCHEDULE)
billing_invoice.save()
@shared_task(bind=True, max_retries=5, default_retry_delay=60)
def monthly_billing_collect(self):
try:
today = datetime.today()
first_day_of_current_month = today.replace(day=1)
last_month_date = (first_day_of_current_month - relativedelta(months=3)).date()
gs = GlobalSettingsObject()
ticket_rate = gs.settings.get('ticket_rate') or 2.5
for organizer in Organizer.objects.all():
for event in Event.objects.filter(organizer=organizer):
try:
logger.info("Collecting billing for event: %s", event.name)
process_event_billing(event, organizer, last_month_date, ticket_rate, today)
except Exception as e:
logger.error('Error processing billing for event %s: %s', event.slug, e)
continue
except Exception as e:
logger.error('Error in monthly billing collection: %s', e)
self.retry(exc=e)This refactoring:
- Extracts event billing logic to a focused function
- Maintains the two-level error handling strategy
- Reduces nesting depth
- Improves readability while keeping all functionality intact
…sk' into feature-380
…lement-schedule-task
…lement-schedule-task
…lement-schedule-task
There was a problem hiding this comment.
Let introduce only 1 DB migration file, to not inflate the number.
There was a problem hiding this comment.
I solved it, only 1 migration file now for new table creation.
| @@ -735,6 +736,31 @@ | |||
| ('pretix.plugins.banktransfer.*', {'queue': 'background'}), | |||
| ],) | |||
|
|
|||
| CELERY_BEAT_SCHEDULE = { | |||
There was a problem hiding this comment.
Tasks which run periodically, better implemented with systemd timer. Benefit:
-
Beside the time point which you scheduled, you can run the task any time, (when you need to test, or there is a need to run task one more time) with the command
sudo systemctl start task-name.service. -
Easily to temporary disable the schedule, for example when you found that there is bug and you want to disable the task until you fix, by
sudo systemctl stop task-name.timer. -
Logs are collected by
journald(part of systemd), with rich feature to filter and view logs. -
The program only runs when needed. If implemented as Celery Beat, there will be a process always running with the whole code of our application, which is wasteful.
|
As discussed elsewhere:
|
| self.retry(exc=e) | ||
| except self.MaxRetriesExceededError: | ||
| logger.error("Max retries exceeded for billing collect.") | ||
| except Exception as e: |
There was a problem hiding this comment.
Don't catch general Exception class. Catch concrete ones.
This PR is part of issue #383
Implement:
Tasks implemented
For testing, I have created 5 endpoints to trigger the schedule task:
Summary by Sourcery
Implement a scheduled task to collect billing invoices for events monthly using Celery Beat, and introduce a new 'BillingInvoice' model to manage billing data. Update the deployment configuration to support the new task scheduling.
New Features:
Enhancements:
Build:
Deployment: