feat: add configurable-actions in publisheddata#2431
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
ActionItems,publisheddatais now always populated and accessed without null checks (e.g.this.actionItems.publisheddata[0]), so consider makingpublisheddatanon-optional in the interface to better reflect actual usage and avoid potential undefined access. - The published data actions are no longer gated by
isLoggedIn$in the template, so it might be worth confirming whether configurable-actions should enforce login/role visibility internally or if an explicit auth-based*ngIfis still needed at themat-card-actionslevel for clarity.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `ActionItems`, `publisheddata` is now always populated and accessed without null checks (e.g. `this.actionItems.publisheddata[0]`), so consider making `publisheddata` non-optional in the interface to better reflect actual usage and avoid potential undefined access.
- The published data actions are no longer gated by `isLoggedIn$` in the template, so it might be worth confirming whether configurable-actions should enforce login/role visibility internally or if an explicit auth-based `*ngIf` is still needed at the `mat-card-actions` level for clarity.
## Individual Comments
### Comment 1
<location path="src/app/publisheddata/publisheddata-details/publisheddata-details.component.html" line_range="29-35" />
<code_context>
- >
- Publish
- </button>
+ <mat-card-actions>
+ <configurable-actions
+ *ngIf="appConfig.publishedDataActionsEnabled"
+ [actionsConfig]="appConfig.publishedDataActions"
+ [actionItems]="actionItems"
+ (actionFinished)="onActionFinished($event)">
+ </configurable-actions>
</mat-card-actions>
</mat-card>
</code_context>
<issue_to_address>
**suggestion:** Avoid rendering an empty mat-card-actions container when actions are disabled
`mat-card-actions` is always rendered, so when `publishedDataActionsEnabled` is false you end up with an empty actions row that can affect spacing. Please move the `*ngIf` to the `<mat-card-actions>` element so the container is not rendered when there are no actions.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| <mat-card-actions> | ||
| <configurable-actions | ||
| *ngIf="appConfig.publishedDataActionsEnabled" | ||
| [actionsConfig]="appConfig.publishedDataActions" | ||
| [actionItems]="actionItems" | ||
| (actionFinished)="onActionFinished($event)"> | ||
| </configurable-actions> |
There was a problem hiding this comment.
suggestion: Avoid rendering an empty mat-card-actions container when actions are disabled
mat-card-actions is always rendered, so when publishedDataActionsEnabled is false you end up with an empty actions row that can affect spacing. Please move the *ngIf to the <mat-card-actions> element so the container is not rendered when there are no actions.
minottic
left a comment
minottic
left a comment
There was a problem hiding this comment.
thanks! some minor comments.
It's a bit unfortunate that one has to do so much configuration, given the form change, but I guess there's no way around it
| .flatMap("files") | ||
| .filter("selected") | ||
| .sumBy((f) => Number(f.size || 0)), | ||
| "#PublishedData0Doi": () => |
There was a problem hiding this comment.
can it be that you don't need the 2 extra keys, and you can just use #publisheddata[0].doi directly in the config? (same as for the user here). Code wise, that should be resolved by the dynamicVariableHandler
|
|
||
| export interface ActionItems { | ||
| datasets: ActionItemDataset[]; | ||
| publisheddata?: PublishedData[]; |
There was a problem hiding this comment.
if you use the dynamicVariableHandler (see comment above), I think this is not strictly needed but having here doesn't harm if you prefer to keep it
| return _.get(this.actionItems, `instruments[${index}].${field}`); | ||
| } | ||
|
|
||
| const publishedDataFieldMatch = selector.match( |
There was a problem hiding this comment.
same as the other comments. I added instruments in my old PR because that came from existing code and I did for backward compatibility. But I think that as long as you pass publisheddata in actionItem from the parent, vai the config you can access any property and any index (e.g. for title it would be publisheddata[0].title directly in the config)
| this.currentData$.subscribe((data) => { | ||
| if (data) { | ||
| this.publishedData = data; | ||
| this.actionItems.publisheddata[0] = data; |
There was a problem hiding this comment.
I find a bit easier if you passed like this:
this.actionItems = {
datasets: [],
firstPublishedData: data,
user: user #if you also need user
};and then in the config you use #firstPublishedData.title
There was a problem hiding this comment.
Pull request overview
This PR extends the existing configurable-actions mechanism to the Published Data details view, allowing publish/register/amend actions to be driven by frontend configuration rather than hardcoded buttons.
Changes:
- Add
publishedDataActionsEnabled/publishedDataActionsto the frontend app config and admin config schema. - Wire Published Data details page to render
<configurable-actions>and provide PublishedData action items/selectors (DOI/status). - Update unit/e2e tests and CI e2e config to exercise Published Data flows via configurable-action buttons.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/app/shared/modules/configurable-actions/configurable-action.interfaces.ts | Extend ActionItems to include PublishedData. |
| src/app/shared/modules/configurable-actions/configurable-action.component.ts | Add PublishedData selector parsing and static tokens (doi/status). |
| src/app/publisheddata/publisheddata-details/publisheddata-details.component.ts | Populate configurable-actions actionItems for PublishedData; refetch on successful action. |
| src/app/publisheddata/publisheddata-details/publisheddata-details.component.html | Replace hardcoded publish/register/amend buttons with <configurable-actions>. |
| src/app/publisheddata/publisheddata-details/publisheddata-details.component.scss | Layout styling tweaks for configurable-actions placement. |
| src/app/publisheddata/publisheddata-details/publisheddata-details.component.spec.ts | Update component test to assert configurable-actions rendering and add required providers. |
| src/app/app-config.service.ts | Add publishedData actions settings to AppConfigInterface. |
| src/app/app-config.service.spec.ts | Update config fixture with new publishedData actions fields and remove unused import. |
| src/app/admin/schema/frontend.config.jsonforms.json | Add schema + UI controls for configuring Published Data Actions. |
| cypress/e2e/published-data/published-data.cy.js | Update e2e assertions/selectors to click configurable-action buttons. |
| CI/e2e/frontend.config.e2e.json | Add publishedDataActions configuration for CI e2e runs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "#PublishedData0Doi": () => | ||
| _.get(this.actionItems, "publisheddata[0].doi"), | ||
| "#PublishedData0Status": () => | ||
| _.get(this.actionItems, "publisheddata[0].status"), |
| "url": "http://localhost:3000/api/v3/publisheddata/{{ @doi }}/", | ||
| "variables": { | ||
| "doi": "#PublishedData0Doi", | ||
| "status": "#PublishedData0Status" | ||
| }, |
| "url": "http://localhost:3000/api/v3/publisheddata/{{ @doi }}/", | ||
| "variables": { | ||
| "doi": "#PublishedData0Doi", | ||
| "status": "#PublishedData0Status" | ||
| }, |
| "url": "http://localhost:3000/api/v3/publisheddata/{{ @doi }}/", | ||
| "variables": { | ||
| "doi": "#PublishedData0Doi", | ||
| "status": "#PublishedData0Status" | ||
| }, |
| <mat-card-actions> | ||
| <configurable-actions | ||
| *ngIf="appConfig.publishedDataActionsEnabled" | ||
| [actionsConfig]="appConfig.publishedDataActions" | ||
| [actionItems]="actionItems" | ||
| (actionFinished)="onActionFinished($event)"> | ||
| </configurable-actions> |
| "inputs": { | ||
| "type": "array", | ||
| "items": { | ||
| "type": "object", | ||
| "properties": { | ||
| "key": { "type": "string" }, | ||
| "value": { "type": "string" } | ||
| } | ||
| } | ||
| }, | ||
| "variables": { | ||
| "type": "array", | ||
| "items": { | ||
| "type": "object", | ||
| "properties": { | ||
| "key": { "type": "string" }, | ||
| "value": { "type": "string" } | ||
| } | ||
| } | ||
| }, |
| </button> | ||
| <mat-card-actions> | ||
| <configurable-actions | ||
| *ngIf="appConfig.publishedDataActionsEnabled" |
There was a problem hiding this comment.
Imo this should default to the old actions if none are provided otherwise it's a breaking change.
There was a problem hiding this comment.
yes, that is the goal. I added action config to the config.json file as sensible defaults which restore the previous behaviour.
There was a problem hiding this comment.
I don't think this is the right solution for backward compatibility:
- if the config comes from the frontend, it's usually overwritten with the facility config
- if it comes from the backend I believe the frontend will first fetch it and ignore the frontend one (not 100% sure)
In both cases it requires some config changes
Description
Configurable Actions added in published data, too.
Motivation
hardcoded publish etc actions are
Fixes:
Please provide a list of the fixes implemented in this PR
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
Integrate the generic configurable-actions mechanism into the published data details view and wire it to the app configuration and action items model.
New Features:
Enhancements:
Tests: