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: validate actions configuration definition #782

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

realSpok
Copy link
Contributor

Definition of Done

General

  • Write an explicit title for the Pull Request, following Conventional Commits specification
  • Test manually the implemented changes
  • Validate the code quality (indentation, syntax, style, simplicity, readability)

Security

  • Consider the security impact of the changes made

@realSpok realSpok changed the base branch from main to alpha-widgets July 27, 2023 19:10
});

describe('documentation samples', () => {
test('it should validate the action at https://docs.forestadmin.com/developer-guide-agents-nodejs/agent-customization/actions#in-your-code', () => {
Copy link
Member

Choose a reason for hiding this comment

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

What if the documentation link change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is not the best.
But I thought that validating our own doc was important, so I took the risk. Do you have another idea ?

Copy link
Member

Choose a reason for hiding this comment

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

I think you can keep you links as comments of each test and specifically define what you are testing as I tried to do (it is not so simple) ^^

field: { label: 'field1', type: 'string' } as unknown as DynamicField,
error: `type must be equal to one of the allowed values ,(Boolean,Collection`,
},
].forEach(wrongField => {
Copy link
Member

Choose a reason for hiding this comment

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

💪

Comment on lines +3 to +9
export type ActionScope = `${ActionScopeEnum}`;

export enum ActionScopeEnum {
Single = 'Single',
Bulk = 'Bulk',
Global = 'Global',
}
Copy link
Member

Choose a reason for hiding this comment

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

👌

Comment on lines +24 to +39
export type ActionFieldType = `${ActionFieldTypeEnum}`;
export enum ActionFieldTypeEnum {
Boolean = 'Boolean',
Collection = 'Collection',
Enum = 'Enum',
EnumList = 'EnumList',
File = 'File',
FileList = 'FileList',
Json = 'Json',
Number = 'Number',
NumberList = 'NumberList',
Date = 'Date',
Dateonly = 'Dateonly',
String = 'String',
StringList = 'StringList',
}
Copy link
Member

Choose a reason for hiding this comment

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

👌

@Thenkei
Copy link
Member

Thenkei commented Jul 31, 2023

You should probably change the packages/forestadmin-client/src/schema/types.ts#ForestServerActionField too.

@Thenkei
Copy link
Member

Thenkei commented Jul 31, 2023

What about old widget definition?

widget: null | 'belongsto select' | 'file picker';

@realSpok realSpok closed this Aug 1, 2023
@realSpok realSpok reopened this Aug 1, 2023
@realSpok
Copy link
Contributor Author

realSpok commented Aug 1, 2023

What about old widget definition?

It will be updated in another PR

@@ -124,6 +129,14 @@ export default class CollectionCustomizer<
* })
*/
addAction(name: string, definition: ActionDefinition<S, N>): this {
try {
ActionValidator.validateActionConfiguration(name, definition as ActionDefinition<any, any>);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ActionValidator.validateActionConfiguration(name, definition as ActionDefinition<any, any>);
ActionValidator.validateActionConfiguration(name, definition as unknown as ActionDefinition);

The linter is not happy with this. 😢

});

describe('documentation samples', () => {
test('it should validate the action at https://docs.forestadmin.com/developer-guide-agents-nodejs/agent-customization/actions#in-your-code', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
test('it should validate the action at https://docs.forestadmin.com/developer-guide-agents-nodejs/agent-customization/actions#in-your-code', () => {
test('it should validate the action - smallest required properties defined', () => {

ActionValidator.validateActionConfiguration('Mark as live', action),
).not.toThrow();
});
test('it should validate the action at https://docs.forestadmin.com/developer-guide-agents-nodejs/agent-customization/actions/scope-context#example-1-getting-data-from-the-selected-records', () => {
Copy link
Member

Choose a reason for hiding this comment

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

This is the same case as the previous definition.

ActionValidator.validateActionConfiguration('Mark as live', action),
).not.toThrow();
});
test('it should validate the action at https://docs.forestadmin.com/developer-guide-agents-nodejs/agent-customization/actions/forms-static#field-configuration', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
test('it should validate the action at https://docs.forestadmin.com/developer-guide-agents-nodejs/agent-customization/actions/forms-static#field-configuration', () => {
test('it should validate the action - Static form definition', () => {

ActionValidator.validateActionConfiguration('Mark as live', action),
).not.toThrow();
});
test('it should validate the action at https://docs.forestadmin.com/developer-guide-agents-nodejs/agent-customization/actions/forms-static#references-to-records', () => {
Copy link
Member

@Thenkei Thenkei Aug 1, 2023

Choose a reason for hiding this comment

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

Suggested change
test('it should validate the action at https://docs.forestadmin.com/developer-guide-agents-nodejs/agent-customization/actions/forms-static#references-to-records', () => {
test('it should validate the action - - Static form definition with field reference to records', () => {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have implemented your suggestion

try {
ActionValidator.validateActionConfiguration(name, definition as ActionDefinition<any, any>);
} catch (error) {
if (error instanceof ActionConfigurationValidationError) {
Copy link
Member

Choose a reason for hiding this comment

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

👌

Base automatically changed from alpha-widgets to main August 28, 2023 08:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants