New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate Modules Catalogue page to Symfony #8372

Merged
merged 5 commits into from Oct 12, 2017

Conversation

Projects
None yet
6 participants
@mickaelandrieu
Contributor

mickaelandrieu commented Sep 27, 2017

Questions Answers
Branch? develop
Description? See screenshot
Type? improvement
Category? BO
BC breaks? no
Deprecations? no
Forge? http://forge.prestashop.com/browse/BOGOSS-50
How to test? Access Improve > Modules > Modules Catalog

ok


This change is Reviewable

@PrestaShop PrestaShop deleted a comment from codacy-bot Sep 27, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Sep 28, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Sep 28, 2017

@Quetzacoalt91

This comment has been minimized.

Show comment
Hide comment
@Quetzacoalt91

Quetzacoalt91 Oct 2, 2017

Member

@mickaelandrieu The link displayed in the side menu must be updated, or we cannot reach your page.

Member

Quetzacoalt91 commented Oct 2, 2017

@mickaelandrieu The link displayed in the side menu must be updated, or we cannot reach your page.

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Oct 3, 2017

Contributor

@Quetzacoalt91 you're right, let me fix it 👍

edit => done

Contributor

mickaelandrieu commented Oct 3, 2017

@Quetzacoalt91 you're right, let me fix it 👍

edit => done

@PrestaShop PrestaShop deleted a comment from codacy-bot Oct 3, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Oct 3, 2017

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless

eternoendless Oct 4, 2017

Member

Reviewed 8 of 9 files at r1, 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 5 unresolved discussions.


src/PrestaShopBundle/Controller/Admin/Improve/StoreController.php, line 36 at r2 (raw file):

 * Responsible of "Improve > Modules > Modules Catalog" page display
 */
class StoreController extends FrameworkBundleAdminController

Why is it called StoreController? 🤔


src/PrestaShopBundle/Controller/Admin/Improve/StoreController.php, line 59 at r2 (raw file):

    }

    private function getAddonsUrl(Request $request)

Please add Phpdoc to all new methods


src/PrestaShopBundle/Controller/Admin/Improve/StoreController.php, line 65 at r2 (raw file):

        $currencyCode = $this->getContext()->currency->iso_code;
        $languageCode = $this->getContext()->language->iso_code;
        $countryCode = $this->getContext()->country->iso_code;

You should store the result of $this->getContext() in a variable in order to avoid repeated calls


src/PrestaShopBundle/Resources/config/admin/routing_module.yml, line 85 at r2 (raw file):

admin_module_store:
    path:     /store

Why is it "store" instead of "catalog"?


src/PrestaShopBundle/Resources/views/Admin/Improve/Module/store.html.twig, line 28 at r2 (raw file):

{% block content %}
  {{ pageContent|raw }}

Why did you drop the iframe and make its content go through the backend?


Comments from Reviewable

Member

eternoendless commented Oct 4, 2017

Reviewed 8 of 9 files at r1, 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 5 unresolved discussions.


src/PrestaShopBundle/Controller/Admin/Improve/StoreController.php, line 36 at r2 (raw file):

 * Responsible of "Improve > Modules > Modules Catalog" page display
 */
class StoreController extends FrameworkBundleAdminController

Why is it called StoreController? 🤔


src/PrestaShopBundle/Controller/Admin/Improve/StoreController.php, line 59 at r2 (raw file):

    }

    private function getAddonsUrl(Request $request)

Please add Phpdoc to all new methods


src/PrestaShopBundle/Controller/Admin/Improve/StoreController.php, line 65 at r2 (raw file):

        $currencyCode = $this->getContext()->currency->iso_code;
        $languageCode = $this->getContext()->language->iso_code;
        $countryCode = $this->getContext()->country->iso_code;

You should store the result of $this->getContext() in a variable in order to avoid repeated calls


src/PrestaShopBundle/Resources/config/admin/routing_module.yml, line 85 at r2 (raw file):

admin_module_store:
    path:     /store

Why is it "store" instead of "catalog"?


src/PrestaShopBundle/Resources/views/Admin/Improve/Module/store.html.twig, line 28 at r2 (raw file):

{% block content %}
  {{ pageContent|raw }}

Why did you drop the iframe and make its content go through the backend?


Comments from Reviewable

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Oct 9, 2017

Contributor

Why is it called StoreController? 🤔

catalog is already taken in the another "Module" page :/ it's a mistake but it's too late.

Why did you drop the iframe and make its content go through the backend?

To be honest, I don't see the value of this iframe: let's the controller do its job. The same was done in the old legacy controller, so I guess we are "iso functional" here.

I'll fix the other comments today 👍

Contributor

mickaelandrieu commented Oct 9, 2017

Why is it called StoreController? 🤔

catalog is already taken in the another "Module" page :/ it's a mistake but it's too late.

Why did you drop the iframe and make its content go through the backend?

To be honest, I don't see the value of this iframe: let's the controller do its job. The same was done in the old legacy controller, so I guess we are "iso functional" here.

I'll fix the other comments today 👍

@PrestaShop PrestaShop deleted a comment from codacy-bot Oct 9, 2017

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless

eternoendless Oct 9, 2017

Member

Review status: all files reviewed at latest revision, 5 unresolved discussions.


src/PrestaShopBundle/Controller/Admin/Improve/StoreController.php, line 36 at r2 (raw file):

catalog is already taken in the another "Module" page :/ it's a mistake but it's too late.

Why not AddonsCatalog or ModulesCatalog? Some lines below there's this:

    /**
     * @var string The controller name for routing.
     */
    const CONTROLLER_NAME = 'AdminAddonsCatalog';

src/PrestaShopBundle/Resources/views/Admin/Improve/Module/store.html.twig, line 28 at r2 (raw file):

To be honest, I don't see the value of this iframe: let's the controller do its job.

There are two problems here:

  1. @rGaillard raised the issue that including addons content directly could open an attack vector on all shops through addons. We need to keep the iframe.
  2. Server-side requests like this create unnecessary load on the server and increase page load time, because the client won't get anything until the server has fully loaded the remote data. It can be the other way around for very specific cases (i.e. if the remote page is slow or if it's a static content than can be stored in cache) -- but I don't think it's worth it for this one. We should generally try to avoid server-side requests unless there's a specific and well-analyzed need.

Comments from Reviewable

Member

eternoendless commented Oct 9, 2017

Review status: all files reviewed at latest revision, 5 unresolved discussions.


src/PrestaShopBundle/Controller/Admin/Improve/StoreController.php, line 36 at r2 (raw file):

catalog is already taken in the another "Module" page :/ it's a mistake but it's too late.

Why not AddonsCatalog or ModulesCatalog? Some lines below there's this:

    /**
     * @var string The controller name for routing.
     */
    const CONTROLLER_NAME = 'AdminAddonsCatalog';

src/PrestaShopBundle/Resources/views/Admin/Improve/Module/store.html.twig, line 28 at r2 (raw file):

To be honest, I don't see the value of this iframe: let's the controller do its job.

There are two problems here:

  1. @rGaillard raised the issue that including addons content directly could open an attack vector on all shops through addons. We need to keep the iframe.
  2. Server-side requests like this create unnecessary load on the server and increase page load time, because the client won't get anything until the server has fully loaded the remote data. It can be the other way around for very specific cases (i.e. if the remote page is slow or if it's a static content than can be stored in cache) -- but I don't think it's worth it for this one. We should generally try to avoid server-side requests unless there's a specific and well-analyzed need.

Comments from Reviewable

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Oct 9, 2017

Contributor

@rGaillard raised the issue that including addons content directly could open an attack vector on all shops through addons. We need to keep the iframe.

We can't "keep" it as there is no iframe at start but ok, I have no strong opinion on it and don't wan't to lose more time on a mostly static page ;)

edit: finally no, using an iframe for the same content ruins the styles.

With iframe:

testiframe

Without iframe:

noiframe

I don't wan't to lose time either at redesigning contents that comes from addons website too, so what do we do here?

Contributor

mickaelandrieu commented Oct 9, 2017

@rGaillard raised the issue that including addons content directly could open an attack vector on all shops through addons. We need to keep the iframe.

We can't "keep" it as there is no iframe at start but ok, I have no strong opinion on it and don't wan't to lose more time on a mostly static page ;)

edit: finally no, using an iframe for the same content ruins the styles.

With iframe:

testiframe

Without iframe:

noiframe

I don't wan't to lose time either at redesigning contents that comes from addons website too, so what do we do here?

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Oct 9, 2017

Contributor

Why not AddonsCatalog or ModulesCatalog? Some lines below there's this:

ModulesCatalog or AddonsCatalog creates confusion with catalogAction from ModuleController.

const CONTROLLER_NAME = 'AdminAddonsCatalog';

this refers to the old Controller, there is nothing I can do here (or I have to update security rights on database: no gain but potential risks on Shops upgrades).

Contributor

mickaelandrieu commented Oct 9, 2017

Why not AddonsCatalog or ModulesCatalog? Some lines below there's this:

ModulesCatalog or AddonsCatalog creates confusion with catalogAction from ModuleController.

const CONTROLLER_NAME = 'AdminAddonsCatalog';

this refers to the old Controller, there is nothing I can do here (or I have to update security rights on database: no gain but potential risks on Shops upgrades).

@Quetzacoalt91

This comment has been minimized.

Show comment
Hide comment
@Quetzacoalt91

Quetzacoalt91 Oct 9, 2017

Member

@eternoendless @mickaelandrieu, for your information this page will be moved / updated on PS 1.7.4 for a merge in the current module page. Do not spend too much time on it. :)

Member

Quetzacoalt91 commented Oct 9, 2017

@eternoendless @mickaelandrieu, for your information this page will be moved / updated on PS 1.7.4 for a merge in the current module page. Do not spend too much time on it. :)

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless

eternoendless Oct 12, 2017

Member

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


src/PrestaShopBundle/Controller/Admin/Improve/StoreController.php, line 36 at r2 (raw file):

ModulesCatalog or AddonsCatalog creates confusion with catalogAction from ModuleController.

Ok, can we settle for AddonsStore?


src/PrestaShopBundle/Resources/views/Admin/Improve/Module/store.html.twig, line 28 at r2 (raw file):

We can't "keep" it as there is no iframe at start

I took a better look at the old controller and it looks like it already did the server-side request, and only showed the iframe if there was no content from the remote server 🤔 ...

So if this is going to be refactored later (it will be an iframe if I'm not mistaken), we can leave the server-side request until then.


Comments from Reviewable

Member

eternoendless commented Oct 12, 2017

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


src/PrestaShopBundle/Controller/Admin/Improve/StoreController.php, line 36 at r2 (raw file):

ModulesCatalog or AddonsCatalog creates confusion with catalogAction from ModuleController.

Ok, can we settle for AddonsStore?


src/PrestaShopBundle/Resources/views/Admin/Improve/Module/store.html.twig, line 28 at r2 (raw file):

We can't "keep" it as there is no iframe at start

I took a better look at the old controller and it looks like it already did the server-side request, and only showed the iframe if there was no content from the remote server 🤔 ...

So if this is going to be refactored later (it will be an iframe if I'm not mistaken), we can leave the server-side request until then.


Comments from Reviewable

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Oct 12, 2017

Contributor

Review status: 6 of 10 files reviewed at latest revision, 3 unresolved discussions.


src/PrestaShopBundle/Controller/Admin/Improve/AddonsStoreController.php, line 65 at r2 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

You should store the result of $this->getContext() in a variable in order to avoid repeated calls

done


src/PrestaShopBundle/Resources/config/admin/routing_module.yml, line 85 at r2 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Why is it "store" instead of "catalog"?

done too :)


Comments from Reviewable

Contributor

mickaelandrieu commented Oct 12, 2017

Review status: 6 of 10 files reviewed at latest revision, 3 unresolved discussions.


src/PrestaShopBundle/Controller/Admin/Improve/AddonsStoreController.php, line 65 at r2 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

You should store the result of $this->getContext() in a variable in order to avoid repeated calls

done


src/PrestaShopBundle/Resources/config/admin/routing_module.yml, line 85 at r2 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Why is it "store" instead of "catalog"?

done too :)


Comments from Reviewable

@PrestaShop PrestaShop deleted a comment from codacy-bot Oct 12, 2017

@PrestaShop PrestaShop deleted a comment from codacy-bot Oct 12, 2017

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless

eternoendless Oct 12, 2017

Member

Reviewed 1 of 11 files at r1, 6 of 6 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

Member

eternoendless commented Oct 12, 2017

Reviewed 1 of 11 files at r1, 6 of 6 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@eternoendless eternoendless added this to the 1.7.3.0 milestone Oct 12, 2017

@codacy-bot

This comment has been minimized.

Show comment
Hide comment
@codacy-bot

codacy-bot Oct 12, 2017

Codacy Here is an overview of what got changed by this pull request:

Complexity increasing per file
==============================
- classes/Link.php  2
         

See the complete overview on Codacy

codacy-bot commented Oct 12, 2017

Codacy Here is an overview of what got changed by this pull request:

Complexity increasing per file
==============================
- classes/Link.php  2
         

See the complete overview on Codacy

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless

eternoendless Oct 12, 2017

Member

:lgtm:


Reviewed 1 of 1 files at r5.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

Member

eternoendless commented Oct 12, 2017

:lgtm:


Reviewed 1 of 1 files at r5.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@marionf marionf added QA ✔️ and removed waiting for QA labels Oct 12, 2017

@Quetzacoalt91 Quetzacoalt91 merged commit f5b4fd4 into PrestaShop:develop Oct 12, 2017

3 checks passed

codacy/pr Good work! A positive pull request.
Details
code-review/reviewable 10 files reviewed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Quetzacoalt91

This comment has been minimized.

Show comment
Hide comment
@Quetzacoalt91

Quetzacoalt91 Oct 12, 2017

Member

One more! Thank you @mickaelandrieu

Member

Quetzacoalt91 commented Oct 12, 2017

One more! Thank you @mickaelandrieu

@mickaelandrieu mickaelandrieu deleted the mickaelandrieu:migration/modules-catalog branch Oct 12, 2017

@matks matks added the migration label Sep 18, 2018

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