Organiser profile billing settings option missing #411
Conversation
Reviewer's Guide by SourceryThis PR implements a new Billing Settings feature in the organizer settings area, allowing organizers to manage their billing information and integrate with Stripe for payment processing. The implementation includes form handling, database models, Stripe API integration, and UI components for managing billing details and payment methods. ER diagram for OrganizerBillingModelerDiagram
ORGANIZER ||--o{ ORGANIZER_BILLING_MODEL : has
ORGANIZER_BILLING_MODEL {
int id
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
}
Class diagram for BillingSettingsForm and OrganizerBillingModelclassDiagram
class BillingSettingsForm {
+CharField primary_contact_name
+EmailField primary_contact_email
+CharField company_or_organization_name
+CharField address_line_1
+CharField address_line_2
+CharField zip_code
+CharField city
+ChoiceField country
+ChoiceField preferred_language
+CharField tax_id
+__init__(*args, **kwargs)
+set_initial_data()
+save(commit=True)
}
class OrganizerBillingModel {
+CharField primary_contact_name
+EmailField primary_contact_email
+CharField company_or_organization_name
+CharField address_line_1
+CharField address_line_2
+CharField city
+CharField zip_code
+CharField country
+CharField preferred_language
+CharField tax_id
+CharField stripe_customer_id
+CharField stripe_payment_method_id
+CharField stripe_setup_intent_id
+delete(*args, **kwargs)
+save(*args, **kwargs)
}
BillingSettingsForm --> OrganizerBillingModel : uses
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 @odkhang - I've reviewed your changes - here's some feedback:
Overall Comments:
- The payment_information field in OrganizerBillingModel has an erroneous trailing comma in its tuple definition that needs to be removed
- Consider adding validation for tax ID formats based on the selected country, as different regions have different requirements
- Storing payment information in plaintext raises security concerns - consider encrypting sensitive data or using a more secure storage method
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 2 issues 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.
| ) | ||
| return slug | ||
|
|
||
|
|
||
| class BillingSettingsForm(forms.ModelForm): |
There was a problem hiding this comment.
issue (complexity): Consider using a data structure to define common form fields with shared properties.
The form field definitions can be simplified by using a data structure to define common fields while keeping special cases explicit. This reduces duplication while maintaining clarity:
class BillingSettingsForm(forms.ModelForm):
# Define common fields in a data structure
COMMON_FIELDS = {
'primary_contact_name': {
'label': _("Primary Contact Name"),
'help_text': _("Please provide your name or the name of the person responsible for this account in your organization.")
},
'primary_contact_email': {
'label': _("Primary Contact Email"),
'help_text': _("We will use this email address for all communication related to your contract and billing, as well as for important updates about your account and our services."),
'field_class': forms.EmailField
},
# ... define other common fields similarly
}
# Generate common fields
for field_name, config in COMMON_FIELDS.items():
locals()[field_name] = config.get('field_class', forms.CharField)(
label=config['label'],
help_text=config['help_text'],
required=True,
max_length=255,
widget=forms.TextInput(attrs={'placeholder': ''})
)
# Special cases defined explicitly
country = forms.ChoiceField(
label=_("Country"),
help_text=_("Select your country."),
required=True,
choices=CachedCountries(),
initial="US",
)
# ... rest of the form implementationThis approach:
- Reduces duplicate code while maintaining readability
- Makes it easier to modify common field properties
- Keeps special cases explicit where needed
- Preserves all functionality
src/pretix/control/utils.py
Outdated
| stripe.api_key = get_stripe_secret_key() | ||
|
|
||
|
|
||
| def handle_stripe_errors(operation_name: str): |
There was a problem hiding this comment.
issue (complexity): Consider refactoring the error handling decorator to use a configuration-based approach.
The error handling decorator can be simplified while maintaining the same detailed error handling capabilities. Here's a suggested refactor:
ERROR_HANDLERS = {
stripe.error.APIError: {
'log_format': "Stripe API error during {operation}: {error}",
'message_format': "Stripe service error: {message}"
},
stripe.error.CardError: {
'log_format': "Card error during {operation}: {error} | Code: {code} | Decline code: {decline_code}",
'message_format': "Card error: {user_message}"
},
# Add other error types similarly...
}
def handle_stripe_errors(operation_name: str):
def decorator(func):
@wraps(func)
def wrapper(*args, **kwargs):
try:
return func(*args, **kwargs)
except tuple(ERROR_HANDLERS.keys()) as e:
handler = ERROR_HANDLERS[type(e)]
# Log the error with specific format
log_args = {
'operation': operation_name,
'error': str(e),
'code': getattr(e, 'code', 'N/A'),
'decline_code': getattr(e, 'decline_code', 'N/A')
}
logger.error(handler['log_format'].format(**log_args))
# Raise formatted validation error
message_args = {
'message': str(e),
'user_message': getattr(e, 'user_message', str(e))
}
raise ValidationError(handler['message_format'].format(**message_args))
except Exception as e:
logger.error(
f"Unexpected error during {operation_name}: {str(e)}",
exc_info=True
)
raise ValidationError(f"An unexpected error occurred: {str(e)}")
return wrapper
return decoratorThis approach:
- Moves error handling configuration to a data structure
- Reduces code duplication while maintaining detailed error handling
- Makes it easier to modify error handling patterns
- Keeps all existing functionality and logging detail
There was a problem hiding this comment.
@odkhang Please also take the sourcery reviews into account, specifically the "erroneous trailing comma". Thanks
| {% endif %} | ||
| </fieldset> | ||
| </div> | ||
| {% endblock %} |
There was a problem hiding this comment.
It has been reminded many times that you need to configure your editor to automatically add 1 blank line at the end of file.
src/pretix/control/utils.py
Outdated
|
|
||
| def get_stripe_key(key_type: str) -> str: | ||
| gs = GlobalSettingsObject() | ||
| prod_key = getattr(gs.settings, f"payment_stripe_connect_{key_type}_key") |
There was a problem hiding this comment.
AttributeError exception could happen here. Please hanlde it.
| const url = window.location.href | ||
| const organizerMatch = url.match(/organizer\/([^/]+)/); | ||
| const organizerSlug = organizerMatch ? organizerMatch[1] : null; | ||
| window.location.href = `${basePath}/control/organizer/${organizerSlug}/settings/billing`; |
There was a problem hiding this comment.
Hhmm, it is easy to see that the URL can be generated as .../control/organizer/null/settings/billing. Do you really want to make browser redirect to invalid URL (with null)?
src/pretix/control/utils.py
Outdated
|
|
||
| if billing_settings and billing_settings.stripe_customer_id: | ||
| return billing_settings.stripe_customer_id | ||
| else: |
There was a problem hiding this comment.
You don't need to write else keyword here, because the if branch makes function return.
src/pretix/control/utils.py
Outdated
| raise ValidationError( | ||
| f"Payment processing error: {getattr(e, 'user_message', str(e))}" | ||
| ) | ||
| except Exception as e: |
There was a problem hiding this comment.
Don't catch the general Exception class here.
src/pretix/control/utils.py
Outdated
| ) | ||
| except stripe.error.APIConnectionError as e: | ||
| logger.error( | ||
| "API connection error during {}: %s", operation_name, str(e) |
There was a problem hiding this comment.
ensuring the message is constructed only if the log level matches.
except stripe.error.APIConnectionError as e:
logger.error(
"API connection error during %s: %s", operation_name, str(e)
)
raise ValidationError("Network communication error: {}".format(str(e)))
src/pretix/base/models/organizer.py
Outdated
|
|
||
| payment_information = models.TextField( | ||
| verbose_name=_("Payment Information"), | ||
| ), |
There was a problem hiding this comment.
The payment_information field in OrganizerBillingModel has an erroneous trailing comma in its tuple definition that needs to be removed
|
hi @mariobehling, as sourcery-ai mentioned: "Consider adding validation for tax ID formats based on the selected country, as different regions have different requirements" |
Yes, thanks. Sounds good. Let's try that. |
There was a problem hiding this comment.
Hey @odkhang - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Avoid exposing internal Stripe error details in user-facing error messages (link)
Overall Comments:
- Consider adding logging for successful Stripe operations, not just errors, to help with monitoring and debugging.
- The frontend JavaScript should provide better error feedback to users when Stripe operations fail.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🔴 Security: 1 blocking issue
- 🟢 Testing: all looks good
- 🟡 Complexity: 2 issues 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/control/utils.py
Outdated
| def wrapper(*args, **kwargs): | ||
| try: | ||
| return func(*args, **kwargs) | ||
| except stripe.error.APIError as e: |
There was a problem hiding this comment.
🚨 issue (security): Avoid exposing internal Stripe error details in user-facing error messages
Replace getattr(e, 'user_message', str(e)) with generic error messages to avoid leaking sensitive information about the Stripe integration.
src/pretix/control/utils.py
Outdated
| return get_stripe_key("publishable") | ||
|
|
||
|
|
||
| def handle_stripe_errors(operation_name: str): |
There was a problem hiding this comment.
issue (complexity): Consider using a context manager pattern instead of decorators for Stripe operations
The current implementation uses decorators that duplicate error handling logic across multiple functions. Consider using a context manager pattern to reduce complexity and handle both API key setting and error handling in one place:
@contextmanager
def stripe_operation(operation_name: str):
stripe.api_key = get_stripe_secret_key()
try:
yield
except stripe.error.APIError as e:
logger.error("Stripe API error during %s: %s", operation_name, str(e))
raise ValidationError(
"Stripe service error: {}".format(getattr(e, "user_message", str(e)))
)
# ... other error handlers ...
# Usage example:
def create_setup_intent(customer_id: str) -> str:
with stripe_operation("create_setup_intent"):
stripe_setup_intent = stripe.SetupIntent.create(
customer=customer_id,
payment_method_types=["card"],
usage="off_session",
)
OrganizerBillingModel.objects.filter(stripe_customer_id=customer_id).update(
stripe_setup_intent_id=stripe_setup_intent.id
)
return stripe_setup_intent.client_secretThis approach:
- Eliminates duplicate error handling code
- Centralizes API key setting
- Maintains the same error handling functionality
- Makes functions shorter and more focused on their core logic
| @@ -22,3 +25,153 @@ def clean_slug(self): | |||
| code='duplicate_slug', | |||
| ) | |||
| return slug | |||
|
|
|||
|
|
|||
| class BillingSettingsForm(forms.ModelForm): | |||
There was a problem hiding this comment.
issue (complexity): Consider refactoring form field definitions into a declarative specification dictionary to reduce code duplication.
The form fields can be simplified using a declarative approach while maintaining readability. Here's how:
class BillingSettingsForm(forms.ModelForm):
FIELD_SPECS = {
'primary_contact_name': {
'type': forms.CharField,
'label': _("Primary Contact Name"),
'help_text': _("Please provide your name or the name of the person responsible for this account in your organization."),
'required': True,
},
'primary_contact_email': {
'type': forms.EmailField,
'label': _("Primary Contact Email"),
'help_text': _("We will use this email address for all communication related to your contract and billing, "
"as well as for important updates about your account and our services."),
'required': True,
},
# ... define other fields similarly
}
class Meta:
model = OrganizerBillingModel
fields = list(FIELD_SPECS.keys())
def __init__(self, *args, **kwargs):
self.organizer = kwargs.pop("organizer", None)
super().__init__(*args, **kwargs)
# Generate fields from specifications
for field_name, specs in self.FIELD_SPECS.items():
field_type = specs.pop('type')
self.fields[field_name] = field_type(
max_length=255,
widget=forms.TextInput(attrs={"placeholder": ""}),
**specs
)
# Special handling for specific fields
self.fields['country'].widget = forms.Select(choices=CachedCountries())
self._setup_language_field()
self.set_initial_data()This approach:
- Reduces repetition while maintaining clarity
- Makes field properties easier to audit and modify consistently
- Keeps special cases explicit
- Reduces the chance of inconsistencies between similar fields
| for code, name in settings.LANGUAGES | ||
| if code in self.organizer.settings.locales | ||
| ] | ||
| self.base_fields["preferred_language"].choices = selected_languages |
There was a problem hiding this comment.
Why do you modify self.base_fields instead of self.fields?
There was a problem hiding this comment.
I use self.base_fields instead of self.fields because I want to modify the original field definitions before the form is initialized. Therefore, I use self.base_fields before calling super().init(*args, **kwargs). On the other hand, if you need to modify form fields after the form has been initialized, you should use self.fields. Thank you for reviewing. If you have any further questions, please let me know
| name=self.cleaned_data.get("primary_contact_name"), | ||
| ) | ||
| billing_settings.save() | ||
| return billing_settings |
There was a problem hiding this comment.
Why this method suddenly returns a different object than what is declared in Meta? This is a hidden magic which will cause confusion and bugs in the future.
There was a problem hiding this comment.
Thank you for reviewing. I have resolved the issue. Please check again. If you have any further questions, please let me know.
| const organizerSlug = organizerMatch ? organizerMatch[1] : null; | ||
| if(!organizerSlug) { | ||
| console.error('Organizer slug not found'); | ||
| return window.location.href = `${basePath}/control/organizers/` |
There was a problem hiding this comment.
It is weird that you put a "statement" in place of "expression". The return keyword, if going with something, expects an "expression". Please write a proper code.
There was a problem hiding this comment.
Thank you for reviewing. I have resolved the issue. Please check again. If you have any further questions, please let me know.
| form.save() | ||
| messages.success(self.request, _("Your changes have been saved.")) | ||
| return redirect(self.get_success_url()) | ||
| except ValidationError as e: |
There was a problem hiding this comment.
You are using transaction.atomic in wrong way: You handle exception inside atomic block (doc).
Another issue, after calling form.is_valid(), you still doing more validation (if vat_number, if not is_valid_vat_number). It shows that you are using Django Form wrong way. Validations should be done in form.is_valid() call, not after it.
There was a problem hiding this comment.
Thank you for reviewing. I have resolved the issue.
- I removed the use of transaction.atomic because I am only saving billing setting information into the billing settings table, which is independent of other tables. If an error occurs while adding data, it won't affect the other tables.
- I moved the tax_id validation into the is_valid function in the form.
Please check again. If you have any further questions, please let me know.
This PR resolve issue #380 Organiser profile billing settings option missing
Implement a new Billing settings page in organizer setting area, allow organizer to input billing information.
Summary by Sourcery
Implement a new Billing Settings page in the organizer settings area, allowing organizers to input and manage billing information, including integration with Stripe for payment processing.
New Features:
Enhancements:
Documentation: