fix: depercate form button widget #14510
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
techbhavin
changed the title
github-actions
bot
added
Enhancement
|
Unable to find test scripts. Please add necessary tests to the PR. |
|
Unable to find test scripts. Please add necessary tests to the PR. |
| @@ -149,7 +150,7 @@ export function* executeActionTriggers( | |||
| return response; | |||
| } | |||
|
|
|||
| export function* executeAppAction(payload: ExecuteTriggerPayload) { | |||
| export function* executeAppAction(payload: ExecuteTriggerPayload): any { | |||
There was a problem hiding this comment.
@Rishabh-Rathod can you take a look at this change in this action saga file?
There was a problem hiding this comment.
@techbhavin i will need some context here about what are we trying to do in this PR and mainly executeAppAction and initiateActionTriggerExecution
There was a problem hiding this comment.
@Rishabh-Rathod
here All changes related to the form button widget deprecate replace it with the button widget.
and here change is related below feature refer comment
- if API action in form widget return success response form should reset.
- if used had set reset on form success and did not reset if API returns an error.
In initiateActionTriggerExecution all trigger execute form this function and wait till execution complete by executeAppAction
previously executeAppAction was not returning any response.
There was a problem hiding this comment.
Please help me understand below points
Why do we need changes made in executeActionTriggers?
// response return only one object into array
set(response, "0.success", true);I can notice in executeAppAction, we call evaluateAndExecuteDynamicTrigger which indeed calls executeActionTriggers but it doesn’t return anything. I couldn’t understand where are we consuming the response set above.
Quoting Dilip's message
Today our "reset form on success" field when switched on also resets if there's an error in the API which is incorrect, i believe this could be fixed here.
Would like to understand how do we plan to handle reset of form in below cases.
cc: @dilippitchika
Note we won't be having context of what each API is meant to do in below cases.
- If 1st action's is successful and onSuccess of it 2nd action fails.
If for an example, Form's button on submit runs
{{
Api1.run(() => { // success
Api2.run() // failure
})
}}
here Api1 was successful but Api2 failed. Do we want to reset for this case?
How do we decide what is success for our users here ?
- Multiple updateAction and one of the fails
If for an example, Form's button on submit runs
{{
updateDataInDB1.run(); // success
updateDataInDB2.run(); // failure
updateDataInDB3.run() // success
}}
Do we want to reset for this case?
How do we decide what is success for our users here ?
There was a problem hiding this comment.
@Rishabh-Rathod answering to your questions
- Why do we need changes made in executeActionTriggers?
here to handle API execution callback, a response is handled. in almost trigger type, previously nothing was returned and not required to return but as per changes related to API execution, here I change for other types
let response: unknown[] = [{ success: true }];
in the location trigger type
ActionTriggerType.GET_CURRENT_LOCATION
ActionTriggerType.WATCH_CURRENT_LOCATION
ActionTriggerType.STOP_WATCHING_CURRENT_LOCATION
if the action executes successfully, we get current location data as a response for that case setting
set(response, "0.success", true);
if there is an error to get the current location, initiateActionTriggerExecution actions catch block will execute
if not set the success flag into executeActionTriggers, I was not found any other way to decide that executed action
const res: unknown[] = yield call(executeAppAction, action.payload);
( at line 191 ) for which trigger type and how to check API success error callback.
- Would like to understand how do we plan to handle the reset of form in the below cases. as per line 198, currently support 1st level trigger if Api1 was successful but Api2 failed. the form will reset as per the current implementation
thanks for giving other possible scenarios. have to create and check that.
There was a problem hiding this comment.
Would like to understand how do we plan to handle reset of form in below cases.
@Rishabh-Rathod i don't think this will ever be foolproof, the user can choose to switch this off and use resetWidget to do it themselves when the 2nd api call succeeds.
| @@ -321,6 +351,8 @@ class ButtonWidget extends BaseWidget<ButtonWidgetProps, ButtonWidgetState> { | |||
| callback: this.handleActionComplete, | |||
| }, | |||
| }); | |||
| } else if (this.props.resetFormOnClick && this.props.onReset) { | |||
There was a problem hiding this comment.
What happens when onClick is also set and reset form on clicked is also toggled? Which of the actions will take priority? I think it should be resetFormOnClick. But from the code, it seems onClick will take priority. @dilippitchika can confirm on this.
There was a problem hiding this comment.
@jsartisan , Here
this is handled by callback if there is onClick added, callback always takes care of post actions of onClick which is handled into handleActionComplete function if there is no onClick, and reset on submit enable. it just reset the form.
here onClick should be on priority coz we are dealing with API success error callbacks. so reset should be executed after onClick executed
|
@Rishabh-Rathod there is one change in the action saga file. Can you take a look at that if that change is okay? |
…r form widget only
|
/ok-to-test sha=201e7fe |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/2533171903. |
|
UI Performance test run logs and artifacts: https://github.com/appsmithorg/appsmith/actions/runs/2533171903. Click to view performance test results
|
|
@aswathkk this impacts the property pane for button widget. Can you also check this once? |
|
Thanks, @dilippitchika. I will take care of this once we merge this PR to release. |
|
@techbhavin DP link is broken |
|
@techbhavin |
|
/ok-to-test sha=919afbd |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/2635841170. |
|
@techbhavin cc: @yaldram |
|
UI Performance test run logs and artifacts: https://github.com/appsmithorg/appsmith/actions/runs/2635841170. Click to view performance test results
|
|
Tested this PR and working as expected.
|
|
/ok-to-test sha=7d03e0e |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/2690007518. |
|
UI Performance test run logs and artifacts: https://github.com/appsmithorg/appsmith/actions/runs/2690007518. Click to view performance test results
|
|
/ok-to-test sha=0ab16cc |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/2716660505. |
|
UI Performance test run logs and artifacts: https://github.com/appsmithorg/appsmith/actions/runs/2716660505. Click to view performance test results
|
Description
Fixes #3613
Type of change
Checklist: