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

ahmetoz · 2020-11-05T10:34:29Z

Introduced ADR to document decisions.

Review Hint:
It would be better to check the file preview otherwise hard to understand because of the HTML source code in markdown.

codecov-io · 2020-11-05T11:39:56Z

Codecov Report

Merging #614 (1ef3339) into shopping-lists-base-branch (9f7eb31) will not change coverage.
The diff coverage is n/a.

@@                      Coverage Diff                      @@
##             shopping-lists-base-branch     #614   +/-   ##
=============================================================
  Coverage                         99.35%   99.35%           
  Complexity                         2424     2424           
=============================================================
  Files                               194      194           
  Lines                              6084     6084           
  Branches                            377      377           
=============================================================
  Hits                               6045     6045           
  Misses                               20       20           
  Partials                             19       19

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9f7eb31...6096a78. Read the comment docs.

docs/adr/0002-shopping-lists-lineitem-and-textlineitem-update-actions.md

…actions.md Co-authored-by: King-Hin Leung <35692276+leungkinghin@users.noreply.github.com>

docs/adr/0002-shopping-lists-lineitem-and-textlineitem-update-actions.md

lojzatran · 2020-11-05T15:11:06Z

docs/adr/0002-shopping-lists-lineitem-and-textlineitem-update-actions.md

+<!-- 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.
+- [Add TextLineItem](https://docs.commercetools.com/api/projects/shoppingLists#add-textlineitem) is not checking the data is exists, so a user could add the


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

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

What kind of explanation? Update action JSON?

docs/adr/0002-shopping-lists-lineitem-and-textlineitem-update-actions.md

leungkinghin-ct · 2020-11-05T15:19:37Z

docs/adr/0002-shopping-lists-lineitem-and-textlineitem-update-actions.md

+     "addedAt": "2020-11-04T09:38:35.571Z"
+    },
+    {
+      "id": "24de3821-e27d-4ddb-bd0b-ecc99365285f",


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

leungkinghin-ct · 2020-11-05T15:56:20Z

docs/adr/0002-shopping-lists-lineitem-and-textlineitem-update-actions.md

+            <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>


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']

Looks like same, #614 (comment)

leungkinghin-ct · 2020-11-05T16:05:55Z

docs/adr/0002-shopping-lists-lineitem-and-textlineitem-update-actions.md

+  "lineItems": [
+    {
+      "sku": "SKU-1",
+      "quantity": 1,


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

leungkinghin-ct · 2020-11-05T16:16:33Z

docs/adr/0002-shopping-lists-lineitem-and-textlineitem-update-actions.md

+    {
+      "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
+        }
+      }
+    }


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.

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

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.

docs/adr/0002-shopping-lists-lineitem-and-textlineitem-update-actions.md

…actions.md Co-authored-by: Lam Tran <lam.tran@commercetools.de>

Co-authored-by: Lam Tran <lam.tran@commercetools.de> Co-authored-by: King-Hin Leung <35692276+leungkinghin@users.noreply.github.com>

lojzatran · 2020-11-06T08:11:34Z

Can you add one more example - no new line item is added or removed, just order is different.
LineItemDrafts: L1 and L2
LineItems: L2 and L1

What is the solution here? [removeLineItem L1, removeLineItem L2, addLineItem L1, addLineItem L2]?

docs/adr/0002-shopping-lists-lineitem-and-textlineitem-update-actions.md

…actions.md Co-authored-by: Lam Tran <lam.tran@commercetools.de>

ahmetoz · 2020-11-06T09:04:55Z

Can you add one more example - no new line item is added or removed, just order is different.
LineItemDrafts: L1 and L2
LineItems: L2 and L1

What is the solution here? [removeLineItem L1, removeLineItem L2, addLineItem L1, addLineItem L2]?

d4f24e6 I don't know the exact algorithm yet, but could be done by removing and adding back.

… actions-line-items

ahmetoz · 2020-11-17T14:00:03Z

@leungkinghin @lojzatran could you please re-check the latest updates: 1ef3339, 3902376, 6096a78

docs/adr/0002-shopping-lists-lineitem-and-textlineitem-update-actions.md

…actions.md Co-authored-by: Lam Tran <lam.tran@commercetools.de>

lojzatran · 2020-11-17T14:09:50Z

docs/adr/0002-shopping-lists-lineitem-and-textlineitem-update-actions.md

+            </p>
+            <p>
+                First example seems reasonable but how you would sync a case like: 
+                '[SKU-1, SKU-2, SKU-3]' to '[SKU-3, SKU-1, SKU-4, SKU-2]', so with a new algorithm it might be done with 


Suggested change

'[SKU-1, SKU-2, SKU-3]' to '[SKU-3, SKU-1, SKU-4, SKU-2]', so with a new algorithm it might be done with

'[SKU-1, SKU-2, SKU-3]' to '[SKU-3, SKU-1, SKU-4, SKU-2]'? With a new algorithm it might be done with

What is the new algorithm?

there is none: 98822a9

lojzatran · 2020-11-17T14:10:48Z

docs/adr/0002-shopping-lists-lineitem-and-textlineitem-update-actions.md

+            <p>
+                First example seems reasonable but how you would sync a case like: 
+                '[SKU-1, SKU-2, SKU-3]' to '[SKU-3, SKU-1, SKU-4, SKU-2]', so with a new algorithm it might be done with 
+                change order [line-item-id-3, line-item-id-1, line-item-id-2] then removeLineItem SKU-2, add back addLineItem SKU-4, 


Please make those actions bold and also in the array [], so we know there are 3 of them.

lojzatran · 2020-11-17T14:11:08Z

docs/adr/0002-shopping-lists-lineitem-and-textlineitem-update-actions.md

+                in total 3 actions.
+            </p>
+            <p>
+                It looks like there are more different cases and that's why we decided to keep the idea of removing and adding back to 


Suggested change

It looks like there are more different cases and that's why we decided to keep the idea of removing and adding back to

It looks like there are more different cases. That's why we decided to keep the idea of removing and adding back to

Make the conclusion more dominant (bold, bigger font).

now better: 98822a9

docs/adr/0002-shopping-lists-lineitem-and-textlineitem-update-actions.md

Co-authored-by: Lam Tran <lam.tran@commercetools.de>

* master: Bump commercetoolsJvmSdkVersion from 1.54.0 to 1.55.0 Version updates and release notes of 3.0.0 (#620) 610 - Fix duration calculation for retry client. (+ refactor client creation logic) (#623) Add user guide for Shopping lists (#613) Cover shopping list line items in integration test scenarios (#626) Revert "Resolve ProductTypeSyncTest" wrapped <li> elements in <ul> tags (#616) Implement text line item actions (#625) Resolve ProductTypeSyncTest Implement LineItem actions (#618) Concept of the shopping lists' line item update actions. (#614) Bump assertj-core from 3.18.0 to 3.18.1 Implement Shoppinglist Sync class with IT/UT (exclude LineItem and TextLineItem) (#615) Add shopping list update actions (without line items) (#611) Create shoppinglist sync utils and helper (#595) Add ShoppingList Reference Resolver helpers/utilities. (#602)

ahmetoz added 5 commits November 4, 2020 13:46

introduce adr,

bbff90a

add cases for the line item orders.

23e6a4e

update link.

3006072

update.

99b3a94

update.

da821b5

ahmetoz requested review from butenkor, leungkinghin-ct and lojzatran November 5, 2020 11:36

Mention the importance of the order and addedAt value.

6d35149

leungkinghin-ct reviewed Nov 5, 2020

View reviewed changes

docs/adr/0002-shopping-lists-lineitem-and-textlineitem-update-actions.md Outdated Show resolved Hide resolved

Update docs/adr/0002-shopping-lists-lineitem-and-textlineitem-update-…

1e9fccf

…actions.md Co-authored-by: King-Hin Leung <35692276+leungkinghin@users.noreply.github.com>

lojzatran requested changes Nov 5, 2020

View reviewed changes

leungkinghin-ct reviewed Nov 5, 2020

View reviewed changes

docs/adr/0002-shopping-lists-lineitem-and-textlineitem-update-actions.md Outdated Show resolved Hide resolved

ahmetoz and others added 10 commits November 5, 2020 17:37

Update docs/adr/0002-shopping-lists-lineitem-and-textlineitem-update-…

b82c453

…actions.md Co-authored-by: Lam Tran <lam.tran@commercetools.de>

Grammar updates

02fc905

Co-authored-by: Lam Tran <lam.tran@commercetools.de> Co-authored-by: King-Hin Leung <35692276+leungkinghin@users.noreply.github.com>

Add link to clarify the variant selection.

3fba6c5

update decision one.

7c9fc7a

mardown fix.

23e8208

mention the action.

baa0c26

move notes to context.

6778f79

fix typo, in quantity

b8f0947

update wrong use case.

97d4d15

use a different id to avoid confusion.

8fe4121

ahmetoz marked this pull request as ready for review November 5, 2020 17:39

json fix.

76eb9d2

lojzatran reviewed Nov 6, 2020

View reviewed changes

docs/adr/0002-shopping-lists-lineitem-and-textlineitem-update-actions.md Outdated Show resolved Hide resolved

ahmetoz and others added 2 commits November 6, 2020 09:15

Update docs/adr/0002-shopping-lists-lineitem-and-textlineitem-update-…

2a78606

…actions.md Co-authored-by: Lam Tran <lam.tran@commercetools.de>

add one more case.

d4f24e6

ahmetoz requested review from lojzatran and leungkinghin-ct and removed request for butenkor November 10, 2020 12:04

ahmetoz mentioned this pull request Nov 11, 2020

Implement LineItem actions #618

Merged

6 tasks

ahmetoz added 5 commits November 11, 2020 13:02

Merge remote-tracking branch 'origin/shopping-lists-base-branch' into…

394c063

… actions-line-items

Update addedAt case.

1ef3339

Update addedAt field.

c745ed0

Update the case 3.

3902376

Update cases for text line item.

6096a78

lojzatran reviewed Nov 17, 2020

View reviewed changes

docs/adr/0002-shopping-lists-lineitem-and-textlineitem-update-actions.md Outdated Show resolved Hide resolved

ahmetoz mentioned this pull request Nov 17, 2020

Implement text line item actions #625

Merged

ahmetoz and others added 2 commits November 17, 2020 15:04

Update docs/adr/0002-shopping-lists-lineitem-and-textlineitem-update-…

9d97f9a

…actions.md Co-authored-by: Lam Tran <lam.tran@commercetools.de>

Update analysis.

18ce8ba

lojzatran requested changes Nov 17, 2020

View reviewed changes

ahmetoz and others added 3 commits November 17, 2020 16:24

Apply suggestions from code review

e0e18bc

Co-authored-by: Lam Tran <lam.tran@commercetools.de>

Reword, also..

ded6277

make actions and decisions bold.

98822a9

ahmetoz requested a review from lojzatran November 17, 2020 15:40

lojzatran approved these changes Nov 17, 2020

View reviewed changes

ahmetoz merged commit a834e77 into shopping-lists-base-branch Nov 17, 2020

ahmetoz deleted the actions-line-items branch November 17, 2020 16:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ahmetoz commented Nov 5, 2020 •

edited

codecov-io commented Nov 5, 2020 •

edited

lojzatran Nov 5, 2020 •

edited by ahmetoz

lojzatran Nov 5, 2020

ahmetoz Nov 6, 2020

leungkinghin-ct Nov 5, 2020

ahmetoz Nov 5, 2020

leungkinghin-ct Nov 5, 2020 •

edited

ahmetoz Nov 5, 2020

leungkinghin-ct Nov 5, 2020

ahmetoz Nov 5, 2020

leungkinghin-ct Nov 5, 2020 •

edited

ahmetoz Nov 5, 2020

ahmetoz Nov 5, 2020

lojzatran commented Nov 6, 2020 •

edited

ahmetoz commented Nov 6, 2020

ahmetoz commented Nov 17, 2020

lojzatran Nov 17, 2020

lojzatran Nov 17, 2020

ahmetoz Nov 17, 2020

lojzatran Nov 17, 2020

ahmetoz Nov 17, 2020

lojzatran Nov 17, 2020

lojzatran Nov 17, 2020

ahmetoz Nov 17, 2020

	- [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

	'[SKU-1, SKU-2, SKU-3]' to '[SKU-3, SKU-1, SKU-4, SKU-2]', so with a new algorithm it might be done with
	'[SKU-1, SKU-2, SKU-3]' to '[SKU-3, SKU-1, SKU-4, SKU-2]'? With a new algorithm it might be done with

	It looks like there are more different cases and that's why we decided to keep the idea of removing and adding back to
	It looks like there are more different cases. That's why we decided to keep the idea of removing and adding back to

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

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

Conversation

ahmetoz commented Nov 5, 2020 • edited

codecov-io commented Nov 5, 2020 • edited

Codecov Report

lojzatran Nov 5, 2020 • edited by ahmetoz

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leungkinghin-ct Nov 5, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leungkinghin-ct Nov 5, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

In scenario 1,

In scenario 2,

lojzatran commented Nov 6, 2020 • edited

ahmetoz commented Nov 6, 2020

ahmetoz commented Nov 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahmetoz commented Nov 5, 2020 •

edited

codecov-io commented Nov 5, 2020 •

edited

lojzatran Nov 5, 2020 •

edited by ahmetoz

leungkinghin-ct Nov 5, 2020 •

edited

leungkinghin-ct Nov 5, 2020 •

edited

lojzatran commented Nov 6, 2020 •

edited