[MDS-5607] - Exception handling + Email Notification #2824
Conversation
isuru-aot
changed the title
isuru-aot
requested review from
matbusby-fw,
simensma-fresh and
henryoforeh-dev
| from app.api.utils.resources_mixins import UserMixin | ||
| from app.api.exception.mds_core_api_exceptions import MDSCoreAPIException | ||
|
|
||
| class EmailResource(Resource, UserMixin): |
There was a problem hiding this comment.
I would suggest that we maybe do not expose this as a generic way of sending emails accross the app. I've got some concerns with this in terms of potential abuse, as this would in essence allow anyone with access to send arbitrary emails on behalf of the mds team. The other concern I have would be that we allow plain html as input which would open up for html injection
As an alternative we could make this a specific "Report Error" endpoint that takes in the required fields so we can do some basic validation on it and create an email template that is specific for the error reporting in the templates folder which would escape what's passed in.
There was a problem hiding this comment.
Thanks Simen,
Added an endpoint for report-error, and only the error details are accepted in that. And email get sent with user info from BE. And the html template is used for the email content.
| <p><b>Business Error:</b> ${business_message}</P> | ||
| <p> | ||
| <b>Reporter's Name:</b> ${user.given_name} ${user.family_name}</br> | ||
| <b>Reporter's Email:</b> ${user.email}</br> |
There was a problem hiding this comment.
With the change suggested above to make the email endpoint specific to error reporting, I would suggest trying to get these details (user email / username etc) from the users token on the endpoint level so you would not be able to send these on behalf of another user
There was a problem hiding this comment.
This is addressed. Thanks.
| message: formatErrorMessage(error?.response?.data?.message ?? String.ERROR), | ||
| description: <p style={{ color: 'grey' }}>If you think this is a system error please help us to improve by informing the system Admin</p>, |
There was a problem hiding this comment.
Could we put this behind a feature flag in case we decide on doing any iterations on this before releasing?
There was a problem hiding this comment.
Added a feature flag from FE. Thanks
| message: formatErrorMessage(error?.response?.data?.message ?? String.ERROR), | ||
| duration: 10, | ||
| }); | ||
| console.error('Detailed Error: ', error?.response?.data?.detailed_error) |
There was a problem hiding this comment.
Missed a console log 😜
There was a problem hiding this comment.
Thanks @matbusby-fw,
Actually, I kept this purposely, as the detailed error will not be visible to FE anymore with this change. But it may not good practice. Shall I remove it?
There was a problem hiding this comment.
Well, I don't suppose it will hurt anything, but it might be of limited usefulness to us since we're not going to be seeing the user's console.
Objective
MDS-5607
MDS-5617
Why are you making this change? Provide a short explanation and/or screenshots
Hackathon work item for exception handling.
Items covered:
VIEW_ALLrightsERROR_NOTIFY_RECIPIENTSenvironment variable to configure email address for error notification.