Update JIRA Issue query + TableIssue UI (#415)

* Update search query stub

* Update search query stub

* Further specify query

* Move 'isLoading' to TableIssues

* Fix python test

amundsen_application/proxy/issue_tracker_clients/jira_client.py

@@ -11,7 +11,7 @@ from amundsen_application.models.issue_results import IssueResults
 import urllib.parse
 import logging
 
-SEARCH_STUB_ALL_ISSUES = 'text ~ "{table_key}" order by createdDate DESC'
+SEARCH_STUB_ALL_ISSUES = 'text ~ "\\"Table Key: {table_key} [PLEASE DO NOT REMOVE]\\"" order by createdDate DESC'
 # this is provided by jira as the type of a bug
 ISSUE_TYPE_ID = 1
 ISSUE_TYPE_NAME = 'Bug'

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

@@ -3,11 +3,10 @@ import { connect } from 'react-redux';
 import { bindActionCreators } from 'redux';
 import { GlobalState } from 'ducks/rootReducer';
 
-import LoadingSpinner from 'components/common/LoadingSpinner';
-import { createIssue } from 'ducks/issue/reducer'; 
+import { createIssue } from 'ducks/issue/reducer';
 import { CreateIssueRequest } from 'ducks/issue/types';
 import './styles.scss';
-import { REPORT_DATA_ISSUE_TEXT, TABLE_OWNERS_NOTE } from './constants'; 
+import { REPORT_DATA_ISSUE_TEXT, TABLE_OWNERS_NOTE } from './constants';
 import { logClick } from 'ducks/utilMethods';
 import { TableMetadata, CreateIssuePayload, NotificationPayload, NotificationType } from 'interfaces';
 
@@ -18,20 +17,19 @@ export interface ComponentProps {
 
 export interface DispatchFromProps {
   createIssue: (
-    createIssuePayload: CreateIssuePayload, 
+    createIssuePayload: CreateIssuePayload,
     notificationPayload: NotificationPayload
     ) => CreateIssueRequest;
 }
 
 export interface StateFromProps {
-  isLoading: boolean;
-  tableOwners: string[]; 
-  userEmail: string; 
-  tableMetadata: TableMetadata; 
+  tableOwners: string[];
+  userEmail: string;
+  tableMetadata: TableMetadata;
 }
 
 interface ReportTableIssueState {
-  isOpen: boolean; 
+  isOpen: boolean;
 }
 
 export type ReportTableIssueProps = StateFromProps & DispatchFromProps & ComponentProps
@@ -47,53 +45,50 @@ 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);
-   
-    const createIssuePayload = this.getCreateIssuePayload(formData); 
-    const notificationPayload = this.getNotificationPayload(); 
+
+    const createIssuePayload = this.getCreateIssuePayload(formData);
+    const notificationPayload = this.getNotificationPayload();
     this.props.createIssue(createIssuePayload, notificationPayload);
-    this.setState({isOpen: false }); 
+    this.setState({isOpen: false });
   };
 
 
   getCreateIssuePayload = (formData: FormData): CreateIssuePayload => {
-    const title = formData.get('title') as string;  
+    const title = formData.get('title') as string;
     const description = formData.get('description') as string;
 
     return {
-      title, 
-      description, 
-      key: this.props.tableKey, 
+      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}`; 
-     
+    const resourceName = `${schema}.${name}`;
+    const resourcePath = `/table_detail/${cluster}/${database}/${schema}/${name}`;
+
     return {
-      recipients: owners, 
-      sender: this.props.userEmail, 
-      notificationType: NotificationType.DATA_ISSUE_REPORTED, 
+      recipients: owners,
+      sender: this.props.userEmail,
+      notificationType: NotificationType.DATA_ISSUE_REPORTED,
       options: {
-        resource_name: resourceName, 
-        resource_path: resourcePath, 
+        resource_name: resourceName,
+        resource_path: resourcePath,
       }
     };
   }
 
   toggle = (event) => {
     if (!this.state.isOpen) {
-      logClick(event); 
+      logClick(event);
     }
     this.setState({ isOpen: !this.state.isOpen });
   };
 
   render() {
-    if (this.props.isLoading) {
-      return <LoadingSpinner />;
-    }
     return (
         <>
           <a href="javascript:void(0)"
@@ -130,13 +125,12 @@ 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 ownerObj = state.tableMetadata.tableOwners.owners;
+  const tableOwnersEmails = Object.keys(ownerObj);
   const userEmail = state.user.loggedInUser.email;
   return {
     userEmail,
-    isLoading: state.issue.isLoading, 
-    tableOwners: tableOwnersEmails, 
+    tableOwners: tableOwnersEmails,
     tableMetadata: state.tableMetadata.tableData
   };
 };

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

@@ -4,30 +4,30 @@ import { shallow } from 'enzyme';
 
 import AppConfig from 'config/config';
 import globalState from 'fixtures/globalState';
-import { 
+import {
   ComponentProps,
-  ReportTableIssue, 
-  ReportTableIssueProps, 
+  ReportTableIssue,
+  ReportTableIssueProps,
   mapDispatchToProps,
-  mapStateToProps, 
+  mapStateToProps,
 } from '..';
 import { NotificationType } from 'interfaces';
 
-const mockFormData = { 
-  'key': 'val1', 
-  'title': 'title', 
-  'description': 'description', 
-  'resource_name': 'resource name', 
-  'resource_path': 'path', 
-  'owners': 'test@test.com', 
+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]; 
+    return mockFormData[key];
   }
  };
 
 const mockCreateIssuePayload = {
-  key: 'key', 
-  title: 'title', 
+  key: 'key',
+  title: 'title',
   description: 'description'
 }
 
@@ -40,7 +40,7 @@ const mockNotificationPayload = {
   recipients: ['owner@email'],
   sender: 'user@email'
 }
-  
+
 // @ts-ignore: How to mock FormData without TypeScript error?
 global.FormData = () => (mockFormData);
 
@@ -48,17 +48,16 @@ describe('ReportTableIssue', () => {
   const setStateSpy = jest.spyOn(ReportTableIssue.prototype, 'setState');
   const setup = (propOverrides?: Partial<ReportTableIssueProps>) => {
     const props: ReportTableIssueProps = {
-      isLoading: false,
-      createIssue: jest.fn(), 
-      tableKey: 'key', 
+      createIssue: jest.fn(),
+      tableKey: 'key',
       tableName: 'name',
-      tableOwners: ['owner@email'], 
-      tableMetadata: {...globalState.tableMetadata.tableData, 
-        schema: 'schema', 
+      tableOwners: ['owner@email'],
+      tableMetadata: {...globalState.tableMetadata.tableData,
+        schema: 'schema',
         name: 'table_name',
-        cluster: 'cluster', 
+        cluster: 'cluster',
         database: 'database'},
-      userEmail: 'user@email', 
+      userEmail: 'user@email',
       ...propOverrides
     };
     const wrapper = shallow<ReportTableIssue>(<ReportTableIssue {...props} />);
@@ -69,11 +68,11 @@ describe('ReportTableIssue', () => {
     it('Renders loading spinner if not ready', () => {
       const { props, wrapper } = setup();
       expect(wrapper.find('.loading-spinner')).toBeTruthy();
-    }); 
+    });
 
     it('Renders modal if open', () => {
-      const { props, wrapper } = setup({isLoading: false});
-      wrapper.setState({isOpen: true}); 
+      const { props, wrapper } = setup();
+      wrapper.setState({isOpen: true});
       expect(wrapper.find('.report-table-issue-modal')).toBeTruthy();
     });
 
@@ -87,37 +86,37 @@ describe('ReportTableIssue', () => {
         expect(setStateSpy).toHaveBeenCalledWith({ isOpen: !previsOpenState });
       });
     });
-    
+
     describe('submitForm', () => {
       it ('calls createIssue with mocked form data', () => {
         const { props, wrapper } = setup();
         // @ts-ignore: mocked events throw type errors
-        wrapper.instance().submitForm({ preventDefault: jest.fn(), 
+        wrapper.instance().submitForm({ preventDefault: jest.fn(),
         currentTarget: {id: 'id', nodeName: 'button'} });
         expect(props.createIssue).toHaveBeenCalledWith(
           mockCreateIssuePayload,
           mockNotificationPayload);
-        expect(wrapper.state().isOpen).toBe(false); 
+        expect(wrapper.state().isOpen).toBe(false);
       });
 
       it ('calls sets isOpen to false', () => {
         const { props, wrapper } = setup();
         // @ts-ignore: mocked events throw type errors
-        wrapper.instance().submitForm({ preventDefault: jest.fn(), 
+        wrapper.instance().submitForm({ preventDefault: jest.fn(),
         currentTarget: {id: 'id', nodeName: 'button'} });
-        expect(wrapper.state().isOpen).toBe(false); 
+        expect(wrapper.state().isOpen).toBe(false);
       });
-    }); 
+    });
 
     describe('mapDispatchToProps', () => {
       let dispatch;
       let props;
-    
+
       beforeAll(() => {
         dispatch = jest.fn(() => Promise.resolve());
         props = mapDispatchToProps(dispatch);
       });
-    
+
       it('sets getIssues on the props', () => {
         expect(props.createIssue).toBeInstanceOf(Function);
       });
@@ -128,10 +127,6 @@ describe('ReportTableIssue', () => {
       beforeAll(() => {
         result = mapStateToProps(globalState);
       });
-    
-      it('sets isLoading on the props', () => {
-        expect(result.isLoading).toEqual(globalState.issue.isLoading);
-      });
-    });  
-  }); 
-}); 
+    });
+  });
+});

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

@@ -3,30 +3,32 @@ import { GlobalState } from 'ducks/rootReducer';
 import { bindActionCreators } from 'redux';
 import { connect } from 'react-redux';
 
-import { Issue } from 'interfaces'; 
-import { getIssues } from 'ducks/issue/reducer'; 
+import { Issue } from 'interfaces';
+import { getIssues } from 'ducks/issue/reducer';
 import { logClick } from 'ducks/utilMethods';
 import { GetIssuesRequest } from 'ducks/issue/types';
+import LoadingSpinner from 'components/common/LoadingSpinner';
 import ReportTableIssue from 'components/TableDetail/ReportTableIssue';
 import { NO_DATA_ISSUES_TEXT } from './constants';
 import './styles.scss';
 
 export interface StateFromProps {
-  issues: Issue[]; 
-  total: number; 
-  allIssuesUrl: string; 
+  issues: Issue[];
+  total: number;
+  allIssuesUrl: string;
+  isLoading: boolean;
 }
 
 export interface DispatchFromProps {
-  getIssues: (key: string) => GetIssuesRequest; 
+  getIssues: (key: string) => GetIssuesRequest;
 }
 
 export interface ComponentProps {
   tableKey: string;
-  tableName: string; 
+  tableName: string;
 }
 
-export type TableIssueProps = StateFromProps & DispatchFromProps & ComponentProps; 
+export type TableIssueProps = StateFromProps & DispatchFromProps & ComponentProps;
 
 export class TableIssues extends React.Component<TableIssueProps> {
   constructor(props) {
@@ -52,12 +54,12 @@ export class TableIssues extends React.Component<TableIssueProps> {
           <span className="issue-title-name">
             { issue.title }
           </span>
-        </span> 
+        </span>
         <span className="table-issue-status">
             {issue.status}
         </span>
       </div>
-    ); 
+    );
   }
 
   renderMoreIssuesMessage = (count: number, url: string) => {
@@ -65,21 +67,21 @@ export class TableIssues extends React.Component<TableIssueProps> {
       <span className="table-more-issues" key="more-issue-link">
         <a id="more-issues-link" className="table-issue-more-issues" target="_blank" href={url} onClick={logClick}>
          View all {count} issues
-        </a> 
-        | 
-        { this.renderReportIssueLink() } 
+        </a>
+        |
+        { this.renderReportIssueLink() }
       </span>
     );
   }
 
   renderReportIssueLink = () => {
     return (
-      <div className="table-report-new-issue"> 
+      <div className="table-report-new-issue">
         <ReportTableIssue tableKey={ this.props.tableKey } tableName={ this.props.tableName }/>
       </div>
-    ); 
+    );
   }
-  
+
   renderIssueTitle = () => {
     return (
       <div className="section-title title-3">
@@ -89,6 +91,17 @@ export class TableIssues extends React.Component<TableIssueProps> {
   }
 
   render() {
+    if (this.props.isLoading) {
+      return (
+        <div>
+          {this.renderIssueTitle()}
+          <div className="table-issues">
+            <LoadingSpinner />
+          </div>
+        </div>
+      )
+    }
+
     if (this.props.issues.length === 0) {
       return (
         <div>
@@ -117,9 +130,10 @@ export class TableIssues extends React.Component<TableIssueProps> {
 
 export const mapStateToProps = (state: GlobalState) => {
   return {
-    issues: state.issue.issues, 
-    total: state.issue.total, 
-    allIssuesUrl: state.issue.allIssuesUrl
+    issues: state.issue.issues,
+    total: state.issue.total,
+    allIssuesUrl: state.issue.allIssuesUrl,
+    isLoading: state.issue.isLoading,
   };
 };
 

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

@@ -8,11 +8,11 @@
     height: 40px;
     margin: 8px 0;
     padding: 8px 16px;
-    display: flex; 
+    display: flex;
     flex-direction: row;
   }
   .table-issue-link {
-    margin-right: $spacer-1; 
+    margin-right: $spacer-1;
     min-width: fit-content;
   }
   .issue-title-name {
@@ -33,15 +33,15 @@
     margin-left: $spacer-2;
   }
   .table-issue-priority {
-    margin-right: $spacer-1; 
-    border: 1px solid $rose80; 
+    margin-right: $spacer-1;
+    border: 1px solid $rose80;
     border-radius: 5px;
     padding: 1px 2px;
-    flex: none; 
+    flex: none;
   }
   .blocker {
-    color: $priority-text-blocker; 
-    // default is $rose80 
+    color: $priority-text-blocker;
+    // default is $rose80
     background-color: rgba($priority-bg-color, 1);
   }
   .critical {
@@ -53,16 +53,19 @@
   .minor {
     background-color: rgba($priority-bg-color, .1);
   }
+  .loading-spinner {
+    margin-top: auto;
+  }
 }
 .table-issue-more-issues {
-  margin-bottom: $spacer-1; 
-  margin-right: $spacer-1; 
+  margin-bottom: $spacer-1;
+  margin-right: $spacer-1;
 }
 .table-more-issues {
-  display: flex; 
-  font-size: 16px; 
+  display: flex;
+  font-size: 16px;
 }
 .table-report-new-issue {
-  margin-bottom: $spacer-1; 
+  margin-bottom: $spacer-1;
   margin-left: $spacer-1;
 }

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

@@ -5,10 +5,10 @@ import { shallow } from 'enzyme';
 import AppConfig from 'config/config';
 import globalState from 'fixtures/globalState';
 
-import { 
-  TableIssues, 
-  TableIssueProps, 
-  mapStateToProps, 
+import {
+  TableIssues,
+  TableIssueProps,
+  mapStateToProps,
   mapDispatchToProps
 } from '..';
 
@@ -18,85 +18,91 @@ import ReportTableIssue from 'components/TableDetail/ReportTableIssue';
 
 describe ('TableIssues', ()=> {
   const setStateSpy = jest.spyOn(TableIssues.prototype, 'setState');
-  
+
   const setup = (propOverrides?: Partial<TableIssueProps>) => {
     const props: TableIssueProps = {
-      issues: [], 
+      isLoading: false,
+      issues: [],
       tableKey: 'key',
       tableName: 'tableName',
-      total: 0, 
-      allIssuesUrl: 'testUrl', 
+      total: 0,
+      allIssuesUrl: 'testUrl',
       getIssues: jest.fn(),
       ...propOverrides
     };
     const wrapper = shallow<TableIssues>(<TableIssues {...props} />);
-    return { props, wrapper }; 
+    return { props, wrapper };
   }
 
   describe('render', () => {
     beforeAll(() => {
       AppConfig.issueTracking.enabled = true;
-    }); 
+    });
+
+    it('renders LoadingSpinner if loading', () => {
+      const { props, wrapper } = setup({ isLoading: true });
+      expect(wrapper.find('LoadingSpinner').exists()).toBe(true);
+    });
 
     it('renders text if no issues', () => {
       const { props, wrapper } = setup({ issues: [] });
       expect(wrapper.find('.issue-banner').text()).toEqual(NO_DATA_ISSUES_TEXT);
-    }); 
+    });
 
     it('renders issues if they exist', () => {
       AppConfig.issueTracking.enabled = true;
       const { props, wrapper } = setup({ issues: [{
-        issue_key: 'issue_key', 
+        issue_key: 'issue_key',
         title: 'title',
-        url: 'http://url', 
-        status: 'Open', 
-        priority_display_name: 'P2', 
+        url: 'http://url',
+        status: 'Open',
+        priority_display_name: 'P2',
         priority_name: 'major'
-      }]}); 
-      expect(wrapper.find('.table-issue-link').text()).toEqual('issue_key'); 
+      }]});
+      expect(wrapper.find('.table-issue-link').text()).toEqual('issue_key');
       expect(wrapper.find('.issue-title-name').text()).toContain('title');
       expect(wrapper.find('.table-issue-status').text()).toContain('Open');
       expect(wrapper.find('.table-issue-priority').text()).toContain('P2');
-    }); 
+    });
 
     it('renders no link to issues if no issues', () => {
       const { props, wrapper } = setup({ issues: [],
-        total: 0, 
+        total: 0,
         allIssuesUrl: null
       });
-      expect(wrapper.find('.table-issue-more-issues').length).toEqual(0); 
-    }); 
+      expect(wrapper.find('.table-issue-more-issues').length).toEqual(0);
+    });
 
     it('renders link if there are issues', () => {
       const { props, wrapper } = setup({ issues: [{
-          issue_key: 'issue_key', 
+          issue_key: 'issue_key',
           title: 'title',
           url: 'http://url',
-          status: 'Open', 
-          priority_display_name: 'P2', 
+          status: 'Open',
+          priority_display_name: 'P2',
           priority_name: 'Major'
         }],
-        total: 1, 
+        total: 1,
         allIssuesUrl: 'url'
       });
-      expect(wrapper.find('.table-issue-more-issues').text()).toEqual('View all 1 issues'); 
-    }); 
+      expect(wrapper.find('.table-issue-more-issues').text()).toEqual('View all 1 issues');
+    });
   });
 
   describe('mapDispatchToProps', () => {
     let dispatch;
     let props;
-  
+
     beforeAll(() => {
       dispatch = jest.fn(() => Promise.resolve());
       props = mapDispatchToProps(dispatch);
     });
-  
+
     it('sets getIssues on the props', () => {
       expect(props.getIssues).toBeInstanceOf(Function);
     });
   });
-  
+
   describe('mapStateToProps', () => {
     let result;
     beforeAll(() => {
@@ -104,7 +110,11 @@ describe ('TableIssues', ()=> {
     });
 
     it('sets issues on the props', () => {
-      expect(result.issues).toEqual(globalState.issue.issues); 
-    }); 
-  }); 
-}); 
+      expect(result.issues).toEqual(globalState.issue.issues);
+    });
+
+    it('sets isLoading on the props', () => {
+      expect(result.isLoading).toEqual(globalState.issue.isLoading);
+    });
+  });
+});

tests/unit/issue_tracker_clients/test_jira_client.py

@@ -3,7 +3,7 @@ from unittest.mock import Mock
 import flask
 import unittest
 from amundsen_application.proxy.issue_tracker_clients.issue_exceptions import IssueConfigurationException
-from amundsen_application.proxy.issue_tracker_clients.jira_client import JiraClient
+from amundsen_application.proxy.issue_tracker_clients.jira_client import JiraClient, SEARCH_STUB_ALL_ISSUES
 from amundsen_application.models.data_issue import DataIssue
 from jira import JIRAError
 from typing import Dict, List
@@ -101,7 +101,7 @@ class JiraClientTest(unittest.TestCase):
             self.assertEqual(results.issues[0], self.mock_issue)
             self.assertEqual(results.total, self.mock_jira_issues.total)
             mock_JIRA_client.return_value.search_issues.assert_called_with(
-                'text ~ "key" order by createdDate DESC',
+                SEARCH_STUB_ALL_ISSUES.format(table_key="key"),
                 maxResults=3)
 
     @unittest.mock.patch('amundsen_application.proxy.issue_tracker_clients.jira_client.JIRA')