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.

Sign up for GitHub

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

Jump to bottom

[API][Order] Filtering on the Orders list #15477

Closed

LGouttefange wants to merge 4 commits into Sylius:1.13 from LGouttefange:feat/api-order-filtration

LGouttefange commented Oct 27, 2023

Q	A
Branch?	1.13
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Related tickets	partially #11250
License	MIT

Included in this PR:

Filtering on the Orders list
All the features from features/order/managing_orders/order_filtration
12 scenarios, 3 files

As this is my first PR here, I am open to any feedback 🙏

LGouttefange requested a review from a team as a code owner

October 27, 2023 12:51

probot-autolabeler bot added the API label

github-actions bot commented Oct 27, 2023 •

edited

Bunnyshell Preview Environment deleted

Available commands:

/bns:deploy to redeploy the environment

diimpp requested changes

View reviewed changes

src/Sylius/Bundle/ApiBundle/Filter/InclusiveDateRangeFilter.php

+                  public function __construct(private ContextAwareFilterInterface $dateFilter, private array $properties)
+                  {
+                  }

Member

diimpp Oct 31, 2023

Please remove second empty line

src/Sylius/Bundle/ApiBundle/Filter/InclusiveDateRangeFilter.php Outdated

+              final class InclusiveDateRangeFilter implements ContextAwareFilterInterface
+              {
+                  /**
+                   * @param ContextAwareFilterInterface $dateFilter

Member

diimpp Oct 31, 2023

This @param is unnecessary, docblocs should only be added to cover arguable cases for static analysis.

src/Sylius/Bundle/ApiBundle/Filter/InclusiveDateRangeFilter.php Outdated

+                  }
+                  /**
+                   * @param QueryBuilder $queryBuilder

Member

diimpp Oct 31, 2023

I guess only

* @param class-string $resourceClass
* @param array<string, mixed> $context

are needed, And

--- * @param string $resourceClass
+++ * @param class-string $resourceClass

src/Sylius/Bundle/ApiBundle/Filter/InclusiveDateRangeFilter.php Outdated

+                  private function isExclusive(mixed $value): bool
+                  {
+                      if(!$value || !is_string($value)) {

Member

diimpp Oct 31, 2023

Suggested change

      
                    if(!$value || !is_string($value)) {
          
                    if (!$value || !is_string($value)) {

src/Sylius/Bundle/ApiBundle/Resources/config/api_resources/Order.xml Outdated

@@ @@ -24,6 +24,13 @@ @@
                               <attribute name="path">/admin/orders</attribute>
                               <attribute name="filters">
                                   <attribute>sylius.api.order_number_filter</attribute>
+                                  <attribute>sylius.api.order_checkout_completed_at_filter</attribute>

Member

diimpp Oct 31, 2023

Please sort them logically or alphabetically.

For example.

                    <attribute>sylius.api.order_number_filter</attribute>
                    <attribute>sylius.api.order_checkout_completed_at_filter</attribute>
                    <attribute>sylius.api.order_currency_code_filter</attribute>
                    <attribute>sylius.api.order_total_filter</attribute>
                    <attribute>sylius.api.order_channel_filter</attribute>
                    <attribute>sylius.api.order_shipping_method_filter</attribute>
                    <attribute>sylius.api.order_products_filter</attribute>
                    <attribute>sylius.api.order_variants_filter</attribute>

src/Sylius/Behat/Context/Api/Admin/ManagingOrdersContext.php Outdated

+                  /**
+                   * @When I specify filter total being greater than :total
+                   */
+                  public function iSpecifyFilterTotalBeingGreaterThan($total)

Member

diimpp Oct 31, 2023

float/string $total?

Suggested change

      
                public function iSpecifyFilterTotalBeingGreaterThan($total)
          
                public function iSpecifyFilterTotalBeingGreaterThan($total): void

src/Sylius/Behat/Context/Api/Admin/ManagingOrdersContext.php Outdated

+                   */
+                  public function iSpecifyFilterTotalBeingGreaterThan($total)
+                  {
+                      $this->client->addFilter('total[gt]', $total* 100);

Member

diimpp Oct 31, 2023

Suggested change

      
                    $this->client->addFilter('total[gt]', $total* 100);
          
                    $this->client->addFilter('total[gt]', $total * 100);

src/Sylius/Behat/Context/Api/Admin/ManagingOrdersContext.php Outdated

+                   */
+                  public function iSpecifyFilterTotalBeingLessThan($total)
+                  {
+                      $this->client->addFilter('total[lt]', $total* 100);

Member

diimpp Oct 31, 2023

Suggested change

      
                    $this->client->addFilter('total[lt]', $total* 100);
          
                    $this->client->addFilter('total[lt]', $total * 100);

src/Sylius/Behat/Context/Api/Admin/ManagingOrdersContext.php Outdated

+                  /**
+                   * @When I specify filter total being less than :total
+                   */
+                  public function iSpecifyFilterTotalBeingLessThan($total)

Member

diimpp Oct 31, 2023

Suggested change

      
                public function iSpecifyFilterTotalBeingLessThan($total)
          
                public function iSpecifyFilterTotalBeingLessThan(float/string? $total): void

src/Sylius/Behat/Context/Api/Admin/ManagingOrdersContext.php Outdated

+                  /**
+                   * @When /^I filter by (variants "([^"]+)" and "([^"]+)")$/
+                   * @param ProductVariantInterface $variants

Member

diimpp Oct 31, 2023

Please remove this line unless necessary by phpstan

Member

diimpp commented Oct 31, 2023

@LGouttefange Hi, great filtering coverage. 👍 I've commented on a few minor points, please take a look.

LGouttefange force-pushed the feat/api-order-filtration branch from 0427857 to 828cc9d Compare

November 2, 2023 13:17


          [API][Order] Implement order filters

48e9449

LGouttefange force-pushed the feat/api-order-filtration branch from 828cc9d to 48e9449 Compare

November 2, 2023 13:21

Loic Gouttefangeas added 3 commits

November 2, 2023 18:58


          [API][Order] Rework InclusiveDateRangeFilter inner workings

45f4867


          [API][Order] Fix coding standards on filters.xml

d0930e2


          [API][Order] Fix coding standards in ManagingOrdersContext.php

e36edea

LGouttefange commented

View reviewed changes

Author

LGouttefange left a comment

Thank you @diimpp for the thorough review. I've updated & commented some points
I also fixed the coding standards issues, but - bit of a newbie question - please let me know if I should resolve them myself, or let you handle those 🙏 thanks !

src/Sylius/Bundle/ApiBundle/Filter/InclusiveDateRangeFilter.php Outdated

+                  private function isExclusive(mixed $value): bool
+                  {
+                      if(!$value || !is_string($value)) {
+                          return false;

Author

LGouttefange Nov 2, 2023

I've pushed a new version of the Filter class, taking inspiration of the mentioned filter.
In the case of a Filter decorator, I didn't want to overstep or reproduce the base DateFilter validation & error handling, hence why I am not actively validating the item and just returning it as such.

Contributor

jakubtobiasz commented Dec 21, 2023

Closing as already done in #15566. Thanks!

jakubtobiasz closed this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment