Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix not handled new tab ID during migration #8020

Merged
merged 2 commits into from Apr 13, 2018
Merged

Fix not handled new tab ID during migration #8020

merged 2 commits into from Apr 13, 2018

Conversation

soullivaneuh
Copy link
Contributor

@soullivaneuh soullivaneuh commented Jun 17, 2017 •
edited by eternoendless

Questions Answers
Branch? develop
Description? Tab IDs are changed since 1.7.0.0 and are not handled during migration
Type? bug fix
Category? IN
BC breaks? no
Deprecations? no
Fixed ticket? http://forge.prestashop.com/browse/BOOM-3269
http://forge.prestashop.com/browse/BOOM-3209
How to test? Use a 1.6.* database and try a migration.

This change is Reviewable

@soullivaneuh
Copy link
Contributor Author

I just found that the old tab IDs relation also affect this: http://forge.prestashop.com/browse/BOOM-3209

I'm working on on solution but this is linked to this PR. I think I'll update it later.

@soullivaneuh soullivaneuh changed the title IN: Update employee default_tab with new tab IDs IN: Fix not handled new tab ID during migration Jun 17, 2017
@soullivaneuh
Copy link
Contributor Author

I updated the PR title and description.

I also add a commit for access tab fix, but this is incomplete.

The access map is indeed restored, but with currently three issues:

First, on this capture:

image

We have a "Multiboutique" right (yellow highlight). I don't know from where this tab appeared, and it appeared twice!

We retrieve some module rights (red highlight), but I don't think they are supposed to be here.

On this capture:

image

You can see that the module access map is completely empty, even after module upgrade process.

This is maybe because I didn't try anything yet with the module_access_old table.

I'm still working on it, but if you have some clue to give, I'm hearing. 👍

@soullivaneuh
Copy link
Contributor Author

Update:

image

According to the old 1.6 app, I found the "Multiboutique" part, but I still don't know why it appeared twice on 1.7.

Plus, the module lines seem to be legitimate, but are misplaced.

This is maybe because I deactivate doDisableModules and doEnableModules during migration (they don't work on CLI). Though?

@soullivaneuh
Copy link
Contributor Author

"Multiboutique" appears twice also on a fresh install of PS 1.7 from GitHub (no data migration):

image

So it seems to be another "bug"...

@soullivaneuh
Copy link
Contributor Author

Concerning module access, it seems to be another problem: Corresponding authorization role are not created at all.

On a fresh install DB, if I run:

SELECT * FROM `ps_authorization_role` WHERE `slug` LIKE '%mod_module%'

I got 208 rows.

On a migrated from 1.6 database, I got nothing.

@soullivaneuh soullivaneuh mentioned this pull request Jun 17, 2017
Merged
@soullivaneuh
Copy link
Contributor Author

Please see #8021 for module missing roles.

@eternoendless eternoendless added the Bug Type: Bug label Aug 10, 2017
@soullivaneuh
Copy link
Contributor Author

I did a rebase of my branch. Ready for review.

LittleBigDev
LittleBigDev previously approved these changes Sep 5, 2017
View reviewed changes
eternoendless
eternoendless reviewed Sep 5, 2017
View reviewed changes
install-dev/upgrade/sql/1.7.0.0.sql Outdated

/* Save the new IDs */
UPDATE `PREFIX_tab_transit` tt SET `id_new_tab` = (
SELECT `id_tab` FROM `PREFIX_tab` WHERE CONCAT(`class_name`, COALESCE(`module`, '')) LIKE tt.`key`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why use LIKE and not =?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless tt.key contains a search pattern, = would be indeed better.

install-dev/upgrade/sql/1.7.0.0.sql Outdated
UPDATE `PREFIX_employee` e SET `default_tab` = (
SELECT IFNULL(`id_new_tab`,
/* If the tab does not exist anymore, fallback to the dashboard. */
(SELECT `id_tab` FROM `PREFIX_tab` WHERE `class_name` LIKE 'AdminDashboard' AND `module` IS NULL)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why use LIKE and not =?

@soullivaneuh
Copy link
Contributor Author

@eternoendless I did a quick modification to replace LIKE by =. It should work, but I don't have time right now for testing. Could you please handle that?

@eternoendless eternoendless added this to the 1.7.4.0 milestone Oct 3, 2017
@mickaelandrieu
Copy link
Contributor

Hello @soullivaneuh, sorry for the delay.

I want a last check by @jocel1, it's our MySQL master :)

For me it's ok, so I put my approuval on this.

Have a nice day!

mickaelandrieu
mickaelandrieu previously approved these changes Oct 30, 2017
View reviewed changes
jocel1
jocel1 previously approved these changes Nov 2, 2017
View reviewed changes
Copy link
Contributor

@jocel1 jocel1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@LittleBigDev LittleBigDev added the Waiting for QA Status: action required, waiting for test feedback label Nov 3, 2017
@soullivaneuh
Copy link
Contributor Author

It seems this can be merged now. :-)

@LittleBigDev
Copy link
Contributor

We are still waiting for a review (see BOOM-3209)

@soullivaneuh
Copy link
Contributor Author

@LittleBigDev Oh, do I have to do anything else or just wait?

@LittleBigDev
Copy link
Contributor

Waiting for QA. Ping @vincentbz, @marionf, @prasanthSelvar

@prasanthSelvar
Copy link

Hello @soullivaneuh,

Sorry for the delay.
I still have the access issue when I try a migration to 1.7. Each access profile are still disabled after upgrade.
Could you please explain more precisely how did you migrate to 1.7 and add your modifications?
Thanks by advance!

@prasanthSelvar prasanthSelvar added the Waiting for author Status: action required, waiting for author feedback label Dec 4, 2017
@prasanthSelvar prasanthSelvar removed the Waiting for QA Status: action required, waiting for test feedback label Dec 4, 2017
@soullivaneuh
IN: Update employee default_tab with new tab IDs
d945628 
Tab IDs are changed since 1.7.0.0 and employee default_tab reference was
wrong.

Fixes http://forge.prestashop.com/browse/BOOM-3269
@soullivaneuh
IN: Update old access tables with new tab IDs
705b294 
This have to be done in order to make new access structure migration
working.
@soullivaneuh
Copy link
Contributor Author

Hello @prasanthSelvar,

I still have the access issue when I try a migration to 1.7. Each access profile are still disabled after upgrade.

Could you please be more specific? You mean the PR don't fix the issue? Also could you please retry? I did a rebase since the other one is merged.

Could you please explain more precisely how did you migrate to 1.7 and add your modifications?

I did a cli upgrade with having to disable some methods. Please see #8020 (comment)

Also, this PR is quite old and I don't fully remember the issue, but by whiling on the history, I see I did report some not resolved issues and asking for some clues: #8020 (comment)

If I didn't say anything else, the help is still needed. I'll try to work again on the PR soon but I'm afraid I will be stuck at the same conclusion.

@marionf marionf added Waiting for QA Status: action required, waiting for test feedback and removed Waiting for author Status: action required, waiting for author feedback labels Apr 11, 2018
@prasanthSelvar prasanthSelvar added QA ✔️ Status: check done, code approved and removed Waiting for QA Status: action required, waiting for test feedback labels Apr 11, 2018
@eternoendless eternoendless changed the title IN: Fix not handled new tab ID during migration Fix not handled new tab ID during migration Apr 13, 2018
@eternoendless
Copy link
Member

Thank you @soullivaneuh !

Better late than never :)

@eternoendless eternoendless merged commit fdf8caf into PrestaShop:develop Apr 13, 2018
@Quetzacoalt91
Copy link
Member

Note this will fix an issue only for people who makes an 1.6 >> 1.7 upgrade from now.

Merchants already on PS 1.7.0.0 probably need a new upgrade file.

This was referenced Jul 3, 2018
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LittleBigDev LittleBigDev LittleBigDev left review comments

@eternoendless eternoendless eternoendless approved these changes

@mickaelandrieu mickaelandrieu mickaelandrieu left review comments

@jocel1 jocel1 jocel1 left review comments

Assignees
No one assigned
Labels
Bug Type: Bug QA ✔️ Status: check done, code approved
Projects
None yet
Milestone
1.7.4.0
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@soullivaneuh @mickaelandrieu @LittleBigDev @prasanthSelvar @eternoendless @Quetzacoalt91 @jocel1 @marionf