Notify table owners (#410)

* wip

* wip

* remove debuggers

* fix tests + python lint

* update tests

* fix lint

* revert some changes that aren't needed

* modify content

* respond to pr comments

* fix lint

* update wording a bit

* update per review comments

* update per pr comments

amundsen_application/api/issue/issue.py

@@ -57,9 +57,9 @@ class IssueAPI(Resource):
                 return make_response(jsonify({'msg': message}), HTTPStatus.ACCEPTED)
             self.client = get_issue_tracker_client()
 
-            self.reqparse.add_argument('title', type=str, location='form')
-            self.reqparse.add_argument('key', type=str, location='form')
-            self.reqparse.add_argument('description', type=str, location='form')
+            self.reqparse.add_argument('title', type=str, location='json')
+            self.reqparse.add_argument('key', type=str, location='json')
+            self.reqparse.add_argument('description', type=str, location='json')
             args = self.reqparse.parse_args()
             response = self.client.create_issue(description=args['description'],
                                                 table_uri=args['key'],

amundsen_application/api/utils/notification_utils.py

@@ -20,6 +20,7 @@ class NotificationType(str, Enum):
     OWNER_REMOVED = 'owner_removed'
     METADATA_EDITED = 'metadata_edited'
     METADATA_REQUESTED = 'metadata_requested'
+    DATA_ISSUE_REPORTED = 'data_issue_reported'
 
     @classmethod
     def has_value(cls, value: str) -> bool:
@@ -51,6 +52,12 @@ NOTIFICATION_STRINGS = {
         'comment': '',
         'end_note': '<br/>Please visit the provided link and improve descriptions on that resource.<br/>',
         'notification': '<br/>{sender} is trying to use <a href="{resource_url}">{resource_name}</a>, ',
+    },
+    NotificationType.DATA_ISSUE_REPORTED.value: {
+        'comment': '<br/>Link to the issue: {data_issue_url}<br/>',
+        'end_note': '<br/>Please visit the provided issue link for more information. You are getting this email '
+                    'because you are listed as an owner of the resource. Please do not reply to this email.<br/>',
+        'notification': '<br/>{sender} has reported a data issue for <a href="{resource_url}">{resource_name}</a>, ',
     }
 }
 
@@ -122,6 +129,11 @@ def get_notification_html(*, notification_type: str, options: Dict, sender: str)
             comment = ('<br/>{sender} has included the following information with their request:'
                        '<br/>{comment}<br/>').format(sender=sender, comment=options_comment)
 
+    if notification_type == NotificationType.DATA_ISSUE_REPORTED:
+        greeting = 'Hello data owner,<br>'
+        data_issue_url = options.get('data_issue_url')
+        comment = comment.format(data_issue_url=data_issue_url)
+
     return '{greeting}{notification}{comment}{end_note}{salutation}'.format(greeting=greeting,
                                                                             notification=notification,
                                                                             comment=comment,
@@ -142,6 +154,7 @@ def get_notification_subject(*, notification_type: str, options: Dict) -> str:
         NotificationType.OWNER_REMOVED.value: 'You have been removed as an owner of {}'.format(resource_name),
         NotificationType.METADATA_EDITED.value: 'Your dataset {}\'s metadata has been edited'.format(resource_name),
         NotificationType.METADATA_REQUESTED.value: 'Request for metadata on {}'.format(resource_name),
+        NotificationType.DATA_ISSUE_REPORTED.value: 'A data issue has been reported for {}'.format(resource_name)
     }
     subject = notification_subject_dict.get(notification_type)
     if subject is None:

amundsen_application/proxy/issue_tracker_clients/jira_client.py

@@ -88,7 +88,6 @@ class JiraClient(BaseIssueTrackerClient):
                              f'\n Reported By: {user_email} '
                              f'\n Table Key: {table_uri} [PLEASE DO NOT REMOVE]'),
                 reporter={'name': jira_id}))
-
             return self._get_issue_properties(issue=issue)
         except JIRAError as e:
             logging.exception(str(e))

amundsen_application/static/js/components/TableDetail/ReportTableIssue/constants.ts

-export const REPORT_DATA_ISSUE_TEXT = "Report an issue"; 
\ No newline at end of file
+export const REPORT_DATA_ISSUE_TEXT = "Report an issue"; 
+export const TABLE_OWNERS_NOTE = "Please note: Table owners will also be notified via email when an issue is reported.";
\ No newline at end of file

amundsen_application/static/js/components/TableDetail/ReportTableIssue/index.tsx

@@ -7,9 +7,10 @@ import LoadingSpinner from 'components/common/LoadingSpinner';
 import { createIssue } from 'ducks/issue/reducer'; 
 import { CreateIssueRequest } from 'ducks/issue/types';
 import './styles.scss';
-import { REPORT_DATA_ISSUE_TEXT } from './constants'; 
+import { REPORT_DATA_ISSUE_TEXT, TABLE_OWNERS_NOTE } from './constants'; 
 import { logClick } from 'ducks/utilMethods';
 import { notificationsEnabled, issueTrackingEnabled } from 'config/config-utils';
+import { TableMetadata, CreateIssuePayload, NotificationPayload, NotificationType } from 'interfaces';
 
 export interface ComponentProps {
   tableKey: string;
@@ -17,11 +18,17 @@ export interface ComponentProps {
 }
 
 export interface DispatchFromProps {
-  createIssue: (data: FormData) => CreateIssueRequest; 
+  createIssue: (
+    createIssuePayload: CreateIssuePayload, 
+    notificationPayload: NotificationPayload
+    ) => CreateIssueRequest;
 }
 
 export interface StateFromProps {
   isLoading: boolean;
+  tableOwners: string[]; 
+  userEmail: string; 
+  tableMetadata: TableMetadata; 
 }
 
 interface ReportTableIssueState {
@@ -41,10 +48,41 @@ export class ReportTableIssue extends React.Component<ReportTableIssueProps, Rep
     event.preventDefault();
     const form = document.getElementById("report-table-issue-form") as HTMLFormElement;
     const formData = new FormData(form);
-    this.props.createIssue(formData);
-    this.setState({isOpen: false}); 
+   
+    const createIssuePayload = this.getCreateIssuePayload(formData); 
+    const notificationPayload = this.getNotificationPayload(); 
+    this.props.createIssue(createIssuePayload, notificationPayload);
+    this.setState({isOpen: false }); 
   };
 
+  getCreateIssuePayload = (formData: FormData): CreateIssuePayload => {
+    const title = formData.get('title') as string;  
+    const description = formData.get('description') as string;
+
+    return {
+      title, 
+      description, 
+      key: this.props.tableKey, 
+    }
+  }
+
+  getNotificationPayload  = (): NotificationPayload => {
+    const { cluster, database, schema, name } = this.props.tableMetadata;
+    const owners = this.props.tableOwners;
+    const resourceName = `${schema}.${name}`;  
+    const resourcePath = `/table_detail/${cluster}/${database}/${schema}/${name}`; 
+     
+    return {
+      recipients: owners, 
+      sender: this.props.userEmail, 
+      notificationType: NotificationType.DATA_ISSUE_REPORTED, 
+      options: {
+        resource_name: resourceName, 
+        resource_path: resourcePath, 
+      }
+    };
+  }
+
   toggle = () => {
     this.setState({ isOpen: !this.state.isOpen });
   };
@@ -64,7 +102,6 @@ export class ReportTableIssue extends React.Component<ReportTableIssueProps, Rep
     if (this.props.isLoading) {
       return <LoadingSpinner />;
     }
-
     return (
         <>
          {this.renderPipe()}
@@ -82,8 +119,6 @@ export class ReportTableIssue extends React.Component<ReportTableIssueProps, Rep
               </h3>
               <button type="button" className="btn btn-close" aria-label={"close"} onClick={this.toggle} />
               <form id="report-table-issue-form" onSubmit={ this.submitForm }>
-                <input type="hidden" name="key" value={ this.props.tableKey }/>
-
                 <div className="form-group">
                   <label>Title</label>
                   <input name="title" className="form-control" required={true} maxLength={200} />
@@ -94,6 +129,9 @@ export class ReportTableIssue extends React.Component<ReportTableIssueProps, Rep
                 </div>
                 <button className="btn btn-primary submit" type="submit" >Submit</button>
               </form>
+              <div className="data-owner-notification">
+                {TABLE_OWNERS_NOTE}
+              </div>
             </div>
           }
       </>
@@ -101,8 +139,14 @@ export class ReportTableIssue extends React.Component<ReportTableIssueProps, Rep
   }
 }
 export const mapStateToProps = (state: GlobalState) => {
+  const ownerObj = state.tableMetadata.tableOwners.owners; 
+  const tableOwnersEmails = Object.keys(ownerObj); 
+  const userEmail = state.user.loggedInUser.email;
   return {
-    isLoading: state.issue.isLoading
+    userEmail,
+    isLoading: state.issue.isLoading, 
+    tableOwners: tableOwnersEmails, 
+    tableMetadata: state.tableMetadata.tableData
   };
 };
 

amundsen_application/static/js/components/TableDetail/ReportTableIssue/styles.scss

@@ -34,4 +34,8 @@
     right: 32px;
   }
 
+  .data-owner-notification {
+    font-size: 10px; 
+    margin: 5px 0px 0px 0px; 
+  }
 }

amundsen_application/static/js/components/TableDetail/ReportTableIssue/tests/index.spec.tsx

@@ -11,8 +11,36 @@ import {
   mapDispatchToProps,
   mapStateToProps, 
 } from '..';
+import { NotificationType } from 'interfaces';
 
-const mockFormData = { key1: 'val1', key2: 'val2' };
+const mockFormData = { 
+  'key': 'val1', 
+  'title': 'title', 
+  'description': 'description', 
+  'resource_name': 'resource name', 
+  'resource_path': 'path', 
+  'owners': 'test@test.com', 
+  get: (key: string) => {
+    return mockFormData[key]; 
+  }
+ };
+
+const mockCreateIssuePayload = {
+  key: 'key', 
+  title: 'title', 
+  description: 'description'
+}
+
+const mockNotificationPayload = {
+  notificationType: NotificationType.DATA_ISSUE_REPORTED,
+  options: {
+    resource_name: 'schema.table_name',
+    resource_path: '/table_detail/cluster/database/schema/table_name',
+  },
+  recipients: ['owner@email'],
+  sender: 'user@email'
+}
+  
 // @ts-ignore: How to mock FormData without TypeScript error?
 global.FormData = () => (mockFormData);
 
@@ -24,6 +52,13 @@ describe('ReportTableIssue', () => {
       createIssue: jest.fn(), 
       tableKey: 'key', 
       tableName: 'name',
+      tableOwners: ['owner@email'], 
+      tableMetadata: {...globalState.tableMetadata.tableData, 
+        schema: 'schema', 
+        name: 'table_name',
+        cluster: 'cluster', 
+        database: 'database'},
+      userEmail: 'user@email', 
       ...propOverrides
     };
     const wrapper = shallow<ReportTableIssue>(<ReportTableIssue {...props} />);
@@ -68,7 +103,9 @@ describe('ReportTableIssue', () => {
         // @ts-ignore: mocked events throw type errors
         wrapper.instance().submitForm({ preventDefault: jest.fn(), 
         currentTarget: {id: 'id', nodeName: 'button'} });
-        expect(props.createIssue).toHaveBeenCalledWith(mockFormData);
+        expect(props.createIssue).toHaveBeenCalledWith(
+          mockCreateIssuePayload,
+          mockNotificationPayload);
         expect(wrapper.state().isOpen).toBe(false); 
       });
 

amundsen_application/static/js/ducks/issue/api/tests/index.spec.ts

 import axios, { AxiosResponse } from 'axios';
 import * as API from '../v0';
+import { NotificationType } from 'interfaces';
+import AppConfig from 'config/config';
+
 
 jest.mock('axios');
 
@@ -42,8 +45,10 @@ describe('getIssues', () => {
 describe('createIssue', () => {
   let mockGetResponse;
   let axiosMock;
-  let formData;
-  const issueResult = { issue_key: 'key' };
+  const issueResult = { issue_key: 'key',
+    data_issue_url: 'url' };
+  let createIssuePayload; 
+  let sendNotificationPayload; 
   beforeAll(() => {
     mockGetResponse = {
       data: {
@@ -55,25 +60,41 @@ describe('createIssue', () => {
       headers: {},
       config: {}
     };
-    formData = new FormData();
+    createIssuePayload = {
+      key: 'key', 
+      title: 'title', 
+      description: 'description'
+    }
+    sendNotificationPayload = {
+      owners: ['owner1'], 
+      sender: 'sender', 
+      notificationType: NotificationType.DATA_ISSUE_REPORTED, 
+      options: {
+        resource_name: 'resource_name', 
+        resource_path: 'resource_path',
+        data_issue_url: 'url'
+      }
+    }
     axiosMock = jest.spyOn(axios, 'post').mockImplementation(() => Promise.resolve(mockGetResponse));
   });
 
-  it('calls expected endpoint with headers', async () => {
-    expect.assertions(1);
-    await API.createIssue(formData).then(data => {
-      expect(axiosMock).toHaveBeenCalledWith(
-        `${API.API_PATH}/issue`,
-        formData, {
-        headers: {'Content-Type': 'multipart/form-data'}
-      });
+  it('returns response data', async () => {
+    AppConfig.mailClientFeatures.notificationsEnabled = false;
+    expect.assertions(3);
+    await API.createIssue(createIssuePayload, sendNotificationPayload).then(data => {
+      expect(data).toEqual(issueResult);
+      expect(axiosMock).toHaveBeenCalledWith(`${API.API_PATH}/issue`, createIssuePayload);
+      expect(axiosMock).toHaveBeenCalledTimes(1);
     });
   });
 
-  it('returns response data', async () => {
-    expect.assertions(1);
-    await API.createIssue(formData).then(data => {
+  it('submits a notification if notifications are enabled', async () => {
+    AppConfig.mailClientFeatures.notificationsEnabled = true;
+    expect.assertions(3);
+    await API.createIssue(createIssuePayload, sendNotificationPayload).then(data => {
       expect(data).toEqual(issueResult);
+      expect(axiosMock).toHaveBeenCalledWith(`${API.API_PATH}/issue`, createIssuePayload);
+      expect(axiosMock).toHaveBeenCalledWith(API.NOTIFICATION_API_PATH, sendNotificationPayload);
     });
   });
 

amundsen_application/static/js/ducks/issue/api/v0.ts

 import axios, { AxiosResponse } from 'axios';
-import { Issue } from 'interfaces';
+import { Issue, CreateIssuePayload, NotificationPayload } from 'interfaces';
+import { notificationsEnabled } from 'config/config-utils';
 
 export const API_PATH = '/api/issue';
+export const NOTIFICATION_API_PATH = '/api/mail/v0/notification';
 
 export type IssuesAPI = {
   issues: {
@@ -22,11 +24,18 @@ export function getIssues(tableKey: string) {
   });
 }
 
-export function createIssue(data: FormData) {
-  const headers =  {'Content-Type': 'multipart/form-data' };
-  return axios.post(`${API_PATH}/issue`, data, { headers }
-    ).then((response: AxiosResponse<IssueApi>) => {
-      return response.data.issue; 
-    });
+export function createIssue(payload: CreateIssuePayload, notificationPayload: NotificationPayload) {
+  return axios.post(`${API_PATH}/issue`, {
+    key: payload.key, 
+    title: payload.title, 
+    description: payload.description
+  })
+  .then((response: AxiosResponse<IssueApi>) => {
+    if (notificationsEnabled()) {
+      notificationPayload.options.data_issue_url = response.data.issue.url; 
+      axios.post(NOTIFICATION_API_PATH, notificationPayload);
+    }
+    return response.data.issue;
+});
 }
 

amundsen_application/static/js/ducks/issue/reducer.ts

-import { Issue } from "interfaces";
+import { Issue, CreateIssuePayload, NotificationPayload } from "interfaces";
 import { 
   GetIssues, 
   CreateIssue, 
@@ -10,10 +10,14 @@ import {
 
 
 /* ACTIONS */
-export function createIssue(formData: FormData): CreateIssueRequest {
+export function createIssue(
+  createIssuePayload: CreateIssuePayload,
+  notificationPayload: NotificationPayload
+  ): CreateIssueRequest {
   return {
     payload: {
-      data: formData,
+      createIssuePayload, 
+      notificationPayload
     },
     type: CreateIssue.REQUEST,
     };

amundsen_application/static/js/ducks/issue/sagas.ts

@@ -7,8 +7,6 @@ import { GetIssues, GetIssuesRequest, CreateIssue, CreateIssueRequest } from './
 
 import * as API from './api/v0';
 
-/** maybe just reload the issues content when there is a new issue created?*/
-
 export function* getIssuesWorker(action: GetIssuesRequest): SagaIterator {
     const { key } = action.payload; 
     let response;
@@ -27,7 +25,7 @@ export function* getIssuesWatcher(): SagaIterator {
 export function* createIssueWorker(action: CreateIssueRequest): SagaIterator {
   try { 
     let response;
-    response = yield call(API.createIssue, action.payload.data);
+    response = yield call(API.createIssue, action.payload.createIssuePayload, action.payload.notificationPayload);
     yield put((createIssueSuccess(response)));
   } catch(error) {
     yield put(createIssueFailure(null));

amundsen_application/static/js/ducks/issue/tests/index.spec.ts

@@ -21,27 +21,32 @@ import {
   GetIssuesRequest,
   CreateIssueRequest
 } from '../types'; 
-import { Issue } from 'interfaces';
+import { Issue, NotificationType } from 'interfaces';
 import { getIssuesWatcher, getIssuesWorker, createIssueWatcher, createIssueWorker } from '../sagas';
 import { throwError } from 'redux-saga-test-plan/providers';
 
 describe('issue ducks', () => {
-  let formData: FormData; 
   let tableKey: string; 
   let issue: Issue; 
   let issues: Issue[]; 
   let remaining: number; 
   let remainingUrl: string; 
+  let key; 
+  let title; 
+  let description; 
+  let resourceName; 
+  let resourcePath; 
+  let owners; 
+  let sender; 
   beforeAll(() => {
     tableKey = 'key'; 
-    const testData = { 
-      key: 'table', 
-      title: 'stuff', 
-      description: 'This is a test' 
-    };
-    formData = new FormData();
-    Object.keys(testData).forEach(key => formData.append(key, testData[key]));
-
+    key = 'table', 
+    title ='stuff';
+    description ='This is a test';
+    resourceName = 'resource_name'; 
+    resourcePath = 'resource_path'; 
+    owners = ['email@email']; 
+    sender = 'sender@email'; 
     issue =  {
       issue_key: 'issue_key', 
       title: 'title', 
@@ -72,10 +77,30 @@ describe('issue ducks', () => {
     });
 
     it('createIssue - returns the action to create items', () => {
-      const action = createIssue(formData);
+      const createIssuePayload = {
+        key, 
+        title, 
+        description
+      };
+      const notificationPayload = {
+        sender, 
+        recipients: owners, 
+        notificationType: NotificationType.DATA_ISSUE_REPORTED, 
+        options: {
+          resource_name: resourceName, 
+          resource_path: resourcePath
+        }
+      }; 
+
+      const action = createIssue(createIssuePayload, notificationPayload);
       const { payload } = action;
       expect(action.type).toBe(CreateIssue.REQUEST);
-      expect(payload.data).toBe(formData);
+      expect(payload.createIssuePayload.key).toBe(key);
+      expect(payload.createIssuePayload.title).toBe(title); 
+      expect(payload.createIssuePayload.description).toBe(description); 
+      expect(payload.notificationPayload.options.resource_name).toBe(resourceName); 
+      expect(payload.notificationPayload.options.resource_path).toBe(resourcePath); 
+      expect(payload.notificationPayload.recipients).toBe(owners); 
     });
 
     it('createIssueFailure - returns the action to process failure', () => {
@@ -142,7 +167,21 @@ describe('issue ducks', () => {
     });
 
     it('should handle CreateIssue.REQUEST', () => {
-      expect(reducer(testState, createIssue(formData))).toEqual({ 
+      const createIssuePayload = {
+        key, 
+        title, 
+        description
+      };
+      const notificationPayload = {
+        sender, 
+        recipients: owners, 
+        notificationType: NotificationType.DATA_ISSUE_REPORTED, 
+        options: {
+          resource_name: resourceName, 
+          resource_path: resourcePath
+        }
+      }; 
+      expect(reducer(testState, createIssue(createIssuePayload, notificationPayload))).toEqual({ 
         issues: [], 
         isLoading: true, 
         remainingIssuesUrl: remainingUrl,
@@ -214,7 +253,21 @@ describe('issue ducks', () => {
     describe('createIssuesWorker', () => {
       let action: CreateIssueRequest;
       beforeAll(() => {
-        action = createIssue(formData);
+        const createIssuePayload = {
+          key, 
+          title, 
+          description
+        };
+        const notificationPayload = {
+          sender, 
+          recipients: owners, 
+          notificationType: NotificationType.DATA_ISSUE_REPORTED, 
+          options: {
+            resource_name: resourceName, 
+            resource_path: resourcePath
+          }
+        }; 
+        action = createIssue(createIssuePayload, notificationPayload);
         issues = [issue];
       });
 

amundsen_application/static/js/ducks/issue/types.ts

-import { Issue } from "interfaces";
+import { Issue, CreateIssuePayload, NotificationPayload } from "interfaces";
 
 export enum GetIssues {
     REQUEST = 'amundsen/issue/GET_ISSUES_REQUEST',
@@ -21,7 +21,8 @@ export interface GetIssuesRequest {
 export interface CreateIssueRequest {
     type: CreateIssue.REQUEST;
     payload: {
-        data: FormData
+        createIssuePayload: CreateIssuePayload,
+        notificationPayload: NotificationPayload
     }
 };
 

amundsen_application/static/js/interfaces/Issue.ts

@@ -2,4 +2,10 @@ export interface Issue {
     issue_key: string;
     title: string;
     url: string;
-};
\ No newline at end of file
+};
+
+export interface CreateIssuePayload {
+  key: string; 
+  title: string; 
+  description: string; 
+}
\ No newline at end of file

amundsen_application/static/js/interfaces/Notifications.ts

@@ -3,6 +3,7 @@ export enum NotificationType {
   OWNER_REMOVED = 'owner_removed',
   METADATA_EDITED = 'metadata_edited',
   METADATA_REQUESTED = 'metadata_requested',
+  DATA_ISSUE_REPORTED = 'data_issue_reported'
 }
 
 export enum RequestMetadataType {
@@ -13,7 +14,15 @@ export enum RequestMetadataType {
 export interface SendNotificationOptions {
   resource_name: string,
   resource_path: string,
-  description_requested: boolean,
-  fields_requested: boolean,
-  comment?: string,
+  description_requested?: boolean,
+  fields_requested?: boolean,
+  comment?: string, 
+  data_issue_url?: string
 };
+
+export interface NotificationPayload {
+  recipients: string[], 
+  sender: string, 
+  notificationType: NotificationType, 
+  options: SendNotificationOptions
+};
\ No newline at end of file