Remove shops from multistore header shop list if they are not associated with current employee #29342

alisamie97 · 2022-08-15T15:07:10Z

Questions	Answers
Branch?	1.7.8.x
Description?	Check issues descriptions. When you create a new employee that is associated to a list of specific shops and not all of them, you expect that employee has access to those shops only and not all the shops. But if you login with the new employee account, you can see in the multistore header that all shops are available. This PR is a fix for this issue.
Type?	bug fix
Category?	BO
BC breaks?	no
Deprecations?	no
Fixed ticket?	Fixes #27704 and #27377
Related PRs	No
How to test?	Enable multistore and create an extra shop. Create a new profile other in BO > Configure > Advanced Parameters > Team > Profiles. Then create give all permissions to that profile. And finally create a new employee with that profile and associate to the new shop you just created. Do not associate to the main shop. Then login via that new employee account and check multistore header. You should see only the associated shops in the create employee form that you have selected. You can also verify this by manually altering the `ps_employee_shop` table
Possible impacts?	Not sure

prestonBot · 2022-08-15T15:07:12Z

Hello @stifler97!

This is your first pull request on the PrestaShop project. Thank you, and welcome to this Open Source community!

alisamie97 · 2022-08-15T15:19:59Z

Keep in mind that if you select Super Admin as the new employee profile in new employee form, you are associating all the shops with this new employee by default and blindly. Just check this:

PrestaShop/src/Core/Form/IdentifiableObject/DataHandler/EmployeeFormDataHandler.php

Lines 136 to 139 in 9887e7e

    
           // Super admins have access to all shops and that cannot be changed by the user. 
        
           if ($data['profile'] == $this->superAdminProfileId) { 
        
               $data['shop_association'] = $this->defaultShopAssociation; 
        
           }

src/PrestaShopBundle/Controller/Admin/MultistoreController.php

alisamie97 · 2022-08-17T10:19:14Z

Can somebody tell me what is wrong with the code that cs fixer is showing me?

matks · 2022-08-17T13:29:21Z

Can somebody tell me what is wrong with the code that cs fixer is showing me?

A simple blank space is needed after if

kpodemski · 2022-08-17T13:32:44Z

Hi @stifler97

I think you could use cs-fixer locally:
https://devdocs.prestashop.com/1.7/modules/testing/basic-checks/#coding-standards

I highly recommend you check how the CI works in PrestaShop and how it helps us maintain the codebase's consistency.

matks · 2022-08-17T13:33:26Z

src/PrestaShopBundle/Controller/Admin/MultistoreController.php

@@ -93,6 +93,16 @@ public function header(bool $lockedToAllShopContext): Response
            $groupList = $this->entityManager->getRepository(ShopGroup::class)->findBy(['active' => true]);
        }

+        $associatedShops = $this->getContext()->employee->getAssociatedShops();


Interesting. I am worried about one usecase: is it possible to configure an employee to "have access to ALL SHOPS" and if yes, does this code work with it?

I think this code works

when an employee is configured to have access to some shops in particular (example: employee Mathieu F has access to shops 1, 4 and 5 but not 2 and 3)

when an employee is configured to have access to some group of shops in particular (example: employee Mathieu F has access to groups 1 and 3 but not 2)

I am worried about the 3rd usecase

I did not see any code that checks shop groups so there is no worries for groups. But it is a nice feature to have.

#29342 (comment) As you mentioned here, I think the 3rd case @matks is worried about is covered by this line. If an employee is super admin and has access to all shops then getAssociatedShops () will simply return all ids. So it looks ok to me, but maybe worth testing the third case :)

MatShir · 2022-08-18T14:04:06Z

Hi @stifler97, we are planning a patch release for September, we would like to have your contribution in it. Have you checked @matks comment ?

src/PrestaShopBundle/Controller/Admin/MultistoreController.php

alisamie97 · 2022-08-18T17:36:46Z

Hi @stifler97, we are planning a patch release for September, we would like to have your contribution in it. Have you checked @matks comment ?

Hi. That is a good news for me as a new comer. I just added a new fix to this PR.

yanmakouf · 2022-08-18T17:55:02Z

src/PrestaShopBundle/Controller/Admin/MultistoreController.php

@@ -93,6 +93,16 @@ public function header(bool $lockedToAllShopContext): Response
            $groupList = $this->entityManager->getRepository(ShopGroup::class)->findBy(['active' => true]);
        }

+        $associatedShops = $this->getContext()->employee->getAssociatedShops();


#29342 (comment) As you mentioned here, I think the 3rd case @matks is worried about is covered by this line. If an employee is super admin and has access to all shops then getAssociatedShops () will simply return all ids. So it looks ok to me, but maybe worth testing the third case :)

alisamie97 · 2022-08-28T12:03:49Z

Hi every one. Is every thing ok with this PR? Just a reminder.

alisamie97 · 2022-09-23T06:00:35Z

just a reminder. It has been a month since I opened this PR

samberrry · 2022-09-23T08:49:25Z

Hey @stifler97,
I thought that it would be better to familiarize you with this space more.

In short, you must be patient. Adding something one month or two months ago does not mean that it should be merged immediately. Normally, Ps team manage and prioritize tasks within projects, please take a look at:
https://github.com/PrestaShop/PrestaShop/projects/26#card-85029280
Ps is a large project with many tasks with different aspects, so they know better when it should be added, and to which version. However, you can track your contribution through the projects and issues. Take a look at the issue: #27377
you can see that it has been labeled many times (take a look at this cart too: https://github.com/PrestaShop/PrestaShop/projects/26#card-85029280)

So, everything is normal ;)

kpodemski · 2022-10-28T22:55:21Z

Thank you, @stifler97, for your PR; I've just approved it and assigned it to the QA team.

Thanks, @samberrry, for your help. The truth is that this PR was lost because we have a problem tracking PRs with only one remaining review.

HanaRebaiQA

hello @stifler97

I have checked this PR. The related issues seem not to be fixed.

Issue multistore_header appears, even if employees has no right on multi shop #27377 : NOK

issue.27377.not.fixed.mp4

Issue Multistore settings accessible by non-permissioned team members #27704 : NOK

issue.27704.not.fixed.mp4

Could you please check again?

Thank You!

kpodemski

Hello @stifler97

1.7.8.x doesn't allow new improvements and features anymore. Would you be interested in moving your improvement to the develop branch and addressing QA feedback?

fix: filter associated shops in multistore header

96413d4

alisamie97 requested a review from a team as a code owner August 15, 2022 15:07

prestonBot added 1.7.8.x Branch Bug fix Type: Bug fix labels Aug 15, 2022

samberrry mentioned this pull request Aug 15, 2022

multistore_header appears, even if employees has no right on multi shop #27377

Open

2 tasks

alisamie97 commented Aug 15, 2022

View reviewed changes

src/PrestaShopBundle/Controller/Admin/MultistoreController.php Outdated Show resolved Hide resolved

fix: minor - remove extra line

0d35a61

matks reviewed Aug 17, 2022

View reviewed changes

alisamie97 commented Aug 18, 2022

View reviewed changes

src/PrestaShopBundle/Controller/Admin/MultistoreController.php Outdated Show resolved Hide resolved

Update src/PrestaShopBundle/Controller/Admin/MultistoreController.php

69aec80

yanmakouf approved these changes Aug 18, 2022

View reviewed changes

atomiix approved these changes Aug 29, 2022

View reviewed changes

atomiix linked an issue Sep 6, 2022 that may be closed by this pull request

multistore_header appears, even if employees has no right on multi shop #27377

Open

2 tasks

kpodemski added this to the 1.7.8.8 milestone Oct 28, 2022

kpodemski approved these changes Oct 28, 2022

View reviewed changes

kpodemski added the Waiting for QA Status: action required, waiting for test feedback label Oct 28, 2022

HanaRebaiQA self-assigned this Oct 31, 2022

HanaRebaiQA reviewed Oct 31, 2022

View reviewed changes

HanaRebaiQA removed the Waiting for QA Status: action required, waiting for test feedback label Oct 31, 2022

HanaRebaiQA added the Waiting for author Status: action required, waiting for author feedback label Oct 31, 2022

AureRita unassigned HanaRebaiQA Nov 8, 2022

matks removed this from the 1.7.8.8 milestone Dec 9, 2022

kpodemski requested changes Jan 12, 2023

View reviewed changes

alisamie97 closed this Jan 28, 2023

alisamie97 deleted the 1.7.8.x branch January 28, 2023 05:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove shops from multistore header shop list if they are not associated with current employee #29342

Remove shops from multistore header shop list if they are not associated with current employee #29342

alisamie97 commented Aug 15, 2022

prestonBot commented Aug 15, 2022

alisamie97 commented Aug 15, 2022

alisamie97 commented Aug 17, 2022

matks commented Aug 17, 2022

kpodemski commented Aug 17, 2022

matks Aug 17, 2022

alisamie97 Aug 18, 2022

yanmakouf Aug 18, 2022

MatShir commented Aug 18, 2022

alisamie97 commented Aug 18, 2022

yanmakouf Aug 18, 2022

alisamie97 commented Aug 28, 2022

alisamie97 commented Sep 23, 2022

samberrry commented Sep 23, 2022

kpodemski commented Oct 28, 2022

HanaRebaiQA left a comment

kpodemski left a comment

Remove shops from multistore header shop list if they are not associated with current employee #29342

Remove shops from multistore header shop list if they are not associated with current employee #29342

Conversation

alisamie97 commented Aug 15, 2022

prestonBot commented Aug 15, 2022

alisamie97 commented Aug 15, 2022

alisamie97 commented Aug 17, 2022

matks commented Aug 17, 2022

kpodemski commented Aug 17, 2022

matks Aug 17, 2022

Choose a reason for hiding this comment

alisamie97 Aug 18, 2022

Choose a reason for hiding this comment

yanmakouf Aug 18, 2022

Choose a reason for hiding this comment

MatShir commented Aug 18, 2022

alisamie97 commented Aug 18, 2022

yanmakouf Aug 18, 2022

Choose a reason for hiding this comment

alisamie97 commented Aug 28, 2022

alisamie97 commented Sep 23, 2022

samberrry commented Sep 23, 2022

kpodemski commented Oct 28, 2022

HanaRebaiQA left a comment

Choose a reason for hiding this comment

kpodemski left a comment

Choose a reason for hiding this comment