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

[Docs] Add docs for custom entity with access per admin channel #12619

Merged

Conversation

Tomanhez
Copy link
Contributor

@Tomanhez Tomanhez commented May 10, 2021

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
License MIT

Screenshot 2021-05-10 at 13 36 53

@Tomanhez Tomanhez requested a review from a team as a code owner May 10, 2021 11:35
@probot-autolabeler probot-autolabeler bot added the Documentation Documentation related issues and PRs - requests, fixes, proposals. label May 10, 2021
Copy link
Member

@lchrusciel lchrusciel left a comment

Choose a reason for hiding this comment

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

Good to have this document here, but we should think about making this process simpler

use Symfony\Component\OptionsResolver\Options;
use Symfony\Component\OptionsResolver\OptionsResolver;

final class ChannelPerAdminChannelType extends AbstractType
Copy link
Member

Choose a reason for hiding this comment

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

Can we have this form type predefined in Plus and use it somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. I think we should replace all places with ChannelChoiceType for ChannelPerAdminChannelChoiceType. This is not scope this task, but good improvement in future. Next good improvement is to add resources name as parameters in services, without decorating ResourceChannelEnabilibityChecker service:
Now:

final class ResourceChannelEnabilibityChecker implements ResourceChannelEnabilibityCheckerInterface
{
    public function forResourceName(string $resourceName): bool
    {
        return in_array($resourceName, [
            'credit_memo',
            'customer',
            'invoice',
            'order',
            'payment',
            'product',
            'product_variant',
            'return_request',
            'shipment',
        ]);
    }
}

It could be more generic, for example:

final class ResourceChannelEnabilibityChecker implements ResourceChannelEnabilibityCheckerInterface
{
    private $resources;

    public function __construct($resources)
    {
        $this->resources = array_merge(
            $resources,
            [
                'credit_memo',
                'customer',
                'invoice',
                'order',
                'payment',
                'product',
                'product_variant',
                'return_request',
                'shipment',
            ]
        );
    }


    public function forResourceName(string $resourceName): bool
    {
        return in_array($resourceName, $this->resources);
    }

After this changes, final application needs overwrite only service and add new entity names

@Tomanhez Tomanhez force-pushed the add-custom-entity-accessing-per-admin-channel branch 2 times, most recently from 8ecdf55 to 2ab2627 Compare May 11, 2021 10:51
@Tomanhez Tomanhez force-pushed the add-custom-entity-accessing-per-admin-channel branch from 2ab2627 to 7ea7c33 Compare May 12, 2021 06:38
use Sylius\Plus\ChannelAdmin\Application\Checker\ResourceChannelCheckerInterface;
use Sylius\Plus\Entity\ChannelInterface;

final class ResourceChannelChecker implements ResourceChannelCheckerInterface
Copy link
Member

Choose a reason for hiding this comment

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

We definitely should at least define in ResourceChannelChecker, in Plus the behaviour for checking the entity with ChannelAwareInterface and then recommend for implementing this interface instead of decorating this service

@Tomanhez Tomanhez force-pushed the add-custom-entity-accessing-per-admin-channel branch 2 times, most recently from 817a2aa to 0af4801 Compare May 12, 2021 19:53
@Tomanhez Tomanhez force-pushed the add-custom-entity-accessing-per-admin-channel branch 3 times, most recently from 676df9e to 1c17671 Compare May 13, 2021 12:45
@Tomanhez Tomanhez force-pushed the add-custom-entity-accessing-per-admin-channel branch from 1c17671 to e8b7bc1 Compare May 13, 2021 20:07
@CoderMaggie CoderMaggie self-requested a review May 13, 2021 20:15
@Tomanhez Tomanhez force-pushed the add-custom-entity-accessing-per-admin-channel branch from e8b7bc1 to 5b49812 Compare May 13, 2021 20:15
@Tomanhez Tomanhez force-pushed the add-custom-entity-accessing-per-admin-channel branch 2 times, most recently from 30ab0f5 to 251beda Compare May 17, 2021 08:57
@Tomanhez Tomanhez force-pushed the add-custom-entity-accessing-per-admin-channel branch from 251beda to da03535 Compare May 17, 2021 16:31
@Tomanhez Tomanhez force-pushed the add-custom-entity-accessing-per-admin-channel branch from 91af348 to 5239b2d Compare May 18, 2021 19:52
@GSadee GSadee changed the base branch from master to 1.9 May 19, 2021 05:11
@GSadee
Copy link
Member

GSadee commented May 19, 2021

The base of this pull-request was changed, you need fetch and reset your local branch
if you want to add new commits to this pull request. Reset before you pull, else commits
may become messed-up.

Unless you added new commits (to this branch) locally that you did not push yet,
execute git fetch origin && git reset "add-custom-entity-accessing-per-admin-channel" to update your local branch.

Feel free to ask for assistance when you get stuck 👍

@GSadee GSadee force-pushed the add-custom-entity-accessing-per-admin-channel branch from 5239b2d to 6dc078e Compare May 19, 2021 05:11
@GSadee GSadee dismissed CoderMaggie’s stale review May 19, 2021 05:20

Everything has been fixed

@GSadee GSadee merged commit 49d29a0 into Sylius:1.9 May 19, 2021
@GSadee
Copy link
Member

GSadee commented May 19, 2021

Thank you, Tomasz! 🎉

AdamKasp added a commit that referenced this pull request May 20, 2021
…ccessing to entities by channel admins (GSadee)

This PR was merged into the 1.9 branch.

Discussion
----------

| Q               | A
| --------------- | -----
| Branch?         | 1.9
| Bug fix?        | no
| New feature?    | no
| BC breaks?      | no
| Deprecations?   | no
| Related tickets | improvements to #12619 
| License         | MIT


Commits
-------

3266c30 [Documentation][Plus] Minor improvements in cookbook for accessing to entities by channel admins
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Documentation related issues and PRs - requests, fixes, proposals.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants