feedback forms machinery #180
Conversation
There was a problem hiding this comment.
The strategy looks appropriate, I like it. Unfortunately this part of adsws was confusing before this PR (due to organic grow I imagine) and I suggest we invest some more time to get rid of some technical debt, refactor things, and make it fit what this part of the code is really doing / will be doing: mainly sending emails, secondarily sending slack notifications.
The main ideas I am suggesting:
- Rename class & endpoint + rewrite description to clarify that this deals with feedback, right now it looks like this is only slack related.
- Use templates for general user feedback and feedback forms, maximizing shared code (I think your strategy of using templates is the way to go!)
- The endpoint errors back if the email is not send, emails get promoted to first class citizens, if slack fails we do not care so much because we'll get the email anyway.
- Make sure we have functions with isolated responsibilities (e.g., one for formatting, one for sending the email) and proper names/descriptions.
Finally, a question I had: Have you considered sending slack notifications for feedback forms without sending the bulk of the data (e.g., user A requested this type of change)? I wonder if this will be useful to follow on slack when we get this kind of request, while making it just "one line" notification it will not pollute the channel.
Let me know what you think, we can chat in person to clarify any point :-)
adsws/feedback/views.py
Outdated
| def post(self): | ||
| """ | ||
| HTTP POST request | ||
| :return: status code from the slack end point | ||
| """ |
There was a problem hiding this comment.
From this description it looks like the only thing that post does is sending data to slack, it is not clear this is also sending emails until we look at the code. Could we change it?
adsws/feedback/views.py
Outdated
| @@ -69,7 +82,7 @@ def prettify_post(post_data): | |||
| feedback_email = 'no email sent' | |||
| if post_data.has_key('_replyto') and post_data.has_key('name'): | |||
| try: | |||
| res = send_feedback_email(name, reply_to, text) | |||
| res = send_feedback_email(name, reply_to, "Bumblebee Feedback", text) | |||
There was a problem hiding this comment.
The prettify_post function, from the function name and description, looks like it is just formatting the post but it is also sending emails. I suggest we separate responsibilities and we create one function to send emails and one function to prettify. I know this is not coming from this PR, the code was like this from before, but since the PR is about sending emails, could we use it to fix this too?
adsws/feedback/views.py
Outdated
| try: | ||
| current_app.logger.info('Prettifiying post data: {0}' | ||
| .format(post_data)) | ||
| formatted_post_data = json.dumps(self.prettify_post(post_data)) |
There was a problem hiding this comment.
As I commented above, this prettify_post is not just formatting data but sending an email. If we split that function into one that only prettifies and another that sends the email, we could call them both here and it would make the code more readable.
adsws/feedback/views.py
Outdated
| if origin == current_app.config['FEEDBACK_FORMS_ORIGIN']: | ||
| current_app.logger.info('Received data from feedback form "{0}" from {1} ({2})'.format(post_data.get('_subject'), post_data.get('name'), post_data.get('email'))) | ||
| try: | ||
| email_body = self.create_email_body(post_data) | ||
| except Exception as error: | ||
| current_app.logger.error('Fatal error creating email body: {0}'.format(error)) | ||
| return err(ERROR_EMAILBODY_PROBLEM) | ||
| try: | ||
| email_sent = self.process_feedbackform_submission(post_data, email_body) | ||
| except Exception as error: | ||
| current_app.logger.error('Fatal error while processing feedback form data: {0}'.format(error)) | ||
| return err(ERROR_FEEDBACKFORM_PROBLEM) | ||
| if not email_sent: | ||
| # If the email could not be sent, we can still log the data submitted | ||
| current_app.logger.error('Sending of email failed. Feedback data submitted by {0} ({1}): {2}'.format(post_data, post_data.get('name'), post_data.get('email'))) |
There was a problem hiding this comment.
We added this new functionality that only sends emails, no data is sent to slack (which is a valid decision, of course!). But this is contained in a SlackFeedback class (name and description only mention slack), used in the /slack endpoint. I suggest we change these names to something like Feedback and /feedback, this would require BBB to also change and send data to /feedback.
There was a problem hiding this comment.
Also, could we put the two if branches (if origin == current_app.config['FEEDBACK_FORMS_ORIGIN']: and elif origin == current_app.config['BBB_FEEDBACK_ORIGIN']:) into functions?
if origin == current_app.config['FEEDBACK_FORMS_ORIGIN']:
email_sent = self.process_feedback_forms(...)
elif origin == current_app.config['BBB_FEEDBACK_ORIGIN']:
email_sent = self.process_feedback(...)
This could increase readability and isolate tasks and responsibilities.
| else: | ||
| return {'msg': 'Unknown error'}, slack_response.status_code | ||
| return err(ERROR_UNKNOWN_ORIGIN) | ||
| return {}, 200 |
There was a problem hiding this comment.
We return a 200 even if emails fail to be sent, despite logging errors, if the user does not get any feedback that something went wrong, we risk to lose this feedback (nobody will be actively checking logs every day to make sure emails are being lost). I suggest to make email the first class citizen in this endpoint/class, if the email is not send (for forms or regular feedback), we return an error. Slack is secondary, if something doesn't reach slack but it reaches the email, we will notice and deal with it. So if slack fails, we log the error but the user request does not need to fail unless the email was also not sent.
| except Exception as error: | ||
| current_app.logger.error('Fatal error while processing feedback form data: {0}'.format(error)) | ||
| return err(ERROR_FEEDBACKFORM_PROBLEM) | ||
| if not email_sent: |
There was a problem hiding this comment.
process_feedbackform_submission returns a string saying success, failed, ... but here, this if statement seems to expect a boolean. I would change the function to return a boolean.
adsws/feedback/config.py
Outdated
| FEEDBACK_FORMS_ORIGIN = 'user_submission' | ||
| BBB_FEEDBACK_ORIGIN = 'bbb_feedback' | ||
| # Email template to be applied based on email subject | ||
| FEEDBACK_TEMPLATES = { | ||
| 'Missing References': 'missing_references.txt', | ||
| 'Associated Articles': 'associated_articles.txt', | ||
| 'Updated Record': 'updated_record.txt', | ||
| 'New Record': 'new_record.txt' | ||
| } |
There was a problem hiding this comment.
Ideally, we could have all this integrated into one. Regular user feedback can also be processed with its own template and all could share code, except that the feedback forms will not be sent to slack (although we could send to slack just a message saying that a user has submitted data, but not the data itself, have you considered that?).
Update of
feedback/slackendpoint to deal with data submitted by the new BBB feedback forms. These submissions will only generate emails. From now on, in general, how feedback data will be processed depends on theoriginparameter in the POST data. In this case, this parameter is supposed to have the valueuser_submission(defined in the config). The data in the POST payload will be used to fill in a template to generate the email body (using jinja2 templates, selected on the basis of the email subject). In the case of abstract submissions (new or corrected), the original JSON data from the form will be send along in the form of attached JSON files. The unittests have been augmented to check this new functionality.