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][Order] Fixed wrong where condition when looking for order #11895

Merged
merged 3 commits into from
Nov 12, 2020

Conversation

stloyd
Copy link
Contributor

@stloyd stloyd commented Sep 30, 2020

Q A
Branch? 1.8
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Related tickets n/a
License MIT

Wrong query:

SELECT s0_.number AS number_0, s0_.notes AS notes_1, s0_.state AS state_2, s0_.checkout_completed_at AS checkout_completed_at_3, s0_.items_total AS items_total_4, s0_.adjustments_total AS adjustments_total_5, s0_.total AS total_6, s0_.created_at AS created_at_7, s0_.updated_at AS updated_at_8, s0_.id AS id_9, s0_.currency_code AS currency_code_10, s0_.locale_code AS locale_code_11, s0_.checkout_state AS checkout_state_12, s0_.payment_state AS payment_state_13, s0_.shipping_state AS shipping_state_14, s0_.token_value AS token_value_15, s0_.customer_ip AS customer_ip_16, s1_.quantity AS quantity_17, s1_.unit_price AS unit_price_18, s1_.units_total AS units_total_19, s1_.adjustments_total AS adjustments_total_20, s1_.total AS total_21, s1_.is_immutable AS is_immutable_22, s1_.id AS id_23, s1_.product_name AS product_name_24, s1_.variant_name AS variant_name_25, s0_.channel_id AS channel_id_26, s0_.promotion_coupon_id AS promotion_coupon_id_27, s0_.customer_id AS customer_id_28, s0_.shipping_address_id AS shipping_address_id_29, s0_.billing_address_id AS billing_address_id_30, s1_.order_id AS order_id_31, s1_.variant_id AS variant_id_32 FROM sylius_order s0_ LEFT JOIN sylius_customer s2_ ON s0_.customer_id = s2_.id LEFT JOIN patient p3_ ON s2_.id = p3_.customer_id LEFT JOIN sylius_order_item s1_ ON s0_.id = s1_.order_id WHERE (s0_.token_value = ? AND p3_.id IS NULL) OR s0_.customer_id IS NULL)) AND s0_.state = ?

Fixed query:

SELECT s0_.number AS number_0, s0_.notes AS notes_1, s0_.state AS state_2, s0_.checkout_completed_at AS checkout_completed_at_3, s0_.items_total AS items_total_4, s0_.adjustments_total AS adjustments_total_5, s0_.total AS total_6, s0_.created_at AS created_at_7, s0_.updated_at AS updated_at_8, s0_.id AS id_9, s0_.currency_code AS currency_code_10, s0_.locale_code AS locale_code_11, s0_.checkout_state AS checkout_state_12, s0_.payment_state AS payment_state_13, s0_.shipping_state AS shipping_state_14, s0_.token_value AS token_value_15, s0_.customer_ip AS customer_ip_16, s1_.quantity AS quantity_17, s1_.unit_price AS unit_price_18, s1_.units_total AS units_total_19, s1_.adjustments_total AS adjustments_total_20, s1_.total AS total_21, s1_.is_immutable AS is_immutable_22, s1_.id AS id_23, s1_.product_name AS product_name_24, s1_.variant_name AS variant_name_25, s0_.channel_id AS channel_id_26, s0_.promotion_coupon_id AS promotion_coupon_id_27, s0_.customer_id AS customer_id_28, s0_.shipping_address_id AS shipping_address_id_29, s0_.billing_address_id AS billing_address_id_30, s1_.order_id AS order_id_31, s1_.variant_id AS variant_id_32 FROM sylius_order s0_ LEFT JOIN sylius_customer s2_ ON s0_.customer_id = s2_.id LEFT JOIN patient p3_ ON s2_.id = p3_.customer_id LEFT JOIN sylius_order_item s1_ ON s0_.id = s1_.order_id WHERE s0_.token_value = ? AND (p3_.id IS NULL OR s0_.customer_id IS NULL) AND s0_.state = ?

With first it will find more than one result cause if you allow guess orders, nor user, nor customer is set.

@stloyd stloyd requested a review from a team as a code owner September 30, 2020 17:03
@probot-autolabeler probot-autolabeler bot added the API APIs related issues and PRs. label Sep 30, 2020
@stloyd stloyd force-pushed the patch-5 branch 2 times, most recently from b436ffb to d8083f2 Compare October 1, 2020 08:43
@stloyd
Copy link
Contributor Author

stloyd commented Oct 6, 2020

@lchrusciel @pamil Anything missing with this bugfix?

@stloyd
Copy link
Contributor Author

stloyd commented Oct 9, 2020

@pamil @AdamKasp @Zales0123 Guys... this bug blocks any usage of shop API with guess orders (you can do one order, next will die with fatal error), is there anything we can push this forward?

@stloyd
Copy link
Contributor Author

stloyd commented Oct 14, 2020

@pamil @lchrusciel Just noticed that exactly same problem is when you ask for order items.

It will die with Doctrine exception founding more than one item for given "parameters".

More than one result was found for query although one row or none was expected.

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.

Hey Joseph,

sorry for the long feedback (and a few patch releases). Your PR fixes a really annoying issue, thanks for that. We will try to have it merged and perhaps released by the end of the week.

The only missing part for this PR is some tests despite specs to avoid regression. @AdamKasp would you be able to finish it?

@lchrusciel
Copy link
Member

Fixes #12023 as well

->shouldBeCalled()
->willReturn($queryBuilder)
->willReturn($expr);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
->willReturn($expr);
->willReturn($expr)
;

->shouldBeCalled()
->willReturn($queryBuilder)
->willReturn($expr);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
->willReturn($expr);
->willReturn($expr)
;

@GSadee GSadee merged commit 8211bf9 into Sylius:1.8 Nov 12, 2020
@GSadee
Copy link
Member

GSadee commented Nov 12, 2020

Thanks, Joseph! 🎉

@mespinosaz
Copy link

thanks @stloyd !!

lchrusciel added a commit that referenced this pull request Nov 12, 2020
…ge (GSadee)

This PR was merged into the 1.9-dev branch.

Discussion
----------

| Q               | A
| --------------- | -----
| Branch?         | master
| Bug fix?        | yes
| New feature?    | no
| BC breaks?      | no
| Deprecations?   | no
| Related tickets | after #11895
| License         | MIT


Commits
-------

e80eae2 [API] Fix PHPSpec of order query extension after upmerge
@lchrusciel
Copy link
Member

Feel free to update your projects, I've just published a new release https://github.com/Sylius/Sylius/releases/tag/v1.8.5 🎉

pamil pushed a commit to Sylius/SyliusApiBundle that referenced this pull request Dec 23, 2020
…ge (GSadee)

This PR was merged into the 1.9-dev branch.

Discussion
----------

| Q               | A
| --------------- | -----
| Branch?         | master
| Bug fix?        | yes
| New feature?    | no
| BC breaks?      | no
| Deprecations?   | no
| Related tickets | after Sylius/Sylius#11895
| License         | MIT


Commits
-------

e80eae242a1dcc5d5def5b847103e9eab9e8f520 [API] Fix PHPSpec of order query extension after upmerge
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.

7 participants