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] Sorting user orders by order number #14990

Merged
merged 3 commits into from
Jun 9, 2023
Merged

[API] Sorting user orders by order number #14990

merged 3 commits into from
Jun 9, 2023

Conversation

dawkaa
Copy link
Contributor

@dawkaa dawkaa commented Apr 26, 2023

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

@dawkaa dawkaa requested a review from a team as a code owner April 26, 2023 10:07
@probot-autolabeler probot-autolabeler bot added the API APIs related issues and PRs. label Apr 26, 2023
@dawkaa dawkaa changed the title Dev [API] Sorting user orders by order number Apr 26, 2023
@dawkaa
Copy link
Contributor Author

dawkaa commented Apr 27, 2023

@jakubtobiasz

@coldic3 coldic3 self-assigned this Apr 28, 2023
jakubtobiasz
jakubtobiasz previously approved these changes May 1, 2023
@dawkaa
Copy link
Contributor Author

dawkaa commented Jun 7, 2023

@NoResponseMate @coldic3

NoResponseMate
NoResponseMate previously approved these changes Jun 8, 2023
Copy link
Contributor

@NoResponseMate NoResponseMate left a comment

Choose a reason for hiding this comment

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

Most likely a rebase will be required as well as we had some issues with static analysis due to 3rd party changes.
Other than that IMO it's good to go.

I did sorting user orders by order number.
I added tests to sorting user orders.
I added the fixes for the conversation.
/**
* @Then I should see an order with :orderNumber number
*/
public function iShouldSeeOrderWithNumber(string $orderNumber)
Copy link
Member

Choose a reason for hiding this comment

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

Missing return type

/**
* @When I switch the way orders are sorted by :fieldName
*/
public function iSwitchSortingBy($fieldName)
Copy link
Member

Choose a reason for hiding this comment

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

Missing type hint and return type

}

/**
* @When I switch the way orders are sorted by :fieldName
Copy link
Member

Choose a reason for hiding this comment

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

It would be good to keep the order of the methods: Given -> When -> Then, so this one should be moved before Then method, which is above

@GSadee GSadee merged commit 0fe3bcd into Sylius:1.13 Jun 9, 2023
23 checks passed
@GSadee
Copy link
Member

GSadee commented Jun 9, 2023

Thanks, Dawka! 🥇

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

5 participants