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

Missing old row on automations #14862

Merged
merged 4 commits into from
Oct 24, 2024
Merged

Conversation

adrinr
Copy link
Collaborator

@adrinr adrinr commented Oct 24, 2024

Description

We had an issue where some chained automations were not passing the proper data, missing some data such as "oldRow". The problem was that it was not properly sent, and while debugging the issue I realised that there is not much typing around automation emitters.
This PR is mainly typing everything around emitters to stop using magic strings, parameters and anys. It also fixes the actual issue, passing the oldRow accordingly.

image

Copy link

qa-wolf bot commented Oct 24, 2024

QA Wolf here! As you write new code it's important that your test coverage is keeping up.
Click here to request test coverage for this PR!

@github-actions github-actions bot added firestorm Data/Infra/Revenue Team size/m labels Oct 24, 2024
@@ -54,7 +66,9 @@ class AutomationEmitter {
appId,
row,
table,
oldRow,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the actual fix

@adrinr adrinr changed the title Budi 8749/missing old row on automations Missing old row on automations Oct 24, 2024
@adrinr adrinr marked this pull request as ready for review October 24, 2024 12:04
@adrinr adrinr requested a review from a team as a code owner October 24, 2024 12:04
@adrinr adrinr requested review from mike12345567 and removed request for a team October 24, 2024 12:04
@@ -49,3 +60,32 @@ export interface UserCtx<RequestBody = any, ResponseBody = any>
export interface BBContext extends Ctx {
user?: ContextUser
}

export interface ContextEmitter {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is really nice, I was actually confused about this the other day when trying to track down some emitter issues - the fact we had the AutomationEmitter and the standard emitter etc. Really nice stuff!

Copy link
Collaborator

@PClmnt PClmnt left a comment

Choose a reason for hiding this comment

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

LGTM!

@adrinr adrinr merged commit ef24656 into v3-ui Oct 24, 2024
11 checks passed
@adrinr adrinr deleted the BUDI-8749/missing-old-row-on-automations branch October 24, 2024 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
firestorm Data/Infra/Revenue Team size/m
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants