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.

Sign up for GitHub

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

Support for automation surface and action settings #1160

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

Support for automation surface and action settings #1160

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
06af2d4
Added support for automation surface.
brunschgi Nov 20, 2024
380e2d7
Add support for automation surface.
brunschgi Nov 28, 2024
5d9617f
Add changeset for the support of the automation surface and action se…
brunschgi Dec 2, 2024
13fd202
Fix manifest validation for workflow ids
brunschgi Dec 6, 2024
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
13 changes: 13 additions & 0 deletions .changeset/nice-cups-sparkle.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
---
"@frontify/platform-app": minor
"@frontify/frontify-cli": patch
---

**Manifest validation**
With the additional validation rules, we make sure that the automation surface definition adheres to the schema.

**Support for automation action settings**
The settings for automation actions can now be defined – in the exact same way as the default settings – via the `settings` property in the `defineApp` method.
This approach is very flexible, allowing to add further settings (e.g. for project, libraries or brand-level settings) easily without the need to adjust the bundler or add additional parameters to the `defineApp` method.
This makes it easier to evolve app settings in the future,

106 changes: 106 additions & 0 deletions packages/cli/src/utils/verifyManifest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,108 @@ const endpointCallSchema = object({
options: requestOptionsSchema,
});

const variableTypes = z.enum([
'STRING',
'NUMBER',
'URL',
'EMAIL',
'BOOLEAN',
'DATE',
'OPTIONS',
'USER',
'CUSTOM_METADATA_PROPERTY',
'WORKFLOW_TASK_STATUS',
]);
const variablesSchema = object({
key: string()
.min(1)
.max(80)
.refine(
(key) => {
return /^[.A-Z_a-z]*$/.test(key);
},
{
message: "Variable key must only contain letters from a-z, A-Z, '.' and '_' without any spaces",
},
),
name: string().min(1).max(80),
type: variableTypes,
});

const actionIdSet = new Set();
const automationActionSchema = object({
id: string()
.min(1)
.max(80)
.refine(
(id) => {
return /^[A-Z_a-z]*$/.test(id);
},
{
message: "Automation action id must only contain letters from a-z, A-Z, and '_' without any spaces",
},
)
.refine(
(id) => {
if (actionIdSet.has(id)) {
return false;
}

actionIdSet.add(id);
return true;
},
{
message: 'Automation action id must be unique',
},
),
name: string().min(1).max(80),
workflowId: string()
.min(16)
.max(16)
.refine(
(workflowId) => {
return /^\w+$/.test(workflowId);
},
{
message:
"Workflow Id must be unique and should only contain letters from a-z, A-Z, numbers from 0-9 and '_' without any spaces",
},
),
description: string().optional(),
variables: array(variablesSchema),
});

const triggerIdSet = new Set();
const automationTriggerSchema = object({
id: string()
.min(1)
.max(80)
.refine(
(id) => {
return /^[A-Z_a-z]*$/.test(id);
},
{
message: "Automation trigger id must only contain letters from a-z, A-Z, and '_' without any spaces",
},
)
.refine(
(id) => {
if (triggerIdSet.has(id)) {
return false;
}

triggerIdSet.add(id);
return true;
},
{
message: 'Automation trigger id must be unique',
},
),
name: string().min(1).max(80),
description: string().optional(),
variables: array(variablesSchema),
});

const hostnameRegex =
/^(([\dA-Za-z]|[\dA-Za-z][\dA-Za-z-]*[\dA-Za-z])\.)*([\dA-Za-z]|[\dA-Za-z][\dA-Za-z-]*[\dA-Za-z])$/;

Expand Down Expand Up @@ -198,6 +300,10 @@ export const platformAppManifestSchemaV1 = object({
}).optional(),
assetCreation: assetCreationShape,
}).optional(),
automation: object({
actions: array(automationActionSchema).optional(),
triggers: array(automationTriggerSchema).optional(),
}).optional(),
}).optional(),
metadata: object({
version: number().int(),
Expand Down
94 changes: 94 additions & 0 deletions packages/cli/tests/utils/verifyManifest.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,42 @@ const VALID_MANIFEST = {
title: 'action title',
},
},
automation: {
actions: [
{
id: 'setAssetTitle',
name: 'Set asset title',
variables: [
{
key: 'asset.title',
name: 'Asset title',
type: 'STRING',
},
],
workflowId: '7DQ92y5AldGBwZ3',
description: 'Changes the title of an asset',
},
],
triggers: [
{
id: 'assetsCreated',
name: 'Assets created',
variables: [
{
key: 'asset.id',
name: 'Asset Id',
type: 'NUMBER',
},
{
key: 'asset.title',
name: 'Asset title',
type: 'STRING',
},
],
description: 'Triggered when assets are created',
},
],
},
},
metadata: {
version: 1,
Expand Down Expand Up @@ -275,6 +311,56 @@ const MANIFEST_WITH_SECRET_BUT_NO_LABEL = {
},
};

const MANIFEST_WITH_DUPLICATE_ACTION_IDS = {
appType: 'platform-app',
appId: 'abcdabcdabcdabcdabcdabcda',
surfaces: {
automation: {
actions: [
{
id: 'setAssetTitle',
name: 'Set asset title',
workflowId: '7DQ92y5AldGBwZ3',
},
{
id: 'setAssetTitle',
name: 'Set asset title duplicate',
workflowId: '7DQ92y5AldGBwZ3',
},
],
},
},
metadata: {
version: 1,
},
};

const MANIFEST_WITH_INVALID_VARIABLE_TYPE = {
appType: 'platform-app',
appId: 'abcdabcdabcdabcdabcdabcda',
surfaces: {
automation: {
actions: [
{
id: 'setAssetTitle',
name: 'Set asset title',
workflowId: '7DQ92y5AldGBwZ3',
variables: [
{
key: 'asset.title',
name: 'Asset title',
type: 'VARCHAR',
},
],
},
],
},
},
metadata: {
version: 1,
},
};

const generateManifestWithEndpointNetworkCall = (networkEndpoints) => {
return {
appType: 'platform-app',
Expand Down Expand Up @@ -870,4 +956,12 @@ describe('Verify Platform App Manifest', () => {
verifyManifest(INVALID_MANIFEST_NETWORK_ALLOWED_HOST_UNDESCORE, platformAppManifestSchemaV1),
).toThrow();
});

it('should detect when automation action ids are not unique', () => {
expect(() => verifyManifest(MANIFEST_WITH_DUPLICATE_ACTION_IDS, platformAppManifestSchemaV1)).toThrow('Automation action id must be unique');
});

it('should detect when action variables have invalid types ', () => {
expect(() => verifyManifest(MANIFEST_WITH_INVALID_VARIABLE_TYPE, platformAppManifestSchemaV1)).toThrow('VARCHAR');
});
});
10 changes: 9 additions & 1 deletion packages/platform-app/src/platformApps.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,18 @@ export type PlatformAppSimpleBlock =
export type PlatformAppSettings = PlatformAppSimpleBlock | DynamicSettingBlock<AppBridgePlatformApp>;

export type PlatformAppSettingsStructureExport = { [customSectionName: string]: PlatformAppSettings[] };
export type PlatformAppMultiSettingsStructureExport = {
default: PlatformAppSettingsStructureExport;
automation?: {
actions?: {
[customActionId: string]: PlatformAppSettingsStructureExport;
};
};
};

export type PlatformAppConfigExport = {
app: FC;
settings: PlatformAppSettingsStructureExport;
settings: PlatformAppMultiSettingsStructureExport | PlatformAppSettingsStructureExport;
Comment on lines +47 to +58
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like a lot of automation specific logic, how about just doing a new Type: "automation"? was there any discussion for that? Then we wouldnt need to adjust the settings structure nor adjust the web-app implementation and could use the default settings in it. Wouldnt the only downside be that then someone would need to have 2 Apps for 2 different functionalities?

Copy link
Author

Choose a reason for hiding this comment

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

Hej @julianiff

We discussed many approaches to this. The main 3 where:

Add settings path to the action definition in the manifest

  • Pro: Everything in one place, no duplicated automation definition (manifest + code)
  • Con: Separate, diverging approach from current settings solution (Themes + Apps) -> leads to bad DX

Add new property "automation" to defineApp method

  • Pro: No need to touch existing usage of settings in web-app
  • Con: Adjustment to bundler needed -> probably resulting in multiple settings bundles
  • Con: We would need to extend the defineApp method with every new settings on the horizon -> as we already know, that we will be in need of different settings for apps in multiple places (e.g. project level etc.), this would lead to more complexity in the Brand SDK and the docs.

Extend the settings structure to support multiple PlatformAppSettingsStructureExport
This was the preferred approach in the end and the one we implemented.

  • Pro: No changes to DX
  • Pro: Future-proof solution, allowing to add further settings (e.g. for project, libraries or brand-level settings) easily without the need to adjust the bundler or add additional parameters to the defineApp method
  • Pro: Shifts the complexity from Brand SDK to the web-app -> preferred by Samuel
  • Con: require changes in the web-app in order to work and provide the right default settings (e.g. in the marketplace).

Moreover, we also discussed the new Type "Automation" quite a few times. While it might seem tempting in the first place (from a code perspective), the downsides are pretty big:

  1. First and foremost: We see tons of future use cases, where functionality in other surfaces is dependent on data provided by an automation (e.g. Visualize extracted data, trigger automation from a surface, especially to process existing assets etc.). Having them splitted in 2 apps makes app development and maintenance for the app builder a nightmare (version upgrades etc.)
  2. Having all functionality for a specifc service bundled in one app (e.g. Everything Slack = Slack App), makes it much easier for App Admins to keep the overview as the Marketplace is much less cluttered
  3. Introducing new Types for every bigger feature would come with repeated adjustments to the Marketplace, making it even more cluttered
  4. The "one app to rule it all" approach comes with many benefits from a DX perspective, as additional functionality can easily be added on every release, using the same app.

Hope this makes our decision more understandable.

Copy link
Contributor

@julianiff julianiff Dec 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

That makes a lot of sense for sure, thanks for going into details about it.

My follow up question is, how would you write documentation for the automations and would you make the distinctions between what an app can do and what an automation can do?

Copy link
Contributor

@julianiff julianiff Dec 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Regarding the downsides:
1: If i understand it correctly, what is behind an Automation action is actions that correspond to the IDs in another system and not inside of an App. To enable a new action doesn't depend on the App, but on the third party system. When we then would like to use the output of an automation in an App, we need to define somehow code that we can run in isolation somewhere. This code will not be the same as the visual Code that is used in an App. This would then create another addition to the App structure. If we would add a new type, we could use the settings for settings, and the src folder later on for executable code that we want to run somewhere.

2 & 3 Im not sure about this, we are aiming to have the Marketplace as a lively place, to have more Apps in there sounds like the way to go.

4: Downside of one app to rule all is that for a developer its not clear what he has to do. We mix different concepts and introduce implicit rules on what goes together and what not. For example, when you build an automation you cannot use methods, or when building an App you shouldn't use the automation part in the mainfest.json because thats for the automation usecase. It makes the communication more difficult because we mix different concepts.

};

/**
Expand Down
Loading