Webhook trigger update by column #832

CamilleLegeron · 2024-01-25T16:57:25Z

Context

Done

Screencast from 06-02-2024 11:44:39.webm
Screencast from 06-02-2024 11:46:48.webm

I used a string for columnIds because it seems to be difficult to show only the list corresponding to the selected tableId to the user. If we change the list shown to the user we change it for all instance of webhooks, that it's not acceptable.

…t in account

CamilleLegeron · 2024-02-06T15:29:17Z

The last error seams to have no link to my feature ..

alexmojaki · 2024-02-07T16:32:50Z

Thanks @CamilleLegeron for the contribution! I'm afraid I'm not working at GristLabs any more though, so I've requested review from others.

fflorent

Some comments I hope that may help :)

app/server/lib/DocApi.ts

sandbox/grist/migrations.py

app/server/lib/DocApi.ts

fflorent · 2024-02-14T11:27:59Z

app/common/Triggers.ts

@@ -10,6 +10,7 @@ export interface WebhookFields {
  url: string;
  eventTypes: Array<"add"|"update">;
  tableId: string;
+  columnIds: string;


You may run yarn build:prod to update the Triggers-ti.ts file (whose classes are used as body validator for the APIs):

$ yarn build:prod yarn run v1.22.21 $ buildtools/build.sh + tsc --build + buildtools/update_type_info.sh app Updating app/common/Triggers-ti.ts from app/common/Triggers.ts Updating app/plugin/CustomSectionAPI-ti.ts from app/plugin/CustomSectionAPI.ts Updating app/plugin/GristData-ti.ts from app/plugin/GristData.ts ...

app/client/ui/WebhookPage.ts

fflorent

I have few more comments :)

app/common/schema.ts

fflorent · 2024-02-15T09:33:53Z

test/server/lib/DocApi.ts

+
+        const webhook = await autoSubscribe('200', docId, {
+          columnIds: 'A', eventTypes: ['add', 'update']
+        });


Would it be hard to add another test with a second column in columnIds?
This would allow to check whether the ; works as expected.

Your right, I will do it

test/server/lib/DocApi.ts

CamilleLegeron · 2024-03-20T10:24:21Z

Hi, I just put a new notification for you paulfitz in case you haven't seen this PR. Sorry if your are too busy, in that case I can ask other dev in gristlabs to review this PR.

paulfitz

Thanks for working on this @CamilleLegeron !

test/server/lib/DocApi.ts

paulfitz · 2024-03-20T20:29:26Z

test/server/lib/DocApi.ts

            };

            // subscribe
-            const webhook = await subscribe('foo', docId, origFields);
+            const { data } = await axios.post(


Is this change just to use newer /webhooks instead of older /_subscribe endpoint?

I think it's good to test the newer endpoint but I don't remember if in this case something went wrong with the older
Do you want me to create a specific subscribe function with the new endpoint call and rename the old one with oldEndpointsubscribe or something like that

paulfitz · 2024-03-20T20:37:49Z

app/server/lib/DocApi.ts

+              throw new ApiError(`Cannot find columns "${columnIds}" because table is not known`, 404);
+            }
+            // columnIds have to be of shape "columnId; columnId; columnId"
+            fields.columnRefList = [GristObjCode.List, ...columnIds.split(";")


So here, we are in the back-end, defining the behavior of a REST API endpoint. Sending ;-separated columns seems pretty inconsistent with how lists are handled elsewhere in the API. I can understand doing that in the minimalist front-end UI, just because of lack of time to do something pretty (the same trade-off we made for isReadyColumn), but is there a way to isolate that to the front end? Once the back-end does something, changing it is hard since someone may rely on it.

It will be sad if we eventually add a pretty UI for this and it ends up having the convert a nice structured list of column ids into a semicolon separated string to send to the back end.

columnIds is a little vague, and a bit inconsistent with isReadyColumn which doesn't specify the Id part. Maybe something like watchedColumns?

Also, if column ids are stored as strings, would need some thinking about updating them when columns are renamed.

This remark is a very good point, I isolated this logical in the frontend.
The watchedColIds is stored in the db as watchedColRefList so if the column is renamed it's works well

sandbox/grist/schema.py

app/client/ui/WebhookPage.ts

sandbox/grist/schema.py

paulfitz · 2024-03-26T19:31:19Z

The test failure looks relevant:

(1) DocApi should work directly with a docworker Webhooks /webhooks endpoint should call to a webhook only when columns updated are in watchedColIds if not empty
AssertionError: expected false to be true
    ...
    at assertSuccessCalled (test/server/lib/DocApi.ts:4448:18)
    at Context.<anonymous> (test/server/lib/DocApi.ts:4490:15)

CamilleLegeron · 2024-03-27T10:48:02Z

The test failure looks relevant:

(1) DocApi should work directly with a docworker Webhooks /webhooks endpoint should call to a webhook only when columns updated are in watchedColIds if not empty
AssertionError: expected false to be true
    ...
    at assertSuccessCalled (test/server/lib/DocApi.ts:4448:18)
    at Context.<anonymous> (test/server/lib/DocApi.ts:4490:15)

Well ... I don't understand why this error happen, reinitializing the migration it works well

fflorent

OK for me modulo my comment related to the options column.

CamilleLegeron · 2024-04-04T08:25:15Z

@paulfitz, send you this notification in case you haven't seen I worked on this PR :)

paulfitz

Thanks for your work on this @CamilleLegeron !

CamilleLegeron added 12 commits January 25, 2024 10:31

show column settings in webhooks and consolelog to know how to take i…

55bde5f

…t in account

Merge branch 'main' into webhook-trigger-update-by-column

6403d7a

remove text typo

f497288

add columnsIds param in webhook register in db

0f0dda3

take in account the columns to check before trigger action

f736afc

remove columnIds if change tableID

d4d6fe8

webhook translatable

62836eb

add test for columnIds in webhooks

3797649

delete samples bd

5819a57

add ignore eslint error

fd0da6d

correct tests for webhookPage

4c813e0

add a delay before testing

dbc8498

CamilleLegeron marked this pull request as ready for review February 6, 2024 15:29

Merge branch 'main' into webhook-trigger-update-by-column

8a878a5

CamilleLegeron added the gouv.fr label Feb 7, 2024

CamilleLegeron requested a review from alexmojaki February 7, 2024 16:27

alexmojaki requested review from paulfitz and JakubSerafin and removed request for alexmojaki February 7, 2024 16:31

CamilleLegeron requested review from paulfitz and removed request for paulfitz February 8, 2024 08:48

fflorent reviewed Feb 14, 2024

View reviewed changes

CamilleLegeron added 2 commits February 14, 2024 16:52

fix forgotten log + typo + simplify if statement

706c8f7

update Triggers-ti.ts for APIs body validator

9d802e0

fflorent reviewed Feb 14, 2024

View reviewed changes

app/client/ui/WebhookPage.ts Show resolved Hide resolved

add forgotten translation

8a75bb9

fflorent reviewed Feb 15, 2024

View reviewed changes

Add test + fix to ignore empty columnId string

3f5ebb3

fflorent approved these changes Feb 26, 2024

View reviewed changes

CamilleLegeron mentioned this pull request Feb 27, 2024

Custom HTTP queries for webhooks #827

Open

CamilleLegeron self-assigned this Mar 5, 2024

paulfitz removed the request for review from JakubSerafin March 20, 2024 13:49

paulfitz reviewed Mar 20, 2024

View reviewed changes

CamilleLegeron added 11 commits March 21, 2024 10:01

Merge branch 'main' into webhook-trigger-update-by-column

2a18e15

rename label for webhook columnIds

22ad13a

rename columnRefList to watchedColRefList

3226372

remove white spaces and bring together comment and function

cca6541

isolate watchedColIds string in frontend

c73e695

Merge branch 'main' into webhook-trigger-update-by-column

c5dfa4a

rename columnIds

35e5b2e

adapt WebhookPage test

b19d169

rename UIWebhookSummary type

f6a9199

add webhookPage test

f2e8ee8

make watchedColIds optional

b20d459

CamilleLegeron requested review from paulfitz and fflorent March 26, 2024 09:48

CamilleLegeron added 2 commits March 26, 2024 12:01

add translation

b4ab5ee

add generic options in _grist_triggers table

410dd28

CamilleLegeron commented Mar 26, 2024

View reviewed changes

sandbox/grist/schema.py Show resolved Hide resolved

CamilleLegeron added 2 commits March 27, 2024 11:22

add migration

e62a844

remove migration

1aeca1b

fflorent approved these changes Mar 27, 2024

View reviewed changes

paulfitz approved these changes Apr 12, 2024

View reviewed changes

paulfitz merged commit 76ef4d5 into gristlabs:main Apr 12, 2024
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Webhook trigger update by column #832

Webhook trigger update by column #832

CamilleLegeron commented Jan 25, 2024 •

edited

Loading

CamilleLegeron commented Feb 6, 2024

alexmojaki commented Feb 7, 2024

fflorent left a comment

fflorent Feb 14, 2024

fflorent left a comment •

edited

Loading

fflorent Feb 15, 2024

CamilleLegeron Feb 15, 2024

CamilleLegeron Feb 21, 2024

CamilleLegeron commented Mar 20, 2024

paulfitz left a comment

paulfitz Mar 20, 2024

CamilleLegeron Mar 26, 2024

paulfitz Mar 20, 2024

paulfitz Mar 20, 2024

paulfitz Mar 20, 2024

CamilleLegeron Mar 25, 2024

paulfitz commented Mar 26, 2024

CamilleLegeron commented Mar 27, 2024

fflorent left a comment

CamilleLegeron commented Apr 4, 2024

paulfitz left a comment

Webhook trigger update by column #832

Webhook trigger update by column #832

Conversation

CamilleLegeron commented Jan 25, 2024 • edited Loading

Context

Done

CamilleLegeron commented Feb 6, 2024

alexmojaki commented Feb 7, 2024

fflorent left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fflorent left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CamilleLegeron commented Mar 20, 2024

paulfitz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulfitz commented Mar 26, 2024

CamilleLegeron commented Mar 27, 2024

fflorent left a comment

Choose a reason for hiding this comment

CamilleLegeron commented Apr 4, 2024

paulfitz left a comment

Choose a reason for hiding this comment

CamilleLegeron commented Jan 25, 2024 •

edited

Loading

fflorent left a comment •

edited

Loading