Skip to content

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

Open
nicosomb wants to merge 3 commits into
devfrom
feature/41971-zone-one-poc
Open

Feed the Symfony dashboard's Zone One hook with KPIs and a chart, add a settings page#63
nicosomb wants to merge 3 commits into
devfrom
feature/41971-zone-one-poc

Conversation

@nicosomb

Copy link
Copy Markdown
Contributor
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

ps-jarvis commented Aug 28, 2026

Copy link
Copy Markdown

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

  • Admin.Orderscustomers.Notification
    • Traffic sources
  • Admin.Actions
    • Activity overview settings

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

@nicosomb
nicosomb force-pushed the feature/41971-zone-one-poc branch from 26ef6d2 to 27c5263 Compare September 3, 2026 09:05
@nicosomb nicosomb changed the title PoC: feed the migrated Symfony dashboard with real KPI and traffic-source data Feed the Symfony dashboard's Zone One hook with KPIs and a chart, add a settings page Sep 3, 2026
@nicosomb
nicosomb marked this pull request as ready for review September 3, 2026 11:58
@github-project-automation github-project-automation Bot moved this to Ready for review in PR Dashboard Sep 3, 2026

@mattgoud mattgoud left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

'DASHACTIVITY_CART_ABANDONED_MAX',
];

public function indexAction(Request $request): Response

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Worth guarding the write branch with update as well.

Comment thread config.xml
<tab><![CDATA[administration]]></tab>
<is_configurable>0</is_configurable>
<need_instance>1</need_instance>
</module> No newline at end of file

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Ready for review

Development

Successfully merging this pull request may close these issues.

Dashboard Migration - Dynamic data refacto

3 participants