From 439f1da4bc007255ef5688688b9f3b84047a3046 Mon Sep 17 00:00:00 2001 From: Thenkei Date: Fri, 6 Oct 2023 12:17:45 +0200 Subject: [PATCH 1/4] feat(approval): allow developers to request approval from the code --- packages/agent/src/routes/modification/action/action.ts | 7 +++++++ .../src/decorators/actions/result-builder.ts | 9 +++++++++ packages/datasource-toolkit/src/interfaces/action.ts | 7 ++++++- 3 files changed, 22 insertions(+), 1 deletion(-) diff --git a/packages/agent/src/routes/modification/action/action.ts b/packages/agent/src/routes/modification/action/action.ts index c28a1af9f6..c38076bf59 100644 --- a/packages/agent/src/routes/modification/action/action.ts +++ b/packages/agent/src/routes/modification/action/action.ts @@ -24,6 +24,7 @@ import SchemaGeneratorActions from '../../../utils/forest-schema/generator-actio import IdUtils from '../../../utils/id'; import QueryStringParser from '../../../utils/query-string'; import CollectionRoute from '../../collection-route'; +import CustomActionRequiresApprovalError from './errors/custom-action-requires-approval-error'; export default class ActionRoute extends CollectionRoute { private readonly actionName: string; @@ -123,7 +124,13 @@ export default class ActionRoute extends CollectionRoute { context.response.set('Access-Control-Expose-Headers', 'Content-Disposition'); context.response.type = result.mimeType; context.response.body = result.stream; + + } else if (result.type === 'RequestApproval') { + // Later on we could create an interface to let user put the role allowed + // For now we put null and the backend we be able to handle it + throw new CustomActionRequiresApprovalError(undefined); } else { + throw new Error('Unexpected Action result.'); } } diff --git a/packages/datasource-customizer/src/decorators/actions/result-builder.ts b/packages/datasource-customizer/src/decorators/actions/result-builder.ts index 3d28cbd467..38fc28c871 100644 --- a/packages/datasource-customizer/src/decorators/actions/result-builder.ts +++ b/packages/datasource-customizer/src/decorators/actions/result-builder.ts @@ -90,4 +90,13 @@ export default class ResultBuilder { redirectTo(path: string): ActionResult { return { type: 'Redirect', path }; } + + /** + * Request an approval from the code + * @example + * .requestApproval(); + */ + requestApproval(): ActionResult { + return { type: 'RequestApproval' }; + } } diff --git a/packages/datasource-toolkit/src/interfaces/action.ts b/packages/datasource-toolkit/src/interfaces/action.ts index 7c6adf4a1d..faefe7d9ae 100644 --- a/packages/datasource-toolkit/src/interfaces/action.ts +++ b/packages/datasource-toolkit/src/interfaces/action.ts @@ -283,9 +283,14 @@ export type RedirectResult = { path: string; }; +export type RequestApprovalResult = { + type: 'RequestApproval'; +}; + export type ActionResult = | SuccessResult | ErrorResult | WebHookResult | FileResult - | RedirectResult; + | RedirectResult + | RequestApprovalResult; From 795df924be7a41e231f5de131adc14b66bea4e58 Mon Sep 17 00:00:00 2001 From: Thenkei Date: Fri, 6 Oct 2023 14:54:25 +0200 Subject: [PATCH 2/4] test: add some tests --- .../src/routes/modification/action/action.ts | 8 ++---- .../routes/modification/action/action.test.ts | 27 +++++++++++++++++++ .../src/interfaces/action.ts | 3 +++ 3 files changed, 32 insertions(+), 6 deletions(-) diff --git a/packages/agent/src/routes/modification/action/action.ts b/packages/agent/src/routes/modification/action/action.ts index c38076bf59..ba9bba9474 100644 --- a/packages/agent/src/routes/modification/action/action.ts +++ b/packages/agent/src/routes/modification/action/action.ts @@ -10,6 +10,7 @@ import Router from '@koa/router'; import { Context, Next } from 'koa'; import ActionAuthorizationService from './action-authorization'; +import CustomActionRequiresApprovalError from './errors/custom-action-requires-approval-error'; import { ForestAdminHttpDriverServices } from '../../../services'; import { SmartActionApprovalRequestBody, @@ -24,7 +25,6 @@ import SchemaGeneratorActions from '../../../utils/forest-schema/generator-actio import IdUtils from '../../../utils/id'; import QueryStringParser from '../../../utils/query-string'; import CollectionRoute from '../../collection-route'; -import CustomActionRequiresApprovalError from './errors/custom-action-requires-approval-error'; export default class ActionRoute extends CollectionRoute { private readonly actionName: string; @@ -124,13 +124,9 @@ export default class ActionRoute extends CollectionRoute { context.response.set('Access-Control-Expose-Headers', 'Content-Disposition'); context.response.type = result.mimeType; context.response.body = result.stream; - } else if (result.type === 'RequestApproval') { - // Later on we could create an interface to let user put the role allowed - // For now we put null and the backend we be able to handle it - throw new CustomActionRequiresApprovalError(undefined); + throw new CustomActionRequiresApprovalError(result.approverRoles); } else { - throw new Error('Unexpected Action result.'); } } diff --git a/packages/agent/test/routes/modification/action/action.test.ts b/packages/agent/test/routes/modification/action/action.test.ts index a70b2d8520..f5f811d633 100644 --- a/packages/agent/test/routes/modification/action/action.test.ts +++ b/packages/agent/test/routes/modification/action/action.test.ts @@ -9,6 +9,7 @@ import { createMockContext } from '@shopify/jest-koa-mocks'; import { Readable } from 'stream'; import ActionRoute from '../../../../src/routes/modification/action/action'; +import CustomActionRequiresApprovalError from '../../../../src/routes/modification/action/errors/custom-action-requires-approval-error'; import CustomActionTriggerForbiddenError from '../../../../src/routes/modification/action/errors/custom-action-trigger-forbidden-error'; import * as factories from '../../../__factories__'; @@ -408,6 +409,32 @@ describe('ActionRoute', () => { }, ); + test('handleExecute should handle the response (RequestApproval)', async () => { + // Mock action + (dataSource.getCollection('books').execute as jest.Mock).mockResolvedValue({ + type: 'RequestApproval', + approverRoles: [1, 2], + }); + + // Test + const context = createMockContext({ + ...baseContext, + requestBody: { + data: { + attributes: { + ...baseContext.requestBody.data.attributes, + values: { firstname: 'John' }, + }, + }, + }, + }); + + // @ts-expect-error: test private method + await expect(route.handleExecute(context)).rejects.toThrow( + new CustomActionRequiresApprovalError([1, 2]), + ); + }); + test('handleExecute should format the response (File)', async () => { // Mock action const stream = Readable.from(['header1,header2mcontent1,content2']); diff --git a/packages/datasource-toolkit/src/interfaces/action.ts b/packages/datasource-toolkit/src/interfaces/action.ts index faefe7d9ae..6e48d55913 100644 --- a/packages/datasource-toolkit/src/interfaces/action.ts +++ b/packages/datasource-toolkit/src/interfaces/action.ts @@ -285,6 +285,9 @@ export type RedirectResult = { export type RequestApprovalResult = { type: 'RequestApproval'; + // Later on we could create an interface to let user put the role allowed + // For now we're not defining it and the backend we be able to handle it + approverRoles?: number[]; }; export type ActionResult = From 0e8834dde65e29c3e84d1a89eb682aadf7f176fc Mon Sep 17 00:00:00 2001 From: Thenkei Date: Fri, 6 Oct 2023 15:07:56 +0200 Subject: [PATCH 3/4] fix: add an example and a test --- packages/_example/src/forest/customizations/dvd.ts | 4 ++++ .../test/decorators/actions/result-builder.test.ts | 6 ++++++ 2 files changed, 10 insertions(+) diff --git a/packages/_example/src/forest/customizations/dvd.ts b/packages/_example/src/forest/customizations/dvd.ts index 7edd243de9..3e764047cc 100644 --- a/packages/_example/src/forest/customizations/dvd.ts +++ b/packages/_example/src/forest/customizations/dvd.ts @@ -28,6 +28,10 @@ export default (collection: DvdCustomizer) => ids: await context.getRecordIds(), }; + // Price increase above 50% needs to be validated by a manager ! + // This feature needs a Paying Plan. + if (context.formValues.percentage > 50) return resultBuilder.requestApproval(); + await sequelizeMsSql.query( 'UPDATE dvd SET rental_price = ROUND(rental_price * :multiplier, 2) WHERE id IN (:ids)', { replacements }, diff --git a/packages/datasource-customizer/test/decorators/actions/result-builder.test.ts b/packages/datasource-customizer/test/decorators/actions/result-builder.test.ts index 881d37baef..aafb844047 100644 --- a/packages/datasource-customizer/test/decorators/actions/result-builder.test.ts +++ b/packages/datasource-customizer/test/decorators/actions/result-builder.test.ts @@ -56,4 +56,10 @@ describe('ResultBuilder', () => { body: {}, }); }); + + test('requestApproval', () => { + expect(builder.requestApproval()).toEqual({ + type: 'RequestApproval', + }); + }); }); From ba3e3936b99bd820f1972bf9e23e94e09357d2bb Mon Sep 17 00:00:00 2001 From: Thenkei Date: Fri, 20 Oct 2023 11:18:08 +0200 Subject: [PATCH 4/4] refactor: rework the feature moved to context --- .../src/routes/modification/action/action.ts | 10 ++-- .../src/decorators/actions/collection.ts | 18 ++++++-- .../src/decorators/actions/context/base.ts | 26 ++++++++++- .../custom-action-requires-approval-error.ts | 9 ++++ .../src/decorators/actions/result-builder.ts | 9 ---- .../test/decorators/actions/context.test.ts | 46 +++++++++++++++++-- .../decorators/actions/result-builder.test.ts | 6 --- .../src/interfaces/action.ts | 10 +--- 8 files changed, 93 insertions(+), 41 deletions(-) create mode 100644 packages/datasource-customizer/src/decorators/actions/errors/custom-action-requires-approval-error.ts diff --git a/packages/agent/src/routes/modification/action/action.ts b/packages/agent/src/routes/modification/action/action.ts index ba9bba9474..9c691a878f 100644 --- a/packages/agent/src/routes/modification/action/action.ts +++ b/packages/agent/src/routes/modification/action/action.ts @@ -10,7 +10,6 @@ import Router from '@koa/router'; import { Context, Next } from 'koa'; import ActionAuthorizationService from './action-authorization'; -import CustomActionRequiresApprovalError from './errors/custom-action-requires-approval-error'; import { ForestAdminHttpDriverServices } from '../../../services'; import { SmartActionApprovalRequestBody, @@ -101,8 +100,11 @@ export default class ActionRoute extends CollectionRoute { filterForCaller, ); - // Now that we have the field list, we can parse the data again. - const data = ForestValueConverter.makeFormData(dataSource, rawData, fields); + const data = { + // Now that we have the field list, we can parse the data again. + fromData: ForestValueConverter.makeFormData(dataSource, rawData, fields), + isApproval: Boolean(requestBody?.data?.attributes?.requester_id), + }; const result = await this.collection.execute(caller, this.actionName, data, filterForCaller); if (result?.type === 'Error') { @@ -124,8 +126,6 @@ export default class ActionRoute extends CollectionRoute { context.response.set('Access-Control-Expose-Headers', 'Content-Disposition'); context.response.type = result.mimeType; context.response.body = result.stream; - } else if (result.type === 'RequestApproval') { - throw new CustomActionRequiresApprovalError(result.approverRoles); } else { throw new Error('Unexpected Action result.'); } diff --git a/packages/datasource-customizer/src/decorators/actions/collection.ts b/packages/datasource-customizer/src/decorators/actions/collection.ts index f04cbd13cf..4a645ada6a 100644 --- a/packages/datasource-customizer/src/decorators/actions/collection.ts +++ b/packages/datasource-customizer/src/decorators/actions/collection.ts @@ -36,7 +36,9 @@ export default class ActionCollectionDecorator extends CollectionDecorator { if (!action) return this.childCollection.execute(caller, name, data, filter); // eslint-disable-next-line @typescript-eslint/no-explicit-any - const context = this.getContext(caller, action, data, filter) as any; + const context = this.getContext(caller, action, data.fromData, filter, { + isApproval: data.isApproval, + }) as any; const resultBuilder = new ResultBuilder(); const result = await action.execute(context, resultBuilder); @@ -66,7 +68,10 @@ export default class ActionCollectionDecorator extends CollectionDecorator { const formValues = data ? { ...data } : {}; const used = new Set(); - const context = this.getContext(caller, action, formValues, filter, used, metas?.changedField); + const context = this.getContext(caller, action, formValues, filter, { + used, + changedField: metas?.changedField, + }); // Convert DynamicField to ActionField in successive steps. let dynamicFields: DynamicField[] = action.form.map(c => ({ ...c })); @@ -117,14 +122,17 @@ export default class ActionCollectionDecorator extends CollectionDecorator { action: ActionSingle | ActionBulk | ActionGlobal, formValues: RecordData, filter: Filter, - used?: Set, - changedField?: string, + options: { + used?: Set; + changedField?: string; + isApproval?: boolean; + } = {}, ): ActionContext { return new { Global: ActionContext, Bulk: ActionContext, Single: ActionContextSingle, - }[action.scope](this, caller, formValues, filter as unknown as PlainFilter, used, changedField); + }[action.scope](this, caller, formValues, filter as unknown as PlainFilter, options); } private async dropDefaults( diff --git a/packages/datasource-customizer/src/decorators/actions/context/base.ts b/packages/datasource-customizer/src/decorators/actions/context/base.ts index 38efa57ac1..4aa2c05fc2 100644 --- a/packages/datasource-customizer/src/decorators/actions/context/base.ts +++ b/packages/datasource-customizer/src/decorators/actions/context/base.ts @@ -12,6 +12,7 @@ import { import CollectionCustomizationContext from '../../../context/collection-context'; import { TCollectionName, TFieldName, TFilter, TRow, TSchema } from '../../../templates'; +import CustomActionRequiresApprovalError from '../errors/custom-action-requires-approval-error'; export default class ActionContext< S extends TSchema = TSchema, @@ -21,6 +22,7 @@ export default class ActionContext< readonly filter: TFilter; private _changedField: string; + private _isApproval: boolean; /** * @deprecated use `hasFieldChange` instead. [linked issue](https://github.com/ForestAdmin/agent-nodejs/issues/815). @@ -45,13 +47,21 @@ export default class ActionContext< caller: Caller, formValue: RecordData, filter: TFilter, - used?: Set, - changedField?: string, + { + used, + changedField, + isApproval, + }: { + used?: Set; + changedField?: string; + isApproval?: boolean; + }, ) { super(collection, caller); this.formValues = formValue; this.filter = filter; this._changedField = changedField; + this._isApproval = isApproval; this.reset(); // Spy on which formValues are accessed to set-up change hooks @@ -127,6 +137,18 @@ export default class ActionContext< return records.map(r => RecordUtils.getPrimaryKey(this.realCollection.schema, r)); } + /** + * Request an approval from the code + * @example + * .requestApproval(); + * + * It will only request approval on trigger not when approving the action. + */ + requestApproval(): void { + // Only requires approval in trigger context + if (!this._isApproval) throw new CustomActionRequiresApprovalError(); + } + private async runQuery(): Promise { const { queries, projection } = this; this.reset(); diff --git a/packages/datasource-customizer/src/decorators/actions/errors/custom-action-requires-approval-error.ts b/packages/datasource-customizer/src/decorators/actions/errors/custom-action-requires-approval-error.ts new file mode 100644 index 0000000000..8cfec76b00 --- /dev/null +++ b/packages/datasource-customizer/src/decorators/actions/errors/custom-action-requires-approval-error.ts @@ -0,0 +1,9 @@ +import { ForbiddenError } from '@forestadmin/datasource-toolkit'; + +export default class CustomActionRequiresApprovalError extends ForbiddenError { + constructor(roleIdsAllowedToApprove?: number[]) { + super('This action requires to be approved.', { + roleIdsAllowedToApprove, + }); + } +} diff --git a/packages/datasource-customizer/src/decorators/actions/result-builder.ts b/packages/datasource-customizer/src/decorators/actions/result-builder.ts index 38fc28c871..3d28cbd467 100644 --- a/packages/datasource-customizer/src/decorators/actions/result-builder.ts +++ b/packages/datasource-customizer/src/decorators/actions/result-builder.ts @@ -90,13 +90,4 @@ export default class ResultBuilder { redirectTo(path: string): ActionResult { return { type: 'Redirect', path }; } - - /** - * Request an approval from the code - * @example - * .requestApproval(); - */ - requestApproval(): ActionResult { - return { type: 'RequestApproval' }; - } } diff --git a/packages/datasource-customizer/test/decorators/actions/context.test.ts b/packages/datasource-customizer/test/decorators/actions/context.test.ts index fc8392e32e..b18a80a322 100644 --- a/packages/datasource-customizer/test/decorators/actions/context.test.ts +++ b/packages/datasource-customizer/test/decorators/actions/context.test.ts @@ -24,7 +24,7 @@ describe('ActionContext', () => { test('should factorize calls to list made at the same time', async () => { const caller = factories.caller.build(); - const context = new ActionContextSingle(books, caller, {}, {}); + const context = new ActionContextSingle(books, caller, {}, {}, {}); const [id, partial1, partial2, partial3] = await Promise.all([ context.getRecordId(), context.getRecord(['title']), @@ -47,7 +47,7 @@ describe('ActionContext', () => { factories.caller.build(), { title: 'Foundation' }, {}, - used, + { used }, ); expect(context.formValues.title).toEqual('Foundation'); @@ -63,7 +63,7 @@ describe('ActionContext', () => { factories.caller.build(), { title: 'Foundation' }, {}, - new Set(), + { used: new Set() }, ); expect(() => { @@ -72,7 +72,13 @@ describe('ActionContext', () => { }); test('should work in bulk mode', async () => { - const context = new ActionContext(books, factories.caller.build(), { title: 'Foundation' }, {}); + const context = new ActionContext( + books, + factories.caller.build(), + { title: 'Foundation' }, + {}, + {}, + ); const [ids, partials] = await Promise.all([ context.getRecordIds(), context.getRecords(['title']), @@ -86,11 +92,41 @@ describe('ActionContext', () => { test('getrecords should reject all promises if the query fails', async () => { (books.list as jest.Mock).mockRejectedValue(new Error('bad request')); - const context = new ActionContext(books, factories.caller.build(), { title: 'Foundation' }, {}); + const context = new ActionContext( + books, + factories.caller.build(), + { title: 'Foundation' }, + {}, + {}, + ); const promise1 = context.getRecords(['title']); const promise2 = context.getRecords(['id']); await expect(promise1).rejects.toThrow('bad request'); await expect(promise2).rejects.toThrow('bad request'); }); + + describe('requestApproval', () => { + test('should not throw when approving', () => { + const context = new ActionContext( + books, + factories.caller.build(), + { title: 'Foundation' }, + {}, + { isApproval: true }, + ); + expect(() => context.requestApproval()).not.toThrow(); + }); + + test('should only throw on trigger', () => { + const context = new ActionContext( + books, + factories.caller.build(), + { title: 'Foundation' }, + {}, + { isApproval: false }, + ); + expect(() => context.requestApproval()).toThrow(); + }); + }); }); diff --git a/packages/datasource-customizer/test/decorators/actions/result-builder.test.ts b/packages/datasource-customizer/test/decorators/actions/result-builder.test.ts index aafb844047..881d37baef 100644 --- a/packages/datasource-customizer/test/decorators/actions/result-builder.test.ts +++ b/packages/datasource-customizer/test/decorators/actions/result-builder.test.ts @@ -56,10 +56,4 @@ describe('ResultBuilder', () => { body: {}, }); }); - - test('requestApproval', () => { - expect(builder.requestApproval()).toEqual({ - type: 'RequestApproval', - }); - }); }); diff --git a/packages/datasource-toolkit/src/interfaces/action.ts b/packages/datasource-toolkit/src/interfaces/action.ts index 6e48d55913..7c6adf4a1d 100644 --- a/packages/datasource-toolkit/src/interfaces/action.ts +++ b/packages/datasource-toolkit/src/interfaces/action.ts @@ -283,17 +283,9 @@ export type RedirectResult = { path: string; }; -export type RequestApprovalResult = { - type: 'RequestApproval'; - // Later on we could create an interface to let user put the role allowed - // For now we're not defining it and the backend we be able to handle it - approverRoles?: number[]; -}; - export type ActionResult = | SuccessResult | ErrorResult | WebHookResult | FileResult - | RedirectResult - | RequestApprovalResult; + | RedirectResult;