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

Misc refactor #200

Merged
merged 7 commits into from
Apr 8, 2020
Merged

Misc refactor #200

merged 7 commits into from
Apr 8, 2020

Conversation

diimpp
Copy link
Member

@diimpp diimpp commented Mar 6, 2020

Tried to collect BC changes in single PR.

Fixes #192
References #191

  • Renames SequenceInterface to CreditMemoSequenceInterface (Like OrderSequenceInterface, InvoiceSequenceInterface)
  • Makes CreditMemo number unique and issuedAt non nullable (In doctrine mappings, for the rest of the app it's already holds true)
  • Rename sylius_refund_payment to sylius_refund_refund_payment (Table was named incorrectly)
  • Splits CustomerBillingData customerName into firstName and lastName (As in Addressing\Model\Address, Customer\Model\Customer, Invoicing\Entity\BillingData)
  • Change CreditMemo issuedAt to datetime_immutable (Note, that I've converted CurrentDateTimeInterface to CurrentDateTimeImmutableInterface due to phpstan unability to handle properly \DateTimeInterface in context of \DateTime and \DateTimeImmutable DateTimeInterface can be only ever implemented by DateTimeImmutable and Datetime, nothing else phpstan/phpstan#314 https://phpstan.org/r/d15041c9-a230-4b18-a627-fc859ea00838 )

@diimpp diimpp changed the title Misc refactor WIP Misc refactor Mar 10, 2020
@diimpp diimpp marked this pull request as ready for review March 10, 2020 08:38
@diimpp diimpp requested a review from a team as a code owner March 10, 2020 08:38
@diimpp diimpp force-pushed the bc_changes branch 2 times, most recently from 7045893 to 055fba9 Compare March 10, 2020 09:23
@diimpp
Copy link
Member Author

diimpp commented Mar 10, 2020

@lchrusciel Hi, sorry, it's still in work. I've moved PR from draft too early, because travis wasn't building properly, but it was the other issue.

@diimpp diimpp force-pushed the bc_changes branch 3 times, most recently from 05f676b to ac59e85 Compare March 10, 2020 17:41
@diimpp diimpp force-pushed the bc_changes branch 4 times, most recently from 705f3a6 to 33f5676 Compare March 11, 2020 10:34
@diimpp diimpp changed the title WIP Misc refactor Misc refactor Mar 11, 2020
@diimpp diimpp requested a review from lchrusciel March 11, 2020 11:32
@diimpp
Copy link
Member Author

diimpp commented Mar 11, 2020

@lchrusciel @Zales0123 it's ready for review. Let me know if you have any questions.

This PR is in fact 6 unrelated PRs, so please review each commit separately. Only difference is last two commits, because last commit is a supporting hack for phpstan.

P.S. There is another problematic code, which is src/StateResolver (No can checks, non-idiomatic files placement, Order->state state resolver doesn't work for me at all, but I didn't look too closely at it yet), but I won't be looking at it for the moment.

Cheers,
Dmitri.

@Zales0123 Zales0123 added Bug Confirmed bugs or bugfixes. Enhancement Minor issues and PRs improving the current solutions (optimizations, typo fixes, etc.). labels Mar 12, 2020
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 Dimnitri,
thanks for your contribution!
It seems ok to go with this implementation, but right now we don't have enough power to do a proper review here and possibly release a new version

@diimpp
Copy link
Member Author

diimpp commented Mar 16, 2020

@lchrusciel thank you for update. I do plan to change Refund/RefundPayment/CreditMemo mappings drastically, so it may be worth waiting for next PR anyway.

Current implementation with Refund/RefundPayment/CreditMemo, which are not related between each other is not usable for my company. We need CreditMemo to be generated only after RefundPayment is completed. Another case, we need to different between Refund's as when they were done and how they were grouped and after the fact of the refund, not at the refund event itself. (Another point is just technically not feasible approach. Let's say event fails for whatever reasons, then Refunds are created, but CreditMemo/RefundPayment probably not. And there is no way to re-send this event.)

@lchrusciel lchrusciel requested a review from Zales0123 April 8, 2020 13:50
Copy link
Member

@Zales0123 Zales0123 left a comment

Choose a reason for hiding this comment

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

It looks very good 👌 Thank you @diimpp for such wonderful work, you're making the world a better place 🖖 🎉

@lchrusciel lchrusciel merged commit 5b5978a into Sylius:master Apr 8, 2020
@lchrusciel
Copy link
Member

Thank you, Dmitri! 🥇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Confirmed bugs or bugfixes. Enhancement Minor issues and PRs improving the current solutions (optimizations, typo fixes, etc.).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace Address::getFirstName . getLastName with Address::getFullName
3 participants