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

Drop unused order state-machine transtion to fully refunded #232

Merged
merged 2 commits into from
Mar 10, 2021

Conversation

diimpp
Copy link
Member

@diimpp diimpp commented Nov 25, 2020

order->state = fully_refunded is defined in the plugin, but never used by anything.

Besides, if such transition should exist, then it should be named refunded as partional refund state is called partially_refunded.

@diimpp diimpp requested a review from a team as a code owner November 25, 2020 05:39
@GSadee
Copy link
Member

GSadee commented Jan 4, 2021

Hello @diimpp! If I see correctly, this transition is used by OrderFullyRefundedStateResolver, which is called in RefundPaymentProcessManager after UnitsRefunded event. So, removing this transition could break the current behaviour and some end applications. Because of that, I'm closing this PR for now, but feel free to reopen it or comment here if you still think that this change is valid.

@GSadee GSadee closed this Jan 4, 2021
@diimpp
Copy link
Member Author

diimpp commented Jan 4, 2021

@GSadee Hi!

this state is not used by OrderFullyRefundedStateResolver, name of this resolver is misleading and it operates on OrderPaymentState, not OrderState.

https://github.com/Sylius/RefundPlugin/search?q=fully_refunded

OrderState:fully_refunded is an artifact from old big refactor at #123 and not used anywhere in the app. This state in unreachable by the app.

@GSadee
Copy link
Member

GSadee commented Jan 4, 2021

Ok, my mistake, I had a wrong look at the interface this transition is going from.

@GSadee GSadee reopened this Jan 4, 2021
Copy link
Member

@GSadee GSadee left a comment

Choose a reason for hiding this comment

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

@diimpp could you add some note in UPGRADE file about removing this state and transition?

@GSadee GSadee force-pushed the drop_sm_order_transtion branch from d344bd8 to b318d89 Compare March 9, 2021 08:29
@diimpp
Copy link
Member Author

diimpp commented Mar 9, 2021

Thanks @GSadee !

@GSadee GSadee force-pushed the drop_sm_order_transtion branch from b318d89 to 15bb7fc Compare March 10, 2021 06:01
@GSadee GSadee force-pushed the drop_sm_order_transtion branch from 15bb7fc to 4b83da9 Compare March 10, 2021 06:06
@GSadee GSadee merged commit 4e6da18 into Sylius:master Mar 10, 2021
@GSadee
Copy link
Member

GSadee commented Mar 10, 2021

Thank you, Dmitri! 🥇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants