-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Build listener to backfill position #4432
Conversation
private readonly recordPositionQueryFactory: RecordPositionQueryFactory, | ||
) {} | ||
|
||
async create( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I simply moved the logic of buildPositionValue
in there
providers: [ | ||
WorkspaceQueryRunnerService, | ||
...workspaceQueryRunnerFactories, | ||
RecordPositionListener, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@charlesBochet @magrinj I don't know if the service / job / factory / listener should live into the query runner folder. Is there a better place ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments @thomtrp
recordId?: string, | ||
): Promise<string> { | ||
const name = | ||
(objectMetadata.isCustom ? '_' : '') + objectMetadata.nameSingular; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: We should probably have a helper for that for when we will remove this logic
dataSourceSchema: string, | ||
recordId: string, | ||
): Promise<string> { | ||
return `UPDATE ${dataSourceSchema}."${name}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prepared statements would be safer
...ages/twenty-server/src/workspace/workspace-query-runner/jobs/record-position-backfill.job.ts
Show resolved
Hide resolved
packages/twenty-server/src/integrations/event-emitter/types/object-record-create.event.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
We want the position set for all our records.
Currently, this is still possible to create records without a position through the rest API.
This PR:
I re-used the existing logic that overrides the position in the args during a normal creation through web. I put it in a new factory
RecordPositionFactory
.TODO: