Feed the Symfony dashboard's Zone One hook with KPIs and a chart, add a settings page

[nicosomb]

Questions	Answers
Description?	Adds hookDisplayAdminDashboardZoneOne(), the modern counterpart of hookDashboardZoneOne(), so this module also feeds the migrated (Symfony) Back Office Dashboard. Reuses hookDashboardData() 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+).
Type?	new feature
BC breaks?	no
Deprecations?	no
Fixed ticket?	Fixes PrestaShop/PrestaShop#41971.
How to test?	1. On a shop with PrestaShop/PrestaShop#42431 and the dashboard feature 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.

[ps-jarvis]

This pull request seems to contain new translation strings. I have summarized them below to ease up review:

• Modules.Dashactivity.Admin
  • How long (in minutes) after the last action a cart is to be considered as abandoned (default: 1440 min, 24h).
  • min
  • How long (in minutes) after the last action a cart is no longer to be considered as abandoned (default: 2880 min, 48h).
• Admin.Actions
  • Activity overview settings

(Note: this is an automated message, but answering it will reach a real human)

[mattgoud]

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): Response

Note 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

• FrameworkBundleAdminController is @deprecated since 9.0 in favour of PrestaShopAdminController. 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 before parent::install(), so a failing install leaves an orphan tab behind. Inverting the order, or cleaning up on failure, would be safer.
• The token in getConfigUrl() is dead weight. router is aliased to prestashop.router which already appends _token, and in PS 9 Tools::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 from aspectRatio; 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.php guard files in the new config/, src/, src/Controller, src/Type directories, while every other directory in the module has one.
• Form labels are hardcoded strings ('label' => 'Active cart') resolved through translation_domain. They will not be picked up by the translation extractor, which only scans trans() calls and Twig.

[jolelievre]

Suggested change

	$this->version = '2.2.0';
	$this->version = '3.0.0';

[jolelievre]

Suggested change

	<version><![CDATA[2.2.0]]></version>
	<version><![CDATA[3.0.0]]></version>

[mattgoud]

Second pass, tested with PrestaShop/PrestaShop#42431 on develop, upgrade replayed from 2.1.2:
version 2.2.0, displayAdminDashboardZoneOne registered, configuration tab created.

Everything from my first review is fixed (upgrade script, ACL, catalogue, install order, config.xml,
index.php guards). KPIs and the traffic sources doughnut render in zone one and survive the AJAX
date range refresh; the legacy zone one is unchanged.

Left: demo mode (inline), @jolelievre's 3.0.0 suggestion (rename upgrade-2.2.0.php with it), and
the release order: the template uses Card / ChartCard, so this must ship after
PrestaShop/PrestaShop#42431. Worth a line in the description.

[mattgoud]

One more point, from a multistore pass I had skipped in my review above.

The settings page writes with the static Configuration::updateValue(), so the scope silently follows the shop context and the page never shows it. On a two shop setup:

1. From the context of shop 2, saving "Abandoned cart (min)" = 12 creates an override for shop 2 only.
2. Back in All stores, the page shows the global value (24), saving 36 reports success, and shop 2 keeps its override with no hint anywhere on the page.

DASHACTIVITY_CART_ABANDONED_MIN
id_shop_group  id_shop  value
NULL           NULL     36     <- saved from All stores
1              2        12     <- saved from shop 2, still what shop 2 uses

The legacy getContent() behaves the same, so this is not a regression. But it is what the core settings form pattern solves (.ai/Component/Forms/SETTINGS.md, .ai/MULTISTORE.md, skill create-settings-form): a configuration class extending AbstractMultistoreConfiguration, with multistore_configuration_key on each field, renders the per shop checkbox and shows the override, and the base Core\Form\Handler gives other modules the action{HookName}Form / action{HookName}Save hooks. All of it exists on 8.2 (AbstractMultistoreConfiguration, MultistoreCheckboxEnabler, Handler with the same constructor), so it fits the 8.2 minimum.

Since these modules are meant as the reference for the new dashboard, worth doing here or tracking as a follow up, your call. Same code in PrestaShop/dashproducts#78 and PrestaShop/dashgoals#57.

[mattgoud]

Retested b3c65c6, upgrade replayed from 2.1.2: version 3.0.0, displayAdminDashboardZoneOne registered, configuration tab created. The demo mode check works (tested above), the page and the zone one widget render.

Correction on my previous review: upgrade-2.2.0.php does not need renaming for 3.0.0, the core runs every script above the installed version and up to the new one, and it did run here.

Approving; ships after PrestaShop/PrestaShop#42431 (Card / ChartCard). The multistore point stays a suggestion.

[cnavarro-prestashop]

Only 2 comments

[cnavarro-prestashop]

Same comment than here : PrestaShop/dashproducts#78 (comment)

[cnavarro-prestashop]

You upgrade dashactivity to 3.0.0, it may be more consistent to rename the method and the file upgrade_module_3_0_0

[cnavarro-prestashop]

It's in minutes right ? DASHACTIVITY_CART_ABANDONED_MIN and DASHACTIVITY_CART_ABANDONED_MAX mention 24 hours as default value. We should have 1440 in delay choices

[mattgoud]

Retested 7cd5fb9 with an upgrade from 2.1.2: the minutes fix breaks existing settings (inline). Everything else OK.

[mattgoud]

Shops upgrading keep the old 24 / 48, which are not in these choices: the page shows 30 for both, and saving it untouched writes 30 / 30, so the abandoned carts window is empty and the KPI stays at 0. Reproduced on an upgraded shop.

The 3.0.0 upgrade script could convert them (24 → 1440, 48 → 2880, what "hrs" meant), or the form could keep a stored value that is not in the list.

[mattgoud]

Screenshot for the inline comment above: upgraded shop with 24 / 48 stored, both fields show 30.