Repository navigation
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) |
4977473 to
c75d3ce
Compare
There was a problem hiding this comment.
Tested locally against PrestaShop/PrestaShop#42431 with the dashboard flag on. The four tables render in zone two, and the fallback branches work correctly (the "No data" case on Best sellers and Top search, and the raw string body on Most viewed products showing the "Save global page views" notice). Reusing hookDashboardData() rather than duplicating the queries is the right call, and rendering tables server-side instead of pushing them through the JSON contract makes sense.
Two blockers and a few 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 dashproducts 2.2.1, neither displayAdminDashboardZoneTwo nor the AdminDashproductsConfiguration tab is ever created.
Reproduced locally: old version installed, 2.3.0 files dropped on disk, then the upgrade path replayed (ModuleManager::upgradeMigration() minus the download step):
dashproducts db=2.2.1 disk=2.3.0 needUpgrade=NO
No hook row, no tab, and Module::upgradeModuleVersion() is never called so ps_module.version stays on 2.2.1 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.3.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.2.php), so it needs an upgrade/upgrade-2.3.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, and the hidden 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:
#[AdminSecurity("is_granted('read', request.get('_legacy_controller'))")]
public function indexAction(Request $request): ResponseOnce 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.
Two rendering issues visible in the browser
-
The details button in Recent orders renders as an empty square.
getTableRecentOrders()builds it with<i class="icon-search"></i>, which is a legacy theme icon class that does not exist in the new BO theme (it uses Material icons). On the legacy dashboard it worked; here it needs<i class="material-icons">search</i>.
-
dashproducts.php:211has'wrapper_end' => '<span>'instead of</span>, which now renders a stray<span></span>in the amount column (€66.80<span></span>). Pre-existing module bug, but the new template surfaces it. Worth fixing while you are here. Side note: the new template readingwrapper_endis correct, and it incidentally fixes a long-standing legacy bug wherejs/admin/dashboard.js:129readswrapper_stopand therefore never closes these wrappers.
No data is a new string in a core catalogue
ps-jarvis flagged it, and it is indeed absent from translations/en-US/AdminGlobal.en-US.xlf. A module cannot add entries to core catalogues, so it will stay untranslated. It belongs in Modules.Dashproducts.Admin.
Smaller points
{{ cell.value|raw }}carries unescaped customer input:getTableTop10MostSearch()puts$term['keywords']straight intovalue. The legacy dashboard has the same hole (js/admin/dashboard.js:128injectsbody.valueas HTML), so this is not a regression, but it is exactly the "untyped arrays / raw HTML, XSS surface" pain point listed in PrestaShop/PrestaShop#41971. Since the transport is being redone anyway, splitting a plain-textvaluefrom an explicitlinkwould close it instead of carrying it over.FrameworkBundleAdminControlleris@deprecated since 9.0in favour ofPrestaShopAdminController. Worth deciding explicitly given the announced 8.2 compatibility.- 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, so the link carries the identical value twice. Verified in the browser on this branch. configUrlonly onloop.first. It works, but tying the gear to whichever table happens to come first is a bit arbitrary; worth confirming with UX.- config.xml churn. Reindented from tabs to spaces,
<limited_countries>dropped, trailing newline gone. Unrelated to the feature. - 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 resolved through
translation_domain, so the translation extractor will not pick them up.
The screenshot is attached to this review (GitHub-hosted). An archive copy also lives in mattgoud/qa-assets/dashproducts-78.
6b514bc to
813612f
Compare
| $this->name = 'dashproducts'; | ||
| $this->tab = 'administration'; | ||
| $this->version = '2.2.1'; | ||
| $this->version = '2.3.0'; |
There was a problem hiding this comment.
| $this->version = '2.3.0'; | |
| $this->version = '3.0.0'; |
| <displayName><![CDATA[Dashboard Products]]></displayName> | ||
| <version><![CDATA[2.2.1]]></version> | ||
| <description><![CDATA[Adds a block with a table of your latest orders and a ranking of your products]]></description> | ||
| <version><![CDATA[2.3.0]]></version> |
There was a problem hiding this comment.
| <version><![CDATA[2.3.0]]></version> | |
| <version><![CDATA[3.0.0]]></version> |
mattgoud
left a comment
There was a problem hiding this comment.
Second pass, tested with PrestaShop/PrestaShop#42431 on develop, upgrade replayed from 2.2.1.
Everything from my first review is fixed, and the XSS goes further than I asked: keywords, customer
names and order states are escaped at the source, so the legacy dashboard is fixed too. The four
tables render and survive the AJAX date range refresh.
Left: demo mode (inline), @jolelievre's 3.0.0 suggestion (rename upgrade-2.3.0.php with it), and
the release order after PrestaShop/PrestaShop#42431. Two nits: the category name at
dashproducts.php:344 is the last string cell reaching the |raw output unescaped (merchant data,
pre-existing), and a #[AdminSecurity] attribute next to the annotation would avoid the 9.x
deprecation notice.
|
One more point, from a multistore pass I had skipped in my review above: the settings page saves with the static Not a regression (the legacy |
mattgoud
left a comment
There was a problem hiding this comment.
Retested 73c1125, upgrade replayed from 2.2.1: version 3.0.0, displayAdminDashboardZoneTwo registered, configuration tab created. The four tables and the settings page render, demo mode is handled.
Correction on my previous review: upgrade-2.3.0.php does not need renaming for 3.0.0, it ran here.
Approving; ships after PrestaShop/PrestaShop#42431 (Card). The multistore point and the two nits stay optional.
| {% for row in entry.table.body %} | ||
| <tr> | ||
| {% for cell in row %} | ||
| <td class="{{ cell.class }}">{{ cell.wrapper_start|default('')|raw }}{{ cell.value|raw }}{{ cell.wrapper_end|default('')|raw }}</td> |
There was a problem hiding this comment.
Not in this PR but later it would be better to escape the cell depending its type instead using |raw which could be a security vulnerability.
| $form->handleRequest($request); | ||
|
|
||
| if ($form->isSubmitted() && $form->isValid()) { | ||
| $this->denyAccessUnlessGranted('update', $request->attributes->get('_legacy_controller')); |
There was a problem hiding this comment.
It's not to handle in @adminsecurity ?
| exit; | ||
| } | ||
|
|
||
| function upgrade_module_2_3_0($object) |
There was a problem hiding this comment.
You upgrade dashactivity to 3.0.0, it may be more consistent to rename the method and the file upgrade_module_3_0_0
hookDisplayAdminDashboardZoneTwo(), the modern counterpart ofhookDashboardZoneTwo(), so this module also feeds the migrated (Symfony) Back Office Dashboard. ReuseshookDashboardData()as-is (recent orders, best sellers, most viewed, top search tables) and renders them as plain server-side Twig — no JSON/JS contract needed for tables, unlike charts. Legacy hooks are untouched. Version bumped 2.2.1 → 2.3.0 (minor, backward compatible with 8.2.0+).dashboardfeature flag enabled, install/reset this module. 2. Open the BO Dashboard: 4 tables render in Zone Two (recent orders, best sellers, most viewed, top search).Draft PoC for the #41971 spike — the legacy data_table shape is reused almost verbatim, only the transport changes.