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

Integration of PrestaTrust in module management #8378

Merged
merged 42 commits into from Nov 6, 2017

Conversation

@Quetzacoalt91
Member

Quetzacoalt91 commented Sep 28, 2017

Questions Answers
Branch? develop
Description? This PR adds PrestaTrust on the module page, which checks & informs the merchant about the property and the integrity of the module uploaded on the shop.
Type? new feature
Category? BO
BC breaks? Nope
Deprecations? Nope
Fixed ticket?
How to test? You must use an API currently on a test server.

capture du 2017-09-28 15-33-44


This change is Reviewable

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

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

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

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

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

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

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

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

@Quetzacoalt91 Quetzacoalt91 removed the WIP label Oct 4, 2017

@Quetzacoalt91 Quetzacoalt91 changed the title from WIP - Integration of PrestaTrust in module management to Integration of PrestaTrust in module management Oct 4, 2017

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

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

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

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

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless

eternoendless Oct 5, 2017

Member

Reviewed 11 of 20 files at r1, 1 of 2 files at r2, 13 of 13 files at r3.
Review status: all files reviewed at latest revision, 42 unresolved discussions, some commit checks failed.


admin-dev/themes/default/js/bundle/module/module_card.js, line 60 at r3 (raw file):

    };
    
    this.confirmPrestaTrust = function(result) {

Please add a JSDoc header on your new functions.

It is important that each function describes:

  • A short summary of what it does
  • Type (and if possible, a description) of each parameter
  • Type and description of what it returns
  • Exceptions thrown and when

Documenting functions is important because

  • It enables autocompletion by IDEs
  • It eases code understanding

Example:

/**
 * Assign the project to an employee.

 * @param {Object} employee - The employee who is responsible for the project.
 * @param {String} employee.name - The name of the employee.
 * @param {String} employee.department - The employee's department.
 *
 * @return {Project} Project instance
 */
Project.prototype.assign = function(employee) {
  // ...
}

You can find out more about JSDoc here.


admin-dev/themes/default/js/bundle/module/module_card.js, line 60 at r3 (raw file):

    };
    
    this.confirmPrestaTrust = function(result) {

You should use a named function expression:

this.confirmPrestaTrust = function confirmPrestaTrust(result) {

Read more here


admin-dev/themes/default/js/bundle/module/module_card.js, line 63 at r3 (raw file):

        var modal = $("#modal-prestatrust");
        var module = result.module.attributes;
        var that = this;

You should use const for all three.

According to Airbnb coding style, we should use const whenever possible, let if reassign is needed, and try to avoid var altogether.


admin-dev/themes/default/js/bundle/module/module_card.js, line 69 at r3 (raw file):

        }

        const alertClass = module.prestatrust.status?'success':'warning';

Please add spaces around the ? and : characters


admin-dev/themes/default/js/bundle/module/module_card.js, line 83 at r3 (raw file):

        modal.find("#pstrust-name").text(module.displayName);
        modal.find("#pstrust-author").text(module.author);
        modal.find("#pstrust-name").text(module.displayName);

Duplicate line (see line 81)


admin-dev/themes/default/js/bundle/module/module_card.js, line 84 at r3 (raw file):

        modal.find("#pstrust-author").text(module.author);
        modal.find("#pstrust-name").text(module.displayName);
        modal.find("#pstrust-label").attr("class", "text-"+alertClass).text(module.prestatrust.status?'OK':'KO');

Please add spaces around the ? and : characters


admin-dev/themes/default/js/bundle/module/module_card.js, line 89 at r3 (raw file):

        modal.find(".pstrust-install").off('click').on('click', function() {
            // Find related form, update it and submit it
            var install_button = $(that.moduleActionMenuInstallLinkSelector, '.module-item[data-tech-name="'+module.name+'"]');

Please add spaces around the + operator


admin-dev/themes/default/js/bundle/module/module_card.js, line 89 at r3 (raw file):

        modal.find(".pstrust-install").off('click').on('click', function() {
            // Find related form, update it and submit it
            var install_button = $(that.moduleActionMenuInstallLinkSelector, '.module-item[data-tech-name="'+module.name+'"]');

Can't we use an id instead of a data-attribute?


admin-dev/themes/default/js/bundle/module/module_card.js, line 189 at r3 (raw file):

                var moduleTechName = Object.keys(result)[0];
                if (result[moduleTechName].status === false) {
                    if (typeof result[moduleTechName].confirmation_subject !== 'undefined') {

If this file is babelized, you can simply make this assertion:

if (result[moduleTechName].confirmation_subject !== undefined) {

src/Adapter/Addons/AddonsDataProvider.php, line 167 at r3 (raw file):

                }
                break;
            case 'must-have-themes':

Did you accidentally delete this case or was it on purpose?


src/Adapter/Module/Module.php, line 310 at r3 (raw file):

    protected function instanciateLegacyModule()
    {
        // Temporary: This test prevents an error when switching branches with the cache. Can be removed at the next release (when we will be sure that it is defined)

What't the "next release" in this context? 1.7.3, 1.7.3.1 or 1.7.4?


src/Adapter/Module/ModulePresenter.php, line 60 at r3 (raw file):


        $attributes = $module->attributes->all();
        $attributes = $this->addPicos($attributes);

I don't really know what pico means in this context, but I think it would be simpler if this method only returned the data we need to add to $attributes['pico'] instead of mutating $attributes.

As an added, bonus, there will be less data to copy over after the method call :)


src/Adapter/Module/ModulePresenter.php, line 83 at r3 (raw file):

    }

    private function addPicos(array $attributes)

Please add a PhpDoc header on your new methods.

It is important that each method describes:

  • A short summary of what it does
  • Type (and if possible, a description) of each parameter
  • Type and description of what it returns
  • Exceptions thrown and when

If your method is an extension of a parent class' method, you can use @inheritdoc.

Documenting methods is important because

  • It enables autocompletion by IDEs
  • It eases code understanding

Example:

   /**
     * Executes the current command.
     *
     * This method is not abstract because you can use this class
     * as a concrete class. In this case, instead of defining the
     * execute() method, you set the code to execute by passing
     * a Closure to the setCode() method.
     *
     * @param InputInterface  $input  An InputInterface instance
     * @param OutputInterface $output An OutputInterface instance
     *
     * @return null|int null or 0 if everything went fine, or an error code
     *
     * @throws LogicException When this abstract method is not implemented
     *
     * @see setCode()
     */
    protected function execute(InputInterface $input, OutputInterface $output)
    {
        // ...
    }

You can find out more about PhpDoc by reading the proposed PSR-5 standard.


src/Adapter/Module/PrestaTrust/ModuleEventSubscriber.php, line 47 at r3 (raw file):

     * @var boolean
     */
    public $enabled;

Why not set this on construction instead and make it private?


src/Adapter/Module/PrestaTrust/ModuleEventSubscriber.php, line 49 at r3 (raw file):

    public $enabled;
    
    public function __construct(PrestaTrustChecker $checker)

Missing Phpdoc for this method


src/Adapter/Module/PrestaTrust/ModuleEventSubscriber.php, line 54 at r3 (raw file):

    }
    
    public static function getSubscribedEvents()

Missing Phpdoc for this method (you can use @inheritdoc)


src/Adapter/Module/PrestaTrust/ModuleEventSubscriber.php, line 61 at r3 (raw file):

    }

    public function onNewModule(ModuleManagementEvent $event)

Missing Phpdoc for this method


src/Adapter/Module/PrestaTrust/PrestaTrustChecker.php, line 39 at r1 (raw file):

Previously, xBorderie (Xavier) wrote…

Seems to me that it was found, but that it is invalid because the module is used on another shop.

"Warning, the purchase proof cannot be found. This license may have already been used on another shop."


src/Adapter/Module/PrestaTrust/PrestaTrustChecker.php, line 40 at r1 (raw file):

is not found.

cannot be found.


src/Adapter/Module/PrestaTrust/PrestaTrustChecker.php, line 60 at r3 (raw file):

    protected $domain;

    public function __construct(Cache $cache, ApiClient $apiClient, array $shop_info)

Missing Phpdoc for this method


src/Adapter/Module/PrestaTrust/PrestaTrustChecker.php, line 71 at r3 (raw file):

     * 
     * @param Module $module
     * @return void

You can omit@return if it is void


src/Adapter/Module/PrestaTrust/PrestaTrustChecker.php, line 71 at r3 (raw file):

     * 
     * @param Module $module
     * @return void

Wait a minute, a getter that returns void? O_o

I think loadDetailsIntoModule would be a more appropriate an descriptive name


src/Adapter/Module/PrestaTrust/PrestaTrustChecker.php, line 174 at r3 (raw file):

     * @return boolean
     */
    protected function isModuleCompliant(Module $module)

For clarity's sake, please rename this method to either moduleIsCompliant or isCompliant. Otherwise it can be confusing.


src/Adapter/Module/PrestaTrust/PrestaTrustChecker.php, line 195 at r3 (raw file):

    }

    protected function hasHexPrefix($str)

Missing Phpdoc for this method


src/Adapter/Module/PrestaTrust/PrestaTrustChecker.php, line 201 at r3 (raw file):

    }

    protected function requestCheck($hash, $shop_url, $contract)

Missing Phpdoc for this method


src/Core/Addon/Module/ModuleRepository.php, line 160 at r3 (raw file):

    }

    public function setPrestaTrustChecker(PrestaTrustChecker $checker)

Missing Phpdoc for this method


src/Core/Addon/Module/Exception/UnconfirmedModuleActionException.php, line 86 at r3 (raw file):

        $this->subject = $subject;
        return $this;
    }

Missing Phpdoc for all methods


src/PrestaShopBundle/Controller/Admin/ModuleController.php, line 354 at r3 (raw file):

                ),
                'Admin.Modules.Notification'
            );

I think array_replace would bring a cleaner syntax:

$response[$module] = array_replace(
    $response[$module],
    array(
        'status' => false,
        'confirmation_subject' => $e->getSubject(),
        // ...
    )
);

src/PrestaShopBundle/Resources/views/Admin/Module/Includes/modal_confirm_prestatrust.html.twig, line 63 at r3 (raw file):

      <div class="modal-footer">
        <div id="pstrust-btn-property-ok">
            <button type="button" class="btn btn-outline-secondary" data-dismiss="modal">{{"Back to modules list"|trans({}, 'Admin.Actions')}}</button>

We should start adding id or data-testid to all buttons, anchors, and containers of dynamic data that might be useful for functional tests


src/PrestaShopBundle/Service/DataProvider/Marketplace/ApiClient.php, line 81 at r3 (raw file):

     * In case you reuse the Client, you may want to clean the previous parameters
     */
    public function init()

I think reset would be a better suited name


src/PrestaShopBundle/Service/DataProvider/Marketplace/ApiClient.php, line 86 at r3 (raw file):

    }

    public function getCheckCustomer()

Missing Phpdoc for this method


src/PrestaShopBundle/Service/DataProvider/Marketplace/ApiClient.php, line 114 at r3 (raw file):

        $responseArray = json_decode($response);

        return $responseArray->modules;

Are you 100% sure that $responseArray is an object? If it is, why is it called responseArray?
Are you certain that it will always contain a property called modules?


src/PrestaShopBundle/Service/DataProvider/Marketplace/ApiClient.php, line 129 at r3 (raw file):

    }

    public function getPrestaTrustCheck($hash, $sc_address)

Missing Phpdoc for this method


src/PrestaShopBundle/Service/DataProvider/Marketplace/ApiClient.php, line 178 at r3 (raw file):

    }

    public function getModuleZip($moduleId)

Missing Phpdoc for this method


src/PrestaShopBundle/Service/DataProvider/Marketplace/ApiClient.php, line 203 at r3 (raw file):

    }

    public function getCustomerThemes()

Missing Phpdoc for this method


src/PrestaShopBundle/Service/DataProvider/Marketplace/ApiClient.php, line 210 at r3 (raw file):

        ;

        $responseArray = json_decode($response);

Same comment here about "Array"


src/PrestaShopBundle/Service/DataProvider/Marketplace/ApiClient.php, line 293 at r3 (raw file):

    }

    public function setShopUrl($shop_url)

Missing Phpdoc for these three methods


Comments from Reviewable

Member

eternoendless commented Oct 5, 2017

Reviewed 11 of 20 files at r1, 1 of 2 files at r2, 13 of 13 files at r3.
Review status: all files reviewed at latest revision, 42 unresolved discussions, some commit checks failed.


admin-dev/themes/default/js/bundle/module/module_card.js, line 60 at r3 (raw file):

    };
    
    this.confirmPrestaTrust = function(result) {

Please add a JSDoc header on your new functions.

It is important that each function describes:

  • A short summary of what it does
  • Type (and if possible, a description) of each parameter
  • Type and description of what it returns
  • Exceptions thrown and when

Documenting functions is important because

  • It enables autocompletion by IDEs
  • It eases code understanding

Example:

/**
 * Assign the project to an employee.

 * @param {Object} employee - The employee who is responsible for the project.
 * @param {String} employee.name - The name of the employee.
 * @param {String} employee.department - The employee's department.
 *
 * @return {Project} Project instance
 */
Project.prototype.assign = function(employee) {
  // ...
}

You can find out more about JSDoc here.


admin-dev/themes/default/js/bundle/module/module_card.js, line 60 at r3 (raw file):

    };
    
    this.confirmPrestaTrust = function(result) {

You should use a named function expression:

this.confirmPrestaTrust = function confirmPrestaTrust(result) {

Read more here


admin-dev/themes/default/js/bundle/module/module_card.js, line 63 at r3 (raw file):

        var modal = $("#modal-prestatrust");
        var module = result.module.attributes;
        var that = this;

You should use const for all three.

According to Airbnb coding style, we should use const whenever possible, let if reassign is needed, and try to avoid var altogether.


admin-dev/themes/default/js/bundle/module/module_card.js, line 69 at r3 (raw file):

        }

        const alertClass = module.prestatrust.status?'success':'warning';

Please add spaces around the ? and : characters


admin-dev/themes/default/js/bundle/module/module_card.js, line 83 at r3 (raw file):

        modal.find("#pstrust-name").text(module.displayName);
        modal.find("#pstrust-author").text(module.author);
        modal.find("#pstrust-name").text(module.displayName);

Duplicate line (see line 81)


admin-dev/themes/default/js/bundle/module/module_card.js, line 84 at r3 (raw file):

        modal.find("#pstrust-author").text(module.author);
        modal.find("#pstrust-name").text(module.displayName);
        modal.find("#pstrust-label").attr("class", "text-"+alertClass).text(module.prestatrust.status?'OK':'KO');

Please add spaces around the ? and : characters


admin-dev/themes/default/js/bundle/module/module_card.js, line 89 at r3 (raw file):

        modal.find(".pstrust-install").off('click').on('click', function() {
            // Find related form, update it and submit it
            var install_button = $(that.moduleActionMenuInstallLinkSelector, '.module-item[data-tech-name="'+module.name+'"]');

Please add spaces around the + operator


admin-dev/themes/default/js/bundle/module/module_card.js, line 89 at r3 (raw file):

        modal.find(".pstrust-install").off('click').on('click', function() {
            // Find related form, update it and submit it
            var install_button = $(that.moduleActionMenuInstallLinkSelector, '.module-item[data-tech-name="'+module.name+'"]');

Can't we use an id instead of a data-attribute?


admin-dev/themes/default/js/bundle/module/module_card.js, line 189 at r3 (raw file):

                var moduleTechName = Object.keys(result)[0];
                if (result[moduleTechName].status === false) {
                    if (typeof result[moduleTechName].confirmation_subject !== 'undefined') {

If this file is babelized, you can simply make this assertion:

if (result[moduleTechName].confirmation_subject !== undefined) {

src/Adapter/Addons/AddonsDataProvider.php, line 167 at r3 (raw file):

                }
                break;
            case 'must-have-themes':

Did you accidentally delete this case or was it on purpose?


src/Adapter/Module/Module.php, line 310 at r3 (raw file):

    protected function instanciateLegacyModule()
    {
        // Temporary: This test prevents an error when switching branches with the cache. Can be removed at the next release (when we will be sure that it is defined)

What't the "next release" in this context? 1.7.3, 1.7.3.1 or 1.7.4?


src/Adapter/Module/ModulePresenter.php, line 60 at r3 (raw file):


        $attributes = $module->attributes->all();
        $attributes = $this->addPicos($attributes);

I don't really know what pico means in this context, but I think it would be simpler if this method only returned the data we need to add to $attributes['pico'] instead of mutating $attributes.

As an added, bonus, there will be less data to copy over after the method call :)


src/Adapter/Module/ModulePresenter.php, line 83 at r3 (raw file):

    }

    private function addPicos(array $attributes)

Please add a PhpDoc header on your new methods.

It is important that each method describes:

  • A short summary of what it does
  • Type (and if possible, a description) of each parameter
  • Type and description of what it returns
  • Exceptions thrown and when

If your method is an extension of a parent class' method, you can use @inheritdoc.

Documenting methods is important because

  • It enables autocompletion by IDEs
  • It eases code understanding

Example:

   /**
     * Executes the current command.
     *
     * This method is not abstract because you can use this class
     * as a concrete class. In this case, instead of defining the
     * execute() method, you set the code to execute by passing
     * a Closure to the setCode() method.
     *
     * @param InputInterface  $input  An InputInterface instance
     * @param OutputInterface $output An OutputInterface instance
     *
     * @return null|int null or 0 if everything went fine, or an error code
     *
     * @throws LogicException When this abstract method is not implemented
     *
     * @see setCode()
     */
    protected function execute(InputInterface $input, OutputInterface $output)
    {
        // ...
    }

You can find out more about PhpDoc by reading the proposed PSR-5 standard.


src/Adapter/Module/PrestaTrust/ModuleEventSubscriber.php, line 47 at r3 (raw file):

     * @var boolean
     */
    public $enabled;

Why not set this on construction instead and make it private?


src/Adapter/Module/PrestaTrust/ModuleEventSubscriber.php, line 49 at r3 (raw file):

    public $enabled;
    
    public function __construct(PrestaTrustChecker $checker)

Missing Phpdoc for this method


src/Adapter/Module/PrestaTrust/ModuleEventSubscriber.php, line 54 at r3 (raw file):

    }
    
    public static function getSubscribedEvents()

Missing Phpdoc for this method (you can use @inheritdoc)


src/Adapter/Module/PrestaTrust/ModuleEventSubscriber.php, line 61 at r3 (raw file):

    }

    public function onNewModule(ModuleManagementEvent $event)

Missing Phpdoc for this method


src/Adapter/Module/PrestaTrust/PrestaTrustChecker.php, line 39 at r1 (raw file):

Previously, xBorderie (Xavier) wrote…

Seems to me that it was found, but that it is invalid because the module is used on another shop.

"Warning, the purchase proof cannot be found. This license may have already been used on another shop."


src/Adapter/Module/PrestaTrust/PrestaTrustChecker.php, line 40 at r1 (raw file):

is not found.

cannot be found.


src/Adapter/Module/PrestaTrust/PrestaTrustChecker.php, line 60 at r3 (raw file):

    protected $domain;

    public function __construct(Cache $cache, ApiClient $apiClient, array $shop_info)

Missing Phpdoc for this method


src/Adapter/Module/PrestaTrust/PrestaTrustChecker.php, line 71 at r3 (raw file):

     * 
     * @param Module $module
     * @return void

You can omit@return if it is void


src/Adapter/Module/PrestaTrust/PrestaTrustChecker.php, line 71 at r3 (raw file):

     * 
     * @param Module $module
     * @return void

Wait a minute, a getter that returns void? O_o

I think loadDetailsIntoModule would be a more appropriate an descriptive name


src/Adapter/Module/PrestaTrust/PrestaTrustChecker.php, line 174 at r3 (raw file):

     * @return boolean
     */
    protected function isModuleCompliant(Module $module)

For clarity's sake, please rename this method to either moduleIsCompliant or isCompliant. Otherwise it can be confusing.


src/Adapter/Module/PrestaTrust/PrestaTrustChecker.php, line 195 at r3 (raw file):

    }

    protected function hasHexPrefix($str)

Missing Phpdoc for this method


src/Adapter/Module/PrestaTrust/PrestaTrustChecker.php, line 201 at r3 (raw file):

    }

    protected function requestCheck($hash, $shop_url, $contract)

Missing Phpdoc for this method


src/Core/Addon/Module/ModuleRepository.php, line 160 at r3 (raw file):

    }

    public function setPrestaTrustChecker(PrestaTrustChecker $checker)

Missing Phpdoc for this method


src/Core/Addon/Module/Exception/UnconfirmedModuleActionException.php, line 86 at r3 (raw file):

        $this->subject = $subject;
        return $this;
    }

Missing Phpdoc for all methods


src/PrestaShopBundle/Controller/Admin/ModuleController.php, line 354 at r3 (raw file):

                ),
                'Admin.Modules.Notification'
            );

I think array_replace would bring a cleaner syntax:

$response[$module] = array_replace(
    $response[$module],
    array(
        'status' => false,
        'confirmation_subject' => $e->getSubject(),
        // ...
    )
);

src/PrestaShopBundle/Resources/views/Admin/Module/Includes/modal_confirm_prestatrust.html.twig, line 63 at r3 (raw file):

      <div class="modal-footer">
        <div id="pstrust-btn-property-ok">
            <button type="button" class="btn btn-outline-secondary" data-dismiss="modal">{{"Back to modules list"|trans({}, 'Admin.Actions')}}</button>

We should start adding id or data-testid to all buttons, anchors, and containers of dynamic data that might be useful for functional tests


src/PrestaShopBundle/Service/DataProvider/Marketplace/ApiClient.php, line 81 at r3 (raw file):

     * In case you reuse the Client, you may want to clean the previous parameters
     */
    public function init()

I think reset would be a better suited name


src/PrestaShopBundle/Service/DataProvider/Marketplace/ApiClient.php, line 86 at r3 (raw file):

    }

    public function getCheckCustomer()

Missing Phpdoc for this method


src/PrestaShopBundle/Service/DataProvider/Marketplace/ApiClient.php, line 114 at r3 (raw file):

        $responseArray = json_decode($response);

        return $responseArray->modules;

Are you 100% sure that $responseArray is an object? If it is, why is it called responseArray?
Are you certain that it will always contain a property called modules?


src/PrestaShopBundle/Service/DataProvider/Marketplace/ApiClient.php, line 129 at r3 (raw file):

    }

    public function getPrestaTrustCheck($hash, $sc_address)

Missing Phpdoc for this method


src/PrestaShopBundle/Service/DataProvider/Marketplace/ApiClient.php, line 178 at r3 (raw file):

    }

    public function getModuleZip($moduleId)

Missing Phpdoc for this method


src/PrestaShopBundle/Service/DataProvider/Marketplace/ApiClient.php, line 203 at r3 (raw file):

    }

    public function getCustomerThemes()

Missing Phpdoc for this method


src/PrestaShopBundle/Service/DataProvider/Marketplace/ApiClient.php, line 210 at r3 (raw file):

        ;

        $responseArray = json_decode($response);

Same comment here about "Array"


src/PrestaShopBundle/Service/DataProvider/Marketplace/ApiClient.php, line 293 at r3 (raw file):

    }

    public function setShopUrl($shop_url)

Missing Phpdoc for these three methods


Comments from Reviewable

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

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

@Quetzacoalt91

This comment has been minimized.

Show comment
Hide comment
@Quetzacoalt91

Quetzacoalt91 Oct 9, 2017

Member

Review status: 22 of 25 files reviewed at latest revision, 49 unresolved discussions, some commit checks failed.


admin-dev/themes/default/js/bundle/module/module_card.js, line 89 at r3 (raw file):

confirmPrestaTrust

There is no ID created in order to display a module card (probably because each of them is created 2 times). The data attribute is already the main way to get the card of a specific module.


admin-dev/themes/default/js/bundle/module/module_card.js, line 189 at r3 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

If this file is babelized, you can simply make this assertion:

if (result[moduleTechName].confirmation_subject !== undefined) {

As far as I know, this file's is sent as-is on the BO pages.


src/Adapter/Addons/AddonsDataProvider.php, line 167 at r3 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Did you accidentally delete this case or was it on purpose?

I do not see the diff on reviewable, but I remember deleting some calls because they actually does not exist (nor called in the code).


src/Adapter/Module/Module.php, line 310 at r3 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

What't the "next release" in this context? 1.7.3, 1.7.3.1 or 1.7.4?

Doesn't matter which one. :)


src/Adapter/Module/PrestaTrust/ModuleEventSubscriber.php, line 47 at r3 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Why not set this on construction instead and make it private?

Because I was thinking to separated the dependencies from the options.
@mickaelandrieu, what is the most common way to proceed?


src/Adapter/Module/PrestaTrust/ModuleEventSubscriber.php, line 49 at r3 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Missing Phpdoc for this method

This constructor is used by the dependency injector and the expected classes are already written before the parameters. :)


src/Adapter/Module/PrestaTrust/PrestaTrustChecker.php, line 40 at r1 (raw file):

inheritdoc
This content MUST be translatable, and I forgot to inject the translator. Will be fixed.


src/Adapter/Module/PrestaTrust/PrestaTrustChecker.php, line 60 at r3 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Missing Phpdoc for this method

The class properties already got their documentation. And the constructor is only called from the Symfony dependency injection.


src/Adapter/Module/PrestaTrust/PrestaTrustChecker.php, line 174 at r3 (raw file):

loadDetailsIntoModule
How can it be confusing? Don't you say "Is the module compliant?"?


src/PrestaShopBundle/Controller/Admin/ModuleController.php, line 354 at r3 (raw file):

Admin.Modules.Notification
I would prefer sticking to the placeholder feature of the Symfony translator, as we all know how it works.
Also, in your example, you removed the percentages signs, which can be disturbing for the translation maintainers and our merchants.


src/PrestaShopBundle/Resources/views/Admin/Module/Includes/modal_confirm_prestatrust.html.twig, line 63 at r3 (raw file):

Admin.Global
Okay, to be done with the QA team on another PR!


src/PrestaShopBundle/Service/DataProvider/Marketplace/ApiClient.php, line 114 at r3 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Are you 100% sure that $responseArray is an object? If it is, why is it called responseArray?
Are you certain that it will always contain a property called modules?

The API documentation says so, even if there is no modules to install. But I added a check for you anyway.


Comments from Reviewable

Member

Quetzacoalt91 commented Oct 9, 2017

Review status: 22 of 25 files reviewed at latest revision, 49 unresolved discussions, some commit checks failed.


admin-dev/themes/default/js/bundle/module/module_card.js, line 89 at r3 (raw file):

confirmPrestaTrust

There is no ID created in order to display a module card (probably because each of them is created 2 times). The data attribute is already the main way to get the card of a specific module.


admin-dev/themes/default/js/bundle/module/module_card.js, line 189 at r3 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

If this file is babelized, you can simply make this assertion:

if (result[moduleTechName].confirmation_subject !== undefined) {

As far as I know, this file's is sent as-is on the BO pages.


src/Adapter/Addons/AddonsDataProvider.php, line 167 at r3 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Did you accidentally delete this case or was it on purpose?

I do not see the diff on reviewable, but I remember deleting some calls because they actually does not exist (nor called in the code).


src/Adapter/Module/Module.php, line 310 at r3 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

What't the "next release" in this context? 1.7.3, 1.7.3.1 or 1.7.4?

Doesn't matter which one. :)


src/Adapter/Module/PrestaTrust/ModuleEventSubscriber.php, line 47 at r3 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Why not set this on construction instead and make it private?

Because I was thinking to separated the dependencies from the options.
@mickaelandrieu, what is the most common way to proceed?


src/Adapter/Module/PrestaTrust/ModuleEventSubscriber.php, line 49 at r3 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Missing Phpdoc for this method

This constructor is used by the dependency injector and the expected classes are already written before the parameters. :)


src/Adapter/Module/PrestaTrust/PrestaTrustChecker.php, line 40 at r1 (raw file):

inheritdoc
This content MUST be translatable, and I forgot to inject the translator. Will be fixed.


src/Adapter/Module/PrestaTrust/PrestaTrustChecker.php, line 60 at r3 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Missing Phpdoc for this method

The class properties already got their documentation. And the constructor is only called from the Symfony dependency injection.


src/Adapter/Module/PrestaTrust/PrestaTrustChecker.php, line 174 at r3 (raw file):

loadDetailsIntoModule
How can it be confusing? Don't you say "Is the module compliant?"?


src/PrestaShopBundle/Controller/Admin/ModuleController.php, line 354 at r3 (raw file):

Admin.Modules.Notification
I would prefer sticking to the placeholder feature of the Symfony translator, as we all know how it works.
Also, in your example, you removed the percentages signs, which can be disturbing for the translation maintainers and our merchants.


src/PrestaShopBundle/Resources/views/Admin/Module/Includes/modal_confirm_prestatrust.html.twig, line 63 at r3 (raw file):

Admin.Global
Okay, to be done with the QA team on another PR!


src/PrestaShopBundle/Service/DataProvider/Marketplace/ApiClient.php, line 114 at r3 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Are you 100% sure that $responseArray is an object? If it is, why is it called responseArray?
Are you certain that it will always contain a property called modules?

The API documentation says so, even if there is no modules to install. But I added a check for you anyway.


Comments from Reviewable

@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

Reviewed 11 of 11 files at r6.
Review status: all files reviewed at latest revision, 32 unresolved discussions, some commit checks failed.


admin-dev/themes/default/js/bundle/module/module_card.js, line 89 at r3 (raw file):

Previously, Quetzacoalt91 (Thomas N) wrote…

confirmPrestaTrust

There is no ID created in order to display a module card (probably because each of them is created 2 times). The data attribute is already the main way to get the card of a specific module.

We really, really need to stop using attributes for selection. Attribute selectors are 50x slower than class selectors, and a whooping 100x slower than id selectors (see: https://jsperf.com/id-vs-class-vs-tag-selectors/2). Maybe not right now, but soon.


admin-dev/themes/default/js/bundle/module/module_card.js, line 189 at r3 (raw file):

Previously, Quetzacoalt91 (Thomas N) wrote…

As far as I know, this file's is sent as-is on the BO pages.

Dammit, I thought it was babelized! I'm going to have to ask you to revert the change from var to const then. Sorry!


src/Adapter/Module/Module.php, line 310 at r3 (raw file):

Previously, Quetzacoalt91 (Thomas N) wrote…

Doesn't matter which one. :)

Ok then


src/Adapter/Module/PrestaTrust/ModuleEventSubscriber.php, line 61 at r3 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Missing Phpdoc for this method

Thanks! Remember that you can skip @return if it's void


src/Adapter/Module/PrestaTrust/PrestaTrustChecker.php, line 60 at r3 (raw file):

The class properties already got their documentation

O rly?

Screen Shot 2017-10-09 at 15.29.02.png

Screen Shot 2017-10-09 at 15.29.14.png

No, seriously, please add Phpdoc. It takes 30 seconds to make and can go a long way to help a developer understand what a class does without having to reverse engineering it and its dependencies.

And really, services.yml doesn't count as documentation.

:D


src/Adapter/Module/PrestaTrust/PrestaTrustChecker.php, line 174 at r3 (raw file):

Previously, Quetzacoalt91 (Thomas N) wrote…

loadDetailsIntoModule
How can it be confusing? Don't you say "Is the module compliant?"?

"Is module compliant" reads like "is module-compliant" which would mean "is (something) compliant with a module". This confusion exists because in english adjectives usually go before nouns, for example "red Ferrari".

Take "Is Ferrari red". Since there's no article (aka "a" in "a Ferrari"), "Ferrari red" can be interpreted as a compound adjective: "red like a Ferrari" instead of a suite of noun and adjective: "a red Ferrari".

A bright red Toyota can be Ferrari-red, but never a red Ferrari 😛


src/Adapter/Module/PrestaTrust/PrestaTrustChecker.php, line 201 at r3 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Missing Phpdoc for this method

What's in the returned array?


src/Core/Addon/Module/Exception/UnconfirmedModuleActionException.php, line 65 at r6 (raw file):


    /**
     * Action getter (install, uninstall, reset ...)

You made me think of this comic 😂

Actually, it's better to explain the semantic and context behind the method and the returned object, for example:

Returns the action that was being performed when the exception was thrown (install, uninstall, reset...)

src/Core/Addon/Module/Exception/UnconfirmedModuleActionException.php, line 105 at r6 (raw file):


    /**
     * Subject setter

I should be able to understand what subject is from reading this doc, but right now I'm not 😉


src/PrestaShopBundle/Controller/Admin/ModuleController.php, line 354 at r3 (raw file):

I would prefer sticking to the placeholder feature of the Symfony translator, as we all know how it works...

I agree... but I'm not talking about the translator 😛

It's my fault for giving a very short sample. When I re-read my comment, for a moment I was confused too.

I'm actually talking about overloading $response[$module][...] like this:

$response[$module] = array_replace(
    $response[$module],
    array(
        'status' => false,
        'confirmation_subject' => $e->getSubject(),
        'module' => $this->getPresentedProducts($modules)[0],
        'msg'    => $translator->trans(
            'Confirmation needed by module %module% on %action% (%subject%).',
            array(
                '%subject%' => $e->getSubject(),
                '%action%' => $e->getAction(),
                '%module%' => $module,
            ),
            'Admin.Modules.Notification'
        )
    )
);

...instead of the current way:

$response[$module]['status'] = false;
$response[$module]['confirmation_subject'] = $e->getSubject();
$response[$module]['module'] = $this->getPresentedProducts($modules)[0];
$response[$module]['msg'] = // ...

src/PrestaShopBundle/Service/DataProvider/Marketplace/ApiClient.php, line 114 at r3 (raw file):

Previously, Quetzacoalt91 (Thomas N) wrote…

The API documentation says so, even if there is no modules to install. But I added a check for you anyway.

It should throw an exception if the response is malformed


src/PrestaShopBundle/Service/DataProvider/Marketplace/ApiClient.php, line 129 at r3 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Missing Phpdoc for this method

What's sc_address?


src/PrestaShopBundle/Service/DataProvider/Marketplace/ApiClient.php, line 193 at r6 (raw file):

     * 
     * @param int $moduleId
     * @return binary content

There's no "binary" type in php. It's actually a binary string, so you can sign this like so:

@return string Binary content

Comments from Reviewable

Member

eternoendless commented Oct 9, 2017

Reviewed 11 of 11 files at r6.
Review status: all files reviewed at latest revision, 32 unresolved discussions, some commit checks failed.


admin-dev/themes/default/js/bundle/module/module_card.js, line 89 at r3 (raw file):

Previously, Quetzacoalt91 (Thomas N) wrote…

confirmPrestaTrust

There is no ID created in order to display a module card (probably because each of them is created 2 times). The data attribute is already the main way to get the card of a specific module.

We really, really need to stop using attributes for selection. Attribute selectors are 50x slower than class selectors, and a whooping 100x slower than id selectors (see: https://jsperf.com/id-vs-class-vs-tag-selectors/2). Maybe not right now, but soon.


admin-dev/themes/default/js/bundle/module/module_card.js, line 189 at r3 (raw file):

Previously, Quetzacoalt91 (Thomas N) wrote…

As far as I know, this file's is sent as-is on the BO pages.

Dammit, I thought it was babelized! I'm going to have to ask you to revert the change from var to const then. Sorry!


src/Adapter/Module/Module.php, line 310 at r3 (raw file):

Previously, Quetzacoalt91 (Thomas N) wrote…

Doesn't matter which one. :)

Ok then


src/Adapter/Module/PrestaTrust/ModuleEventSubscriber.php, line 61 at r3 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Missing Phpdoc for this method

Thanks! Remember that you can skip @return if it's void


src/Adapter/Module/PrestaTrust/PrestaTrustChecker.php, line 60 at r3 (raw file):

The class properties already got their documentation

O rly?

Screen Shot 2017-10-09 at 15.29.02.png

Screen Shot 2017-10-09 at 15.29.14.png

No, seriously, please add Phpdoc. It takes 30 seconds to make and can go a long way to help a developer understand what a class does without having to reverse engineering it and its dependencies.

And really, services.yml doesn't count as documentation.

:D


src/Adapter/Module/PrestaTrust/PrestaTrustChecker.php, line 174 at r3 (raw file):

Previously, Quetzacoalt91 (Thomas N) wrote…

loadDetailsIntoModule
How can it be confusing? Don't you say "Is the module compliant?"?

"Is module compliant" reads like "is module-compliant" which would mean "is (something) compliant with a module". This confusion exists because in english adjectives usually go before nouns, for example "red Ferrari".

Take "Is Ferrari red". Since there's no article (aka "a" in "a Ferrari"), "Ferrari red" can be interpreted as a compound adjective: "red like a Ferrari" instead of a suite of noun and adjective: "a red Ferrari".

A bright red Toyota can be Ferrari-red, but never a red Ferrari 😛


src/Adapter/Module/PrestaTrust/PrestaTrustChecker.php, line 201 at r3 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Missing Phpdoc for this method

What's in the returned array?


src/Core/Addon/Module/Exception/UnconfirmedModuleActionException.php, line 65 at r6 (raw file):


    /**
     * Action getter (install, uninstall, reset ...)

You made me think of this comic 😂

Actually, it's better to explain the semantic and context behind the method and the returned object, for example:

Returns the action that was being performed when the exception was thrown (install, uninstall, reset...)

src/Core/Addon/Module/Exception/UnconfirmedModuleActionException.php, line 105 at r6 (raw file):


    /**
     * Subject setter

I should be able to understand what subject is from reading this doc, but right now I'm not 😉


src/PrestaShopBundle/Controller/Admin/ModuleController.php, line 354 at r3 (raw file):

I would prefer sticking to the placeholder feature of the Symfony translator, as we all know how it works...

I agree... but I'm not talking about the translator 😛

It's my fault for giving a very short sample. When I re-read my comment, for a moment I was confused too.

I'm actually talking about overloading $response[$module][...] like this:

$response[$module] = array_replace(
    $response[$module],
    array(
        'status' => false,
        'confirmation_subject' => $e->getSubject(),
        'module' => $this->getPresentedProducts($modules)[0],
        'msg'    => $translator->trans(
            'Confirmation needed by module %module% on %action% (%subject%).',
            array(
                '%subject%' => $e->getSubject(),
                '%action%' => $e->getAction(),
                '%module%' => $module,
            ),
            'Admin.Modules.Notification'
        )
    )
);

...instead of the current way:

$response[$module]['status'] = false;
$response[$module]['confirmation_subject'] = $e->getSubject();
$response[$module]['module'] = $this->getPresentedProducts($modules)[0];
$response[$module]['msg'] = // ...

src/PrestaShopBundle/Service/DataProvider/Marketplace/ApiClient.php, line 114 at r3 (raw file):

Previously, Quetzacoalt91 (Thomas N) wrote…

The API documentation says so, even if there is no modules to install. But I added a check for you anyway.

It should throw an exception if the response is malformed


src/PrestaShopBundle/Service/DataProvider/Marketplace/ApiClient.php, line 129 at r3 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Missing Phpdoc for this method

What's sc_address?


src/PrestaShopBundle/Service/DataProvider/Marketplace/ApiClient.php, line 193 at r6 (raw file):

     * 
     * @param int $moduleId
     * @return binary content

There's no "binary" type in php. It's actually a binary string, so you can sign this like so:

@return string Binary content

Comments from Reviewable

@Quetzacoalt91

This comment has been minimized.

Show comment
Hide comment
@Quetzacoalt91

Quetzacoalt91 Oct 30, 2017

Member

Here you go @marionf

Member

Quetzacoalt91 commented Oct 30, 2017

Here you go @marionf

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

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

@Quetzacoalt91

This comment has been minimized.

Show comment
Hide comment
@Quetzacoalt91

Quetzacoalt91 Oct 30, 2017

Member

New commit added, in order to handle PrestaTrust on module zips coming from the upload feature. :)

Member

Quetzacoalt91 commented Oct 30, 2017

New commit added, in order to handle PrestaTrust on module zips coming from the upload feature. :)

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

@codacy-bot

This comment has been minimized.

Show comment
Hide comment
@codacy-bot

codacy-bot Nov 2, 2017

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

Issues
======
- Added 6
           

Complexity increasing per file
==============================
- src/Core/Addon/Module/ModuleManager.php  4
- admin-dev/themes/default/js/bundle/module/module.js  1
- src/Adapter/Module/ModulePresenter.php  5
- admin-dev/themes/default/js/bundle/module/module_card.js  9
- src/PrestaShopBundle/Service/DataProvider/Marketplace/ApiClient.php  10
- src/Adapter/Module/Module.php  1
- src/Core/Addon/Module/ModuleRepository.php  1
         

Complexity decreasing per file
==============================
+ src/Adapter/Addons/AddonsDataProvider.php  -23
+ src/Adapter/Module/ModuleZipManager.php  -3
+ src/PrestaShopBundle/Controller/Admin/ModuleController.php  -2
         

See the complete overview on Codacy

codacy-bot commented Nov 2, 2017

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

Issues
======
- Added 6
           

Complexity increasing per file
==============================
- src/Core/Addon/Module/ModuleManager.php  4
- admin-dev/themes/default/js/bundle/module/module.js  1
- src/Adapter/Module/ModulePresenter.php  5
- admin-dev/themes/default/js/bundle/module/module_card.js  9
- src/PrestaShopBundle/Service/DataProvider/Marketplace/ApiClient.php  10
- src/Adapter/Module/Module.php  1
- src/Core/Addon/Module/ModuleRepository.php  1
         

Complexity decreasing per file
==============================
+ src/Adapter/Addons/AddonsDataProvider.php  -23
+ src/Adapter/Module/ModuleZipManager.php  -3
+ src/PrestaShopBundle/Controller/Admin/ModuleController.php  -2
         

See the complete overview on Codacy

@PrestaShop PrestaShop deleted a comment from codacy-bot Nov 2, 2017

@Quetzacoalt91

This comment has been minimized.

Show comment
Hide comment
@Quetzacoalt91

Quetzacoalt91 Nov 2, 2017

Member

Today's changes are related to the last QA checks.

  • Send shop domain to all API requests
  • Only display the pico near the module name on module grid view
  • Fix display of large icon in the confirmation modal (assets needs to be regenerated)
Member

Quetzacoalt91 commented Nov 2, 2017

Today's changes are related to the last QA checks.

  • Send shop domain to all API requests
  • Only display the pico near the module name on module grid view
  • Fix display of large icon in the confirmation modal (assets needs to be regenerated)

@toutantic toutantic merged commit 637323a into PrestaShop:develop Nov 6, 2017

1 of 3 checks passed

codacy/pr Not so good... This pull request quality could be better.
Details
code-review/reviewable 13 files, 21 discussions left (AlexEven, eternoendless, margchop-prestashop, mickaelandrieu, Quetzacoalt91, xBorderie)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment