-
Notifications
You must be signed in to change notification settings - Fork 51
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change added as a speaker email #142
Conversation
🧙 Sourcery has finished reviewing your pull request! Tips
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @odkhang - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a test case to verify the new email content is generated correctly.
- Please clarify if you've manually tested this change, for example by sending a test email.
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
You have been set as the speaker of a proposal to the Call for Participation | ||
of {event}, titled “{title}”. An account has been created for you – please follow | ||
this link to set your account password. | ||
We are excited to inform you that you have been selected as a speaker for {event} with your proposal titled "{title}". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: Consider using single quotes for consistency.
The rest of the code appears to use single quotes for strings. For consistency and to avoid potential issues, it might be better to use single quotes here as well.
We look forward to your participation and contribution to {event}. | ||
|
||
Best regards, | ||
The {event} Organizing Team""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Consider using a consistent term for the organizing team.
In the subject line, 'orga' is used, while in the body, 'Organizing Team' is used. It might be better to use a consistent term throughout the email for clarity.
The {event} Organizing Team""" | |
The {event} Orga Team""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, good catch. Please use "Organizing Team"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, thanks for the clarification, @mariobehling. I'll update the suggestion to use "Organizing Team" consistently throughout the email. Here's the revised suggestion:
The {event} Organizing Team""" | |
Best regards, | |
The {event} Organizing Team""" |
I'll make this change in the code.
@@ -102,7 +104,7 @@ | |||
event=submission.event, | |||
reply_to=submission.event.email, | |||
subject=str( | |||
_("You have been added to a proposal for {event}").format( | |||
_("Welcome as a Speaker at {event}!").format( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Consider rephrasing the subject line for clarity.
The subject line 'Welcome as a Speaker at {event}!' could be rephrased to 'Welcome as a Speaker for {event}!' to improve clarity and readability.
_("Welcome as a Speaker at {event}!").format( | |
_("Welcome as a Speaker for {event}!").format( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have skipped reviewing this pull request. It looks like we've already reviewed the commit 0a19adf in this pull request.
Hi @mariobehling, Please help me review this PR |
It looks like you are testing the wrong email. This email is for adding a co-speaker. |
This PR references issue #138. In the submission.py change the existing email to the new version.
How has this been tested?
Checklist
Summary by Sourcery
This pull request updates the email template used to notify speakers about their proposal acceptance, enhancing the tone and clarity of the message.