Sync upstream dec #247
Merged
Sync upstream dec #247
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Currently translated at 100.0% (1508 of 1508 strings) Co-authored-by: Tobias Kunze <[email protected]> Translate-URL: http://translate.pretalx.com/projects/pretalx/pretalx/de/ Translation: pretalx/pretalx
Currently translated at 100.0% (1508 of 1508 strings) Co-authored-by: Tobias Kunze <[email protected]> Translate-URL: http://translate.pretalx.com/projects/pretalx/pretalx/de_FORMAL/ Translation: pretalx/pretalx
closes #1864
closes #1302, closes #1913
closes #1909
closes #1908
Currently translated at 99.7% (1508 of 1512 strings) Co-authored-by: r spoor <[email protected]> Translate-URL: http://translate.pretalx.com/projects/pretalx/pretalx/nl/ Translation: pretalx/pretalx
Reviewer's Guide by SourceryThe changes refactor the email template system to use roles instead of fixed templates, improve the handling of external links with a new safelink feature, and enhance various UI components. The changes also include significant improvements to the build system, test infrastructure, and code organization. Sequence diagram for adding a speaker to a proposalsequenceDiagram
participant User
participant Submission
participant Event
participant MailTemplate
User->>Submission: add_speaker(email, name, locale, user)
Submission->>User: create_user or get existing
Submission->>Event: get_mail_template(role)
Event->>MailTemplate: return template
MailTemplate->>User: to_mail(user, event, context, locale)
Submission->>User: add to speakers
Class diagram for the refactored email template systemclassDiagram
class Event {
+get_mail_template(role)
+build_initial_data()
+reorder_review_phases()
+update_review_phase()
}
class MailTemplate {
+role: MailTemplateRoles
+to_mail(user, event, context_kwargs, skip_queue, commit, submissions, attachments)
}
class MailTemplateRoles {
<<enumeration>>
NEW_SUBMISSION
NEW_SUBMISSION_INTERNAL
SUBMISSION_ACCEPT
SUBMISSION_REJECT
NEW_SPEAKER_INVITE
EXISTING_SPEAKER_INVITE
QUESTION_REMINDER
NEW_SCHEDULE
}
Event --> MailTemplate : uses
MailTemplate --> MailTemplateRoles : has role
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 @HungNgien - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding documentation for the new email template roles system to help future maintainers understand the architecture. This could include explaining the different roles and when they are used.
- The email sending code could benefit from more robust error handling, particularly around template rendering failures and invalid placeholders. Consider adding try/except blocks with specific error types.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟡 Testing: 3 issues found
- 🟢 Complexity: all looks good
- 🟢 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.
| if base_path and max_length: | ||
| # We need to resolve the base path for its actual total length, as absolute | ||
| # paths are stored in the database. | ||
| full_base_path = Path(settings.MEDIA_ROOT) / base_path |
There was a problem hiding this comment.
issue: Handle symlinks in MEDIA_ROOT path resolution
Use Path.resolve() to handle cases where MEDIA_ROOT contains symlinks, otherwise path length calculations may be incorrect.
Comment on lines
391
to
+392
| assert sub.speakers.count() == 1 | ||
| assert sub.mails.all().count() == 1 |
There was a problem hiding this comment.
suggestion (testing): Add test coverage for email notifications when creating submissions
Good addition of test coverage for the mail sending functionality. Consider also testing the content of the email to ensure the correct template is used and properly formatted.
Suggested implementation:
assert sub.title == "title"
assert sub.speakers.count() == 1
assert sub.mails.all().count() == 1
# Verify email content
mail = sub.mails.first()
assert mail.subject == f"Submission received: {sub.title}"
assert "Thank you for your submission" in mail.text
assert sub.title in mail.text
assert "Foo Speaker" in mail.text
Note: I've added basic assertions for the email content, but you may want to:
- Adjust the expected subject line and content to match your actual email templates
- Add more specific content checks based on your submission confirmation email template
- Consider testing HTML content if you're sending HTML emails
Comment on lines
+37
to
+38
| assert getattr(submission, "_state_change_called", 0) == ( | ||
| 1 if state != SubmissionStates.ACCEPTED else 0 |
There was a problem hiding this comment.
suggestion (testing): Improve state change signal test coverage
Good test for state change signals, but consider adding test cases for other state transitions and edge cases like invalid state changes.
Suggested implementation:
@pytest.mark.parametrize(
"initial_state,new_state,expected_signal_count",
[
(SubmissionStates.SUBMITTED, SubmissionStates.ACCEPTED, 1),
(SubmissionStates.SUBMITTED, SubmissionStates.REJECTED, 1),
(SubmissionStates.ACCEPTED, SubmissionStates.CONFIRMED, 1),
(SubmissionStates.ACCEPTED, SubmissionStates.WITHDRAWN, 1),
(SubmissionStates.ACCEPTED, SubmissionStates.ACCEPTED, 0), # No change
],
)
@pytest.mark.django_db
def test_state_change_signals(submission, initial_state, new_state, expected_signal_count):
submission.event.plugins = "tests"
submission.event.save()
with scope(event=submission.event):
submission.state = initial_state
submission.save()
# Reset signal counter
if hasattr(submission, "_state_change_called"):
delattr(submission, "_state_change_called")
# Perform state change
submission.state = new_state
submission.save()
assert getattr(submission, "_state_change_called", 0) == expected_signal_count
@pytest.mark.parametrize(
"invalid_transition",
[
(SubmissionStates.REJECTED, SubmissionStates.ACCEPTED),
(SubmissionStates.WITHDRAWN, SubmissionStates.CONFIRMED),
],
)
@pytest.mark.django_db
def test_invalid_state_transitions(submission, invalid_transition):
initial_state, new_state = invalid_transition
submission.event.plugins = "tests"
submission.event.save()
with scope(event=submission.event):
submission.state = initial_state
submission.save()
with pytest.raises(Exception): # Replace with specific exception if one exists
submission.state = new_state
submission.save()
@pytest.mark.parametrize("state", (SubmissionStates.WITHDRAWN,))
@pytest.mark.parametrize("force", (True, False))
@pytest.mark.django_db
def test_accept_fail(submission, state, force):
submission.event.plugins = "tests"
submission.event.save()
with scope(event=submission.event):
submission.state = state
submission.save()You may need to:
- Import any additional SubmissionStates that aren't already imported
- Adjust the specific exception type in test_invalid_state_transitions based on what your application raises
- Update the signal counting mechanism if it's implemented differently in your codebase
Comment on lines
111
to
+114
|
|
||
| @pytest.fixture | ||
| def client(live_server, selenium, user): | ||
| selenium.implicitly_wait(10) | ||
| selenium.implicitly_wait(2) |
There was a problem hiding this comment.
issue (testing): Reduced implicit wait time in screenshot tests
While reducing wait time improves test speed, ensure this doesn't make tests flaky. Consider adding explicit waits for specific elements instead.
odkhang
approved these changes
Dec 18, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
87004f2 | Do not permit login to non-live event on cfp side |
01a712b | Fix broken internal template editing |
f8e2acd | Fix scrolling to day with multiple grids |
07f684a | Notify users after password change |
2ee208d | Make safelinks work on custom domains |
8e2916a | Update translations |
bdeae13 | 🗺 Update translations (Dutch) |
719f40b | Fix error message on failing to add new speaker |
ef07a66 | Fix breaking tests |
c648316 | Fix deadline widget in event creation wizard |
81524ca | Improve qrcode image position |
587ec68 | Fix look of email list with very long proposals |
48a10bf | Provide outbound link signing and redirects |
b1e3696 | Fix custom help texts not showing in user edit |
32430bf | Update look'n'feel screenshot |
e1a6c10 | Fix migration dependency |
42c5807 | Possibly fix new spurious test failures |
3917f13 | Keep model imports late to avoid circular imports |
79ace89 | Fix code style and failing tests |
4f9e4ec | Add configurable speaker invite email templates |
5db8d1c | Fix display of avatars in headings |
dd797f2 | Allow to customise organiser proposal notifications |
0ce9c1a | Code style |
59b2e41 | Make iCal export available without showing in list |
3d85df2 | Remove unused screenshots |
71217dd | Fix use of SpeakerFilterForm |
3c53087 | Allow to filter by track in email lists |
be86ec0 | Move filtering to forms where sensible |
2d4b842 | Correctly show number of submissions per track |
68cd571 | Fix broken timezone toggle |
0f00674 | Fix janky scroll behaviour in schedule |
96cd143 | Add powered-by line to text schedule |
9a81803 | Fix duplicate translation strings |
e36011c | Consistent English spelling of email |
ba32d10 | 🗺 Update translations (Czech) |
91e7ce9 | Fix HTML export. |
6c7aa16 | Hide unused rooms on a day-by-day basis |
5221bb9 | Make sure dropdowns don’t wrap indicator |
e622bab | Add big button for organiser area if allowed |
b38f375 | Redirect to current page after event login |
274be63 | Don’t hide language select and login |
a4a5923 | Improve display of public questions |
af0a454 | Move import to make test runner happy |
f8f64a5 | Fix incorrectly escaped ampersand in drop-downs |
521d5c4 | Fix access of new email list structure |
65c15ad | Fix breaking tests |
95c1bba | Fix email submission link some more |
72078ac | Fix bug in email submission mapping |
14be2d5 | Don’t updae last_login in tests |
f68e450 | Fix and document (most of) automated screenshots |
c3a9a19 | Allow to send/discard filtered outbox list |
22d952f | Make dropdown colour transition smoother |
89c2797 | Log out only on POST |
2545da7 | Fix link to organiser dashboard |
1dafd98 | Show proposals in email list |
5c117f1 | Update translations |
7c22564 | Fix new Django warning |
7b40447 | Use tabs in email editor |
efcba1a | Fix placeholder insertion |
4b2e190 | Fix incorrect widget generation |
d92c540 | Render text after pre_send has run | ignore errors
21107a9 | Link notification proposals to emails |
6906c62 | Reword changelog and docs |
0827ebe | Add signal queuedmail_post_send. |
241ccac | Store submission references in queued mails from orga forms |
47e2f88 | Add submissions to outgoing mails to facilitate plugin integrations. |
01e6bdb | 🗺 Update translations (Czech) |
27ac039 | 🗺 Update translations (Vietnamese) |
2ee0618 | 🗺 Update translations (Russian) |
66dae8b | Fix Python 3.10 support |
2d12d0e | Remove weird full_submission_content interface |
981194a | Remove useless assert |
5a54190 | Initialize variables clearly |
455d044 | Don’t use deprecated utcnow() |
3762d3c | Don’t name unused variables |
3f7fa4e | Add sane default pyright config |
2731f88 | Code style |
cefd902 | Allow to export datetimes as two separate IDs |
ee47a79 | Send submission_state_change signal on create |
77a6b95 | Fix breaking tests due to env pollution |
869f99d | Mark CSRF cookie as secure |
8f15c24 | Improve HTML export speed and reliability |
d320fc9 | Fix invisible badges/indicators |
1a6b1f5 | Fix code style |
2aed9e5 | Allow to use the schedule widget for selected days |
08f5267 | Fix session display bug |
49d6cb9 | Update schedule.js to fix break bug |
583292a | Bump bleach |
98b0b6e | Fix immovable active tab indicator in schedule |
47d1ba8 | Fix schedule being always in English |
f3ccfee | Stop the active review phase deletion |
d94397d | Make sure review phases are always ordered |
0b51428 | Fix drag'n'drop in Chrome |
45d50bb | Improve print.css slightly |
d135282 | Bump redis and whitenoise |
d1d5d0b | Fix HTML export |
52144aa | Truncate very long file names |
e04057d | Fix spelling |
35ab81c | Use localised start time in exporter |
e030ee0 | Stop expanding colours while users are typing |
e9439ff | Colour top nav in current event’s colour |
f20e206 | Show state badge instead of dropdown to reviewers |
d6373c7 | Some tests seem to actually need the reruns |
ffecc28 | Bump development version |
55d1ccc | Release v2024.3.1 |
78396de | Query counts in tests are a joy |
9ef2fb0 | Sigh | breaking test once more
2be1c9d | Move speaker links below organiser event list |
a803f8d | Fix breaking test |
8c6c9af | Install deps with uv for speedup |
7ca4369 | Fix breaking tests |
6ca1683 | Stop using pytest-rerunfailures |
c2f29a2 | Fix test |
b0d70c5 | Fix test |
9a1571e | Make sure apt runs non-interactively |
29f736d | Use postgres as service rather than as sub-action |
7b5c3e6 | We haven’t been using Travis in ages |
e5be51b | I'd rather fix flaky tests than rerun failures |
a8fa483 | Fix form error |
bbde16b | Code style |
ca438d1 | Fix bug in submission filter form |
9a4bec4 | Fix missing embed css |
c4f466f | 🗺 Update translations (German (de_FORMAL)) |
84fa7ae | 🗺 Update translations (German) |
Summary by Sourcery
Refactor the email template system to use roles for better flexibility and management. Introduce new features for handling external links safely and filtering emails by track. Fix various bugs related to email generation, speaker invitations, and UI inconsistencies. Enhance the schedule widget and improve CI workflows for better performance.
New Features:
Bug Fixes:
Enhancements:
Build:
CI:
Documentation:
Tests:
Summary by Sourcery
Refactor the email template system to use roles for better flexibility and management. Introduce new features for handling external links safely and filtering emails by track. Fix various bugs related to email generation, speaker invitations, and UI inconsistencies. Enhance the schedule widget and improve CI workflows for better performance.
New Features:
Bug Fixes:
Enhancements:
Build:
CI:
Documentation:
Tests: