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

[API] Add Address resources #16240

Conversation

Wojdylak
Copy link
Member

@Wojdylak Wojdylak commented May 9, 2024

Q A
Branch? api-platform-3
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Related tickets N/A
License MIT

@Wojdylak Wojdylak requested review from a team as code owners May 9, 2024 19:13
@probot-autolabeler probot-autolabeler bot added the API APIs related issues and PRs. label May 9, 2024
Copy link

github-actions bot commented May 9, 2024

Bunnyshell Preview Environment deployment failed

Check https://github.com/Sylius/Sylius/actions/runs/9032584601 for details.

Available commands:

  • /bns:deploy to redeploy the environment
  • /bns:delete to remove the environment

<values>
<value name="groups">
<values>
<value>sylius:admin:address:log_entry:show</value>
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a minor thing, but having a collection endpoint with a show serialization group looks weird

src/Sylius/Bundle/ApiBundle/Security/CustomerVoter.php Outdated Show resolved Hide resolved
@Wojdylak Wojdylak force-pushed the SYL-3115-upgrade-address-resource branch from ebc1dec to 4b685a9 Compare May 10, 2024 12:57
@NoResponseMate NoResponseMate merged commit 41b271d into Sylius:api-platform-3 May 10, 2024
16 checks passed
@NoResponseMate
Copy link
Contributor

Thank you, @Wojdylak!

uriTemplate="/shop/addresses"
security="is_granted('SYLIUS_SHOP_USER')"
itemUriTemplate="/shop/addresses/{id}"
processor="sylius_api.state_processor.post.address"
Copy link
Member

Choose a reason for hiding this comment

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

sylius_api.state_processor.address.post
🤔

Comment on lines +21 to +27
<operation
class="ApiPlatform\Metadata\Get"
controller="ApiPlatform\Action\NotFoundAction"
read="false"
output="false"
routePrefix="admin"
/>
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it doesn't make sense to also retrieve individually 🤔 but we can think about that in the future, it wasn't possible before either

@@ -10,6 +10,7 @@ parameters:
sylius.api.doctrine_extension.order_visitor_item.filter_cart.allowed_non_get_operations:
- "shop_select_payment_method"
sylius.api.paths_to_hide:
- "%sylius.security.new_api_route%/admin/address-log-entry/{id}"
Copy link
Member

Choose a reason for hiding this comment

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

I'm afraid it does not work currently on APIP 3 branch if I see correctly

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but I have added it so we wouldn't forget about it.

return false;
}

public function supportsAttribute(string $attribute): bool
Copy link
Member

Choose a reason for hiding this comment

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

I would be for putting public methods before protected ones

Comment on lines +26 to +30
if (self::SYLIUS_SHOP_USER === $attribute) {
return true;
}

return false;
Copy link
Member

Choose a reason for hiding this comment

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

Simply:

return self::SYLIUS_SHOP_USER === $attribute;

or even use supportsAttribute method

@Wojdylak Wojdylak deleted the SYL-3115-upgrade-address-resource branch May 23, 2024 07:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API APIs related issues and PRs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants