universal logging to administration of public demoshop #68
Conversation
TomasLudvik
commented
Aug 15, 2019
TomasLudvik
commented
| Q | A |
|---|---|
| Description, reason for the PR | On public demoshop we need to have administrator, that cannot be changed or deleted so users can always log into administration with same credentials. Also we need to enable multiple users to log with same credentials. |
| New feature | Yes |
| Fixes issues | ... |
| Have you read and signed our License Agreement for contributions? | Yes |
TomasLudvik
force-pushed
the
tl-administrator-logging
branch
from
3ab59d5 to
1127f53
Compare
src/Shopsys/ShopBundle/Resources/views/Admin/Inline/Menu/menu.html.twig
Outdated
Show resolved
Hide resolved
src/Shopsys/ShopBundle/Model/Administrator/Security/AdministratorUserProvider.php
Show resolved
Hide resolved
There was a problem hiding this comment.
Hi @TomasLudvik, @s3tezsky
I check the PR and have just small objections.
And I saw you deleted back the overriden menu related controller/template. Why?
Looking forward to testing :)
| @@ -28,3 +28,4 @@ parameters: | |||
| Shopsys\FrameworkBundle\Model\Transport\Transport: Shopsys\ShopBundle\Model\Transport\Transport | |||
| Shopsys\FrameworkBundle\Model\Customer\BillingAddress: Shopsys\ShopBundle\Model\Customer\BillingAddress | |||
| Shopsys\FrameworkBundle\Model\Customer\User: Shopsys\ShopBundle\Model\Customer\User | |||
| Shopsys\FrameworkBundle\Model\Administrator\Administrator: Shopsys\ShopBundle\Model\Administrator\Administrator | |||
There was a problem hiding this comment.
what about \Tests\ShopBundle\Functional\EntityExtension\EntityExtensionTest?
Currently, this entity is not used in tests and it could be surprising in the future
There was a problem hiding this comment.
That tests uses extension map and then adds some entities that it uses.
There was a problem hiding this comment.
Okay, that's valid, thanks.
But then the tests should be fixed, don't they? I'm talking for example about AdministratorRepositoryTest, where
$administrator is annotated as a Framework entity.
I know that in this test it doesn't matter (no extra fields are used), but couldn't it be confusing in the future?
There was a problem hiding this comment.
Well we do not do that. @vitek-rostislav is just implementing fixer for updating annotations, so maybe he will solve this.
There was a problem hiding this comment.
Okay, I don't agree with "we don't do that", but fixer in progress is a strong enough argument to keep me in peace 😄
src/Shopsys/ShopBundle/Model/Administrator/Security/AdministratorUserProvider.php
Show resolved
Hide resolved
TomasLudvik
force-pushed
the
tl-administrator-logging
branch
from
3251f44 to
1ee9aa8
Compare
There was a problem hiding this comment.
TESTS
Everything works as expected. I just added a few more notes, please check it out.
And one thing – Grid limit (Items per page) is shared between two logged admins (admin one set limit to 200, second admin refresh page and see 200 entries too). Not sure if it's in the scope of this PR.
src/Shopsys/ShopBundle/Controller/Admin/AdministratorController.php
Outdated
Show resolved
Hide resolved
| @@ -28,3 +28,4 @@ parameters: | |||
| Shopsys\FrameworkBundle\Model\Transport\Transport: Shopsys\ShopBundle\Model\Transport\Transport | |||
| Shopsys\FrameworkBundle\Model\Customer\BillingAddress: Shopsys\ShopBundle\Model\Customer\BillingAddress | |||
| Shopsys\FrameworkBundle\Model\Customer\User: Shopsys\ShopBundle\Model\Customer\User | |||
| Shopsys\FrameworkBundle\Model\Administrator\Administrator: Shopsys\ShopBundle\Model\Administrator\Administrator | |||
There was a problem hiding this comment.
Okay, that's valid, thanks.
But then the tests should be fixed, don't they? I'm talking for example about AdministratorRepositoryTest, where
$administrator is annotated as a Framework entity.
I know that in this test it doesn't matter (no extra fields are used), but couldn't it be confusing in the future?
- admin with ID 2 is now known as default administrator
TomasLudvik
force-pushed
the
tl-administrator-logging
branch
from
3bc0683 to
fefd2b0
Compare
