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

Concept of the shopping lists' line item update actions. #614

Merged
merged 30 commits into from Nov 17, 2020
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
bbff90a
introduce adr,
ahmetoz Nov 4, 2020
23e6a4e
add cases for the line item orders.
ahmetoz Nov 5, 2020
3006072
update link.
ahmetoz Nov 5, 2020
99b3a94
update.
ahmetoz Nov 5, 2020
da821b5
update.
ahmetoz Nov 5, 2020
6d35149
Mention the importance of the order and addedAt value.
ahmetoz Nov 5, 2020
1e9fccf
Update docs/adr/0002-shopping-lists-lineitem-and-textlineitem-update-…
ahmetoz Nov 5, 2020
b82c453
Update docs/adr/0002-shopping-lists-lineitem-and-textlineitem-update-…
ahmetoz Nov 5, 2020
02fc905
Grammar updates
ahmetoz Nov 5, 2020
3fba6c5
Add link to clarify the variant selection.
ahmetoz Nov 5, 2020
7c9fc7a
update decision one.
ahmetoz Nov 5, 2020
23e8208
mardown fix.
ahmetoz Nov 5, 2020
baa0c26
mention the action.
ahmetoz Nov 5, 2020
6778f79
move notes to context.
ahmetoz Nov 5, 2020
b8f0947
fix typo, in quantity
ahmetoz Nov 5, 2020
97d4d15
update wrong use case.
ahmetoz Nov 5, 2020
8fe4121
use a different id to avoid confusion.
ahmetoz Nov 5, 2020
76eb9d2
json fix.
ahmetoz Nov 5, 2020
2a78606
Update docs/adr/0002-shopping-lists-lineitem-and-textlineitem-update-…
ahmetoz Nov 6, 2020
d4f24e6
add one more case.
ahmetoz Nov 6, 2020
394c063
Merge remote-tracking branch 'origin/shopping-lists-base-branch' into…
ahmetoz Nov 11, 2020
1ef3339
Update addedAt case.
ahmetoz Nov 11, 2020
c745ed0
Update addedAt field.
ahmetoz Nov 17, 2020
3902376
Update the case 3.
ahmetoz Nov 17, 2020
6096a78
Update cases for text line item.
ahmetoz Nov 17, 2020
9d97f9a
Update docs/adr/0002-shopping-lists-lineitem-and-textlineitem-update-…
ahmetoz Nov 17, 2020
18ce8ba
Update analysis.
ahmetoz Nov 17, 2020
e0e18bc
Apply suggestions from code review
ahmetoz Nov 17, 2020
ded6277
Reword, also..
ahmetoz Nov 17, 2020
98822a9
make actions and decisions bold.
ahmetoz Nov 17, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions .adr-dir
@@ -0,0 +1 @@
docs/adr
19 changes: 19 additions & 0 deletions docs/adr/0001-record-architecture-decisions.md
@@ -0,0 +1,19 @@
# 1. Record architecture decisions

Date: 2020-11-04

## Status

Accepted

## Context

We need to record the architectural decisions made on this project.

## Decision

We will use Architecture Decision Records, as [described by Michael Nygard](http://thinkrelevance.com/blog/2011/11/15/documenting-architecture-decisions).

## Consequences

See Michael Nygard's article, linked above. For a lightweight ADR toolset, see Nat Pryce's [adr-tools](https://github.com/npryce/adr-tools).
@@ -0,0 +1,332 @@
# 2. LineItem and TextLineItem update actions of the ShoppingLists.

Date: 2020-11-04

## Status

[Proposed](https://github.com/commercetools/commercetools-sync-java/pull/614)

## Context

<!-- The issue motivating this decision, and any context that influences or constrains the decision. -->

In a commerce application, a shopping list is a personal wishlist of a customer, (i.e. Ingredients for a recipe, birthday wishes).
ahmetoz marked this conversation as resolved.
Show resolved Hide resolved
Shopping lists hold line items of products in the platform or any other items that can be described as text line items.

We have challenges to build update actions of the `LineItem` and `TextLineItem` because of the nature of the synchronization,
so in this document, we will describe the reasons and constraints.

> Note: The cases below are the same for the `TextLineItem` so here those are no added as a separate case.
ahmetoz marked this conversation as resolved.
Show resolved Hide resolved
ahmetoz marked this conversation as resolved.
Show resolved Hide resolved

### How to ensure line item order?

<table>
<tr>
<th>LineItemDrafts</th>
<th>LineItems</th>
</tr>
<tr>
<td><pre lang="json">
{
"lineItems": [
{
"sku": "SKU-1",
"quantity": 1,
"addedAt": "2020-11-04T09:38:35.571Z",
"custom": {
"type": {
"key": "custom-type-for-shoppinglists"
},
"fields": {
"textField": "text-1"
}
}
},
{
"sku": "SKU-2",
"quantity": 2,
"addedAt": "2020-11-04T09:40:12.341Z",
"custom": {
"type": {
"key": "custom-type-for-shoppinglists"
},
"fields": {
"textField": "text-2"
}
}
}
]
}
</pre>
</td>
<td><pre lang="json">
{
"lineItems": [
{
"id": "24de3821-e27d-4ddb-bd0b-ecc99365285f",
"variant": {
"sku": "SKU-2"
},
"quantity": 2,
"custom": {
"type": {
"id": "4796e155-f5a4-403a-ae1a-04b10c9dfc54",
"obj": {
"key": "custom-type-for-shoppinglists"
}
},
"fields": {
"textField": "text-2"
}
},
"addedAt": "2020-11-04T09:38:35.571Z"
}
]
}
</pre>
</td>
</tr>
<tr>
<th colspan="2">Analysis</th>
</tr>
<tr>
<td colspan="2">
<p>Draft has line items with <b>SKU-1</b> and <b>SKU-2</b> also in target project line item with
ahmetoz marked this conversation as resolved.
Show resolved Hide resolved
<b>SKU-2</b> exists, so <b>SKU-1</b> is a new line item. </p>
<p> So we need to create an <a href="https://docs.commercetools.com/api/projects/shoppingLists#add-lineitem">AddLineItem</a> action
and a <a href="https://docs.commercetools.com/api/projects/shoppingLists#change-lineitems-order">Change LineItems Order</a>
of the line items <b>SKU-1</b> and <b>SKU-2</b>, because when we add line item with <b>SKU-1</b>
the order will be <b>SKU-2</b> and <b>SKU-1</b>.</p>
<p>The <b>challenge</b> in here is, those actions can not be added in one request because we don't know the
line item id of the new line item
with <b>SKU-1</b>, so we need to find another way to create a new line item with the right order.</p>
</td>
</tr>
<tr>
<th colspan="2">Proposed solution</th>
</tr>
<tr>
<td colspan="2">
<p>
Normally, for a difference, we might do a set intersection and then calculate action for differences,
but that does not make sense because we are not aware of the order from the draft.
So in this case the one request could be created but we might need to remove line item with <b>SKU-2</b>
and line items in the draft with the given order with the line items <b>SKU-1</b> and <b>SKU-2</b>.
</p>
<pre lang="json">
{
"version": 1,
"actions": [
{
"action": "removeLineItem",
"lineItemId": "24de3821-e27d-4ddb-bd0b-ecc99365285f"
},
{
"action": "addLineItem",
"sku": "SKU-1",
"quantity": 1,
"addedAt": "2020-11-04T09:38:35.571Z",
"custom": {
"type": {
"key": "custom-type-for-shoppinglists"
},
"fields": {
"textField": "text-1"
}
}
},
{
"action": "addLineItem",
"sku": "SKU-2",
"quantity": 2,
"addedAt": "2020-11-04T09:40:12.341Z",
"custom": {
"type": {
"key": "custom-type-for-shoppinglists"
},
"fields": {
"textField": "text-2"
}
}
}
]
}
</pre>
</td>
</tr>
</table>

### Do we need to remove all line items when the order changes ?

<table>
<tr>
<th>LineItemDrafts</th>
<th>LineItems</th>
</tr>
<tr>
<td><pre lang="json">
{
"lineItems": [
{
"sku": "SKU-1",
"quantity": 1,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is quantity supposed to update? I see the quantity of existing lineitem 'sku-1' is different.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"addedAt": "2020-11-04T09:38:35.571Z",
"custom": {
"type": {
"key": "custom-type-for-shoppinglists"
},
"fields": {
"textField": "text-1"
}
}
},
{
"sku": "SKU-3",
"quantity": 3,
"addedAt": "2020-11-05T10:00:10.101Z",
"custom": {
"type": {
"key": "custom-type-for-shoppinglists"
},
"fields": {
"textField": "text-3"
}
}
}
]
}
</pre>
</td>
<td><pre lang="json">
{
"lineItems": [
{
"id": "24de3821-e27d-4ddb-bd0b-ecc99365285f",
"variant": {
"sku": "SKU-1"
},
"quantity": 2,
"custom": {
"type": {
"id": "4796e155-f5a4-403a-ae1a-04b10c9dfc54",
"obj": {
"key": "custom-type-for-shoppinglists"
}
},
"fields": {
"textField": "text-1"
}
},
"addedAt": "2020-11-04T09:38:35.571Z"
},
{
"id": "24de3821-e27d-4ddb-bd0b-ecc99365285f",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it supposed to have different ID as LineItem SKU-1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"variant": {
"sku": "SKU-2"
},
"quantity": 2,
"custom": {
"type": {
"id": "4796e155-f5a4-403a-ae1a-04b10c9dfc54",
"obj": {
"key": "custom-type-for-shoppinglists"
}
},
"fields": {
"textField": "text-2"
}
},
"addedAt": "2020-11-04T09:40:12.341Z"
}
]
}
</pre>
</td>
</tr>
<tr>
<th colspan="2">Analysis</th>
</tr>
<tr>
<td colspan="2">
<p>Draft has line items with <b>SKU-1</b> and <b>SKU-3</b> also in target project line item with
<b>SKU-1</b> exists, so <b>SKU-3</b> is a new line item, and <b>SKU-2</b> needs to be removed.</p>
<p> So we need to create an <a href="https://docs.commercetools.com/api/projects/shoppingLists#add-lineitem">AddLineItem</a> action
, a <a href="https://docs.commercetools.com/api/projects/shoppingLists#remove-lineitem">RemoveLineItem</a> action and
a <a href="https://docs.commercetools.com/api/projects/shoppingLists#change-lineitems-order">Change LineItems Order</a>
of the line items <b>SKU-1</b> and <b>SKU-3</b>, because when we add line item with <b>SKU-1</b>
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand here.
In the drafts, the order is ['sku-1', 'sku-3']. Isn't it expected and no need to change the order?
I would understand ChangeLineitemOrder is required if the order in drafts is ['sku-3', 'sku-1']

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like same, #614 (comment)

the order will be <b>SKU-1</b> and <b>SKU-3</b>.</p>
<p>The <b>challenge</b> in here is, those actions can not be added in one request because we don't know the
line item id of the new line item
with <b>SKU-3</b>, so we need to find another way to create a new line item with the right order.</p>
<p>Also another <b>challenge</b> in here is about the line item with <b>SKU-1</b>, as the order and data is
exactly same, we need to find a better way to avoid creating unnecessary actions.
</p>
</td>
</tr>
<tr>
<th colspan="2">Proposed solution</th>
</tr>
<tr>
<td colspan="2">
<p>
The solution idea about the new line item and changed order is still same with the case-1, but do we
ahmetoz marked this conversation as resolved.
Show resolved Hide resolved
need to remove and add line item with <b>SKU-1</b>? Which is not needed and we could start the removing
ahmetoz marked this conversation as resolved.
Show resolved Hide resolved
and adding from the changed order.
</p>
<pre lang="json">
{
"version": 1,
"actions": [
{
"action": "removeLineItem",
"lineItemId": "24de3821-e27d-4ddb-bd0b-ecc99365285f"
},
{
"action": "addLineItem",
"sku": "SKU-3",
"quantity": 3,
"addedAt": "2020-11-05T10:00:10.101Z",
"custom": {
"type": {
"key": "custom-type-for-shoppinglists"
},
"fields": {
"textField": "text-3
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose it is going to remove existing 'sku-2' line item.

These two updateActions are appropriate. I am just curious how we determine which updateActions are needed in UpdateActionUtil for each LineItem.

For example
existing line item : ['sku-4', 'sku-1', 'sku-2']
new line items : ['sku-3', 'sku-4', 'sku-1']

Besides adding 'sku-3' and removing 'sku-2', we have to remove 'sku-1', 'sku-4' from existing line items as well. Otherwise the order is not the same as expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch!, You are right, the scenario was the same as scenario 1. 97d4d15

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From your example, you checked sku-3 and sku-4 is different than to able to match the order, all actions need to be removed and added back because you need to ensure the order.

In scenario 1,

draft : [sku1,..., sku2]
target: [sku2...]

In scenario 2,

draft: [sku1,..., sku3, sku2]
target: [sku1,..., sku2]

sku1 is exactly the same until you find the first difference assume you have N same but a difference in the middle, you don't need to create actions for those N items, here how we will know the difference depends on the comparisons between draft and target objects, so it's like doing sequence comparisons with their order. During the implementation, I could find more examples and could add here.

]
}
</pre>
</td>
</tr>
</table>

### How addedAt will be compared?

In commercetools shopping lists API, there is no [update action](https://docs.commercetools.com/api/projects/shoppingLists#update-actions)
to change the `addedAt` field of the `LineItem` and `TextLineItem`, also in API it has a default value described as `Defaults to the current date and time.`,
when it's not set in the draft, so how to compare and update this field?

**Proposed solution:**

A sync option for excluding `addedAt` (based on the draft) comparisons, which defaults to `true`. The advantage of this, customers could decide themself sync `addedAt`.
The drawback is when the option set to `false` and a difference is found by our utils, to ensure the order we need to remove and add all line items.

## Decision (not agreed yet with the team)

<!-- The change that we're proposing or have agreed to implement. -->

- Product variant will be matched by its SKU, if the SKU not set for a LineItemDraft, the draft will not be synced and an error callback will be triggered.
> Note: In commercetools API, the product variant to be selected in the LineItemDraft can be specified either by its product ID plus variant ID or by its SKU.
lojzatran marked this conversation as resolved.
Show resolved Hide resolved

- When a [Change LineItems Order](https://docs.commercetools.com/api/projects/shoppingLists#change-lineitems-order">) action is needed
ahmetoz marked this conversation as resolved.
Show resolved Hide resolved
the line items will be removed and added back with the order provided in the `ShoppingListDraft`.

## Consequences

<!-- What becomes easier or more difficult to do and any risks introduced by the change that will need to be mitigated. -->

- To ensure the order, we need to remove and add line items, which means a bigger payload, so performance overhead.
lojzatran marked this conversation as resolved.
Show resolved Hide resolved
- [Add TextLineItem](https://docs.commercetools.com/api/projects/shoppingLists#add-textlineitem) is not checking the data is exists, so a user could add the
Copy link
Collaborator

@lojzatran lojzatran Nov 5, 2020

Choose a reason for hiding this comment

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

Suggested change
- [Add TextLineItem](https://docs.commercetools.com/api/projects/shoppingLists#add-textlineitem) is not checking the data is exists, so a user could add the
- [Add TextLineItem](https://docs.commercetools.com/api/projects/shoppingLists#add-textlineitem) is not checking if the data exists. A user could add the

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand well this consequence. Can you explain?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What kind of explanation? Update action JSON?

exact same data multiple times, so it's hard to know the order by just checking the object, so in this case, if the data is the same, the order will not be changed.
ahmetoz marked this conversation as resolved.
Show resolved Hide resolved