From 52ccc1aadca82879259cfb191837bbde09e9257d Mon Sep 17 00:00:00 2001 From: Joachim Dalen <15696189+joachimdalen@users.noreply.github.com> Date: Mon, 16 May 2022 20:18:10 +0200 Subject: [PATCH] Add processing comment (#21) --- .azext/changelog-cache.json | 14 ++ .azext/changelog.json | 10 +- CHANGELOG.md | 11 +- .../services/CriteriaHistoryService.test.ts | 172 ++++++++++++++++++ .../components/ProcessingContainer.test.tsx | 43 ++++- .../tabs/OrphanedCriteriaDetailsTab.tsx | 1 - src/common/services/CriteriaHistoryService.ts | 66 +++++-- src/common/services/CriteriaService.ts | 15 +- src/common/types.ts | 6 +- src/criteria-panel/CriteriaPanel.tsx | 11 +- src/criteria-panel/components/HistoryList.tsx | 2 +- .../components/ProcessingContainer.tsx | 62 ++++--- 12 files changed, 357 insertions(+), 56 deletions(-) create mode 100644 src/__tests__/common/services/CriteriaHistoryService.test.ts diff --git a/.azext/changelog-cache.json b/.azext/changelog-cache.json index 6d7c646..e422eb2 100644 --- a/.azext/changelog-cache.json +++ b/.azext/changelog-cache.json @@ -1,5 +1,12 @@ { "issues": [ + { + "id": 1168793478, + "number": 4, + "submitter": "joachimdalen", + "title": "Approval / Rejection comments", + "url": "https://github.com/joachimdalen/azdevops-acceptance-criterias/issues/4" + }, { "id": 1168792610, "number": 3, @@ -16,6 +23,13 @@ } ], "pullRequests": [ + { + "id": 936583972, + "number": 21, + "submitter": "joachimdalen", + "title": "Add processing comment", + "url": "https://github.com/joachimdalen/azdevops-acceptance-criterias/pull/21" + }, { "id": 930221978, "number": 19, diff --git a/.azext/changelog.json b/.azext/changelog.json index 70764cc..fc0d536 100644 --- a/.azext/changelog.json +++ b/.azext/changelog.json @@ -1,6 +1,6 @@ [ { - "publishDate": "2022-05-XX", + "publishDate": "2022-05-16", "version": "1.1.0", "changes": [ { @@ -38,10 +38,16 @@ "changes": [ { "type": "feature", - "description": "Added processing history", + "description": "Added processing history. See [history](https://devops-extensions.dev/docs/extensions/acceptance-criterias/processing/history)", "pullRequest": 19, "issue": 3 }, + { + "type": "feature", + "description": "Added processing comments. See [approvals and rejections](https://devops-extensions.dev/docs/extensions/acceptance-criterias/processing#approvals-and-rejections)", + "pullRequest": 21, + "issue": 4 + }, { "type": "fix", "description": "Fixed an issue where you could uncheck chacklist items while waiting for approval", diff --git a/CHANGELOG.md b/CHANGELOG.md index 262a0f9..1710cca 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,6 @@ # Changelog -## 1.1.0 (2022-05-XX) +## 1.1.0 (2022-05-16) ### 🐛 Fixes (1) @@ -32,14 +32,19 @@ - Remove unused code - Changed in [PR#19 - Refactor and introduce base history](https://github.com/joachimdalen/azdevops-acceptance-criterias/pull/19) -### 🚀 Features (1) +### 🚀 Features (2) #### `criteria-panel@1.1.0` -- Added processing history +- Added processing history. See [history](https://devops-extensions.dev/docs/extensions/acceptance-criterias/processing/history) + - Suggested in [GH#3 - Processing history](https://github.com/joachimdalen/azdevops-acceptance-criterias/issues/3) - Added in [PR#19 - Refactor and introduce base history](https://github.com/joachimdalen/azdevops-acceptance-criterias/pull/19) +- Added processing comments. See [approvals and rejections](https://devops-extensions.dev/docs/extensions/acceptance-criterias/processing#approvals-and-rejections) + - Suggested in [GH#4 - Approval / Rejection comments](https://github.com/joachimdalen/azdevops-acceptance-criterias/issues/4) + - Added in [PR#21 - Add processing comment](https://github.com/joachimdalen/azdevops-acceptance-criterias/pull/21) + ### 📝 Documentation (1) #### `admin-hub@1.0.1` diff --git a/src/__tests__/common/services/CriteriaHistoryService.test.ts b/src/__tests__/common/services/CriteriaHistoryService.test.ts new file mode 100644 index 0000000..f63295f --- /dev/null +++ b/src/__tests__/common/services/CriteriaHistoryService.test.ts @@ -0,0 +1,172 @@ +import { IInternalIdentity } from '@joachimdalen/azdevops-ext-core/CommonTypes'; + +import CriteriaHistoryService from '../../../common/services/CriteriaHistoryService'; +import { StorageService } from '../../../common/services/StorageService'; +import { HistoryDocument, HistoryEvent, ProcessEvent } from '../../../common/types'; + +const identity: IInternalIdentity = { + displayName: 'Test User', + entityId: '1234', + entityType: 'User', + id: '54321', + descriptor: 'user1234', + image: '/image.png' +}; + +const historyWithContent: HistoryDocument = { + __etag: 1, + id: 'AC-1-2', + items: [ + { + date: new Date(), + event: HistoryEvent.Completed, + actor: identity + } + ] +}; + +describe('CriteriaHistoryService', () => { + const getHistorySpy = jest.spyOn(StorageService.prototype, 'getHistory'); + const setHistorySpy = jest.spyOn(StorageService.prototype, 'setHistory'); + + beforeEach(() => { + jest.clearAllMocks(); + getHistorySpy.mockReset(); + }); + + describe('getProcessEvent', () => { + it('should return approved event without actor', () => { + const service = new CriteriaHistoryService(); + + const evnt = service.getProcessEvent(ProcessEvent.Approve); + expect(evnt.event).toEqual(HistoryEvent.Approved); + expect(evnt.actor).toBeUndefined(); + expect(evnt.properties).toBeUndefined(); + expect(evnt.date).not.toBeUndefined(); + }); + + it('should return rejected event without actor', () => { + const service = new CriteriaHistoryService(); + + const evnt = service.getProcessEvent(ProcessEvent.Reject); + expect(evnt.event).toEqual(HistoryEvent.Rejected); + expect(evnt.actor).toBeUndefined(); + expect(evnt.properties).toBeUndefined(); + expect(evnt.date).not.toBeUndefined(); + }); + + it('should return complete event without actor', () => { + const service = new CriteriaHistoryService(); + + const evnt = service.getProcessEvent(ProcessEvent.Complete); + expect(evnt.event).toEqual(HistoryEvent.Completed); + expect(evnt.actor).toBeUndefined(); + expect(evnt.properties).toBeUndefined(); + expect(evnt.date).not.toBeUndefined(); + }); + + it('should return reset-to-new event without actor', () => { + const service = new CriteriaHistoryService(); + + const evnt = service.getProcessEvent(ProcessEvent.ResetToNew); + expect(evnt.event).toEqual(HistoryEvent.ReOpened); + expect(evnt.actor).toBeUndefined(); + expect(evnt.properties).toBeUndefined(); + expect(evnt.date).not.toBeUndefined(); + }); + + it('should return resubmit-for-approval event without actor', () => { + const service = new CriteriaHistoryService(); + + const evnt = service.getProcessEvent(ProcessEvent.ResubmitForApproval); + expect(evnt.event).toEqual(HistoryEvent.ReApprove); + expect(evnt.actor).toBeUndefined(); + expect(evnt.properties).toBeUndefined(); + expect(evnt.date).not.toBeUndefined(); + }); + + it('should return event with actor', () => { + const service = new CriteriaHistoryService(); + + const evnt = service.getProcessEvent(ProcessEvent.Approve, identity); + expect(evnt.event).toEqual(HistoryEvent.Approved); + expect(evnt.actor).toEqual(identity); + expect(evnt.properties).toBeUndefined(); + expect(evnt.date).not.toBeUndefined(); + }); + it('should return event with actor and comment', () => { + const service = new CriteriaHistoryService(); + + const evnt = service.getProcessEvent(ProcessEvent.Approve, identity, 'Some comment'); + expect(evnt.event).toEqual(HistoryEvent.Approved); + expect(evnt.actor).toEqual(identity); + expect(evnt.properties).not.toBeUndefined(); + expect(evnt.properties?.comment).toEqual('Some comment'); + expect(evnt.date).not.toBeUndefined(); + }); + }); + + describe('getHistory', () => { + it('should return default when 404 is thrown', async () => { + getHistorySpy.mockRejectedValue({ status: 404 }); + const service = new CriteriaHistoryService(); + + const result = await service.getHistory('AC-1-1'); + + expect(result).not.toBeUndefined(); + expect(result.__etag).toEqual(-1); + expect(result.items.length).toEqual(0); + }); + + it('should return default when fetched is undefined', async () => { + getHistorySpy.mockResolvedValue(undefined); + const service = new CriteriaHistoryService(); + + const result = await service.getHistory('AC-1-1'); + + expect(result).not.toBeUndefined(); + expect(result.__etag).toEqual(-1); + expect(result.items.length).toEqual(0); + }); + + it('should return history', async () => { + getHistorySpy.mockResolvedValue(historyWithContent); + const service = new CriteriaHistoryService(); + + const result = await service.getHistory('AC-1-1'); + + expect(result).not.toBeUndefined(); + expect(result.__etag).toEqual(1); + expect(result.items.length).toEqual(1); + }); + + it('should throw when error is not 404', async () => { + getHistorySpy.mockRejectedValue({ status: 500 }); + const service = new CriteriaHistoryService(); + + expect(async () => await service.getHistory('AC-1-1')).rejects.toThrowError(); + }); + }); + + describe('createOrUpdate', () => { + it('should update item', async () => { + getHistorySpy.mockResolvedValue(historyWithContent); + setHistorySpy.mockImplementation(d => new Promise(resolve => resolve(d))); + + const service = new CriteriaHistoryService(); + + const result = await service.createOrUpdate('AC-1-1', { + date: new Date(), + event: HistoryEvent.Completed, + actor: identity, + properties: { + comment: 'Hello' + } + }); + + expect(result).not.toBeUndefined(); + expect(result.items.length).toEqual(2); + expect(result.items[0].event).toEqual(HistoryEvent.Completed); + }); + }); +}); diff --git a/src/__tests__/criteria-panel/components/ProcessingContainer.test.tsx b/src/__tests__/criteria-panel/components/ProcessingContainer.test.tsx index 011b44c..ab8be0c 100644 --- a/src/__tests__/criteria-panel/components/ProcessingContainer.test.tsx +++ b/src/__tests__/criteria-panel/components/ProcessingContainer.test.tsx @@ -15,8 +15,28 @@ describe('ProcessingContainer', () => { fireEvent.click(approve); fireEvent.click(save); - expect(process).toHaveBeenCalledWith('1234', ProcessEvent.Approve); + expect(process).toHaveBeenCalledWith('1234', ProcessEvent.Approve, undefined); }); + + it('should approve with comment when entered', async () => { + const process = jest.fn(); + render(); + + const approve = screen.getByRole('radio', { name: 'Approve' }); + + const comment = screen.getByPlaceholderText( + 'A short reason for rejecting or approving the criteria' + ); + fireEvent.change(comment, { target: { value: 'I approve this' } }); + + const save = screen.getByRole('button'); + + fireEvent.click(approve); + fireEvent.click(save); + + expect(process).toHaveBeenCalledWith('1234', ProcessEvent.Approve, 'I approve this'); + }); + it('should reject when selected', async () => { const process = jest.fn(); render(); @@ -28,6 +48,25 @@ describe('ProcessingContainer', () => { fireEvent.click(reject); fireEvent.click(save); - expect(process).toHaveBeenCalledWith('1234', ProcessEvent.Reject); + expect(process).toHaveBeenCalledWith('1234', ProcessEvent.Reject, undefined); + }); + + it('should reject with comment when entered', async () => { + const process = jest.fn(); + render(); + + const reject = screen.getByRole('radio', { name: 'Reject' }); + + const comment = screen.getByPlaceholderText( + 'A short reason for rejecting or approving the criteria' + ); + fireEvent.change(comment, { target: { value: 'I reject this' } }); + + const save = screen.getByRole('button'); + + fireEvent.click(reject); + fireEvent.click(save); + + expect(process).toHaveBeenCalledWith('1234', ProcessEvent.Reject, 'I reject this'); }); }); diff --git a/src/admin-hub/tabs/orphaned/tabs/OrphanedCriteriaDetailsTab.tsx b/src/admin-hub/tabs/orphaned/tabs/OrphanedCriteriaDetailsTab.tsx index ac23294..31487e3 100644 --- a/src/admin-hub/tabs/orphaned/tabs/OrphanedCriteriaDetailsTab.tsx +++ b/src/admin-hub/tabs/orphaned/tabs/OrphanedCriteriaDetailsTab.tsx @@ -100,7 +100,6 @@ const OrphanedCriteriaDetailsTab = (): React.ReactElement => { tableItem: OrphanedCriteriaDetail, ariaRowIndex?: number ) => { - console.log('rerender'); return ( {tableItem.type === 'Unknown' ? ( diff --git a/src/common/services/CriteriaHistoryService.ts b/src/common/services/CriteriaHistoryService.ts index 05eeb51..71cdf60 100644 --- a/src/common/services/CriteriaHistoryService.ts +++ b/src/common/services/CriteriaHistoryService.ts @@ -10,58 +10,92 @@ class CriteriaHistoryService { this._dataStore = dataStore || new StorageService(); } - public getProcessEvent(event: ProcessEvent, actor?: IInternalIdentity): HistoryItem { + public getProcessEvent( + event: ProcessEvent, + actor?: IInternalIdentity, + comment?: string + ): HistoryItem { switch (event) { case ProcessEvent.Approve: { - return this.getApprovedEvent(actor); + return this.getApprovedEvent(actor, comment); } case ProcessEvent.Reject: { - return this.getRejectedEvent(actor); + return this.getRejectedEvent(actor, comment); } case ProcessEvent.Complete: { - return this.getCompletedEvent(actor); + return this.getCompletedEvent(actor, comment); } case ProcessEvent.ResetToNew: { - return this.getReOpenEvent(actor); + return this.getReOpenEvent(actor, comment); } case ProcessEvent.ResubmitForApproval: { - return this.getReApproveEvent(actor); + return this.getReApproveEvent(actor, comment); } } } - public getApprovedEvent(actor?: IInternalIdentity): HistoryItem { + public getApprovedEvent(actor?: IInternalIdentity, comment?: string): HistoryItem { return { event: HistoryEvent.Approved, date: new Date(), - actor: actor + actor: actor, + properties: + comment !== undefined + ? { + comment + } + : undefined }; } - public getReApproveEvent(actor?: IInternalIdentity): HistoryItem { + public getReApproveEvent(actor?: IInternalIdentity, comment?: string): HistoryItem { return { event: HistoryEvent.ReApprove, date: new Date(), - actor: actor + actor: actor, + properties: + comment !== undefined + ? { + comment + } + : undefined }; } - public getRejectedEvent(actor?: IInternalIdentity): HistoryItem { + public getRejectedEvent(actor?: IInternalIdentity, comment?: string): HistoryItem { return { event: HistoryEvent.Rejected, date: new Date(), - actor: actor + actor: actor, + properties: + comment !== undefined + ? { + comment + } + : undefined }; } - public getCompletedEvent(actor?: IInternalIdentity): HistoryItem { + public getCompletedEvent(actor?: IInternalIdentity, comment?: string): HistoryItem { return { event: HistoryEvent.Completed, date: new Date(), - actor: actor + actor: actor, + properties: + comment !== undefined + ? { + comment + } + : undefined }; } - public getReOpenEvent(actor?: IInternalIdentity): HistoryItem { + public getReOpenEvent(actor?: IInternalIdentity, comment?: string): HistoryItem { return { event: HistoryEvent.ReOpened, date: new Date(), - actor: actor + actor: actor, + properties: + comment !== undefined + ? { + comment + } + : undefined }; } diff --git a/src/common/services/CriteriaService.ts b/src/common/services/CriteriaService.ts index d2418a6..9708231 100644 --- a/src/common/services/CriteriaService.ts +++ b/src/common/services/CriteriaService.ts @@ -1,7 +1,7 @@ import { DevOpsService } from '@joachimdalen/azdevops-ext-core/DevOpsService'; import { getLoggedInUser } from '@joachimdalen/azdevops-ext-core/IdentityUtils'; import { getClient } from 'azure-devops-extension-api'; -import { CoreRestClient, Process, WebApiTeam } from 'azure-devops-extension-api/Core'; +import { CoreRestClient, WebApiTeam } from 'azure-devops-extension-api/Core'; import { GraphMembership, GraphRestClient } from 'azure-devops-extension-api/Graph'; import { WorkItemQueryResult, @@ -16,7 +16,6 @@ import { CriteriaDocument, CriteriaPanelConfig, FullCriteriaStatus, - HistoryEvent, HistoryItem, IAcceptanceCriteria, ProcessEvent @@ -197,6 +196,7 @@ class CriteriaService { criteria.state = AcceptanceCriteriaState.New; } details.processed = undefined; + details.latestComment = undefined; } else { if (criteria.requiredApprover) { criteria.state = AcceptanceCriteriaState.AwaitingApproval; @@ -216,7 +216,8 @@ class CriteriaService { public async processCriteria( workItemId: string, id: string, - action: ProcessEvent + action: ProcessEvent, + comment?: string ): Promise<{ criteria: IAcceptanceCriteria; details: CriteriaDetailDocument } | undefined> { try { const doc = await this._dataStore.getCriteriasForWorkItem(workItemId); @@ -238,7 +239,7 @@ class CriteriaService { processedBy: approver }; } - + details.latestComment = comment; criteria.state = action === ProcessEvent.Approve ? AcceptanceCriteriaState.Approved @@ -246,7 +247,11 @@ class CriteriaService { const res = await this.update(doc, criteria, details); - const historyEvent: HistoryItem = this._historyService.getProcessEvent(action, approver); + const historyEvent: HistoryItem = this._historyService.getProcessEvent( + action, + approver, + comment + ); await this._historyService.createOrUpdate(id, historyEvent); this.emitChange(false, true); return res; diff --git a/src/common/types.ts b/src/common/types.ts index 25bd0ed..15358ee 100644 --- a/src/common/types.ts +++ b/src/common/types.ts @@ -39,6 +39,7 @@ export const criteriaIcons: Map = new Map< export interface CriteriaDetailDocument { id: string; + latestComment?: string; processed?: IAcceptanceCriteriaProcess; scenario?: IScenario; text?: ITextCriteria; @@ -165,7 +166,10 @@ export const historyEventProperties: Map = new Ma HistoryEvent.Completed, { icon: 'SkypeCircleCheck', iconColor: 'text-blue', title: 'Completed criteria' } ], - [HistoryEvent.ReOpened, { icon: 'CirclePlus', iconColor: 'text-blue', title: 'Reset back to new' }], + [ + HistoryEvent.ReOpened, + { icon: 'CirclePlus', iconColor: 'text-blue', title: 'Reset back to new' } + ], [ HistoryEvent.Approved, { icon: 'CheckMark', iconColor: 'text-green', title: 'Approved criteria' } diff --git a/src/criteria-panel/CriteriaPanel.tsx b/src/criteria-panel/CriteriaPanel.tsx index 459c679..2237a37 100644 --- a/src/criteria-panel/CriteriaPanel.tsx +++ b/src/criteria-panel/CriteriaPanel.tsx @@ -329,9 +329,9 @@ const CriteriaPanel = (): React.ReactElement => { }; }; - async function processCriteria(id: string, approve: ProcessEvent) { + async function processCriteria(id: string, approve: ProcessEvent, comment?: string) { if (workItemId && parseInt(workItemId) > 0) { - const result = await criteriaService.processCriteria(workItemId, id, approve); + const result = await criteriaService.processCriteria(workItemId, id, approve, comment); if (result !== undefined) { toggleWasChanged(true); setCriteriaInfo(result.criteria, result.details); @@ -552,6 +552,13 @@ const CriteriaPanel = (): React.ReactElement => { + + + {details?.latestComment} + + { {item?.properties?.comment && ( - + {item?.properties?.comment} )} diff --git a/src/criteria-panel/components/ProcessingContainer.tsx b/src/criteria-panel/components/ProcessingContainer.tsx index 4624e98..9a58c53 100644 --- a/src/criteria-panel/components/ProcessingContainer.tsx +++ b/src/criteria-panel/components/ProcessingContainer.tsx @@ -1,15 +1,17 @@ import { Button } from 'azure-devops-ui/Button'; +import { FormItem } from 'azure-devops-ui/FormItem'; import { RadioButton, RadioButtonGroup, RadioButtonGroupDirection } from 'azure-devops-ui/RadioButton'; +import { TextField, TextFieldWidth } from 'azure-devops-ui/TextField'; import { useState } from 'react'; import { ProcessEvent } from '../../common/types'; interface ProcessingContainerProps { criteriaId: string; - processCriteria: (id: string, action: ProcessEvent) => Promise; + processCriteria: (id: string, action: ProcessEvent, comment: string | undefined) => Promise; } const ProcessingContainer = ({ @@ -17,6 +19,7 @@ const ProcessingContainer = ({ processCriteria }: ProcessingContainerProps): JSX.Element => { const [selected, setSelected] = useState('dummy'); + const [comment, setComment] = useState(); return (

This criteria needs your attention

@@ -24,28 +27,41 @@ const ProcessingContainer = ({ This acceptance require you to process it. You can chose to Approve or{' '} Reject it.

-
- setSelected(selectedId)} - selectedButtonId={selected} - direction={RadioButtonGroupDirection.Horizontal} - className="margin-right-16" - > - - - -
);