Add co-speaker feature broken #130
Conversation
Reviewer's Guide by SourceryThis pull request implements a feature to send an invitation email to an additional speaker when the 'Additional Speaker' field is filled in the proposal edit form. The changes include adding a check for the 'additional_speaker' field, sending the invitation email, and handling potential email sending errors with appropriate logging and user notifications. 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 @lcduong - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟡 Security: 1 issue found
- 🟢 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.
src/pretalx/cfp/views/user.py
Outdated
| form.instance.send_invite(to=[form.cleaned_data.get('additional_speaker')], | ||
| _from=self.request.user) | ||
| except SendMailException as exception: | ||
| logging.getLogger("").warning(str(exception)) |
There was a problem hiding this comment.
Very bad usage of logger here.
A logger should be defined at the top of the file:
logger = logging.getLogger(__name__)The value passed to getLogger() is important, because later on, the log system can let you know where (which module) the message originated from.
Then use logger where you want to write message:
logger.warning('Failed to send email with error: %s', exception)Note: Don't build the string from the variables:
logger.warning(f'Failed to send email with error: {exception}')because it will always evaluate the variables regardless we want to emit messages or not.
| @@ -8,6 +8,7 @@ | |||
| from allauth.socialaccount.helpers import render_authentication_error | |||
| from django.conf import settings | |||
| from django.urls import reverse | |||
| from urllib.parse import urlencode | |||
There was a problem hiding this comment.
Move this import line to the 1st group. PEP8 coding style:
Imports should be grouped in the following order:
Standard library imports.
Related third party imports.
Local application/library specific imports.
|
Thanks this solves the core issue. Please also ensure the log message on the proposal page is showing the action correctly. When the speaker field invites a speaker the log message for the session should show "A speaker was invited to the proposal." Currently the log message is as follows:
|
This PR closes/references issue #129 . It does so by:
How has this been tested?
Checklist
Summary by Sourcery
This pull request introduces a new feature that sends an invitation email to an additional speaker when specified in the proposal edit form. It includes error handling for email sending failures.