Upgrade django version from 4.2 to 5.1 #375
Conversation
Reviewer's Guide by SourceryThis pull request upgrades Django from version 4.2 to 5.1 and django-debug-toolbar from 4.0 to 4.4. The changes primarily affect the EventPluginSignal class in the base signals module and introduce a new JavaScript catalog template. Class diagram for EventPluginSignal changesclassDiagram
class EventPluginSignal {
+get_live_receivers(sender)
+_is_active(sender, receiver)
+send(sender: Event, **named) List~Tuple~Callable, Any~~
+send_chained(sender: Event, chain_kwarg_name, **named) List~Tuple~Callable, Any~~
+send_robust(sender: Event, **named) List~Tuple~Callable, Any~~
}
class GlobalSignal {
+send_chained(sender: Event, chain_kwarg_name, **named) List~Tuple~Callable, Any~~
}
JavaScript catalog template structureclassDiagram
class JSCatalogTemplate {
+pluralidx(n)
+gettext(msgid)
+ngettext(singular, plural, count)
+gettext_noop(msgid)
+pgettext(context, msgid)
+npgettext(context, singular, plural, count)
+interpolate(fmt, obj, named)
+get_format(format_type)
}
JSCatalogTemplate : +catalog
JSCatalogTemplate : +formats
JSCatalogTemplate : +jsi18n_initialized
JSCatalogTemplate : +globals
JSCatalogTemplate : +django
File-Level Changes
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 - here's some feedback:
Overall Comments:
- Could you provide some comments explaining the rationale behind the changes in
signals.py, particularly the removal of_sorted_receiversand addition ofget_live_receivers? This would be helpful for future maintenance. - The PR description mentions upgrading django-debug-toolbar, but there are no visible changes related to this in the diff. Could you clarify if this upgrade is part of this PR, and if so, include the relevant changes?
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 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.
| @@ -0,0 +1,104 @@ | |||
| js_catalog_template = r""" | |||
There was a problem hiding this comment.
issue (complexity): Consider refactoring the large template string into smaller, more manageable pieces.
While the internationalization functionality is crucial, the current implementation mixes Python and JavaScript in a way that could lead to maintainability issues. Consider the following improvements:
- Break down the large template string into smaller, more manageable pieces:
js_catalog_template = r"""
{% autoescape off %}
'use strict';
{{ django_object }}
{{ gettext_functions }}
{{ formatting_functions }}
{{ global_assignments }}
{% endautoescape %}
"""
django_object = r"""
const globals = this;
const django = globals.django || (globals.django = {});
"""
gettext_functions = r"""
django.gettext = function(msgid) {
// ... (existing implementation)
};
django.ngettext = function(singular, plural, count) {
// ... (existing implementation)
};
// ... (other gettext-related functions)
"""
formatting_functions = r"""
django.formats = {{ formats_str }};
django.get_format = function(format_type) {
// ... (existing implementation)
};
"""
global_assignments = r"""
globals.pluralidx = django.pluralidx;
globals.gettext = django.gettext;
// ... (other global assignments)
django.jsi18n_initialized = true;
"""- Use a template engine or string formatting to combine these pieces:
from string import Template
js_catalog_template = Template(js_catalog_template).safe_substitute(
django_object=django_object,
gettext_functions=gettext_functions,
formatting_functions=formatting_functions,
global_assignments=global_assignments
)This approach maintains the benefits of keeping related logic together while significantly improving readability and maintainability. It allows for easier updates to specific parts of the internationalization logic and makes the code more modular without necessarily splitting it into separate files.
There was a problem hiding this comment.
Tests should be passing as much as possible in major upgrade. Please fix
- Code Style / isort (pull_request)
- Tests / Tests (3.11, postgres)
- Code Style / flake8
Opened an issue here #377
|
In order to keep things clearer could you separate the upgrade and the fix tests into separate PRs, please and match the PR for failing tests with #377 ? |
src/pretix/api/views/event.py
Outdated
| @@ -429,7 +430,7 @@ def get(self, request, *args, **kwargs): | |||
| # Get all orders of customer which belong to this event | |||
| order_list = (Order.objects.filter(Q(event=event) | |||
| & (Q(customer=customer) | Q( | |||
| email__iexact=customer.email))).select_related('event').order_by('-datetime')) | |||
| email__iexact=customer.email))).select_related('event').order_by('-datetime')) | |||
There was a problem hiding this comment.
Why a redundant whitespace here?
There was a problem hiding this comment.
Hi @hongquan, its was format to add more whitespace when I run black fix: black src/pretix/api/views/event.py
There was a problem hiding this comment.
Why change from single quote to double quote?
There was a problem hiding this comment.
Hi @hongquan, single quote is replaced with double quote when I run black fix: black src/pretix/base/configurations/default_setting.py
Shouldn't we use single quote for this case?
| from .sso_provider_view import SSOProviderListView | ||
| from .team_view import TeamListView, TeamUpdateView, TeamCreateView, TeamMemberView, TeamDeleteView | ||
| from .web_hook_view import WebHookCreateView, WebHookListView, WebHookUpdateView | ||
| from .customer_view import CustomerDetailView # noqa |
There was a problem hiding this comment.
Please be more specific with "noqa".
Hi @mariobehling, I have separated it as your request. |
There was a problem hiding this comment.
@lcduong After merging the dev branch here Docker and other tests are failing. Please check.
This PR related to issue #372
Upgrade Django version from 4.2 to 5.1
Changes:
Summary by Sourcery
Upgrade Django to version 5.1 and update django-debug-toolbar to version 4.4. Introduce a new JavaScript catalog template for internationalization. Refactor the EventPluginSignal class to enhance the management of live receivers.
New Features:
Enhancements: