Reject a webhook wait_time over the 5 minute cap in the node editor - #220
Open
AmishaBisht wants to merge 1 commit into
Open
Reject a webhook wait_time over the 5 minute cap in the node editor#220AmishaBisht wants to merge 1 commit into
AmishaBisht wants to merge 1 commit into
Conversation
…ditor Glific caps how long an async webhook node parks the flow at 5 minutes and reads wait_time straight out of this body. Over-cap values were only caught at publish, as a warning, after the backend had already silently clamped them -- so the node behaved differently from what the author typed and they found out late, if at all. Validate it where the author is looking instead. mergeForm folds body failures into the form's validity and handleSave gates on that, so this both shows the message under the body field and blocks Ok until it is fixed. Stays quiet on an undecodable body, which isValidJson already reports, and on a blank wait_time, which is what the body template ships and means "use the webhook's default". Refs glific/glific#5633
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Companion to glific/glific#5638 (issue glific/glific#5633). Backend-side, an async webhook node can now set its own await window via
wait_timein the webhook body, capped at 5 minutes.Why
Without this, an over-cap value is only caught at publish, as a Warning, after the backend has already silently clamped it. So the node behaves differently from what the author typed, and they find out late — or never, if they only ever test in the simulator.
What
Adds an
isWaitTimeWithinCap()validator inwebhook/helpers.tsand wires it into the body's existing validation inWebhookRouterForm:mergeFormfolds body failures into the form'svalidflag andhandleSavegates on it, so this both shows the message under the body field (same place as "Not a valid JSON") and blocks Ok until it's fixed. Stronger than the backend warning: the bad value can't be saved at all.Behaviour
{"wait_time": 240}/"240"/300wait_timekey{"wait_time": ""}— what the body template ships{"wait_time": 600}wait_time cannot be more than 300 seconds (5 minutes)0,-30,"abc","60s",12.5wait_time must be a whole number of seconds greater than 0{},[],truewait_time must be a number of secondsisValidJson()already reports it, no double errorTesting
New
webhook/helpers.test.ts— 9 tests covering the table above plus that the entry value is returned unchanged. All webhook suites pass: 4 suites, 19 tests, 15 snapshots. Prettier clean.tsc --noEmitreports no errors in the changed files (the repo has 36 pre-existing TS7018 errors in other test files, untouched).Note for the reviewer
isValidJson()just above has a latent bug — noreturnwhenJSON.parsesucceeds on a non-object ("123",true), so it falls through toundefined, andvalidate()then reads.failuresoff it. Not touched here, but worth a separate fix. The new validator is written total to avoid the same trap.Release path
This needs a published
@glific/flow-editorversion and a bump of the pin in glific-frontend (currently1.43.0-14) before NGOs see it.