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

[Order]WIP order identities #2243

Merged
merged 3 commits into from
Dec 26, 2014
Merged

[Order]WIP order identities #2243

merged 3 commits into from
Dec 26, 2014

Conversation

inspiran
Copy link
Contributor

This PR is an attempt to allow to add identities to an order. For instance, when an order is synched to an external system such as an ERP system, you would want to track the ERP order id. Similary if you would import orders from an external party (for instance ebay orders) you would want to track a reference to the ebay id.

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? no
License MIT

How it currently can be used :

$identity = new Identity();
$identity->setName('ebayId');
$identity->setValue('123213213213');
$order->addIdentity($identity);

$identity = new Identity();
$identity->setName('ERP_SAP_ID);
$identity->setValue('4000123213');
$order->addIdentity($identity);

Identities are stored in table sylius_order_identities

Allow multiple identities for orders, typically used for storing external identifications, such as an backend (ERP) order id.
@@ -48,6 +48,12 @@
</cascade>
</one-to-many>

<one-to-many field="identities" target-entity="Sylius\Component\Order\Model\IdentityInterface" mapped-by="order" orphan-removal="true">
<cascade>
<cascade-all/>
Copy link
Member

Choose a reason for hiding this comment

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

Maybe cascade persist, because order is soft-deleted and we would prefer to keep these as well. :)

@pjedrzejewski
Copy link
Member

Forget the blank line comments, I just saw the WIP comment, but this is an excellent feature. I would definitely like to have it in core. :) Thanks man and let me know if you need any help/answers.

@inspiran
Copy link
Contributor Author

@pjedrzejewski Cool. The Identity concept could also be used for Products and Users (eg. if you want to sync them to OroCRM as account). In such case it would be even better to have an abstract Identity class from which Order/Identity and Product/Identity do extend. Where should such a common class be expected to reside? I first thought of Component/Core/Model but Component/Identity/Model could also be considered.

@pjedrzejewski
Copy link
Member

If we do it only in core then let's go in Core namespace, but if generic and default in Product component for example, then let's go with a new component... What do you guys think? Is it generic enough to go to a component (I think it is), but let me hear your opinion.

@inspiran
Copy link
Contributor Author

@pjedrzejewski I just had a small look at the Originator component. The documentation is unfortunately lacking and I was wondering what that will be used for and would be different from the Identification component.

Anyhow, identification component could also contain (abstract) identification / mapping resolver classes.

@inspiran
Copy link
Contributor Author

@pjedrzejewski Processed your comments, let me know what else needs to be done before it could be merged. I propose to start with having Identity class in Order, I will most likely refactor it into the Identity at a later moment.

@inspiran
Copy link
Contributor Author

@pjedrzejewski Any feedback?

@inspiran
Copy link
Contributor Author

@pjedrzejewski Pong

@pjedrzejewski
Copy link
Member

@inspiran All is good, could you just generate a doctrine migration? I think diff will be enough. :)

@inspiran
Copy link
Contributor Author

@pjedrzejewski valid point, added it.

pjedrzejewski pushed a commit that referenced this pull request Dec 26, 2014
[Order]WIP order identities
@pjedrzejewski pjedrzejewski merged commit 94153bb into Sylius:master Dec 26, 2014
@pjedrzejewski
Copy link
Member

Awesome, thank you Daniel! 👍

@@ -19,6 +19,7 @@
use Sylius\Component\Promotion\Model\PromotionInterface;
use Sylius\Component\Resource\Exception\UnexpectedTypeException;


Copy link
Contributor

Choose a reason for hiding this comment

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

Extra line.

pamil pushed a commit to pamil/Sylius that referenced this pull request May 7, 2019
pamil pushed a commit to pamil/Sylius that referenced this pull request May 7, 2019
pamil pushed a commit to pamil/Sylius that referenced this pull request May 7, 2019
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.

None yet

3 participants