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

Handle 1.7 method in update function 'add_new_tab' #9194

Merged
merged 3 commits into from Jun 22, 2018

Conversation

Projects
None yet
6 participants
@Quetzacoalt91
Member

Quetzacoalt91 commented Jun 18, 2018

Questions Answers
Branch? 1.7.4.x
Description? While running a specific upgrade script, it appeared the access system was not adapted for 1.7 versions. The upgrade method has been updated to prevent merchants to loose access to the module pages with the new tabs.
Type? bug fix
Category? BO
BC breaks? Nope
Deprecations? Nope
Fixed ticket? /
How to test? From PrestaShop 1.7.3.3, start an upgrade to 1.7.4.0 or launch the following code on a database from version older than 1.7.4.0.

You can use the following code, for instance in index.php after require(dirname(__FILE__).'/config/config.inc.php');

require_once(__DIR__.'/install-dev/upgrade/php/ps_1740_update_module_tabs.php');
ps_1740_update_module_tabs();
die('ok');

This change is Reviewable

@prestonBot prestonBot added the 1.7.4.x label Jun 18, 2018

@Quetzacoalt91 Quetzacoalt91 added this to the 1.7.4.0 milestone Jun 18, 2018

');
}
foreach (array('CREATE', 'READ', 'UPDATE', 'DELETE') as $role) {

This comment has been minimized.

@PierreRambaud

PierreRambaud Jun 18, 2018

Contributor

Maybe use const instead string =)

This comment has been minimized.

@Quetzacoalt91

Quetzacoalt91 Jun 18, 2018

Member

You mean these one, on which I should apply a strtoupper?

class PageVoter extends Voter
{
    const CREATE = 'create';

    const UPDATE = 'update';

    const DELETE = 'delete';

    const READ   = 'read';

This comment has been minimized.

@PierreRambaud

PierreRambaud Jun 18, 2018

Contributor

Yep like this:

$roleToAdd = strtoupper('ROLE_MOD_TAB_' . $className . '_' . $role);

This comment has been minimized.

@Quetzacoalt91

Quetzacoalt91 Jun 18, 2018

Member

Here you go!

@@ -71,16 +73,16 @@ function add_new_tab($className, $name, $id_parent, $returnId = false, $parentTa
');
}
foreach (array('CREATE', 'READ', 'UPDATE', 'DELETE') as $role) {
foreach (array(PageVoter::CREATE, PageVoter::READ, PageVoter::UPDATE, PageVoter::DELETE) as $role) {

This comment has been minimized.

@PierreRambaud

PierreRambaud Jun 18, 2018

Contributor

❤️

@@ -53,12 +55,42 @@ function add_new_tab($className, $name, $id_parent, $returnId = false, $parentTa
');
}
Db::getInstance()->execute('INSERT IGNORE INTO `'._DB_PREFIX_.'access` (`id_profile`, `id_tab`, `view`, `add`, `edit`, `delete`)
if (version_compare(_PS_VERSION_, '1.7.0.0', '<')) {

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 18, 2018

Contributor

why do we need to do that on a commit only available in 1.7.4+?

This comment has been minimized.

@Quetzacoalt91

Quetzacoalt91 Jun 18, 2018

Member

What do you mean? This method is used by several methods in install-dev/upgrade/php.

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 18, 2018

Contributor

yes, but why do we do that? in which case you enter in the "if"?

This comment has been minimized.

@PierreRambaud

PierreRambaud Jun 18, 2018

Contributor

Agreed with @mickaelandrieu, you are pushing this change in 1.7.4.x, so PS_VERSION can't be 1.6 or something under 1.7 oO

This comment has been minimized.

@Quetzacoalt91

Quetzacoalt91 Jun 18, 2018

Member

You never know from where you start the upgrade. PS_VERSION is not updated at each upgrade file. 😉

It might be 1.6 in some case.

@Quetzacoalt91

This comment has been minimized.

Member

Quetzacoalt91 commented Jun 18, 2018

I declared a new method for 1.7.

I want to call explicitly the method for 1.7, instead of letting the script guessing on which version I'm running.

@eternoendless

This comment has been minimized.

Member

eternoendless commented Jun 20, 2018

Reviewed 2 of 2 files at r3.
Review status: all files reviewed, 4 unresolved discussions (waiting on @PierreRambaud, @mickaelandrieu, and @Quetzacoalt91)


install-dev/upgrade/php/add_new_tab.php, line 132 at r3 (raw file):

        $roleToAdd = strtoupper('ROLE_MOD_TAB_'.$className.'_'.$role);
        Db::getInstance()->execute('INSERT IGNORE INTO `'._DB_PREFIX_.'authorization_role` (`slug`)
            VALUES ("'.$roleToAdd.'")');

Aren't you missing a pSQL here?


Comments from Reviewable

@Quetzacoalt91

This comment has been minimized.

Member

Quetzacoalt91 commented Jun 20, 2018

install-dev/upgrade/php/add_new_tab.php, line 132 at r3 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Aren't you missing a pSQL here?

I considered it wasn't needed, as the data is not provided by any user.


Comments from Reviewable

@marionf marionf added QA ✔️ and removed waiting for QA labels Jun 21, 2018

@eternoendless

This comment has been minimized.

Member

eternoendless commented Jun 22, 2018

Thank you @Quetzacoalt91

@eternoendless eternoendless merged commit a8566fe into PrestaShop:1.7.4.x Jun 22, 2018

1 of 3 checks passed

Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details
code-review/reviewable 4 discussions left (eternoendless, mickaelandrieu, PierreRambaud)
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