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.

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

Moved all entities to mapped-superclasses for better extensibility #130

Merged
merged 3 commits into from
Jun 10, 2019
Merged

Moved all entities to mapped-superclasses for better extensibility #130

merged 3 commits into from
Jun 10, 2019

Conversation

peterukena
Copy link

@peterukena peterukena commented Apr 17, 2019 •
edited by pamil
Loading

Can you also please tag a version of this for Sylius 1.3?

Closes #132.

@peterukena peterukena requested a review from a team as a code owner April 17, 2019 10:50
@peterukena peterukena mentioned this pull request Apr 17, 2019
Merged
@peterukena
Copy link
Author

We need to finish a fix for the InvoicePlugin and port that over to this repository as well.

lchrusciel
lchrusciel approved these changes Apr 18, 2019
View reviewed changes
tests/Behat/Page/Admin/OrderRefundsPage.php Outdated Show resolved Hide resolved
@Zales0123
Copy link
Member

The main problem with this change is that we have a few embeddables used in the doctrine mapping... This is the same bug as here: Sylius/InvoicingPlugin#97. Do you have any idea how it can be fixed? Every help would be helpful, as it for sure needs to be resolved.

@peterukena
Copy link
Author

I currently have no idea how to fix that and I will probably not get the time to look into that in depth before our setup crashes. ;)

And yes, its the same as Sylius/InvoicingPlugin#97 - I also mentioned this. Can we maybe tag these issues over Refund/Invoice/Rbac Plugin - because Rbac will probably get the same issues as soon as its starting to use embeddables.

Voting to keep the Invioice-Issue as the main discussion though :)

@Zales0123 Zales0123 mentioned this pull request Apr 30, 2019
Closed
@pamil
Copy link
Contributor

pamil commented Jun 7, 2019

One solution to rule them all: Sylius/SyliusResourceBundle#89

Zales0123
Zales0123 approved these changes Jun 10, 2019
View reviewed changes
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.

I've made a CreditMemoSequence entity again - I suppose it should not be easily extendable, as it's just a helper entity for credit memo number creation... And it was braking some tests 😄

@pamil
Copy link
Contributor

pamil commented Jun 10, 2019

Merging it as it improves the current state of this plugin, though I'll investigate the issue with CreditMemoSequence.

@pamil pamil merged commit 915cb7b into Sylius:master Jun 10, 2019
@pamil
Copy link
Contributor

pamil commented Jun 10, 2019

Thank you, Peter! 🎉

@peterukena peterukena deleted the entities_to_mapped_superclasses branch June 11, 2019 07:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lchrusciel lchrusciel lchrusciel approved these changes

@Zales0123 Zales0123 Zales0123 approved these changes

@pamil pamil pamil approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@peterukena @Zales0123 @pamil @lchrusciel