fixes for isort, flake8, black, broken postgres tests #184
Conversation
Reviewer's Guide by SourceryThis pull request focuses on code formatting and style improvements across multiple files in the project. The changes primarily involve adjusting indentation, line breaks, and import ordering to conform to a consistent coding style. Additionally, there are some minor functional changes and updates to test cases. 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 @it-tma-tri - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider separating purely stylistic changes from functional changes in future PRs to make review easier and reduce the risk of overlooking important modifications.
- Please provide more context in the PR description about the functional changes, particularly those related to PostgreSQL tests and query count adjustments in the tests.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟡 Security: 2 issues found
- 🟡 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.
| ticket_url = options.get("ticket-url") or input( | ||
| "Enter Ticket Url which be using for retrieve customer data from Ticket: " | ||
| ) | ||
| ticket_token = options.get("ticket-token") or input( |
There was a problem hiding this comment.
🚨 suggestion (security): Improve security for handling sensitive information
Using input() for sensitive data like tokens is not secure. Consider using environment variables or a secure configuration file for such sensitive information.
ticket_token = options.get("ticket-token") or os.environ.get("TICKET_TOKEN")
if not ticket_token:
raise ValueError("Ticket token not provided in options or environment variables")
|
|
||
| def handle(self, *args, **options): | ||
| site = Site.objects.get(pk=settings.SITE_ID) | ||
| eventyay_ticket_client_id = options.get('eventyay-ticket-client-id') or input('Enter Eventyay-Ticket Provider Client ID: ') | ||
| eventyay_ticket_secret = options.get('eventyay-ticket-secret') or input('Enter Eventyay-Ticket Provider Secret: ') | ||
| eventyay_ticket_client_id = options.get("eventyay-ticket-client-id") or input( |
There was a problem hiding this comment.
🚨 suggestion (security): Improve input validation and security
Add basic validation to ensure client ID and secret are not empty or malformed. Also, consider using a configuration file or environment variables for these sensitive values instead of command-line input.
eventyay_ticket_client_id = options.get("eventyay-ticket-client-id") or os.environ.get("EVENTYAY_TICKET_CLIENT_ID")
if not eventyay_ticket_client_id:
eventyay_ticket_client_id = input("Enter Eventyay-Ticket Provider Client ID: ").strip()
if not eventyay_ticket_client_id:
raise ValueError("Eventyay-Ticket Provider Client ID cannot be empty")
| @@ -129,7 +129,7 @@ def test_schedule_frab_json_export( | |||
| orga_user, | |||
| schedule_schema_json, | |||
| ): | |||
| with django_assert_max_num_queries(13): | |||
| with django_assert_max_num_queries(14): | |||
There was a problem hiding this comment.
issue (testing): Significant increase in maximum number of queries for schedule_ical_export test
The maximum number of queries for the schedule_ical_export test has been increased from 9 to 14, which is a significant jump. This could indicate a substantial performance regression. Can you provide more context on why this change was necessary and if there are any plans to optimize this?
| @@ -44,7 +44,7 @@ def test_widget_data( | |||
| ): | |||
| event.feature_flags["show_schedule"] = True | |||
| event.save() | |||
| with django_assert_num_queries(11): | |||
| with django_assert_num_queries(19): | |||
There was a problem hiding this comment.
issue (testing): Significant increase in maximum number of queries for widget_data test
The maximum number of queries for the widget_data test has been increased from 11 to 19, which is a substantial increase. This could indicate a significant performance regression. Can you explain why this large increase was necessary and if there are plans to optimize this?
| @@ -36,7 +36,7 @@ def test_edit_cfp(orga_client, event): | |||
| ) | |||
| assert response.status_code == 200 | |||
| event = Event.objects.get(slug=event.slug) | |||
| assert str(event.cfp.headline) == "new headline" | |||
| assert str(event.cfp.headline) == "" | |||
There was a problem hiding this comment.
question (testing): Changed expected value in test_edit_cfp assertion
The expected value for event.cfp.headline has been changed from "new headline" to an empty string. This change might affect the validity of the test. Can you explain why this change was made and if it correctly reflects the expected behavior?
| :members: periodic_task, register_locales, register_data_exporters, register_my_data_exporters | ||
|
|
||
| .. automodule:: pretalx.submission.signals | ||
| :members: submission_state_change | ||
|
|
||
| .. automodule:: pretalx.schedule.signals | ||
| :members: schedule_release | ||
| :members: schedule_release, register_my_data_exporters |
There was a problem hiding this comment.
question (documentation): Clarify the presence of 'register_my_data_exporters' in two different modules
The method 'register_my_data_exporters' appears in both pretalx.common.signals and pretalx.schedule.signals. Is this intentional? If not, it might need to be removed from one of these locations.
|
I wanted to replace isort, flake8, black with Ruff but haven't had a chance. Now because a coding-style PR emerges, I think it is time to do. So please convert this PR to "Replace isort, flake8, black with Ruff". For coding style, I propose to use single quote ( |
I think this should be solved in a separate issue. Is this current PR apart from that? |
This PR closes/references issue #137.
Summary by Sourcery
Fix broken PostgreSQL tests and improve code quality by applying consistent formatting and refactoring. Update test cases to align with the new code structure and ensure proper exception handling in API views.
Bug Fixes:
Enhancements:
Tests: