fix(chore): global settings redesign #1030
Conversation
Reviewer's GuideRedesigns the global settings interface by grouping form fields into categorized tabs (including segregating Stripe and PayPal under Payment Gateways), updates template logic accordingly, refactors plugin loading in settings.py using entry_points.select with exclusion support, and adds a get_item template filter. Class diagram for updated global settings form structureclassDiagram
class GlobalSettingsForm {
+field_groups: list
__init__(*args, **kwargs)
}
class SecretKeySettingsField
class StripeKeyValidator
class forms.CharField
class forms.DecimalField
GlobalSettingsForm --> SecretKeySettingsField
GlobalSettingsForm --> StripeKeyValidator
GlobalSettingsForm --> forms.CharField
GlobalSettingsForm --> forms.DecimalField
GlobalSettingsForm : payment_stripe_connect_publishable_key
GlobalSettingsForm : payment_stripe_connect_test_secret_key
GlobalSettingsForm : payment_stripe_connect_test_publishable_key
GlobalSettingsForm : field_groups
Class diagram for new get_item template filterclassDiagram
class hierarkey_form {
+get_item(dictionary, key)
}
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 there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `app/eventyay/control/templatetags/hierarkey_form.py:79-81` </location>
<code_context>
return PropagatedNode(nodelist, event, [f[1:-1] for f in args], url)
+
+
[email protected]
+def get_item(dictionary, key):
+ return dictionary.get(key)
</code_context>
<issue_to_address>
**suggestion:** Consider renaming the filter to 'getitem' for consistency with template usage.
Register the filter as 'getitem' to align with its usage in templates and prevent confusion.
</issue_to_address>
### Comment 2
<location> `app/eventyay/control/templates/pretixcontrol/global_settings.html:4` </location>
<code_context>
{% extends "pretixcontrol/global_settings_base.html" %}
{% load i18n %}
{% load bootstrap3 %}
+{% load getitem %}
{% load static %}
</code_context>
<issue_to_address>
**issue (bug_risk):** Template filter name should match the registered filter.
Verify that the filter name used here matches its registration in 'hierarkey_form.py' to prevent template errors.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
I have a big doubt on 81866cd and 9107d16 I don't know what was(is) the problem, but these commits happen to duplicate stripe form fields in Global settings. |
Gagan-Ram
force-pushed
the
fix/global_settings_redesign
branch
from
d8ab718 to
b04903b
Compare
|
Please Note: |
There was a problem hiding this comment.
Please note that we need Stripe two times:
- We need Stripe as a payment method where organiser pay for platform fees. This is implemented in the core system.
- We need Stripe to accept payments for event attendees. This is implemented in the Stripe plugin.
So, in the admin settings we need one option for the platform stripe implementation and if the Stripe plugin is activated it should take this information and display additional information if needed.
There was a problem hiding this comment.
Must fix before merge
- i18n bug in template section check
global_settings.htmlcompares a translated label to a hard-coded English string:
{% if group_label == "Payment Gateways" %}Since group_label comes from _(...), this will break in non-English locales. Compare on a stable key, not the translated label (e.g., make field_groups tuples of (key, label, fields) and check key == 'payment_gateways').
- Duplicate “Save” text in submit button
Button renders “Save” twice:
{% trans "Save" %}
{% translate "Save" %}Keep one.
- Broken/ambiguous plugin discovery in settings
-
Uses
entry_points(group='pretalx.plugin')without importing a top-levelentry_pointsfunction (onlyimport importlib_metadataexists). -
Then shadows the name with
entry_points = importlib_metadata.entry_points()and later calls.select(...). -
Also mixes
PLUGINS.append(...)with directINSTALLED_APPS += (...), risking duplicates and non-deterministic order.
Fix:- Remove the old/undefined
entry_points(group=...)usage. - Don’t shadow—use
eps = importlib_metadata.entry_points()and iteratefor ep in eps.select(group='pretix.plugin'):. - Choose a single source of truth: either build
PLUGINSand extendINSTALLED_APPSonce afterward, or append toINSTALLED_APPSonly—consistently and in a deterministic order.
- Remove the old/undefined
- Exclude list parsing can produce spurious empty entries
config.get(...).split(',')yields['']for empty values. Strip empties before use:
raw = config.get('eventyay', 'plugins_exclude', fallback='')
PRETIX_PLUGINS_EXCLUDE = [m.strip() for m in raw.split(',') if m.strip()]Prevents accidental (non)matches. (
🔎 Minimal test plan (manual)
- Apart from manually testing Stripe and Paypal keys change locale (e.g., German) and confirm the Payment Gateways tab logic still works (after switching to key-based check).
- Configure
plugins_excludein the config → excluded plugins are not added toINSTALLED_APPS; non-excluded ones are added once in stable order.
Gagan-Ram
force-pushed
the
fix/global_settings_redesign
branch
from
83e381f to
f3c6738
Compare
|
Done. |
Gagan-Ram
force-pushed
the
fix/global_settings_redesign
branch
from
f3c6738 to
cc05ab2
Compare
There was a problem hiding this comment.
Hi @Gagan-Ram, this pull request seems to be causing crashes on the Docker setup. Some of these also happen when I run via python setup The following is what I see:
RuntimeError: Python package 'stripe' is not installed.
ModuleNotFoundError: No module named 'pretix'
RuntimeError: Python package 'paypal' is not installed.
can you take a look on this
|
@Saksham-Sirohi This PR requires corresponding changes in both eventyay-tickets-paypal and eventyay-tickets-stripe, as reflected in fossasia/eventyay-tickets-paypal#5 and fossasia/eventyay-tickets-stripe#16 |
There was a problem hiding this comment.
Pull Request Overview
This PR updates payment gateway dependencies (PayPal and Stripe plugins) to newer versions and enhances the global settings UI by organizing fields into tabs. It also refactors the plugin loading mechanism to support both Pretix and Pretalx plugins.
Key changes:
- Updated git revision hashes for
eventyay-paypalandeventyay-stripedependencies - Added tabbed navigation to global settings form for better organization (Basics, Localization, Email, Payment Gateways, Ticket fee, Maps)
- Implemented a
getitemtemplate filter inhierarkey_form.pyto dynamically access form fields - Refactored plugin loading to distinguish between Pretix and Pretalx plugins with exclusion capability
Reviewed Changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| app/uv.lock | Updated PayPal and Stripe plugin dependency revisions |
| app/pyproject.toml | Updated PayPal and Stripe plugin dependency revisions |
| app/eventyay/control/templatetags/hierarkey_form.py | Added getitem template filter for dynamic form field access |
| app/eventyay/control/templates/pretixcontrol/global_settings.html | Restructured template with tabbed navigation and payment gateway grouping |
| app/eventyay/control/forms/global_settings.py | Added field_groups attribute to organize form fields into categories |
| app/eventyay/config/settings.py | Refactored plugin loading with support for Pretix/Pretalx plugins and exclusion list |
Comments suppressed due to low confidence (2)
app/eventyay/config/settings.py:257
- Print statement may execute during import.
print(f"Plugin not found: {module_name} ({e})")
app/eventyay/config/settings.py:259
- Print statement may execute during import.
print(f"Failed to import plugin {module_name}: {e}")
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @register.filter | ||
| def getitem(value, key): | ||
| """ | ||
| Template filter to safely access dictionary or object attributes by key. | ||
| Example: {{ form|getitem:field_name }} | ||
| """ | ||
| try: | ||
| if isinstance(value, dict): | ||
| return value.get(key) | ||
| return getattr(value, key, None) | ||
| except (AttributeError, KeyError, TypeError): | ||
| # - AttributeError: when object has no such attribute | ||
| # - KeyError: when key not found in a dict-like object | ||
| # - TypeError: when value is not subscriptable or key is wrong type | ||
| return None |
There was a problem hiding this comment.
A getitem template filter already exists in app/eventyay/control/templatetags/getitem.py with the same name but different implementation. This creates a duplicate filter registration that could lead to unpredictable behavior depending on which module Django loads first. The existing filter only handles dictionary-like access (value[itemname]), while this new one handles both dictionaries and object attributes. Consider consolidating these into a single filter in one location, or rename one of them to avoid conflicts (e.g., getitem_safe or get_field).
There was a problem hiding this comment.
My bad, the code in app/eventyay/control/templates/pretixcontrol/global_settings.html do use filter registered in getitem.py. I'll delete the redundant code from here.
| {% extends "pretixcontrol/global_settings_base.html" %} | ||
| {% load i18n %} | ||
| {% load bootstrap3 %} | ||
| {% load getitem %} |
There was a problem hiding this comment.
The template loads the getitem module but there are two separate files defining a getitem filter (getitem.py and hierarkey_form.py). Django's template loading may produce unpredictable results when multiple filters share the same name. The load statement should explicitly specify which module to use, or the duplicate filter definitions should be consolidated. Since hierarkey_form is loaded implicitly through template tag discovery, consider either removing the duplicate filter or using a unique name.
| {% load getitem %} | |
| {% load getitem from getitem %} |
|
|
||
| import configparser | ||
| import importlib | ||
| import importlib_metadata |
There was a problem hiding this comment.
Both importlib_metadata (line 15) and importlib.metadata (imported via line 20: from importlib.metadata import entry_points) are being used. Since Python 3.8+, importlib.metadata is available in the standard library. The importlib_metadata backport is only needed for older Python versions. Consider using only importlib.metadata for consistency, or add a comment explaining why both are required if there's a specific compatibility reason.
| import importlib_metadata |
Gagan-Ram
force-pushed
the
fix/global_settings_redesign
branch
from
ecf835e to
58ea52e
Compare
|
@hongquan The PR needs review of fossasia/eventyay-tickets-paypal#5 and fossasia/eventyay-tickets-stripe#16 |
Gagan-Ram
force-pushed
the
fix/global_settings_redesign
branch
from
bf56253 to
33a295b
Compare
4 participants
Fixes: #479
global_admin_design.webm
Summary by Sourcery
Redesign the global settings UI into a tabbed interface with organized field groups, add new Stripe Connect key fields, update templates for dynamic grouping, introduce a utility template filter, and improve plugin loading logic in configuration
New Features:
Enhancements: