Sync upstream with conflict #187
Conversation
Co-authored-by: Michal Stanke <michal@stanke.cz> Co-authored-by: Yucheng Lin <yuchenglinedu@gmail.com>
Co-authored-by: Francisco <francisco.joao.2003@outlook.com> Co-authored-by: Weblate <noreply@weblate.org> Co-authored-by: Hoi thao Khoa hoc <bantochuc@hoithaokhoahoc.vn>
Co-authored-by: Lucie Anglade <lucie@courtbouillon.org>
Co-authored-by: Lucie Anglade <lucie@courtbouillon.org> Co-authored-by: Tobias Kunze <r@rixx.de>
closes #1803
Closes #1794
odkhang
force-pushed
the
sync-upstream-with-conflict
branch
from
50e3cf0 to
9ae0363
Compare
Reviewer's Guide by SourceryThis pull request includes a large number of changes across multiple files, focusing on code refactoring, performance improvements, and feature enhancements. Key changes include updating the CFP flow, improving user management views, enhancing markdown sanitization, refactoring the event model, and updating various API endpoints and permissions. File-Level Changes
Tips
|
![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 and they look great!
Here's what I looked at during the review
- 🟡 General issues: 11 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 2 issues found
- 🟡 Complexity: 12 issues found
- 🟡 Documentation: 1 issue found
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| def serialize_value(value): | ||
| if getattr(value, "pk", None): | ||
| return value.pk | ||
| if getattr(value, "__iter__", None): | ||
| return [serialize_value(element) for element in value] | ||
| if getattr(value, "serialize", None): | ||
| return value.serialize() | ||
| return str(value) |
There was a problem hiding this comment.
suggestion: Consider using hasattr instead of getattr with None default
Using hasattr before getattr would make it more explicit when an attribute is missing and prevent potential error masking. Also, consider adding documentation about when this function should be used, especially regarding objects with a 'serialize' method.
| def serialize_value(value): | |
| if getattr(value, "pk", None): | |
| return value.pk | |
| if getattr(value, "__iter__", None): | |
| return [serialize_value(element) for element in value] | |
| if getattr(value, "serialize", None): | |
| return value.serialize() | |
| return str(value) | |
| def serialize_value(value): | |
| """Serialize various types of values, including objects with 'pk', iterables, and objects with 'serialize' method.""" | |
| if hasattr(value, "pk"): | |
| return value.pk | |
| if hasattr(value, "__iter__"): | |
| return [serialize_value(element) for element in value] | |
| if hasattr(value, "serialize"): | |
| return value.serialize() | |
| return str(value) |
| TEMPLATE_LOG_NAMES = { | ||
| "pretalx.event.delete": _("The event {name} ({slug}) by {organiser} was deleted."), | ||
| "pretalx.organiser.delete": _("The organiser {name} was deleted."), | ||
| } |
There was a problem hiding this comment.
suggestion: Consider using a more robust templating engine for log messages
The new templating system for log messages is a good improvement for flexibility. However, if you plan to expand on this feature, consider using a more robust templating engine that can handle more complex scenarios and provide better error handling.
from jinja2 import Template
TEMPLATE_LOG_NAMES = {
"pretalx.event.delete": Template("The event {{ name }} ({{ slug }}) by {{ organiser }} was deleted."),
"pretalx.organiser.delete": Template("The organiser {{ name }} was deleted."),
}
| availabilities = [] | ||
|
|
There was a problem hiding this comment.
suggestion: Simplified availability validation logic
The removal of the separate is_valid function and its inline replacement in the list comprehension improves code conciseness without sacrificing readability.
if instance:
availabilities = AvailabilitySerializer(
instance.availabilities.all(), many=True
).data
else:
availabilities = []
result = {
"availabilities": [
avail for avail in availabilities if avail["end"] > avail["start"]
],
| @@ -121,7 +120,7 @@ | |||
| raise forms.ValidationError(message) | |||
| rawavail.pop("id", None) | |||
| rawavail.pop("allDay", None) | |||
There was a problem hiding this comment.
suggestion: Optimized set comparison
Using != instead of not == for set comparison is slightly more efficient and idiomatic Python.
| rawavail.pop("allDay", None) | |
| if not {"start", "end"}.issubset(rawavail) or len(rawavail) > 2: |
| @@ -269,7 +269,7 @@ class SubmissionDraftDiscardView( | |||
|
|
|||
| def get_object(self): | |||
| submission = super().get_object() | |||
| if not submission.state == SubmissionStates.DRAFT: | |||
There was a problem hiding this comment.
suggestion: Simplified state comparison
The condition has been rewritten in a more idiomatic and readable way, directly using != instead of not ==.
| request.session[child_session_key] = s.session_key | ||
| store = SessionStore() | ||
| if not child_session or not store.exists(child_session): | ||
| store[f"pretalx_event_access_{request.event.pk}"] = ( |
There was a problem hiding this comment.
issue (complexity): Consider simplifying the logic by removing unnecessary line breaks and using more descriptive variable names.
The new code introduces additional complexity due to the use of parentheses for line continuation and changes in formatting, which can make it harder to read and maintain. Consider simplifying the logic by removing unnecessary line breaks and using more descriptive variable names consistently. Here's a suggestion for a more readable implementation:
collect_signal(html_head, {"sender": request.event, "request": request})
if (
not request.event.is_public
and request.event.custom_domain
and request.user.has_perm("cfp.view_event", request.event)
):
child_session_key = f"child_session_{request.event.pk}"
child_session = request.session.get(child_session_key)
session_store = SessionStore()
if not child_session or not session_store.exists(child_session):
session_store[f"pretalx_event_access_{request.event.pk}"] = request.session.session_key
session_store.create()
context["new_session"] = session_store.session_key
request.session[child_session_key] = session_store.session_key
request.session["event_access"] = True
else:
context["new_session"] = child_session
request.session["event_access"] = True
return contextThis version maintains clarity and reduces complexity, making it easier to follow the logic.
| @@ -31,6 +31,16 @@ | |||
| ENCRYPTED_PASSWORD_PLACEHOLDER = "*******" | |||
|
|
|||
|
|
|||
| def make_naive(moment): | |||
There was a problem hiding this comment.
issue (complexity): Consider keeping make_naive as a local function within the method.
The recent change to move the make_naive function outside of its original method introduces unnecessary complexity. Since make_naive is only used in one place, keeping it as a local function within the method maintains better encapsulation and readability. By making it a global function, it could lead to potential misuse and disrupts the logical flow of the class. Consider keeping make_naive as a local function within the method to simplify the code and maintain clarity.
| @@ -182,6 +182,19 @@ class DirectionForm(forms.Form): | |||
| required=False, | |||
| ) | |||
|
|
|||
| class ReviewAssignmentForm(forms.Form): | |||
There was a problem hiding this comment.
issue (complexity): Consider refactoring common logic into a helper method to reduce redundancy.
The new code introduces a more complex class hierarchy with ReviewAssignmentForm as a base class and two subclasses, ReviewerForProposalForm and ProposalForReviewerForm. This adds layers to the class structure, making it harder to trace data flow and understand class relationships. Additionally, there's redundant logic in setting up fields across the subclasses, which can complicate maintenance. The __init__ methods are doing a lot of work, including database queries and dynamic field setup, which can impact readability and performance. Consider refactoring common logic into a helper method to reduce redundancy and simplify the code. Also, evaluate if all logic in __init__ is necessary or if it can be moved elsewhere. This will help adhere to the Single Responsibility Principle and make the forms easier to maintain and test.
| @@ -78,3 +83,104 @@ def result_table(self): | |||
|
|
|||
| def get_success_url(self): | |||
| return reverse("orga:admin.update") | |||
|
|
|||
|
|
|||
| class AdminUserList(PermissionRequired, ListView): | |||
There was a problem hiding this comment.
issue (complexity): Consider refactoring common logic to reduce complexity.
The new code introduces additional complexity compared to the original. Here are some observations:
-
Increased Complexity: The addition of new classes (
AdminUserList,AdminUserDetail,AdminUserDelete) and methods increases the overall complexity, making the code harder to understand and maintain. -
Additional Dependencies: The use of new modules and decorators like
csp_updateandscopes_disabledadds to the cognitive load, requiring developers to understand their impact. -
Complex Logic: Methods contain more complex logic, such as handling multiple actions and constructing querysets with filters and annotations, which can be challenging to read and maintain.
-
Repetition: The
dispatchmethod is overridden in multiple classes to disable scopes, which could be refactored to reduce redundancy. -
Potential for Errors: The increased complexity can lead to a higher potential for bugs and errors.
Suggestions for simplification:
-
Refactor Common Logic: Consider using a mixin or utility function for common logic like the
dispatchmethod. -
Simplify Querysets: Break down complex queryset logic into smaller functions or use helper methods.
-
Evaluate Dependencies: Review the necessity of new dependencies and simplify where possible.
Overall, simplifying these aspects could improve readability and maintainability.
| team = super().get_object() | ||
| if "user_pk" in self.kwargs: | ||
| return team.members.filter(pk=self.kwargs.get("user_pk")).first() | ||
| return team | ||
|
|
||
| def action_object_name(self): |
There was a problem hiding this comment.
issue (complexity): Consider refactoring common logic into a mixin to reduce code duplication.
The recent changes have increased the complexity of the code. Here are some observations and suggestions for refactoring:
-
Increased Complexity: The
get_objectmethod now includes an optionalquerysetparameter in multiple classes, but it doesn't seem to be used effectively. Consider removing it if it's unnecessary. -
Redundant Methods: Methods like
action_object_nameandaction_back_urlare repeated across classes. Extract these into a mixin or utility function to reduce duplication. -
Inconsistent Method Usage: The
form_validmethod inTeamDetailhas been split intoform_validandget_success_url. While this can be beneficial, ensure the logic is streamlined to avoid unnecessary complexity. -
Additional Properties: The addition of properties like
action_title,action_text, etc., increases maintenance overhead. Consider consolidating these into a base class or mixin.
Refactoring suggestions:
- Refactor Common Logic: Use a mixin for common methods like
action_object_nameandaction_back_url. - Simplify Method Signatures: Remove unused parameters like
queryset. - Consolidate Properties: Use a base class or mixin for shared properties.
These changes should help simplify the code and improve maintainability.
|
Please check failing tests. |
|
Thank you. We just got most tests succeeding in the previous sync, but with this PR a number of tests are failing again. How to deal with it? Could you help to make tests succeeding? |
There was a problem hiding this comment.
Hey @odkhang - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 3 issues found
- 🟢 Complexity: all looks good
- 🟡 Documentation: 1 issue found
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| def serialize_value(value): | ||
| if getattr(value, "pk", None): | ||
| return value.pk | ||
| if getattr(value, "__iter__", None): | ||
| return [serialize_value(element) for element in value] | ||
| if getattr(value, "serialize", None): | ||
| return value.serialize() | ||
| return str(value) |
There was a problem hiding this comment.
suggestion: Consider handling more specific types in serialize_value function
While the new serialize_value function is a good improvement, you might want to consider handling more specific types. For example, you could add explicit handling for datetime objects or custom classes with a str method.
def serialize_value(value):
if getattr(value, "pk", None):
return value.pk
if isinstance(value, datetime):
return value.isoformat()
if getattr(value, "__iter__", None):
return [serialize_value(element) for element in value]
if getattr(value, "serialize", None):
return value.serialize()
if hasattr(value, "__str__"):
return str(value)
return repr(value)
| @@ -121,7 +120,7 @@ def _validate_availability(self, rawavail): | |||
| raise forms.ValidationError(message) | |||
| rawavail.pop("id", None) | |||
| rawavail.pop("allDay", None) | |||
| if not set(rawavail.keys()) == {"start", "end"}: | |||
There was a problem hiding this comment.
issue (bug_risk): Logic change in key validation could introduce bugs
The change from set(rawavail.keys()) == {"start", "end"} to set(rawavail.keys()) != {"start", "end"} alters the logic of this check. Please verify that this change is intentional and doesn't introduce any bugs or unintended behavior.
| @@ -490,13 +490,7 @@ def test_orga_can_assign_reviewer_to_submission(orga_client, review_user, submis | |||
| response = orga_client.post( | |||
| submission.event.orga_urls.reviews + "assign/?direction=submission", | |||
| { | |||
| "formset-TOTAL_FORMS": 1, | |||
There was a problem hiding this comment.
suggestion (testing): Update test to reflect new form structure for assigning reviewers
The test has been updated to use the new form structure for assigning reviewers to submissions. This change aligns with modifications in the view logic. Ensure that the new structure correctly represents how the form data is now submitted and processed.
| "formset-TOTAL_FORMS": 1, | |
| f"submission-{submission.code}": [str(review_user.id)], |
| reverse( | ||
| "agenda:export.schedule.ics", | ||
| kwargs={"event": slot.submission.event.slug}, | ||
| ), | ||
| follow=True, | ||
| ) | ||
| assert response.status_code == 200 | ||
| assert response.status_code == 200 |
There was a problem hiding this comment.
question (testing): Update iCal export test to use orga_client and increase query count
The test has been updated to use orga_client instead of client, and the maximum number of queries allowed has been increased from 14 to 16. This change might indicate that the iCal export now requires additional database queries. It would be beneficial to investigate if these extra queries are necessary or if there's room for optimization.
| @@ -232,7 +232,7 @@ | |||
| @pytest.mark.django_db | |||
| @pytest.mark.parametrize( | |||
| "exporter", | |||
| ("schedule.xml", "schedule.json", "schedule.xcal", "schedule.ics", "feed"), | |||
| ("schedule.xml", "schedule.json", "schedule.xcal", "feed"), | |||
There was a problem hiding this comment.
issue (testing): Remove 'schedule.ics' from export test parameters
The 'schedule.ics' export has been removed from the test parameters. This change suggests that the iCal export is no longer being tested in the same way as other formats. It's important to ensure that the iCal export is still being adequately tested, possibly in a separate test case that accounts for its unique requirements.
| @@ -37,7 +37,7 @@ Take-off and landing | |||
| 3. Make a release commit: ``RELEASE=vx.y.z; git commit -am "Release $RELEASE" && git tag -m "Release $RELEASE" $RELEASE`` | |||
| 4. Build a new release: ``rm -rf dist/ build/ pretalx.egg-info && python -m build -n`` | |||
| 5. Upload the release: ``twine upload dist/pretalx-*`` | |||
| 6. Push the release: ``git push && git push --tags`` | |||
| 6. Push the release: ``git push`` | |||
There was a problem hiding this comment.
question (documentation): The '--tags' option has been removed from the git push command.
Was this intentional? If tags should be pushed during the release process, consider keeping the command as git push && git push --tags.
Summary by Sourcery
Refactor and enhance the codebase with new features like user management views, improved plugin handling, and better serialization. Fix various bugs related to locale handling, QR code generation, and formset management. Update CI workflows and documentation to align with the latest changes.
New Features:
serialize_valuefunction to handle serialization of various data types in thecfp_sessionfunction.AdminUserListview to list users with search functionality and actions like password reset and deletion.AdminUserDetailandAdminUserDeleteviews for detailed user management and deletion.ActionConfirmMixinto provide variables for action confirmation templates.ReviewAssignmentFormfor handling review assignments in a more structured way.plugin_sort_keyandplugin_group_keyfunctions to improve plugin sorting and grouping.get_config_jsonmethod in theFlowclass to return the configuration as JSON.get_success_urlmethod in various views to determine the redirect URL after form submission.Bug Fixes:
Enhancements:
cfp_sessionfunction to improve session data handling and serialization.AdminDashboardview to include additional context data and improve form handling.ReviewAssignmentview to use a more structured form approach.ActivityLogdisplay by adding more detailed log messages and object links.Eventmodel with new methods for plugin management and data copying.SubmissionSerializerto improve initialization and field handling.get_all_pluginsfunction to better handle plugin visibility and sorting.Build:
docs.ymlworkflow to include a step for summarizing spelling errors in the GitHub summary.CI:
Documentation:
Tests:
AdminUserListandAdminUserDetailviews to ensure proper functionality.Chores:
2024.3.0.dev0to reflect the latest changes and improvements.