Skip to content
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

[MDS-5607] - Exception handling + Email Notification #2824

Merged
merged 20 commits into from
Dec 4, 2023
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions services/common/src/constants/API.js
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,9 @@ export const CREATE_EXPLOSIVES_PERMIT_AMENDMENT = (mineGuid) =>
export const EXPLOSIVES_PERMIT_AMENDMENT = (mineGuid, explosivesPermitGuid) =>
`/mines/${mineGuid}/explosives-permits-amendment/${explosivesPermitGuid}`;

// Common
export const COMMONS_EMAIL = `/commons/email`;

// EPIC Mine Information
export const EPIC_INFO = (mineGuid) => `/mines/${mineGuid}/epic`;

Expand Down
10 changes: 8 additions & 2 deletions services/common/src/constants/environment.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ export const DEFAULT_ENVIRONMENT = {
keycloak_url: "https://test.loginproxy.gov.bc.ca/auth",
flagsmithKey: "4Eu9eEMDmWVEHKDaKoeWY7",
flagsmithUrl: "https://mds-flags-dev.apps.silver.devops.gov.bc.ca/api/v1/",
errorNotifyRecipients: "[email protected]"
};

export const ENVIRONMENT = {
Expand All @@ -23,6 +24,7 @@ export const ENVIRONMENT = {
environment: "<ENV>",
flagsmithKey: "<FLAGSMITH_KEY>",
flagsmithUrl: "<FLAGSMITH_URL>",
errorNotifyRecipients: "<ERROR_NOTIFY_RECIPIENTS>",
_loaded: false,
};

Expand Down Expand Up @@ -55,7 +57,8 @@ export function setupEnvironment(
matomoUrl,
environment,
flagsmithKey,
flagsmithUrl
flagsmithUrl,
errorNotifyRecipients
) {
if (!apiUrl) {
throw new Error("apiUrl Is Mandatory");
Expand All @@ -82,14 +85,17 @@ export function setupEnvironment(
if (!flagsmithUrl) {
throw new Error("flagsmithUrl Is Mandatory");
}

if (!errorNotifyRecipients) {
throw new Error("errorNotifyRecipients is Mandatory")
}
ENVIRONMENT.apiUrl = apiUrl;
ENVIRONMENT.docManUrl = docManUrl;
ENVIRONMENT.filesystemProviderUrl = filesystemProviderUrl;
ENVIRONMENT.matomoUrl = matomoUrl;
ENVIRONMENT.environment = environment || "development";
ENVIRONMENT.flagsmithKey = flagsmithKey;
ENVIRONMENT.flagsmithUrl = flagsmithUrl;
ENVIRONMENT.errorNotifyRecipients = errorNotifyRecipients;

ENVIRONMENT._loaded = true;
}
Expand Down
74 changes: 73 additions & 1 deletion services/common/src/redux/customAxios.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
import axios from "axios";
import { notification } from "antd";
import { notification, Button } from "antd";
import * as String from "@mds/common/constants/strings";
import React from 'react';
import * as API from "@mds/common/constants/API";
import { ENVIRONMENT } from "@mds/common";
import { createRequestHeader } from "./utils/RequestHeaders";

// https://stackoverflow.com/questions/39696007/axios-with-promise-prototype-finally-doesnt-work
// eslint-disable-next-line @typescript-eslint/no-var-requires
Expand All @@ -15,6 +19,61 @@ const formatErrorMessage = (errorMessage) => {
return errorMessage.replace("(psycopg2.", "(DatabaseError.");
};

const decodeJWT = (token) => {
var base64Url = token.split('.')[1];
var base64 = base64Url.replace(/-/g, '+').replace(/_/g, '/');
var jsonPayload = decodeURIComponent(window.atob(base64).split('').map(function(c) {
return '%' + ('00' + c.charCodeAt(0).toString(16)).slice(-2);
}).join(''));

return JSON.parse(jsonPayload);
};

const notifymAdmin = (error) => {

const business_message = error?.response?.data?.message;
const detailed_error = error?.response?.data?.detailed_error;
let date = new Date()
const reported_date = `${date} ${date.getHours()}:${date.getMinutes()}`;
const user = decodeJWT(createRequestHeader().headers.Authorization)
const environment_name = ENVIRONMENT.environment.toString().toUpperCase();

const email_title = "[MDS_INCIDENT_REPORT] [" + environment_name + "] - " + reported_date + " - " + business_message;
const email_body = `<p><b>Environment:</b> ${environment_name}</P>
<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>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is addressed. Thanks.

<b>Reporter's IDIR:</b> ${user.idir_username}<br/>
<b>Reported Date:</b> ${reported_date}
</P>
<p><b>Detailed error:</b> ${detailed_error}</P><br/>
<p>To create a Jira ticket
<a href="https://bcmines.atlassian.net/jira/software/c/projects/MDS/boards/34/backlog">
Click here</a> </P><br/>`;

// TODO Recipients needs to be read from property file.
const payload = {
"title" : email_title,
"body": email_body,
"recipients" : ENVIRONMENT.errorNotifyRecipients
};

console.log("Sending error details to ", ENVIRONMENT.errorNotifyRecipients)

CustomAxios().post(ENVIRONMENT.apiUrl + API.COMMONS_EMAIL, payload, createRequestHeader())
.then((response) => {
notification.success({
message: "Error details sent to Admin. Thank you.",
duration: 5,
});
return response;
})
.catch((err) => {
throw new Error(err);
})
};

// @ts-ignore
const CustomAxios = ({ errorToastMessage, suppressErrorNotification = false } = {}) => {
const instance = axios.create();
Expand All @@ -34,9 +93,22 @@ const CustomAxios = ({ errorToastMessage, suppressErrorNotification = false } =
(errorToastMessage === "default" || errorToastMessage === undefined) &&
!suppressErrorNotification
) {
console.error('Detailed Error: ', error?.response?.data?.detailed_error)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missed a console log 😜

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const notificationKey = 'errorNotification';
notification.error({
key: notificationKey,
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>,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we put this behind a feature flag in case we decide on doing any iterations on this before releasing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a feature flag from FE. Thanks

duration: 10,
btn: (
<Button type="primary" size="small"
onClick={() => {
notifymAdmin(error);
notification.close(notificationKey);
}}>
Tell Admin
</Button>
),
});
} else if (errorToastMessage && !suppressErrorNotification) {
notification.error({
Expand Down
19 changes: 15 additions & 4 deletions services/core-api/app/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
from app.api.activity.namespace import api as activity_api
from app.api.dams.namespace import api as dams_api
from app.api.verifiable_credentials.namespace import api as verifiable_credential_api
from app.api.commons.namespace import api as commons_api

from app.commands import register_commands
from app.config import Config
Expand All @@ -47,6 +48,7 @@
from sqlalchemy.sql import text
from app.tasks.celery import celery
from app.tasks.celery_health_check import HealthCheckProbe
from app.api.exception.mds_core_api_exceptions import MDSCoreAPIException

from opentelemetry import trace
from opentelemetry.sdk.trace import TracerProvider
Expand Down Expand Up @@ -189,6 +191,7 @@ def register_routes(app):
root_api_namespace.add_namespace(activity_api)
root_api_namespace.add_namespace(dams_api)
root_api_namespace.add_namespace(verifiable_credential_api)
root_api_namespace.add_namespace(commons_api)

@root_api_namespace.route('/version/')
class VersionCheck(Resource):
Expand Down Expand Up @@ -336,7 +339,15 @@ def _add_sqlalchemy_error_handlers(classname):
@root_api_namespace.errorhandler(Exception)
def default_error_handler(error):
app.logger.error(str(error))
return {
'status': getattr(error, 'code', 500),
'message': str(error),
}, getattr(error, 'code', 500)
if isinstance(error, MDSCoreAPIException):
return {
"status": getattr(error, "code", 500),
"message": str(getattr(error, "message", "")),
"detailed_error": str(getattr(error, "detailed_error", "")),
}, getattr(error, 'code', 500)
else:
return {
"status": getattr(error, "code", 500),
"message": str(error),
"detailed_error": str(getattr(error, "detailed_error", "Not provided")),
}, getattr(error, 'code', 500)
7 changes: 7 additions & 0 deletions services/core-api/app/api/commons/namespace.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
from flask_restplus import Namespace

from app.api.commons.resources.email_resource import EmailResource

api = Namespace('commons', description='Common operations')

api.add_resource(EmailResource, '/email')
25 changes: 25 additions & 0 deletions services/core-api/app/api/commons/resources/email_resource.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
from app.api.services.email_service import EmailService
from flask_restplus import Resource
from flask import request

from app.api.utils.access_decorators import (requires_any_of, VIEW_ALL)
from app.api.utils.resources_mixins import UserMixin
from app.api.exception.mds_core_api_exceptions import MDSCoreAPIException

class EmailResource(Resource, UserMixin):
Copy link
Collaborator

@simensma-fresh simensma-fresh Nov 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


@requires_any_of([VIEW_ALL])
def post(self):
try:
data = request.get_json()
# Extract title and body from the JSON data
title = data.get('title', '')
body = data.get('body', '')
recipients = data.get("recipients")

EmailService.send_email(title, recipients, body)

except Exception as e:
raise MDSCoreAPIException("Error in sending email", detailed_error = e)

return True, 201
12 changes: 12 additions & 0 deletions services/core-api/app/api/exception/mds_core_api_exceptions.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
class MDSCoreAPIException(Exception):
"""A base Exception class for MDS Core API"""

description = (
"Exception occurred in MDS Core API"
)

def __init__(self, message="Oops! Something went wrong", **kwargs):
super().__init__(message)
self.code = int(kwargs.get("status_code", 500))
self.message = message
self.detailed_error = kwargs.get("detailed_error", "")
42 changes: 42 additions & 0 deletions services/core-api/app/api/mines/exceptions/mine_exceptions.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
import json
from app.api.exception.mds_core_api_exceptions import MDSCoreAPIException

class MineException(MDSCoreAPIException):
"""A Custom Exception for Errors in MINE Namespace"""

description = (
"Generic exeption for errors occurred in the Mine module"
)
def __init__(self, message = "Oops! Something went wrong", **kwargs):
super().__init__(message, **kwargs)
self.code = int(kwargs.get("status_code", 500))

class ExplosivesPermitDocumentExeption(MineException):
matbusby-fw marked this conversation as resolved.
Show resolved Hide resolved
"""Exception for Explosives Permit Document Related Exception"""

description = (
"Exception For Explosives Permit Document Related Errors"
)
def __init__(self, message = "An error occurred while processing Explosives Permit Document", **kwargs):
super().__init__(message, **kwargs)
self.code = int(kwargs.get("status_code", 500))

class ExplosivesPermitExeption(MineException):
"""Exception for Explosive Permit related errors"""

description = (
"Exception for errors in Explosive Permit"
)
def __init__(self, message = "Error in Explosive Permit", **kwargs):
super().__init__(message, **kwargs)
self.code = int(kwargs.get("status_code", 500))

class ExplosivePermitNumberAlreadyExistExeption(MineException):
"""Exception for already existing permit number"""

description = (
"Exception for already existing permit number"
)
def __init__(self, message = "A record already exists with the provided 'Explosives Permit Number'", **kwargs):
super().__init__(message, **kwargs)
self.code = int(kwargs.get("status_code", 422))
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
from app.api.utils.helpers import create_image_with_aspect_ratio
from app.api.utils.models_mixins import AuditMixin, Base
from app.extensions import db
from app.api.mines.exceptions.mine_exceptions import ExplosivesPermitExeption

PERMIT_SIGNATURE_IMAGE_HEIGHT_INCHES = 0.8
LETTER_SIGNATURE_IMAGE_HEIGHT_INCHES = 0.8
Expand Down Expand Up @@ -42,7 +43,8 @@ def get_with_context(cls, document_type_code, context_guid):
def transform_template_data(self, template_data, explosives_permit):
def validate_issuing_inspector(explosives_permit):
if not explosives_permit.issuing_inspector:
raise Exception('No Issuing Inspector has been assigned')
raise ExplosivesPermitExeption("No Issuing Inspector has been assigned",
status_code = 422)
if not explosives_permit.issuing_inspector.signature:
raise Exception('No signature for the Issuing Inspector has been provided')

Expand All @@ -57,33 +59,39 @@ def validate_issuing_inspector(explosives_permit):
issuing_inspector_party_guid = template_data['issuing_inspector_party_guid']
issuing_inspector = Party.find_by_party_guid(issuing_inspector_party_guid)
if issuing_inspector is None:
raise Exception('Party for Issuing Inspector not found')
raise ExplosivesPermitExeption("Can't find the provided issuing inspector",
status_code = 404)
else:
validate_issuing_inspector(explosives_permit)
issuing_inspector = explosives_permit.issuing_inspector
template_data['issuing_inspector_name'] = issuing_inspector.name

mine_manager = explosives_permit.mine_manager
if mine_manager is None:
raise Exception('Appointment for Mine Manager not found')
raise ExplosivesPermitExeption("Provided Mine Manager not found",
status_code = 404)

if mine_manager.party.first_address is None:
raise Exception('Address for Mine Manager not found')
raise ExplosivesPermitExeption("Mine Manager does not have an address",
status_code = 422)
template_data['mine_manager_address'] = mine_manager.party.first_address.full
template_data['mine_manager_name'] = mine_manager.party.name

permittee = explosives_permit.permittee
if permittee is None:
raise Exception('Appointment for Permittee not found')
raise ExplosivesPermitExeption("Provided permittee not found",
status_code = 404)

if permittee.party.first_address is None:
raise Exception('Address for Permittee not found')
raise ExplosivesPermitExeption("Permittee does not have an address",
status_code = 422)
template_data['permittee_address'] = permittee.party.first_address.full
template_data['permittee_name'] = permittee.party.name

permit_number = 'BC-XXXXX' if is_draft else explosives_permit.permit_number
if permit_number is None:
raise Exception('Explosives Permit has not been issued')
raise ExplosivesPermitExeption("Explosives Permit has not been issued",
status_code = 400)
template_data['permit_number'] = permit_number

# Transform template data for "Explosives Storage and Use Permit" (PER)
Expand Down
Loading
Loading