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

feat(approval): allow developers to request approval from the code #846

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all 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
4 changes: 4 additions & 0 deletions packages/_example/src/forest/customizations/dvd.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Thenkei marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
if (context.formValues.percentage > 50) return resultBuilder.requestApproval();
if (context.formValues.percentage > 50) context.requestApproval();


await sequelizeMsSql.query(
'UPDATE dvd SET rental_price = ROUND(rental_price * :multiplier, 2) WHERE id IN (:ids)',
{ replacements },
Expand Down
7 changes: 5 additions & 2 deletions packages/agent/src/routes/modification/action/action.ts
Original file line number Diff line number Diff line change
Expand Up @@ -100,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') {
Expand Down
27 changes: 27 additions & 0 deletions packages/agent/test/routes/modification/action/action.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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__';

Expand Down Expand Up @@ -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']);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down Expand Up @@ -66,7 +68,10 @@ export default class ActionCollectionDecorator extends CollectionDecorator {

const formValues = data ? { ...data } : {};
const used = new Set<string>();
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 }));
Expand Down Expand Up @@ -117,14 +122,17 @@ export default class ActionCollectionDecorator extends CollectionDecorator {
action: ActionSingle | ActionBulk | ActionGlobal,
formValues: RecordData,
filter: Filter,
used?: Set<string>,
changedField?: string,
options: {
used?: Set<string>;
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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -21,6 +22,7 @@ export default class ActionContext<
readonly filter: TFilter<S, N>;

private _changedField: string;
private _isApproval: boolean;

/**
* @deprecated use `hasFieldChange` instead. [linked issue](https://github.com/ForestAdmin/agent-nodejs/issues/815).
Expand All @@ -45,13 +47,21 @@ export default class ActionContext<
caller: Caller,
formValue: RecordData,
filter: TFilter<S, N>,
used?: Set<string>,
changedField?: string,
{
used,
changedField,
isApproval,
}: {
used?: Set<string>;
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
Expand Down Expand Up @@ -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<void> {
const { queries, projection } = this;
this.reset();
Expand Down
Original file line number Diff line number Diff line change
@@ -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,
});
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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']),
Expand All @@ -47,7 +47,7 @@ describe('ActionContext', () => {
factories.caller.build(),
{ title: 'Foundation' },
{},
used,
{ used },
);

expect(context.formValues.title).toEqual('Foundation');
Expand All @@ -63,7 +63,7 @@ describe('ActionContext', () => {
factories.caller.build(),
{ title: 'Foundation' },
{},
new Set<string>(),
{ used: new Set<string>() },
);

expect(() => {
Expand All @@ -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']),
Expand All @@ -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();
});
});
});