Introducing reusable way to display KPIs blocks in Back Office modern pages #9242

rokaszygmantas · 2018-07-01T17:52:30Z

Questions	Answers
Branch?	develop
Description?	We needed a common and re-usable system to display KPIs block everywhere in Symfony pages
Type?	improvement
Category?	BO
BC breaks?	no
Deprecations?	no
Fixed ticket?	no
How to test?	Compare to legacy translations page (only the page that you land in when going to International > Translations), for the KPI block on top of the page.

This change is

rokaszygmantas · 2018-07-01T17:58:20Z

src/Adapter/Kpi/EnabledLanguagesKpi.php

+    {
+        $enabledLanguages = $this->configuration->get('ENABLED_LANGUAGES');
+
+        $kpi = new HelperKpi();


Didn't want to increase the scope of this PR so I'm still relying on legacy HelperKpi to render the view, but the overall kpi implementation is not dependent on legacy code.

you're right :) it's the main purpose of using adapters

mickaelandrieu · 2018-07-02T08:29:39Z

src/Core/Kpi/Row/KpiRowFactory.php

+    /**
+     * @param KpiInterface ...$kpis
+     */
+    public function __construct(KpiInterface ...$kpis)


this won't works in PHP 5.6, isn't it? I'm sad because it's an elegant implementation :/

I think it will: http://php.net/manual/en/functions.arguments.php#functions.variable-arg-list

mickaelandrieu · 2018-07-02T12:22:36Z

Hello @rokaszygmantas,

thinking about it, can we merge only this part "first" and then make another contribution to migrate the page?

Wdyt?

mickaelandrieu · 2018-07-02T12:24:52Z

src/PrestaShopBundle/Controller/Admin/TranslationsController.php

+            'layoutTitle' => $this->trans('Translations', 'Admin.Navigation.Menu'),
+            'enableSidebar' => true,
+            'help_link' => $this->generateSidebarLink($legacyController),
+            'kpiRow' => $presentedKpiRow,


as KPI stuff is present in multiples pages, can we create a specific action to render the KPI block in CommonController instead?

sounds interesting, I'll see if that works

rokaszygmantas · 2018-07-02T12:25:39Z

yes sounds really good :) should I remove the stuff related to translations controller or can it stay unused? e.g. new action, routing

Edit: if the CommonController implementation works fine, I'll just remove the action from translations controller and will do that in a new PR.

rokaszygmantas · 2018-07-02T12:30:18Z

In this case we will also need to provide a way for QA to test this PR, how should we handle that?

mickaelandrieu · 2018-07-02T12:35:50Z

@rokaszygmantas you can keep the work done on translations controller so the QA team can test the "KPI block" only 👍

Great job making this reusable everywhere, I don't have time to report issues you have already fixed them :p Lithuanian people are wizards!

edit: I've updated the PR label, description and testing sections

rokaszygmantas · 2018-07-02T19:01:42Z

...dle/Resources/views/Admin/Improve/International/Translations/translations_settings.html.twig

-            {{ include('@PrestaShop/Admin/Common/Kpi/kpi_row.html.twig', {'kpiRow': kpiRow }) }}
+            {{ render(controller(
+                'PrestaShopBundle:Admin\\Common:renderKpiRow',
+                { 'factory': kpiRowFactory }


@mickaelandrieu can I render it somehow without passing the factory to twig? 🤔

why not pass the Row instead of the factory?

good idea :)

rokaszygmantas · 2018-07-02T20:58:54Z

js/admin.js

@@ -935,6 +935,10 @@ $(document).ready(function()
    if ($('.kpi-container').length) {
        refresh_kpis();
    }
+
+    $('.kpi-refresh').on('click', '.refresh-kpis', function () {


I had to add this code to legacy js for the legacy refresh code to pick up the new refresh button, otherwise I would have to create all the javascript for refreshing KPIs, which I want to avoid doing in this PR.

make sense :)

mickaelandrieu

good job

mickaelandrieu · 2018-07-03T01:01:42Z

...dle/Resources/views/Admin/Improve/International/Translations/translations_settings.html.twig

@@ -32,7 +32,7 @@
        <div class="row">
            {{ render(controller(
                'PrestaShopBundle:Admin\\Common:renderKpiRow',
-                { 'factory': kpiRowFactory }
+                { 'kpiRow': kpiRow }


does it sound better for you? it does for me :)

mickaelandrieu · 2018-07-03T01:02:33Z

js/admin.js

@@ -935,6 +935,10 @@ $(document).ready(function()
    if ($('.kpi-container').length) {
        refresh_kpis();
    }
+
+    $('.kpi-refresh').on('click', '.refresh-kpis', function () {


make sense :)

mickaelandrieu · 2018-07-03T01:05:08Z

src/Adapter/Kpi/MainCountryKpi.php

+            'action' => 'getKpi',
+            'kpi' => 'main_country',
+        ];
+        $kpi->source = $this->legacyContext->getAdminLink('AdminStats', true, $params);


if you only need legacy context to render an url, how about making this url an argument of your constructor instead? :)

oh yes, good idea :) no need to inject the whole context

my ~~secret~~ (not anymore!) challenge is to remove LegacyContext at some point, so when I can remove it from a class I'm happy 💃

hehe now everyone knows! 🙈

mickaelandrieu · 2018-07-03T06:46:18Z

src/Core/Kpi/Row/KpiRow.php

+    /**
+     * @return bool
+     */
+    public function getAllowRefresh()


maybe use a isser (isRefreshAllowed()) or an hasser here (hasRefreshAllowed) ?

WDYT?

sure, why not :)

rokaszygmantas · 2018-07-03T10:13:59Z

@mickaelandrieu thanks - addressed your feedback

mickaelandrieu · 2018-07-03T12:26:34Z

src/PrestaShopBundle/Resources/config/services/adapter/kpi.yml

            - '@translator'
            - '@prestashop.adapter.legacy.kpi_configuration'
+            - '@=service("prestashop.adapter.legacy.context").getAdminLink("AdminLanguages")'
+            - '@=service("prestashop.adapter.legacy.context").getAdminLink("AdminStats", true, {"ajax": 1, "action": "getKpi", "kpi": "enabled_languages"})'


lulz didn't even know this syntax was possible! I asked you to do some black voodoo here :D

it took me a while to make this work :D

it was not intended ^^

but it's still good, isn't it? Better than to inject the legacy context in the class :D

mickaelandrieu · 2018-07-06T22:32:57Z

Thanks @rokaszygmantas and @marionf

@rokaszygmantas we need docs in prestafony project, something basic like "how to add a KPI block in modern pages?" article

rokaszygmantas · 2018-07-06T22:38:32Z

I'll add some docs :)

Add implementation of KPIs

b1e36ad

prestonBot added develop Branch Waiting for wording Status: action required, waiting for wording labels Jul 1, 2018

rokaszygmantas commented Jul 1, 2018

View reviewed changes

rokaszygmantas added 4 commits July 1, 2018 21:09

Fix indentation

77929ae

Remove unused argument

5d597c3

Fix KPI legacy template path

8f2f542

Fix kpi row style

209832b

mickaelandrieu reviewed Jul 2, 2018

View reviewed changes

mickaelandrieu changed the title ~~[WIP] Migrate Improve > International > Translations page~~ Introducing reusable way to display KPIs blocks in Back Office modern pages Jul 2, 2018

LouiseBonnard removed the Waiting for wording Status: action required, waiting for wording label Jul 2, 2018

mickaelandrieu added the Improvement Type: Improvement label Jul 2, 2018

Add an action for KPI rendering in CommonController

24c50b8

rokaszygmantas commented Jul 2, 2018

View reviewed changes

rokaszygmantas added 2 commits July 2, 2018 22:52

Build kpi row before passing it to twing

443d127

Add kpi refresh button

58023a9

rokaszygmantas commented Jul 2, 2018

View reviewed changes

mickaelandrieu reviewed Jul 3, 2018

View reviewed changes

rokaszygmantas added 2 commits July 3, 2018 13:08

Get rid of legacy context in kpi adapters

d63f347

Rename getter to isser for refreshAllowed

80c5b7d

mickaelandrieu reviewed Jul 3, 2018

View reviewed changes

mickaelandrieu approved these changes Jul 5, 2018

View reviewed changes

mickaelandrieu added the Waiting for QA Status: action required, waiting for test feedback label Jul 5, 2018

marionf added QA ✔️ Status: check done, code approved and removed Waiting for QA Status: action required, waiting for test feedback labels Jul 5, 2018

mickaelandrieu merged commit decc6e8 into PrestaShop:develop Jul 6, 2018

mickaelandrieu added this to the 1.7.5.0 milestone Jul 6, 2018

rokaszygmantas deleted the migration/international_translations branch July 6, 2018 22:38

matks added the migration symfony migration project label Sep 18, 2018

matks mentioned this pull request Nov 22, 2021

Refactor KPI bar #26667

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introducing reusable way to display KPIs blocks in Back Office modern pages #9242

Introducing reusable way to display KPIs blocks in Back Office modern pages #9242

rokaszygmantas commented Jul 1, 2018 •

edited by mickaelandrieu

rokaszygmantas Jul 1, 2018

mickaelandrieu Jul 2, 2018

mickaelandrieu Jul 2, 2018

rokaszygmantas Jul 2, 2018

mickaelandrieu Jul 2, 2018

mickaelandrieu commented Jul 2, 2018

mickaelandrieu Jul 2, 2018

rokaszygmantas Jul 2, 2018

rokaszygmantas commented Jul 2, 2018 •

edited

rokaszygmantas commented Jul 2, 2018

mickaelandrieu commented Jul 2, 2018 •

edited

rokaszygmantas Jul 2, 2018

mickaelandrieu Jul 2, 2018

rokaszygmantas Jul 2, 2018

rokaszygmantas Jul 2, 2018

mickaelandrieu Jul 3, 2018

mickaelandrieu left a comment

mickaelandrieu Jul 3, 2018

mickaelandrieu Jul 3, 2018

mickaelandrieu Jul 3, 2018

rokaszygmantas Jul 3, 2018

mickaelandrieu Jul 3, 2018 •

edited

rokaszygmantas Jul 3, 2018

mickaelandrieu Jul 3, 2018

rokaszygmantas Jul 3, 2018

rokaszygmantas commented Jul 3, 2018

mickaelandrieu Jul 3, 2018

rokaszygmantas Jul 3, 2018

mickaelandrieu Jul 3, 2018

rokaszygmantas Jul 3, 2018

mickaelandrieu commented Jul 6, 2018 •

edited

rokaszygmantas commented Jul 6, 2018

Introducing reusable way to display KPIs blocks in Back Office modern pages #9242

Introducing reusable way to display KPIs blocks in Back Office modern pages #9242

Conversation

rokaszygmantas commented Jul 1, 2018 • edited by mickaelandrieu

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mickaelandrieu commented Jul 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rokaszygmantas commented Jul 2, 2018 • edited

rokaszygmantas commented Jul 2, 2018

mickaelandrieu commented Jul 2, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mickaelandrieu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mickaelandrieu Jul 3, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rokaszygmantas commented Jul 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mickaelandrieu commented Jul 6, 2018 • edited

rokaszygmantas commented Jul 6, 2018

rokaszygmantas commented Jul 1, 2018 •

edited by mickaelandrieu

rokaszygmantas commented Jul 2, 2018 •

edited

mickaelandrieu commented Jul 2, 2018 •

edited

mickaelandrieu Jul 3, 2018 •

edited

mickaelandrieu commented Jul 6, 2018 •

edited