feat: add configurable dialog action#2350
Open
minottic wants to merge 5 commits into
Open
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The new selector implementations in
buildDatasetStaticMapdon’t yet cover all the keywords documented inconfigurable-actions-technical.md(e.g.#DatasetsPidEmptyFilesMap,#DatasetsTotalSize,#DatasetsTotalPackedSize), so either add support for these tokens or remove them from the docs to avoid configuration surprises. authorizationHandlersis used byresolve()for any#...token, but it only handles auth-related keywords; if a misconfigured payload or header accidentally contains a dataset selector like#Dataset0Piddirectly, it will silently pass through unchanged—consider narrowing whereauthorizationHandlersis applied or failing fast on unknown#tokens to make misconfigurations easier to detect.- In
DialogComponentyou define a reactiveFormGroup(form) but the template uses template-driven forms (ngForm/ngModel), so either remove the unusedFormGroupor wire the template up to it to keep the implementation consistent and avoid confusion.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new selector implementations in `buildDatasetStaticMap` don’t yet cover all the keywords documented in `configurable-actions-technical.md` (e.g. `#DatasetsPidEmptyFilesMap`, `#DatasetsTotalSize`, `#DatasetsTotalPackedSize`), so either add support for these tokens or remove them from the docs to avoid configuration surprises.
- `authorizationHandlers` is used by `resolve()` for any `#...` token, but it only handles auth-related keywords; if a misconfigured payload or header accidentally contains a dataset selector like `#Dataset0Pid` directly, it will silently pass through unchanged—consider narrowing where `authorizationHandlers` is applied or failing fast on unknown `#` tokens to make misconfigurations easier to detect.
- In `DialogComponent` you define a reactive `FormGroup` (`form`) but the template uses template-driven forms (`ngForm`/`ngModel`), so either remove the unused `FormGroup` or wire the template up to it to keep the implementation consistent and avoid confusion.
## Individual Comments
### Comment 1
<location path="src/app/shared/modules/configurable-actions/configurable-action.component.ts" line_range="419-423" />
<code_context>
+ return data;
+ }
+
+ private actionFinishedEmit(success: boolean, resultOrError?: Error) {
+ this.actionFinished.emit({
+ success,
+ result: success ? resultOrError : undefined,
+ error: !success ? resultOrError : undefined,
+ });
+ }
</code_context>
<issue_to_address>
**issue:** The `actionFinishedEmit` parameter type is too narrow and will conflict with successful XHR responses.
In `typeXhr` you pass the parsed JSON response as the second argument to `actionFinishedEmit`, but that parameter is typed as `Error`. This doesn’t match usage and will either fail type-checking or require casts. Consider widening the parameter type, e.g.:
```ts
private actionFinishedEmit(success: boolean, payload?: unknown) {
this.actionFinished.emit({
success,
result: success ? payload : undefined,
error: !success ? (payload as Error) : undefined,
});
}
```
or split success and error into separate, explicitly typed parameters.
</issue_to_address>
### Comment 2
<location path="src/app/shared/modules/configurable-actions/configurable-action.component.ts" line_range="359-376" />
<code_context>
- duration: 2000,
- },
- );
+ private typeDialog() {
+ const dialogRef = this.dialog.open(DialogComponent, {
+ width: this.actionConfig.dialog?.width || "450px",
+ data: this.prepareDialogData(),
+ });
+
+ dialogRef.afterClosed().subscribe((result: Record<string, any>) => {
+ if (!result) return;
+ const dialogRes: Record<string, any> = {};
+ this.actionConfig.dialog?.fields?.forEach((f) => {
+ const v = result[f.key];
+ dialogRes[f.key] =
+ v && typeof v === "object" && "option" in v ? v.option : v;
});
+ this.variables["dialog"] = dialogRes;
+ this.executeNextStep(this.actionConfig.onSuccess || "xhr");
+ });
+ }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Defaulting `onSuccess` for dialogs to `"xhr"` may trigger unintended follow-up requests.
In `typeDialog`, `executeNextStep` is always called with `this.actionConfig.onSuccess || "xhr"`, so any dialog without `onSuccess` configured will fall back to an XHR using the same `ActionConfig`. For dialogs meant only for confirmation (no follow-up action), this can trigger an unintended network request. Consider only invoking `executeNextStep` when `onSuccess` is set:
```ts
if (this.actionConfig.onSuccess) {
this.executeNextStep(this.actionConfig.onSuccess);
}
```
so configs explicitly opt in to a follow-up step.
```suggestion
private typeDialog() {
const dialogRef = this.dialog.open(DialogComponent, {
width: this.actionConfig.dialog?.width || "450px",
data: this.prepareDialogData(),
});
dialogRef.afterClosed().subscribe((result: Record<string, any>) => {
if (!result) return;
const dialogRes: Record<string, any> = {};
this.actionConfig.dialog?.fields?.forEach((f) => {
const v = result[f.key];
dialogRes[f.key] =
v && typeof v === "object" && "option" in v ? v.option : v;
});
this.variables["dialog"] = dialogRes;
if (this.actionConfig.onSuccess) {
this.executeNextStep(this.actionConfig.onSuccess);
}
});
}
```
</issue_to_address>
### Comment 3
<location path="src/app/shared/modules/dialog/dialog.component.ts" line_range="35-36" />
<code_context>
standalone: false,
})
export class DialogComponent {
+ form: FormGroup = new FormGroup({});
+ fieldKeys: string[] = [];
+
constructor(
</code_context>
<issue_to_address>
**suggestion:** Reactive `FormGroup` is declared but not used while the template relies on template-driven forms.
`form` and `fieldKeys` are never used in the template, which currently relies on `ngForm`/`ngModel`. This introduces an unused reactive-form setup and mixes patterns. Please either remove these properties or update the template to use the reactive form consistently.
Suggested implementation:
```typescript
export class DialogComponent {
constructor(
```
You should also:
1. Remove the `FormGroup` (and possibly `FormControl`, `FormBuilder`, etc., if present) import from the top of `dialog.component.ts` since it's no longer used.
2. Ensure there are no remaining references to `form` or `fieldKeys` elsewhere in the file (methods, lifecycle hooks).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
Author
|
even if this does some refactor of configurable-actions, I believe some more refactor is necessary, but that would introduce breaking changes. In particular, I believe we should only support computation in the variables block and everywhere else only reference what defined in variables. For example, we should favor "variables": {
"pid": "#Dataset0Pid",
"files": "#Dataset0FilesPath",
"totalSize": "#Dataset0FilesTotalSize",
"folder": "#Dataset0SourceFolder"
"maxSize": "#MaxDownloadableSize(@totalSize)"
},
"enabled": "@maxSize",inplace of "variables": {
"pid": "#Dataset0Pid",
"files": "#Dataset0FilesPath",
"totalSize": "#Dataset0FilesTotalSize",
"folder": "#Dataset0SourceFolder"
},
"enabled": "#MaxDownloadableSize(@totalSize)",this has the advantage of moving the resolution to one single field allowing for more control |
minottic
force-pushed
the
generalize_dialog
branch
5 times, most recently
from
0f79676 to
f997b85
Compare
New action type dialog that allows defining a dialog in config and emits on close This includes a big refactor (improvement) of configurable action
minottic
force-pushed
the
generalize_dialog
branch
from
f997b85 to
ee22024
Compare
5 tasks
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
1 participant
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.
Description
It adds a new action called dialog which can be configured in config.json allowing to set the shape of the dialog (fields, type of fields and required properties) and selecting a follow-up action.
Motivation
This will later make the cart and the overview datasets actions configurable, effectively replacing the hardcoded job submissions and enabling easy extensions to published data
dialog configuration:
generated form
Changes:
Please provide a list of the changes implemented by this PR
Tests included
Documentation
official documentation info
If you have updated the official documentation, please provide PR # and URL of the pages where the updates are included
Backend version
Summary by Sourcery
Introduce a configurable dialog-based action type and richer action evaluation to the configurable actions framework, along with button styling controls and expanded configuration typing.
New Features:
Enhancements:
Documentation:
Tests: