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

Handle multistore customers sharing in customers listing at group level #16047

Merged
merged 1 commit into from Oct 25, 2019

Conversation

@matks
Copy link
Contributor

matks commented Oct 21, 2019

Questions Answers
Branch? 1.7.6.x
Description? Handle multistore customers sharing in customers listing at group level
Type? bug fix
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #16043
How to test? Please see issue, and check multistore behavior for Customers listing. Please also check if "customers sharing" setting is working fine when disabled

This change is Reviewable

@matks matks requested a review from PrestaShop/prestashop-core-developers as a code owner Oct 21, 2019
@@ -66,7 +66,7 @@ services:
arguments:
- '@prestashop.core.query.doctrine_search_criteria_applicator'
- "@=service('prestashop.adapter.legacy.context').getContext().language.id"
- '@=service("prestashop.adapter.shop.context").getContextListShopID()'
- '@=service("prestashop.adapter.shop.context").getContextListShopID(parameter("const_shop_share_customer"))'

This comment has been minimized.

Copy link
@matks

matks Oct 21, 2019

Author Contributor

Although this behavior was handled by a long if/else in legacy code:
https://github.com/PrestaShop/PrestaShop/blob/develop/classes/shop/Shop.php
It seems using a proper Shop::getContextListShopID($share)) I can deal with it more simply
https://github.com/PrestaShop/PrestaShop/blob/develop/classes/shop/Shop.php#L923

This comment has been minimized.

Copy link
@matks

matks Oct 21, 2019

Author Contributor

To sum it up:
It was implemented in another way in legacy customers listing
but ... it seems I can fix the bug passing a single param to what was used before
so: I was reused a legacy function, but different from the one that was used before

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Oct 21, 2019

Contributor

Really cool if we can fix it like that! 👍

This comment has been minimized.

Copy link
@jolelievre

jolelievre Oct 21, 2019

Contributor

The only question is: are we sure that prestashop.core.grid.query_builder.customer is only used in this context and/or always needs to use the group sharing?

This comment has been minimized.

Copy link
@matks

matks Oct 21, 2019

Author Contributor

@jolelievre ah you're right ... I think I "forced" the "shared data" behavior although this is an option that merchant can choose not to use

This comment has been minimized.

Copy link
@jolelievre

jolelievre Oct 22, 2019

Contributor

Mmm, so does it mean you should use the value from a Configuration::get (if it exists) instead of the const value?

This comment has been minimized.

Copy link
@matks

matks Oct 22, 2019

Author Contributor

I'm going to need to re-implement legacy way to work
which means this if/else
https://github.com/PrestaShop/PrestaShop/blob/develop/classes/shop/Shop.php#L1077

to make it short

if ($group['share_customer']) {
    $ids = Shop::getContextListShopID($share));
} else {
    $ids = Shop::getContextListShopID())
}

and I need to do this at configuration/constructor level 🤔

I'm afraid a BC break will be needed 😭

This comment has been minimized.

Copy link
@jolelievre

jolelievre Oct 22, 2019

Contributor

You mean a BC because you will have to change the constructor of CustomerQueryBuilder

Maybe you could instead create a new service ShopRepository with a getContextListId() method that would manage this if/else case Then you can simply change the service definition and there is no BC (all the naming can of course be improved, it's a quick proposition)

This comment has been minimized.

Copy link
@matks

matks Oct 22, 2019

Author Contributor

Idea is good 👍

@colinegin

This comment has been minimized.

Copy link
Collaborator

colinegin commented Oct 22, 2019

@Robin-Fischer-PS did you find any issue on this PR ?

@Robin-Fischer-PS

This comment has been minimized.

Copy link

Robin-Fischer-PS commented Oct 22, 2019

@colinegin No, but @jolelievre told me it wasn't ready for QA yet on Slack !

Copy link
Contributor

jolelievre left a comment

Just to clarify this PR status and write what we discussed:

  • create a service to get the shop list
  • this service is able to adapt this list to the ShopContext (especially the shared data depending on the group/shop)
  • update prestashop.core.grid.query_builder.customer to use this new service for its dependencies
  • check if some other services might have the same problem in their definition
@matks matks force-pushed the matks:fix-multistore-share-users branch from 1ece2c2 to b5f3007 Oct 23, 2019
@matks matks requested a review from jolelievre Oct 23, 2019
@matks

This comment has been minimized.

Copy link
Contributor Author

matks commented Oct 23, 2019

I implemented @jolelievre idea and ammended the commit. Should be fine now.

$groupSettings = Shop::getGroupFromShop(Shop::getContextShopID(), false);
if ($groupSettings['share_customer']) {
return Shop::getContextListShopID(Shop::SHARE_CUSTOMER);

This comment has been minimized.

Copy link
@jolelievre

jolelievre Oct 24, 2019

Contributor

Maybe we should manage the other two sharing types since we're at it ^^
Shop::SHARE_ORDER and SHOP::SHARE_STOCK
cd

public static function getSharedShops($shop_id, $type)

This comment has been minimized.

Copy link
@jolelievre

jolelievre Oct 24, 2019

Contributor

I wonder if we should also take a look at other places where this method is used like:

$id_shops = (new Context())->getContextListShopID();

But I guess this bug only occurs for entities that have sharing capabilities, so Customer, Order, Stock Then we must be careful about (develop branch only):

Copy link
Contributor

jolelievre left a comment

Thanks @matks I approve this one since I think it's enough for the bug
Although there might be other places in the code that could have the same issue (see my last comments)

@Robin-Fischer-PS

This comment has been minimized.

Copy link

Robin-Fischer-PS commented Oct 25, 2019

Hi here ! It's good for QA.

Thanks @matks :)

@Robin-Fischer-PS Robin-Fischer-PS added this to the 1.7.6.2 milestone Oct 25, 2019
@jolelievre jolelievre merged commit 8ed22e9 into PrestaShop:1.7.6.x Oct 25, 2019
2 checks passed
2 checks passed
PrettyCI Code formatting
Details
Travis CI - Pull Request Build Passed
Details
@jolelievre

This comment has been minimized.

Copy link
Contributor

jolelievre commented Oct 25, 2019

Thank you @matks

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