#300: Add support for sync of custom types by ahmetoz · Pull Request #304 · commercetools/commercetools-sync-java

ahmetoz · 2018-09-13T09:57:29Z

Summary

Adds implementations for the type sync.

Description

type resource sync implementations

Relevant Issues

#300

Todo

Hints for Review

- Adds the core type sync implementations - Adds some unit tests

codecov-io · 2018-09-20T12:22:54Z

Codecov Report

Merging #304 into master will increase coverage by 0.17%.
The diff coverage is 98.89%.

@@             Coverage Diff              @@
##             master     #304      +/-   ##
============================================
+ Coverage     96.91%   97.09%   +0.17%     
- Complexity     1097     1203     +106     
============================================
  Files           104      116      +12     
  Lines          2693     2963     +270     
  Branches        129      137       +8     
============================================
+ Hits           2610     2877     +267     
- Misses           71       72       +1     
- Partials         12       14       +2

Impacted Files	Coverage Δ	Complexity Δ
.../types/utils/FieldDefinitionUpdateActionUtils.java	`100% <100%> (ø)`	`9 <9> (?)`
...mercetools/sync/services/impl/TypeServiceImpl.java	`100% <100%> (ø)`	`16 <9> (+9)`	⬆️
.../commercetools/sync/types/utils/TypeSyncUtils.java	`100% <100%> (ø)`	`1 <1> (?)`
...mercetools/sync/types/helpers/FieldTypeAssert.java	`100% <100%> (ø)`	`7 <7> (?)`
...types/utils/FieldDefinitionsUpdateActionUtils.java	`100% <100%> (ø)`	`20 <20> (?)`
...tools/sync/types/utils/EnumUpdateActionsUtils.java	`100% <100%> (ø)`	`11 <11> (?)`
.../com/commercetools/sync/types/TypeSyncOptions.java	`100% <100%> (ø)`	`1 <1> (?)`
...etools/sync/types/utils/TypeUpdateActionUtils.java	`100% <100%> (ø)`	`5 <5> (?)`
...mmercetools/sync/types/TypeSyncOptionsBuilder.java	`100% <100%> (ø)`	`4 <4> (?)`
...cetools/sync/types/helpers/TypeSyncStatistics.java	`100% <100%> (ø)`	`2 <2> (?)`
... and 15 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 eb67c4d...880ae86. Read the comment docs.

lojzatran

Nice work with this PR! I found few things that needs to be addressed or answered.

Pls also update the product type sync if you address the changes, because it seems like you got a lot of inspirations from there.

lojzatran · 2018-09-24T14:08:27Z

+        // remove enum value actions not exists for type resources
+        assertThat(updateActions).containsExactly(
+            ChangeLocalizedEnumValueOrder.of(FIELD_NAME_1, asList(
+                ENUM_VALUE_C.getKey(),


What about A? Don't you have to have all enums in this action?

A should be removed but it's not supported by the API.

I would keep a test that its failing for it but mark it with @ignore annotation and a TODO comment to fix it once the API provides it.

ahalberkamp · 2018-09-25T09:16:40Z

+    - Retries on 5xx errors with a retry strategy. This can be achieved by decorating the `sphereClient` with the
+   [RetrySphereClientDecorator](http://commercetools.github.io/commercetools-jvm-sdk/apidocs/io/sphere/sdk/client/RetrySphereClientDecorator.html)
+
+   You can use the same client instantiating used in the integration tests for this library found


...You can use the same client instance in the integration tests

f4fc415#diff-649a74fcd11f3ef15302f4486a9ddbd1R33

@ahalberkamp @ahmetoz no actually it's not meant to say you can use the "same client instance". It should be along the lines: "You can instantiate the client the same way it is instantiated in the integration tests for this library found.."

@ahmetoz As mentioned here. It would be great if you adapt any comments in the similar docs :)

ahalberkamp · 2018-09-25T10:56:30Z

+            @Nonnull final List<TypeDraft> typeDrafts) {
+
+        final List<List<TypeDraft>> batches = batchElements(typeDrafts, syncOptions.getBatchSize());
+


Remove empty line

f4fc415#diff-cbaa8f553c4a4edc2f17bf36e0578668L72

ahmetoz · 2018-10-04T14:36:52Z

What is empty updated/created statistics?

I could not find a fancy name for it 😅

lojzatran · 2018-10-04T15:45:52Z

I could not find a fancy name for it 😅

You should find a fancy name :) It's open-source repo and everybody can see our code.

heshamMassoud · 2018-10-04T21:14:17Z

Usually I add a release notes entry for what has changed/added/enhances/fixed (that the user of the library needs to know) in the PR in the commented out section that it will be released in. The release notes file is: docs/RELEASE_NOTES.md.

This also includes any public methods/utils that users from outside the library can use or cannot use anymore. Or are already using and some behaviour has changed for example.

Please add entries related to your PR there too :)

heshamMassoud

Good work @ahmetoz! I added a first review of this PR.

heshamMassoud · 2018-10-04T21:21:58Z

 [Javadoc](https://commercetools.github.io/commercetools-sync-java/v/v1.0.0-M14/) | 
 [Jar](https://bintray.com/commercetools/maven/commercetools-sync-java/v1.0.0-M14)

+**New Features** (1)


Please add also entries for all the public utils that were added.

6f14845#diff-aa75d2e7837774f5cb565717b6744a35R59

heshamMassoud · 2018-10-04T21:23:17Z

+
+1. Types are either created or updated. Currently the tool does not support type deletion.
+2. Updating the label of enum values and localized enum values of field definition is not supported yet.
+3. Removing the enum values from the field definition is not supported yet.


nice that you added them as caveats here 😊Would be great if you can also mention/link the CTP support tickets related to them (of course only if they are public) so we can cross relate them in the future.

Tickets for the issues are not public 👎

heshamMassoud · 2018-10-04T21:26:08Z

+
+__Note__ The statistics object contains the processing time of the last batch only. This is due to two reasons:
+ 1. The sync processing time should not take into account the time between supplying batches to the sync.
+ 2. It is not not known by the sync which batch is going to be the last one supplied.


It is not not -> It is not

6f14845#diff-649a74fcd11f3ef15302f4486a9ddbd1L91

heshamMassoud · 2018-10-04T21:34:51Z

+
+    /**
+     * Creates new instance of {@link TypeSyncOptions} enriched with all fields provided to {@code this}
+     * builder.


builder doesn't have to be on a separate line 😉

9801dbd#diff-b970a8b59ae976dd58682c2b661d40c6L31

heshamMassoud · 2018-10-04T21:35:39Z

+import javax.annotation.Nonnull;
+
+public final class TypeSyncOptionsBuilder extends BaseSyncOptionsBuilder<TypeSyncOptionsBuilder,
+        TypeSyncOptions, Type, TypeDraft> {


I think this line is extra indented..

9801dbd#diff-b970a8b59ae976dd58682c2b661d40c6L10

heshamMassoud · 2018-10-04T22:36:25Z

+     * @return A list with the update actions or an empty list if the field definition fields are identical.
+     */
+    @Nonnull
+    public static List<UpdateAction<Type>> buildActions(


@ahmetoz you are handling type changes here by the work around of (remove-then-add)

heshamMassoud · 2018-10-04T22:39:20Z

+        final Map<String, Type> oldTypeMap = getKeysTypeMap(oldTypes);
+
+        return CompletableFuture.allOf(newTypes
+            .stream()


Its always more readable if you indent the chain dots of the same level with the same indentation level.

9801dbd#diff-cbaa8f553c4a4edc2f17bf36e0578668L193

heshamMassoud · 2018-10-04T22:41:20Z

     */
    @Nonnull
    CompletionStage<Optional<String>> fetchCachedTypeId(@Nonnull final String key);
+


Please cover the newly added services with integration tests.

heshamMassoud · 2018-10-04T22:43:24Z

+        // remove enum value actions not exists for type resources
+        assertThat(updateActions).containsExactly(
+            ChangeLocalizedEnumValueOrder.of(FIELD_NAME_1, asList(
+                ENUM_VALUE_C.getKey(),


I would keep a test that its failing for it but mark it with @ignore annotation and a TODO comment to fix it once the API provides it.

heshamMassoud · 2018-10-04T22:47:50Z

+        );
+
+        assertThat(updateActions).containsExactly(
+            RemoveFieldDefinition.of(FIELD_A),


In this test you are testing the fact the correct update actions have been built. Only in this test, the order doesn't matter. So you should actually assert with containsExactlyInAnyOrder not with containsExactly.

However, order definitely matters which update actions are generated before which. But this should have it's own unit test focusing on testing the order of the generated actions with a strict containsExactly assert. This way you don't test two things together.

heshamMassoud · 2018-10-05T08:23:12Z

In general I see a lot of code duplication between the Type sync and the Product Type sync especially with the enum implementations. I think we should move the common parts to the commons package and resuse:

Less code duplication
Less test duplication
Less error prone.

….com/commercetools/commercetools-sync-java into 300-feature-type-sync # Conflicts: # docs/RELEASE_NOTES.md

…ols/commercetools-sync-java into 300-feature-type-sync

….com/commercetools/commercetools-sync-java into 300-feature-type-sync # Conflicts: # docs/RELEASE_NOTES.md # src/benchmark/java/com/commercetools/sync/benchmark/BenchmarkUtils.java

ahmetoz added 4 commits September 13, 2018 11:53

#300: Adds implementations for the type sync.

0dcb394

- Adds the core type sync implementations - Adds some unit tests

#300: fix checkStyles

aedf087

#300: add integration test for type sync utilities.

878c733

#300: add unit and integration tests for the type sync feature.

7b129fd

ahmetoz requested a review from a team September 19, 2018 15:18

#300: suppress warning coming from findBugs

f6e2bc7

ahmetoz changed the title ~~#300: Add support for sync of custom types~~ WIP #300: Add support for sync of custom types Sep 24, 2018

ahmetoz added 3 commits September 24, 2018 12:02

#300: documentation and benchmark for type sync.

c2bb617

#300: add constant to type sync benchmark util

5d1fc58

#300: fix checkstyleBenchmark

62923b5

ahmetoz changed the title ~~WIP #300: Add support for sync of custom types~~ #300: Add support for sync of custom types Sep 24, 2018

Merge branch 'master' into 300-feature-type-sync

73cb3af

ahmetoz closed this Sep 24, 2018

ahmetoz deleted the 300-feature-type-sync branch September 24, 2018 12:23

ahmetoz restored the 300-feature-type-sync branch September 24, 2018 12:24

#300: updates releases notes.

973cb09

ahmetoz reopened this Sep 24, 2018

lojzatran suggested changes Sep 24, 2018

View reviewed changes

ahalberkamp reviewed Sep 25, 2018

View reviewed changes

Comment thread docs/usage/TYPE_SYNC.md

ahalberkamp reviewed Sep 25, 2018

View reviewed changes

ahmetoz added 2 commits September 25, 2018 15:46

#300: PR review changes, mostly documentation and indentions.

f4fc415

#300: PR changes

2ddf05b

heshamMassoud suggested changes Oct 4, 2018

View reviewed changes

heshamMassoud mentioned this pull request Oct 5, 2018

Type sync and ProductType sync should handle changes to a Set of Enums or LocalizedEnums #313

Closed

#300 - update type sync docs.

6f14845

ahmetoz added 5 commits October 8, 2018 11:17

#300 - refactor, do not block error callback

850246d

#300 - refactor, comment

4807bf5

#300 - checkStyle fix

c6b88d1

#300 - adds tests for TypeServiceImpl

2dc8da9

#300 - refactor tests.

1d882cb

This was referenced Oct 8, 2018

#302 - Refactor Product Type #316

Merged

document improvements #318

Closed

#318 - fix some typos and grammars #319

Merged

ahmetoz added 13 commits October 9, 2018 17:01

#300 - fix some typos and grammar in type document.

8f45ac2

#300 - Review changes.

4f5a08e

#300 - Review changes for benchmark.

19a5800

#301 - update release notes for type sync

026071b

#301 - fix import.

f448662

Merge branches '300-feature-type-sync' and 'master' of https://github…

416c974

….com/commercetools/commercetools-sync-java into 300-feature-type-sync # Conflicts: # docs/RELEASE_NOTES.md

Merge branch 'master' into 300-feature-type-sync

bf6aa7a

#300 - remove uuid flag from type sync options

3913290

Merge branch '300-feature-type-sync' of https://github.com/commerceto…

7b35d79

…ols/commercetools-sync-java into 300-feature-type-sync

#300- refactor type service and add test.

256554e

#300- refactor docs

8f1362f

#300- refactor make internal

cb2dabd

#300- update release notes - add benchmark test as an enhancement entity

8879163

ahmetoz self-assigned this Oct 18, 2018

ahmetoz added 5 commits October 18, 2018 15:24

#300 - check controls for the field type on update actions

d608fea

#300 - fix checkStyle

ea28dfb

#300 - fix IT.

ac55840

Merge branch 'master' into 300-feature-type-sync

c8f7220

Merge branches '300-feature-type-sync' and 'master' of https://github…

880ae86

….com/commercetools/commercetools-sync-java into 300-feature-type-sync # Conflicts: # docs/RELEASE_NOTES.md # src/benchmark/java/com/commercetools/sync/benchmark/BenchmarkUtils.java

ahmetoz mentioned this pull request Nov 14, 2018

#300 - Type Sync Feature. #333

Merged

5 tasks

ahmetoz closed this Nov 14, 2018

heshamMassoud deleted the 300-feature-type-sync branch December 7, 2018 09:59

		@Nonnull final List<TypeDraft> typeDrafts) {

		final List<List<TypeDraft>> batches = batchElements(typeDrafts, syncOptions.getBatchSize());

Conversation

ahmetoz commented Sep 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Description

Relevant Issues

Todo

Hints for Review

Uh oh!

codecov-io commented Sep 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

lojzatran left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

heshamMassoud Oct 4, 2018 • 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!

heshamMassoud Oct 4, 2018 • 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!

Uh oh!

ahmetoz commented Oct 4, 2018

Uh oh!

lojzatran commented Oct 4, 2018

Uh oh!

heshamMassoud commented Oct 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

heshamMassoud left a comment

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

ahmetoz commented Sep 13, 2018 •

edited

Loading

codecov-io commented Sep 20, 2018 •

edited

Loading

heshamMassoud Oct 4, 2018 •

edited

Loading

heshamMassoud Oct 4, 2018 •

edited

Loading

heshamMassoud commented Oct 4, 2018 •

edited

Loading

heshamMassoud Oct 4, 2018 •

edited

Loading