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.

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

Apply shop context in configuration #9311

Merged
merged 1 commit into from Jul 12, 2018

Conversation

@Quetzacoalt91
Copy link
Member

commented Jul 12, 2018

Questions Answers
Branch? develop
Description? Handle the shop context automatically when updating the configuration. This PR is set in the develop branch because it updates the behavior of every controller running under Symfony.
Type? improvement
Category? BO
BC breaks? Nope
Deprecations? Nope
Fixed ticket? http://forge.prestashop.com/browse/BOOM-5946
How to test? Try to enable the maintenance mode of a single shop, only the selected one must be impacted

This change is Reviewable

@prestonBot prestonBot added the develop label Jul 12, 2018

@mickaelandrieu mickaelandrieu added the BO label Jul 12, 2018

@PrestaShop PrestaShop deleted a comment from prestonBot Jul 12, 2018

@mickaelandrieu

This comment has been minimized.

Copy link
Member

commented Jul 12, 2018

This will fix the bug, but I'm not sure this is a good idea for the future to rely on a hidden context :/

@hadjedjvincent

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2018

Isn't that should be targeted on 1.7.4.x ?
As you said @mickaelandrieu, it is a bug fix. It's more like a regression.

@Quetzacoalt91

This comment has been minimized.

Copy link
Member Author

commented Jul 12, 2018

@hadjedjvincent do you mean this PR brings a regression?

@mickaelandrieu, what would you suggest? Does every form handler should specify on which context they want to write the configuration?

@hadjedjvincent

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2018

@Quetzacoalt91 Not really, it looks like behaviour changed on 1.7.4 (if i refer to the forge ticket).
I was meaning it's a regression from previous releases.

@marionf

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2018

Yes, @hadjedjvincent it's a regression of 1.7.4.0
Can you do it on 1.7.4.x @Quetzacoalt91 ?

@Quetzacoalt91 Quetzacoalt91 changed the base branch from develop to 1.7.4.x Jul 12, 2018

@prestonBot prestonBot added the develop label Jul 12, 2018

@Quetzacoalt91 Quetzacoalt91 force-pushed the Quetzacoalt91:configuration-shop branch from 85a5210 to 6409219 Jul 12, 2018

@marionf marionf self-assigned this Jul 12, 2018

@marionf

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2018

@Quetzacoalt91
We have another related issue.
If I choose in the drop down "All shop" context and change something, it's not applied to all shops.
Can we solve it btw or in another PR ?

@mickaelandrieu in multishop on legacy pages we have this button and checkboxes:
capture d ecran_47
Do you think it's possible to add it on symfony pages ?

@rGaillard

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2018

@marionf i have opened an issue about it long time ago http://forge.prestashop.com/browse/BOOM-1678

@marionf marionf removed their assignment Jul 12, 2018

@marionf marionf added QA ✔️ and removed waiting for QA labels Jul 12, 2018

@eternoendless eternoendless added this to the 1.7.4.1 milestone Jul 12, 2018

@eternoendless

This comment has been minimized.

Copy link
Member

commented Jul 12, 2018

Thank you @Quetzacoalt91

@eternoendless eternoendless merged commit e8b40ac into PrestaShop:1.7.4.x Jul 12, 2018

2 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mickaelandrieu

This comment has been minimized.

Copy link
Member

commented Jul 12, 2018

Do you think it's possible to add it on symfony pages ?

Yes, it's possible. This needs to be a forge issue and given to someone that add the field if we're in multishop context only.

@rGaillard not the same issue at all xD

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.