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

Reference resolution #47

Closed
heshamMassoud opened this issue May 19, 2017 · 4 comments
Closed

Reference resolution #47

heshamMassoud opened this issue May 19, 2017 · 4 comments

Comments

@heshamMassoud
Copy link
Contributor

heshamMassoud commented May 19, 2017

Description

Some resources have references to other resources like supply channels on inventory entries or custom types on categories. The sync will always use the key to identify the referenced resources. This is because

  • when syncing from an external feed, the internal CTP resource id is not known.
  • when syncing from another CTP project, the CTP resource id is different

To better describe the issue, I will use supply channels on inventory entries as an example.

Currently, for the two scenarios:
1. Syncing inventory entries from a data feed (XML, CSV)
In order for the sync to be able to access this key, it needs to be accessible in one of the following ways:

  • Either provide the inventory entry with reference expansion for the supply channels. This way the key is directly accessible from the expanded channel.
  • Or don't do reference expansion, but provide the key value of the channel in place of the reference id. For example: Reference.of("Channel", "keyValue");

2. Syncing inventory entries from another CTP project.
In order for the sync to be able to access this key, it needs to be accessible in only one way:

  • The Inventory entry has to have the channel reference expanded in order to get the key.

The problem with the current approach is:-
Wrong channel creation
In the first scenario, if ensureChannel is set to true, the inventory is not reference expanded and the key is not set in the id field, then the sync will see this as an Inventory Entry without a Channel and thus will wrongly create a new one.

In the second scenario, if ensureChannel is set to true and the reference is not expanded, then the sync will see this as an Inventory Entry without a Channel and thus will wrongly create a new one.

Proposed Solution

referenceresoltion
For resources that have references to other resources on them, for example InventoryEntry has Custom which references a Type resource and supplyChannel which references a Channel resource.

  • This would be input in the library in a wrapper which has the InventoryEntryDraft, the supplyChannelKey and customTypeKey. Let's say this wrapper is called CustomInventoryEntry (open for better naming proposals

  • The user of the library should care how to provide these keys.

  • If the key is not set (null), then the library will consider this InventoryEntry without a Channel and will not create one for it. Even if ensureChannel is set.

  • If the key is set, the library will check if this channel exists, if it doesn't and ensureChannel is true, it will create this Channel.

FYI: @JakubChrobak @butenkor @piobra

@butenkor
Copy link
Member

CustomInventoryEntry -> SyncInventoryEntry or InventoryEntryWrapper?

@heshamMassoud
Copy link
Contributor Author

@butenkor SyncInventoryEntry feels a bit like a verb. I would go more for InventoryEntryWrapper.

How about InventoryEntrySyncWrapper, CategorySyncWrapper, etc..? or too long?

heshamMassoud added a commit that referenced this issue May 24, 2017
heshamMassoud added a commit that referenced this issue May 29, 2017
heshamMassoud added a commit that referenced this issue May 29, 2017
heshamMassoud added a commit that referenced this issue May 29, 2017
heshamMassoud added a commit that referenced this issue May 29, 2017
heshamMassoud added a commit that referenced this issue May 29, 2017
heshamMassoud added a commit that referenced this issue Jun 6, 2017
heshamMassoud added a commit that referenced this issue Jun 7, 2017
heshamMassoud added a commit that referenced this issue Jun 7, 2017
heshamMassoud added a commit that referenced this issue Jun 8, 2017
heshamMassoud added a commit that referenced this issue Jun 12, 2017
@heshamMassoud heshamMassoud self-assigned this Jun 12, 2017
@heshamMassoud heshamMassoud modified the milestone: 1.0.0-M1 Jun 19, 2017
heshamMassoud added a commit that referenced this issue Jun 19, 2017
#47: Add check for null/empty new custom type id.
@ValeSauer
Copy link
Contributor

ValeSauer commented Aug 29, 2017

Do you also have customFields or customAttributes in mind, that are of type reference? Not all referenceable resources have static identifiers:

  • product: key
  • product-type: key
  • channel: key
  • customer: key
  • state: key
  • zone: name
  • shipping Method: key
  • category: key
  • review: key
  • customObject: container + key

@heshamMassoud
Copy link
Contributor Author

heshamMassoud commented Aug 30, 2017

Thanks @ValeSauer for pointing that out 😊 I have been investigating it. And actually the library will sync these field types:

  1. BooleanType
  2. StringType
  3. LocalizedStringType
  4. EnumType
  5. LocalizedEnumType
  6. NumberType
  7. MoneyType
  8. DateType
  9. TimeType
  10. DateTimeType

But it will actually fail to sync custom fields of a field type: ReferenceType. Due to reference resolution not being done there. I just created an issue for it there: #87 It will probably be added in the next release (1.0.0-M2)

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

No branches or pull requests

3 participants