Feed the Symfony dashboard's Zone One hook with KPIs and a chart, add a settings page - #63
Feed the Symfony dashboard's Zone One hook with KPIs and a chart, add a settings page#63nicosomb wants to merge 3 commits into
Conversation
|
This pull request seems to contain new translation strings. I have summarized them below to ease up review:
(Note: this is an automated message, but answering it will reach a real human) |
26ef6d2 to
27c5263
Compare
mattgoud
left a comment
There was a problem hiding this comment.
Tested locally against PrestaShop/PrestaShop#42431 with the dashboard flag on. The KPI list and the traffic sources doughnut both render in zone one, the palette is applied automatically, and reusing hookDashboardData() rather than duplicating the queries is the right call. The Configure gear correctly disappears when getPermission('configure') is false.
Two blockers and a handful of smaller points.
Blocker 1: no upgrade script, so no existing shop gets the hook or the tab
install() only runs on a fresh install. On a shop that already has dashactivity 2.1.2, neither displayAdminDashboardZoneOne nor the AdminDashactivityConfiguration tab is ever created.
Reproduced locally: old version installed, 2.2.0 files dropped on disk, then the upgrade path replayed (ModuleManager::upgradeMigration() minus the download step):
dashactivity db=2.1.2 disk=2.2.0 needUpgrade=NO
No hook row, no tab, and Module::upgradeModuleVersion() is never called so ps_module.version stays on 2.1.2 forever.
autoupgrade behaves the same way: ModuleMigration::needMigration() returns false when upgrade/*.php is empty, it logs "Module does not need to be migrated", then calls saveVersionInDb() anyway. After a core upgrade the shop reports 2.2.0 while the hook is still missing, with no way back other than uninstall/reinstall (losing the configuration). autoupgrade cannot fix this on its own: core upgrade SQL never inserts into ps_hook_module, it only cleans orphans.
The module already has the pattern (upgrade/upgrade-2.1.0.php does unregisterHook(...)), so it needs an upgrade/upgrade-2.2.0.php that registers the hook and creates the tab, sharing the tab creation with install() rather than duplicating it.
Blocker 2: the settings controller has no ACL
indexAction() carries no #[AdminSecurity]. The admin firewall (app/config/admin/security.yml) only requires IS_AUTHENTICATED, and AdminSecurityListener does nothing when the attribute is absent, so any logged-in employee can open and save these settings regardless of their profile. I checked that the Translator profile has none of the four ROLE_MOD_TAB_ADMINDASHACTIVITYCONFIGURATION_* roles, so the tab created by install() is doing no work today.
90 of the 92 core admin controllers carry the attribute (the two exceptions are Login and Security), and ps_linklist does too, so this is the established convention:
#[AdminSecurity("is_granted('read', request.get('_legacy_controller'))")]
public function indexAction(Request $request): ResponseNote that once the attribute is there, the id_parent = -1 tab only grants the roles to SuperAdmin by default, so the other profiles will need their permissions initialised.
Traffic sources is a new string in a core catalogue
ps-jarvis flagged it, and it is indeed absent from translations/en-US/AdminOrderscustomersNotification.en-US.xlf. A module cannot add entries to core catalogues, so it will stay untranslated. It belongs in Modules.Dashactivity.Admin. The Admin.Global reuses (Orders, Abandoned Carts) are fine, those keys exist.
Smaller points
FrameworkBundleAdminControlleris@deprecated since 9.0in favour ofPrestaShopAdminController. Worth deciding explicitly: if the Symfony settings page is 9.x-only anyway, use the modern base class; if the deprecated one is kept for 8.2 compatibility, a short comment saying so would help.$tab->add()runs beforeparent::install(), so a failing install leaves an orphan tab behind. Inverting the order, or cleaning up on failure, would be safer.- The
tokeningetConfigUrl()is dead weight.routeris aliased toprestashop.routerwhich already appends_token, and in PS 9Tools::getAdminToken()ignores its argument and returns that same CSRF token. The generated link carries the identical value twice (?token=<csrf>&_token=<csrf>). Verified in the browser on this branch. $data['data_chart']['dash_trends_chart1']is accessed without a guard. It works today, but the key is oddly named for this module and a??would avoid a 500 if it ever moves.height="180"is ignored. Chart.js is responsive by default and recomputes the height fromaspectRatio; the canvas renders at 367px on this branch.- config.xml churn. The file is reindented from tabs to spaces,
<limited_countries>is dropped and the trailing newline is gone. Unrelated to the feature, and the missing end-of-file newline is worth restoring. - No
index.phpguard files in the newconfig/,src/,src/Controller,src/Typedirectories, while every other directory in the module has one. - Form labels are hardcoded strings (
'label' => 'Active cart') resolved throughtranslation_domain. They will not be picked up by the translation extractor, which only scanstrans()calls and Twig.
| 'DASHACTIVITY_CART_ABANDONED_MAX', | ||
| ]; | ||
|
|
||
| public function indexAction(Request $request): Response |
There was a problem hiding this comment.
No ACL check here. The admin firewall only requires IS_AUTHENTICATED and AdminSecurityListener is a no-op without the attribute, so any logged-in employee can read and save these settings whatever their profile. The hidden tab created in install() is not consulted by anything today.
#[AdminSecurity("is_granted('read', request.get('_legacy_controller'))")]
public function indexAction(Request $request): ResponseWorth guarding the write branch with update as well.
| <tab><![CDATA[administration]]></tab> | ||
| <is_configurable>0</is_configurable> | ||
| <need_instance>1</need_instance> | ||
| </module> No newline at end of file |
There was a problem hiding this comment.
The trailing newline is gone (\ No newline at end of file), and the file also switches from tabs to spaces and drops <limited_countries>. All unrelated to the feature, worth restoring to keep the diff focused.
hookDisplayAdminDashboardZoneOne(), the modern counterpart ofhookDashboardZoneOne(), so this module also feeds the migrated (Symfony) Back Office Dashboard. ReuseshookDashboardData()as-is (KPI values + traffic-source chart, real or simulated alike) instead of duplicating queries, and serializes the chart as a plain Chart.js config following the contract from PrestaShop/PrestaShop#42431; KPIs are rendered as plain server-side Twig, no JS needed. Legacy hooks are untouched. Version bumped 2.1.2 → 2.2.0 (minor, backward compatible with 8.2.0+).dashboardfeature flag enabled, install/reset this module. 2. Open the BO Dashboard: a KPI card and a traffic-sources doughnut chart render in Zone One.Draft PoC for the #41971 spike — the only module of the four covering both a chart and plain KPI values.