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: add support for variables in filter values #4724

Conversation

ainouzgali
Copy link
Contributor

What change does this PR introduce?

When filtering a step, allow using payload/subscriber/actor.tenant values
for example:

Payload :
Key: data.customProp
Value: {{subscriber.data.someOtherValue}}

Subscriber:
Key: data.subscriberValue
Value: {{someProp.from.payload}}

Why was this change needed?

Other information (Screenshots)

Copy link

linear bot commented Oct 31, 2023

NV-3049 Allow using variables as values in the step filters

What?

When filtering a step, allow using payload/subscriber values for example:

  • Payload >
    • Key: data.customProp
    • Value: {{subscriber.data.someOtherValue}}
  • Subscriber >
    • Key: data.subscriberValue
    • Value: {{someProp.from.payload}}

Why? (Context)

In some cases, the filters would love to be applied based of some other stored dynamic variables instead of hardcoded ones in the system.

Customer Notes:

We discussed the ability to filter based on passed variables, both the value and the threshold, and to allow those to be passed both on the subscriber level as well as the workflow.

To explain, look at this example:

Send a notification based on a user-configured threshold, or filters, so with that will be sent on the subscriber level a property called 

```jsx
{
	"notification_threshold_amount": 50
}

And then on the filter config in our workflow to allow the use of the above property. Similar to:


### Definition of Done

* Users are able to specify variables similar to usage in the templates using the `{{payloadData}}` syntax
* Topic, subscriber and actor data should be allowed to used aswell
* User can specify a combined value: "Text and {{variable}}", the variable will just be replaced and then the filter evaluation be performed.
</p>
</details>

@@ -164,8 +177,7 @@ export class SendMessage {
isTest: false,
isRetry: false,
raw: JSON.stringify({
payload: shouldRun.variables,
filters: command.step.filters,
conditions: shouldRun.conditions,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So it presented with the filters ui

Comment on lines +164 to +165
step: command.step,
job: command.job,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unrelated to this PR. It was not filtering correctly a group filter. This fixes it.

Comment on lines +255 to +265
return {
subscriber,
payload: command.payload,
step: {
digest: !!command.events?.length,
events: command.events,
total_count: command.events?.length,
},
...(tenant && { tenant }),
...(actor && { actor }),
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is used in all send message usecases and now in the filters as well. Extracted this object so we fetch and build it once.

Comment on lines 423 to +429
if (child.on === FilterPartTypeEnum.WEBHOOK) {
if (process.env.NODE_ENV === 'test') return true;

child.value = await this.compileFilter(
child.value,
variables,
command.job
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should this be in the value of webhook? I am not 100% certain

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, the webhook URL could be also a dynamic value or have dynamic parameters :)

…-allow-using-variables-as-values-in-the-step-filters1
_templateId: template._id,
});

expect(messages).to.equal(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't we have for this case 0 messages?

Comment on lines 423 to +429
if (child.on === FilterPartTypeEnum.WEBHOOK) {
if (process.env.NODE_ENV === 'test') return true;

child.value = await this.compileFilter(
child.value,
variables,
command.job
);
Copy link
Contributor

Choose a reason for hiding this comment

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

yes, the webhook URL could be also a dynamic value or have dynamic parameters :)

private analyticsService: AnalyticsService
) {}

@InstrumentUsecase()
public async execute(command: SendMessageCommand) {
const shouldRun = await this.filter(command);
const preferred = await this.filterPreferredChannels(command.job);
const payload = await this.buildCompileContext(command);
Copy link
Contributor

Choose a reason for hiding this comment

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

From what I see the payload is a type of IFilterVariables and we are passing it also as the compileContext. But under the hood, the actual "compile context" is a destructured payload + partial type of IFilterVariables.

So I have a tiny suggestion to improve the code readability and types (not much different from what you have):

  • creating a function for the common parts like subscriber, tenant, step, and actor
  • then pass appropriate objects to the filter and compile context
  • type the SendMessageCommand.compileContext prop

Code:

// common object that has subscriber, tenant, step, actor
const sendMessageContext = await this.buildSendMessageContext(command);

const [shouldRun, preferred] = await Promise.all([
      this.filter(command, { payload: command.payload, ...sendMessageContext }),
      this.filterPreferredChannels(command.job),
    ]);

...

const { payload, ...rest } = filterVariables;
const sendMessageCommand = SendMessageCommand.create({
   ...command,
   compileContext:  { payload: command.payload, ...sendMessageContext },
});

Copy link
Contributor

Choose a reason for hiding this comment

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

And I'm wondering wouldn't it be better to create this sendMessageContext even outside of the SendMessage use-case, like in the RunJob use-case, and then just pass it down? 🤔 then the compileContext will become sendMessageContext and will be always defined (now it's optional).

Types:

export class SendMessageCommand extends EnvironmentWithUserCommand {
...

@IsDefined()
sendMessageContext: {
  subscriber?: SubscriberEntity;
  actor?: SubscriberEntity;
  webhook?: Record<string, unknown>;
  tenant?: TenantEntity;
  step?: {
    digest: boolean;
    events: any[] | undefined;
    total_count: number | undefined;
  };
}

}

Comment on lines +579 to +580
filterVariables.step = command.variables?.step ?? undefined;
filterVariables.actor = command.variables?.actor ?? undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

no need in ?? undefined part ;)

@ainouzgali ainouzgali merged commit 42e26ca into variants-condition Nov 2, 2023
23 of 24 checks passed
@ainouzgali ainouzgali deleted the nv-3049-allow-using-variables-as-values-in-the-step-filters1 branch November 2, 2023 12:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants