Feature/agile 360 work packages graph on sprint report page - #24796
Feature/agile 360 work packages graph on sprint report page#24796EinLama wants to merge 12 commits into
Conversation
269658b to
b4a290f
Compare
|
Caution The provided work package version does not match the core version Details:
Please make sure that:
|
There was a problem hiding this comment.
Pull request overview
Adds a sprint report widget that reuses the existing Work Package overview graph, while hardening Chart.js plugin registration to avoid global plugin conflicts when multiple charts render on the same page.
Changes:
- Add a new sprint report grid widget rendering
opce-wp-overview-graphscoped to sprint + project, with the group-by select hidden. - Extend the WP overview graph component with a
showGroupByOptionsinput/attribute to optionally hide the group-by select. - Move Chart.js registerables + plugin registration to a single global initializer and remove per-component
provideCharts(...)registrations; update graphs to explicitly opt-in to datalabel rendering.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| modules/backlogs/spec/features/backlogs/sprint_reports/work_package_graph_widget_spec.rb | Feature spec asserting the widget renders on sprint report and displays scoped counts without the group-by select. |
| modules/backlogs/spec/components/backlogs/sprint_reports/widgets/work_package_graph_spec.rb | Component spec verifying widget attributes (scoping filters + hiding group-by) and permission gating. |
| modules/backlogs/config/locales/en.yml | Adds title translation for the new sprint report widget. |
| modules/backlogs/app/views/backlogs/sprint_reports/show.html.erb | Adds the new widget to the sprint report grid. |
| modules/backlogs/app/components/backlogs/sprint_reports/widgets/work_package_graph.rb | New grid widget component rendering the WP overview graph with sprint/project filters and hidden group-by. |
| frontend/src/main.ts | Ensures global Chart.js initialization runs before Angular bootstraps. |
| frontend/src/app/shared/components/work-package-graphs/overview/wp-overview-graph.template.html | Conditionally renders the group-by select based on showGroupByOptions. |
| frontend/src/app/shared/components/work-package-graphs/overview/wp-overview-graph.component.ts | Parses show-group-by-options / data-show-group-by-options attribute and adds @Input() for the new behavior. |
| frontend/src/app/shared/components/work-package-graphs/embedded/wp-embedded-graph.component.ts | Removes per-component chart registration and explicitly opts in to datalabel rendering. |
| frontend/src/app/shared/components/budget-graphs/overview/budget-by-cost-type.component.ts | Removes per-component chart registration in favor of global Chart.js init. |
| frontend/src/app/shared/components/budget-graphs/overview/actual-costs.component.ts | Removes per-component chart registration in favor of global Chart.js init. |
| frontend/src/app/features/backlogs/burndown-chart.component.ts | Removes per-component chart registration in favor of global Chart.js init. |
| frontend/src/app/core/setup/init-chartjs.ts | New global Chart.js initializer registering registerables + required plugins and setting datalabels default to opt-in. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
myabc
left a comment
There was a problem hiding this comment.
Visually I think the graph could benefit from a min-height and/or different aspect ratio. It's a bit squashed (vertically). Otherwise the rendering is fine.
The rest of my feedback is inline. Removing the data- attribute boilerplate/hackery in favour of signals would be my biggest priority. Then handling Chart setup in its own dependency graph.
I've taken the liberty of doing a quick implementation of these suggestions, since signals and providers are Angular-isms that might not have the easiest learning curve. Take a look at #24892 and feel free to cherry-pick and adapt to your needs.
You may want to ping @HDinger if you need help or follow up review!
| const showGroupByOptionsAttr = | ||
| element.getAttribute('show-group-by-options') ?? | ||
| element.getAttribute('data-show-group-by-options'); | ||
|
|
||
| if (showGroupByOptionsAttr !== null) { | ||
| this.showGroupByOptions = showGroupByOptionsAttr !== 'false'; | ||
| } |
There was a problem hiding this comment.
🔴 I realise this follows an existing pattern, but this boilerplate isn't necessary!
This component is registered through createCustomElement, and Angular Elements already maps dasherised attribute names onto inputs and applies input transforms. Moving to signal inputs would take care of the whole job:
readonly showGroupByOptions = input(true, { transform: booleanAttribute });with @if (showGroupByOptions()) in the template. While we are here I would convert the other two the same way (globalScope with booleanAttribute, initialFilters with a JSON-parsing transform) and drop the getAttribute code from ngOnInit entirely.
💡 Hinweis: Angular provides a migration for components to do this switch to signal inputs automatically!
npm install -g @angular/cli if you haven't already done so and run:
ng generate @angular/core:signal-input-migration --path src/app/shared/components/work-package-graphs/overview
Choose Y when asked "Do you want to migrate as much as possible, even if it may break your build?" You will still need to make some additional fixes, e.g. removing lines from ngOnIt.
I had a go locally and this is what is necessary to get it building again: https://gist.github.com/myabc/761d408bf1958d9f8826cf15910e7128
❗ N.B. on the Rails side that means passing plain attributes instead of inputs: (which JSON-encodes into data-*, and Angular Elements never observes data-*), the same way the burndown widget in this PR already does with "chart-data"::
helpers.angular_component_tag(
"opce-wp-overview-graph",
"global-scope": false,
"initial-filters": graph_filters.to_json,
"show-group-by-options": false
)WorkPackagesGraphWidgetComponent in the versions settings and the data-* assertions in the component spec need the same change.
| Chart.register(...registerables, ChartDataLabels, PrimerColorsPlugin); | ||
|
|
||
| // Require charts using the data labels plugin to opt-in instead of the default opt-out: | ||
| Chart.defaults.set('plugins.datalabels', { display: false }); |
There was a problem hiding this comment.
🔴 Two things here.
-
Importing this from
main.tsmakes chart.js a hard dependency of the bootstrap path: esbuild only splits on dynamicimport(), so a static side-effect import from the entry file pins chart.js to the main chunk for good. Today that makes no visible difference, becauseapp.module.tsstatically imports every chart component forregisterCustomElementand chart.js lands inmainanyway. But that is an accident we want to undo, and this import would keep chart.js inmaineven once those components load lazily. IMO chart setup should live inside the chart dependency graph, not in the app entry point. -
Secondly, I think this fixes the plugin leak one level too high. The leak (
38a2dbbc94b) is thatwithDefaultRegisterables(ChartDataLabels, ...)puts datalabels into Chart.js's global registry, so every chart on the page gets it. Registering it globally and then flippingChart.defaults.set('plugins.datalabels', { display: false })plus an explicitdisplay: trueopt-in is two global overrides to cure one global leak. The ng2-chartsBaseChartDirectivehas apluginsinput for exactly this: inline plugins are scoped to that chart instance and never touch the registry.
| <select [(ngModel)]="groupBy" | ||
| (ngModelChange)="setQueryProps()"> | ||
| @for (option of availableGroupBy; track option.key) { | ||
| <option [ngValue]="option.key">{{option.label}}</option> |
| "opce-wp-overview-graph", | ||
| inputs: { | ||
| "global-scope": false, | ||
| "initial-filters": graph_filters, | ||
| "show-group-by-options": false | ||
| } |
There was a problem hiding this comment.
| "opce-wp-overview-graph", | |
| inputs: { | |
| "global-scope": false, | |
| "initial-filters": graph_filters, | |
| "show-group-by-options": false | |
| } | |
| "opce-wp-overview-graph", | |
| "global-scope": false, | |
| "initial-filters": graph_filters.to_json, | |
| "show-group-by-options": false |
| // This guarantees the registry is fully populated *before* Angular constructs | ||
| // any component and multiple charts on the same page do not compete for a conflicting |
There was a problem hiding this comment.
nitpick: Comment overstates this. The ng2-charts constructor already guaranteed that per chart.
Add a `showGroupByOptions` input to WorkPackageOverviewGraphComponent so embedding widgets (e.g. the sprint report work package graph widget) can hide the group-by <select>.
Otherwise the first chart rendered on a page sets the plugins for all charts of that page. This can cause bugs, depending on which chart loads first. This happened for sprint reports.
1. a quick smoke test to check that the correct options are passed to the chart component 2. a feature spec that inspects the DOM to see if the correct data is actually rendered
a141cdf to
82e4993
Compare
82e4993 to
fb60262
Compare
Co-authored-by: Alexander Brandon Coles <a.coles@openproject.com>
Ticket
https://community.openproject.org/wp/AGILE-360
What are you trying to accomplish?
Render the work package graph component that is also present on the project dashboard.
Screenshots
What approach did you choose and why?
Introduced a new option to the existing graph Angular component so that the group-by select can be hidden. For the sprint report, we only care about the WP status.
When rendering multiple chart.js charts on the same page, there was a problem when loading plugins. Chart.js loads their plugins globally, so different charts with different plugin requirements can cause conflicts and JS crashes. To fix that, I moved plugin registration to a global handler that is loaded before Angular does anything. This ensures that this conflict will not be decided by what chart lazily renders first - and all charts render successfully.
Merge checklist