169: beforeUpdateCallback by heshamMassoud · Pull Request #184 · commercetools/commercetools-sync-java

heshamMassoud · 2017-11-14T09:38:53Z

Summary

Rename the option updateActionsFilter to beforeUpdateCallback and make it more generic in the base sync option builders for all sync options to use: 2af6c3e, 6118485, 943b76b, 252a43a
beforeUpdateCallback is now a TriFunction which accepts three arguments update actions list, old resource and new resource draft: 1428876, 274af7d
Remove the set prefix in all option builders (e.g. setSyncFilter, setBatchSize, etc.. to syncFilter, batchSize, etc..) : fba0ba5, 3beab28, 904bd1e, 93f7dba
Removal of option removeOtherLocales since it can now be achieved through new beforeUpdateCallback: c117e1f
Provide IT for beforeUpdateCallback to sync only a sinlgle localization (french) of products: here

Relevant Issues

#169 #183

…ptionsBuilder.

…tils.

… and warningCallback.

…hanges.

…in old resource and new resource draft.

… type change.

codecov-io · 2017-11-14T09:53:05Z

Codecov Report

Merging #184 into master will decrease coverage by 2.47%.
The diff coverage is 23.94%.

@@             Coverage Diff              @@
##             master     #184      +/-   ##
============================================
- Coverage     95.22%   92.74%   -2.48%     
+ Complexity      795      789       -6     
============================================
  Files            65       66       +1     
  Lines          2052     2096      +44     
  Branches         98      112      +14     
============================================
- Hits           1954     1944      -10     
- Misses           79      133      +54     
  Partials         19       19

Impacted Files	Coverage Δ	Complexity Δ
...ls/sync/commons/helpers/BaseReferenceResolver.java	`100% <ø> (ø)`	`11 <0> (ø)`	⬇️
...cetools/sync/inventories/InventorySyncOptions.java	`100% <ø> (ø)`	`2 <0> (ø)`	⬇️
...ercetools/sync/categories/CategorySyncOptions.java	`100% <ø> (ø)`	`1 <0> (-1)`	⬇️
...mplates/beforeupdatecallback/SyncSingleLocale.java	`0% <0%> (ø)`	`0 <0> (?)`
...ercetools/sync/commons/BaseSyncOptionsBuilder.java	`100% <100%> (ø)`	`10 <4> (ø)`	⬇️
...tools/sync/categories/utils/CategorySyncUtils.java	`96.15% <100%> (-0.15%)`	`4 <0> (-2)`
...ols/sync/inventories/utils/InventorySyncUtils.java	`91.66% <100%> (ø)`	`1 <0> (ø)`	⬇️
...ommercetools/sync/products/ProductSyncOptions.java	`87.5% <100%> (-2.5%)`	`3 <1> (-1)`
...om/commercetools/sync/commons/BaseSyncOptions.java	`100% <100%> (ø)`	`17 <3> (+2)`	⬆️
...rcetools/sync/products/utils/ProductSyncUtils.java	`97.72% <100%> (-0.06%)`	`15 <0> (-2)`
... and 4 more

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 4693ab8...24b61bf. Read the comment docs.

…er strings.

butenkor · 2017-11-15T15:53:08Z

+which adds more information about the generated list of update actions, namley, the old resource being updated and the new
+resource draft. [#169](https://github.com/commercetools/commercetools-sync-java/issues/169)
+
+**Compatibility notes** (8)


Maybe better: Compatibility notes -> Migration guide

butenkor · 2017-11-15T15:56:07Z

+[Jar](https://bintray.com/commercetools/maven/commercetools-sync-java/v1.0.0-M5)
+
+**New Features** (1)
+- **Inventory Sync** - Support of `beforeUpdateCallback` which is callback function that is applied on the generated list


-> Introduced beforeUpdateCallback which is applied after generation of update actions and before actual resource update.

I intentionally use present tense to convey the meaning that this release "ships", "introduces", "supports", etc. this feature to the library. I've seen it in several release notes of different applications (also mentioned here: https://en.wikipedia.org/wiki/Release_notes)

Change of wording according to your request is here: 5346956

I have found that I was not consistent throughout this release notes though. What do you think should be the tense? Is there a specific reason you use paste tense? (accordingly I will make the release notes use consistent tense)

I like wording and the way things are described in our JVM SDK. We could apply same wording, wdyt?

I personally don't have a strong opinion about, so we can be consistent with JVM SDK as another lib of CT and also use past tense

changed to past tense: c1c6629

butenkor · 2017-11-15T15:58:04Z

+of update actions from syncing an old InventoryEntry with a new InventoryEntryDraft. [#169](https://github.com/commercetools/commercetools-sync-java/issues/169)
+
+**Enhancements** (1)
+- **Commons** - `setUpdateActionsCallback` is now `beforeUpdateCallback` and takes a TriFunction instead of Function, 


is now -> has been renamed to.

c1c6629#diff-aa75d2e7837774f5cb565717b6744a35R53

butenkor · 2017-11-15T15:58:33Z

+
+**Enhancements** (1)
+- **Commons** - `setUpdateActionsCallback` is now `beforeUpdateCallback` and takes a TriFunction instead of Function, 
+which adds more information about the generated list of update actions, namley, the old resource being updated and the new


namley -> namely

butenkor · 2017-11-15T16:06:20Z

+resource draft. [#169](https://github.com/commercetools/commercetools-sync-java/issues/169)
+
+**Compatibility notes** (8)
+- **Commons** - Change name of `setUpdateActionsCallback` to `beforeUpdateCallback`. [#169](https://github.com/commercetools/commercetools-sync-java/issues/169)


Change name -> Renamed

butenkor · 2017-11-15T16:11:28Z

    }

-    private List<UpdateAction<Product>> updateActionsCallback(@Nonnull final List<UpdateAction<Product>> builtActions) {
+    private List<UpdateAction<Product>> beforeUpdateCallback(@Nonnull final List<UpdateAction<Product>> builtActions,


Why not updateActions instead of builtActions?

butenkor · 2017-11-15T17:29:00Z

+  },
+  "catalogs": [],
+  "masterData": {
+    "current": {


We probably need stage only?

butenkor · 2017-11-15T17:37:42Z

+            .contains(LocalizedString.of(Locale.ENGLISH, "english description.", Locale.FRENCH, "french description."));
+
+        assertThat(updateActionsFromSync.stream()
+                                        .filter(updateAction -> updateAction instanceof ChangeSlug)


Maybe it makes sense to check for attribute update action with defined locales here too: https://github.com/commercetools/commercetools-sync-java/pull/184/files#diff-833857ed4fc9f1f796dae935c89b5d12R189

related to: #184 (comment)

butenkor · 2017-11-15T17:40:36Z

+        if (updateAction instanceof SetMetaKeywords) {
+            return filterLocalizedField(newDraft, oldProduct, locale, ProductDraft::getMetaKeywords,
+                ProductData::getMetaKeywords, SetMetaKeywords::of);
+        }


Also here it might be interesting to filter also localized attribute changes.

But for this, we would need the product type to know the attribute type if its localized or not. Could be done in a "hacky" to check if the JSON object has the keys "en", "de", etc.. as keys for example. But I didn't want to include that hacky why in the test and left up to the user of the callback how they want to implement it. WDYT?

butenkor · 2017-11-15T18:01:10Z

+        final String oldLocaleValue = oldLocalizedField.get(locale);
+        if (!Objects.equals(newLocaleValue, oldLocaleValue)) {
+            LocalizedString withLocaleChange = oldLocalizedField;
+            if (oldLocaleValue != null) {


If oldLocaleValue is null than it should ensure that newLocalizedField has French locale only, right?

if oldLocalValue is null, then it will set the new French locale (if it exists). But it shouldn't touch other locales.

The thing is that for some properties (like product description) "oldLocalizedField" might be not set yet so you will run into NPE.

Ah, now I know what you mean. You are right: I fixed the logic here: 2f3bb99

# Conflicts: # docs/RELEASE_NOTES.md

heshamMassoud added 24 commits November 13, 2017 14:17

#169: Adapt callback name change in IT usages.

252a43a

#169: Adapt name change in unit tests.

943b76b

#169: Change name of callback option and make it generic in BaseSyncO…

2af6c3e

…ptionsBuilder.

#169: Make beforeUpdateCallback generic in BaseSyncOptions.

6118485

#169: Implement applyBeforeUpdateCallback function and reuse in syncU…

e1accef

…tils.

#169: Add unit test for applying beforeUpdateCallback.

96915bc

#169: Change setAllowUuid to allowUuid.

61fff35

#169: Add release notes for changes till now.

edf1791

#169: Update usage docs.

1396d7f

#169: Adapt error and warning callback name changes in ITs.

476f242

#169: Adapt name change of warning and error callbacks in unit tests.

74af150

#169: Change setErrorCallBack and setWarningCallBack to errorCallback…

fba0ba5

… and warningCallback.

#169: Add release notes entries for error and warning callback name c…

8c82fc5

…hanges.

#169: Change name of setBatchSize to batchSize.

3beab28

#169: Remove 'set' prefixes from option builder setters.

904bd1e

#169: Add release notes for option builder setters name changes.

a7f1e44

#169: Implement TriFunction with unit tests.

1428876

#169: Change beforeUpdateCallback function to TriFunction that takes …

274af7d

…in old resource and new resource draft.

#169: Pass new and old resources to beforeUpdateCallback function.

13caf15

#169: Update unit tests of category and product options with callback…

2550aab

… type change.

#169: Adapt syncUtils unit test to callback type change.

ca0ef79

#169: Adapt ITs to new beforeUpdateCallback.

abd6024

#169: Add more documentation to TriFunction functional interface.

9782e43

#169: Make Checkstyle happy.

40cfd31

heshamMassoud added 5 commits November 15, 2017 10:21

#169: Change name, slug and description of dummy test product to easi…

88359c5

…er strings.

#169: Add test product with french localisations.

1d50c6f

#169: Adapt tests to changes in JSON resource.

27a7012

#169: Add IT for single localisation sync.

1279788

#169: Change setSyncFilter to syncFilter.

d08a3bf

heshamMassoud added 2 commits November 15, 2017 12:35

#169: Remove otherLocales option.

c117e1f

#160: Remove set prefix from unit test names.

93f7dba

heshamMassoud requested a review from andrii-kovalenko-ct November 15, 2017 13:41

heshamMassoud changed the title ~~WIP: 169 beforeUpdateCallback~~ 169: beforeUpdateCallback Nov 15, 2017

heshamMassoud requested a review from butenkor November 15, 2017 13:41

heshamMassoud added 3 commits November 15, 2017 14:50

#169: Update release notes.

a7fd15b

#169: Assert on counters as opposed to report message.

6aae4ad

#169: Use one line.

2cf55a0

butenkor reviewed Nov 15, 2017

View reviewed changes

heshamMassoud added 11 commits November 16, 2017 11:27

Merge branch 'master' into 169-update-actions-filter

0d55b18

# Conflicts: # docs/RELEASE_NOTES.md

#169: Reword release notes.

5346956

#169: Change Compatibility notes to Migration guide.

f967831

#169: Fix spelling mistake.

ada2d25

#169: Fix doc description.

a3ee8fa

#169: Fix param naming.

e62c3a8

#169: Remove current projections of product test jsons.

b8f073c

#169: Use past tense in release notes.

c1c6629

#169: Changed name -> renamed.

3cbc6c3

#169: Remove migration guide when not needed.

a4d75f0

#169: Fix localisation syncing logic.

2f3bb99

butenkor approved these changes Nov 16, 2017

View reviewed changes

heshamMassoud added 2 commits November 16, 2017 16:37

#169: Make ProductSyncSingleLocalizationIT a template instead of IT.

b45d279

#169: Remove unneeded main method in template.

24b61bf

heshamMassoud merged commit 7fbad35 into master Nov 16, 2017

heshamMassoud deleted the 169-update-actions-filter branch November 16, 2017 15:59

Conversation

heshamMassoud commented Nov 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Relevant Issues

Uh oh!

codecov-io commented Nov 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

heshamMassoud Nov 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

heshamMassoud commented Nov 14, 2017 •

edited

Loading

codecov-io commented Nov 14, 2017 •

edited

Loading

heshamMassoud Nov 16, 2017 •

edited

Loading