286 Add product type sync#296
Conversation
Codecov Report
@@ Coverage Diff @@
## master #296 +/- ##
============================================
+ Coverage 96.29% 96.47% +0.18%
- Complexity 1066 1102 +36
============================================
Files 101 103 +2
Lines 2642 2722 +80
Branches 130 134 +4
============================================
+ Hits 2544 2626 +82
+ Misses 83 81 -2
Partials 15 15
Continue to review full report at Codecov.
|
* Add process method * Add process batch method * Add update existing product type method * Add create product type method * Update product type service * Add integration tests
a02e50a to
b144368
Compare
…ew attribute when the attributeDefinitions with the same name but of different types we shouldn't just throw an exception but rather we should remove the old attributeDefinition and create a new one with the new type.
heshamMassoud
left a comment
There was a problem hiding this comment.
I am still not done with the review 😉 but in general I see a lot of code not covered. Is this still coming?
Regardless, I will submit my review for now so I don't block you. Then I will submit another review.
| import java.util.function.Function; | ||
|
|
||
| public final class ProductTypeSyncOptions extends BaseSyncOptions<ProductType, ProductTypeDraft> { | ||
| public class ProductTypeSyncOptions extends BaseSyncOptions<ProductType, ProductTypeDraft> { |
There was a problem hiding this comment.
why did you remove the final?
There was a problem hiding this comment.
honestly, I don't know... it's been a mistake
|
|
||
| import static java.lang.String.format; | ||
|
|
||
| public class ProductTypeSyncStatistics extends BaseSyncStatistics { |
There was a problem hiding this comment.
We should have private constructor for this class, since we only expect it to be instantiated through the builder. Also we should make it final. I know this is not the case for the other *Statistics classes, but maybe we should change that.
There was a problem hiding this comment.
I created an integration test for that: https://github.com/commercetools/commercetools-sync-java/pull/296/files#diff-07965ad8d45b1a77e4bf0fc63ff109f8R393
There was a problem hiding this comment.
We should have private constructor for this class, since we only expect it to be instantiated through the builder. Also we should make it final. I know this is not the case for the other *Statistics classes, but maybe we should change that.
Is there a builder for the *Statistics classes? or you mean that I have to develop one?
There was a problem hiding this comment.
I created an integration test for that
I would say:
- We should cover as much of the code (as possible) with unit tests.
- a unit test is sufficient for the statistics since the integration tests have an overhead of the setting up/tearing down requests. So they are quite costly in the sense that they increase the time of the build significantly, etc..
There was a problem hiding this comment.
Is there a builder for the *Statistics classes? or you mean that I have to develop one?
Oh, my bad. I thought there is already one :( disregard the comment then. Just consider using final..
There was a problem hiding this comment.
I would say:
We should cover as much of the code (as possible) with unit tests.
a unit test is sufficient for the statistics since the integration tests have an overhead of the setting up/tearing down requests. So they are quite costly in the sense that they increase the time of the build significantly, etc..
Yes, I'm familiar with the test pyramid and I love unit tests (I usually develop using TDD when possible) and I prefer them over integration tests.
For the ProductTypeSync class and related classes and methods (basically the ones I've added in this PR and that don't create update actions), I found complex to add unit tests to these classes due to coupling/architecture, so I decided to follow the same existing approach (cover those test cases with integration tests ).
In this case is true that is possible to add a unit test for the getReportMessage method but in order to avoid a single test class for this unit test, I thought that the integration test was enough.
Anyways, I add the unit test :)
| oldAttributeDefinition.getName(), | ||
| oldAttributeDefinition.getAttributeType().getClass().getName(), | ||
| newAttributeDefinitionDraft.getAttributeType().getClass().getName())); | ||
| updateActions = Stream |
There was a problem hiding this comment.
what about the case when the type of the attributeDefinition has changed?
There was a problem hiding this comment.
ok I saw it down there. Please disregard this comment.
| * result. If no update action is needed, for example in case where both the {@link ProductType} and the | ||
| * {@link ProductTypeDraft} have the same description, an empty {@link List} is returned. | ||
| * | ||
| * @param oldProductType the product which should be updated. |
There was a problem hiding this comment.
the product which should be updated. -> the {@link ProductType} which should be updated.
| * {@link ProductTypeDraft} have the same description, an empty {@link List} is returned. | ||
| * | ||
| * @param oldProductType the product which should be updated. | ||
| * @param newProductType the product draft where we get the new data. |
There was a problem hiding this comment.
the product draft where we get the new data. -> the {@link ProductTypeDraft} which should be updated.
| return new ArrayList<UpdateAction<ProductType>>(); | ||
| } | ||
|
|
||
| }) |
There was a problem hiding this comment.
general questions about this method: Why does it take already a map newAttributeDefinitionDraftsNameMap as a parameter and not a list? can't the map be already created inside of it as it's only needed by it?
There was a problem hiding this comment.
you are right! it was an old version that used a map. After a code review in the price sync we thought it was better to use a list because is less coupled and more close to the original parameters.
I change it
| import io.sphere.sdk.commands.UpdateAction; | ||
| import io.sphere.sdk.products.attributes.AttributeDefinition; | ||
| import io.sphere.sdk.products.attributes.AttributeDefinitionBuilder; | ||
| import io.sphere.sdk.products.attributes.LocalizedStringAttributeType; |
There was a problem hiding this comment.
There is a type in the name of the package of this test class:
producttuypeactionutils -> producttypeactionutils ;)
|
|
||
| import javax.annotation.Nullable; | ||
|
|
||
| public class ProductTypeSyncStatisticsAssert extends |
There was a problem hiding this comment.
Great that you are creating your own statistics assertion 🎉
but maybe final 😊 ?
| return CtpQueryUtils.queryAll(syncOptions.getCtpClient(), ProductTypeQuery.of(), productTypePageConsumer) | ||
| .thenAccept(result -> isCached = true) | ||
| .thenApply(result -> Optional.ofNullable(keyToIdCache.get(key))); | ||
| .thenAccept(result -> isCached = true) |
| .collect( | ||
| Collectors.toMap(AttributeMetaData::getName, attributeMetaData -> attributeMetaData)); | ||
| .map(AttributeMetaData::of) | ||
| .collect( |
…te constructor" This reverts commit 123c8e1.
…ctionUtils#buildRemoveAttributeDefinitionOrAttributeDefinitionUpdateActions
68641cc to
f2e2841
Compare
| public class ProductTypeSyncIT { | ||
|
|
||
| /** | ||
| * Deletes inventories and supply channels from source and target CTP projects. |
There was a problem hiding this comment.
The comment doesn't reflect what is happening in the method.
| * injected {@link ProductTypeSyncOptions} instance. | ||
| * | ||
| * @param productTypeSyncOptions the container of all the options of the sync process including the CTP project | ||
| * client and/ormconfiguration and other sync-specific options. |
There was a problem hiding this comment.
I have removed the javadoc from the constructors and add it to the class, so this typo doesn't exist anymore.
| /** | ||
| * Takes a {@link ProductTypeSyncOptions} instance to instantiate a new {@link ProductTypeSync} instance that | ||
| * could be used to sync product type drafts with the given product types in the CTP project specified in the | ||
| * injected {@link ProductTypeSyncOptions} instance. |
There was a problem hiding this comment.
This is quite long sentence and not easy to read. Can you make it a bit clearer?
There was a problem hiding this comment.
Actually I would just describe what this class do instead of describing the constructors:
This class syncs product type drafts with the given product types in the CTP project.| protected CompletionStage<ProductTypeSyncStatistics> process( | ||
| @Nonnull final List<ProductTypeDraft> productTypeDrafts) { | ||
|
|
||
| final List<List<ProductTypeDraft>> batches = batchElements(productTypeDrafts, syncOptions.getBatchSize()); |
There was a problem hiding this comment.
What about validate drafts here? If they're not valid, you don't have to process them further.
There was a problem hiding this comment.
The invalid drafts count as a processed items in the statistics so they have to be "processed" (they are not actually processed, but the process method will consider them as a processed items)
There was a problem hiding this comment.
What I meant is this:
final List<ProductTypeDraft> validProductTypeDrafts = productTypeDrafts.stream()
.filter(this::validateDraft)
.collect(toList());
final List<List<ProductTypeDraft>> batches = batchElements(validProductTypeDrafts, syncOptions.getBatchSize());
and remove validateDraft part from processBatch() method.
There was a problem hiding this comment.
mmm... but if I validate there, then the counter for "processed" items is not increased in processBatch because the invalid drafts are removed before and processBatch is responsible for increasing the counter of processed items in the statistics.
In the other synchronization of entities, the invalid drafts are considered as processed. Furthermore, in the processBatch the invalid drafts are filtered before processing, so it seems that there is no performance advantage of moving the filtering to the process.
Maybe I'm wrong, what do you think @heshamMassoud ?
There was a problem hiding this comment.
No, you are arguments are absolutely correct @jortizsao.
| statistics.incrementProcessed(batch.size()); | ||
| return completedFuture(statistics); | ||
| } else { | ||
| final Map<String, ProductTypeDraft> keysProductTypeDraftMap = getKeysProductTypeMap(validProductTypeDrafts); |
There was a problem hiding this comment.
Why do you need map? Is set of keys enough?
There was a problem hiding this comment.
I wanted to reuse the existing method getKeysProductTypeMap, but I change it.
| .thenApply(Optional::of) | ||
| .exceptionally(exception -> { | ||
| final String errorMessage = format(CTP_PRODUCT_TYPE_FETCH_FAILED, keys); | ||
| handleError(errorMessage, exception, keys.size()); |
There was a problem hiding this comment.
I'm confused with keys.size() - if you fail on 1 request, shouldn't you count as 1?
There was a problem hiding this comment.
This error is when the request of a batch fails (i.e a batch of 50 product type keys), so if the request fails, it fails for the 50 product type keys, and the counter of failed items should be incremented by 50.
| * {@link ProductTypeUpdateActionUtils#buildAttributesUpdateActions}) of a {@link ProductType} and a | ||
| * {@link ProductTypeDraft}. It returns a {@link List} of {@link UpdateAction}<{@link ProductType}> as a | ||
| * result. If no update action is needed, for example in case where both the {@link ProductType} and the | ||
| * {@link ProductTypeDraft} have the same description, an empty {@link List} is returned. |
There was a problem hiding this comment.
for example in case where both the {@link ProductType} and the {@link ProductTypeDraft} have the same description - I thought this applies only when both product types are exactly the same?
There was a problem hiding this comment.
Yes, the javadoc is confusing. It should say when all fields are the same. I change it
| newAttributeDefinitionDraft.getAttributeType().getClass().getName())); | ||
| updateActions = Stream | ||
| .of( | ||
| buildChangeLabelUpdateAction(oldAttributeDefinition, newAttributeDefinitionDraft), |
There was a problem hiding this comment.
What about changeAttributeName?
There was a problem hiding this comment.
The attribute name is the 'key' of the attribute (it's unique), so there can't be a changeAttributeName as update action.
A change in the attribute name will mean a deletion of the old attribute and the creation of the new attribute.
| buildActions(oldAttributeDefinition, attributeDefinitionDraft) | ||
| ) | ||
| .map(attributeDefinitionDraft -> { | ||
| // attribute type is required so if null we let commercetools to throw exception |
There was a problem hiding this comment.
What do you mean by this comment? In the line below you check for nullability anyway.
attributeDefinitionDraft.getAttributeType() != null
There was a problem hiding this comment.
This comment means that even knowing that the product type is invalid (because the attribute type is required for an attribute) we don't throw any exception.
We let commercetools to throw the exception. This is a decision we made because the commercetools documentation states that the attribute type is required, and we decided not throw library validations for those cases that are documented in commercetools.
In this case the validation is just for checking if there is attribute type to avoid a null pointer exception in the haveSameAttributeType.
| public void getReportMessage_WithIncrementedStats_ShouldGetCorrectMessage() { | ||
| productTypeSyncStatistics.incrementCreated(1); | ||
| productTypeSyncStatistics.incrementFailed(1); | ||
| productTypeSyncStatistics.incrementUpdated(1); |
There was a problem hiding this comment.
I would give created=1, failed=2, updated=3 so you know that you didn't mix the variables.
heshamMassoud
left a comment
There was a problem hiding this comment.
With these comments I finalise my review on the PR. So It should be approved as soon as they are addressed ;) @jortizsao
| import static com.commercetools.sync.integration.commons.utils.SphereClientUtils.CTP_SOURCE_CLIENT; | ||
| import static com.commercetools.sync.integration.commons.utils.SphereClientUtils.CTP_TARGET_CLIENT; | ||
|
|
||
| public class ProductTypeITUtils { |
There was a problem hiding this comment.
final and private empty constructor.
| * injected {@link ProductTypeSyncOptions} instance. | ||
| * | ||
| * @param productTypeSyncOptions the container of all the options of the sync process including the CTP project | ||
| * client and/ormconfiguration and other sync-specific options. |
There was a problem hiding this comment.
and/or configuration
There was a problem hiding this comment.
The comment has been removed from the constructors and added it to the class as recommended by @lojzatran in his code review
| * injected {@link ProductTypeSyncOptions} instance. | ||
| * | ||
| * @param productTypeSyncOptions the container of all the options of the sync process including the CTP project | ||
| * client and/ormconfiguration and other sync-specific options. |
There was a problem hiding this comment.
and/or configuration
There was a problem hiding this comment.
The comment has been removed from the constructors and added it to the class as recommended by @lojzatran in his code review
|
|
||
| ## Caveats | ||
|
|
||
| 1. Product types are either created or updated. Currently the tool does not support product type deletion. |
There was a problem hiding this comment.
I think in your implementation of the product type sync, syncing product types with an attribute of type NestedType will not work as you will need to resolve the references on this product type. So I would add it as a Caveat that currently product type sync does not support product types with attributes of nested type yet.
Would be nice if we create an issue for it also, so we can address it in the future ;)
There was a problem hiding this comment.
you are right. I add it. thanks!
| - [Categories](/docs/usage/CATEGORY_SYNC.md) | ||
| - [Products](/docs/usage/PRODUCT_SYNC.md) (_Beta_: Not recommended for production use yet.) | ||
| - [InventoryEntries](/docs/usage/INVENTORY_SYNC.md) (_Beta_: Not recommended for production use yet.) | ||
| - [ProductTypes](/docs/usage/PRODUCT_TYPE_SYNC.md) (_Beta_: Not recommended for production use yet.) |
There was a problem hiding this comment.
The reason Products and InventoryEntries was marked as Beta, is because the Product Sync is still missing reference resolution of values of attributes with type referenceType and because Inventory Sync is not completely tested.
So I was wondering why the product type sync would still be marked with this note.
There was a problem hiding this comment.
but now I see it makes sense because it's still missing support for NestedType attributes..
There was a problem hiding this comment.
we can keep it in beta or remove it (the ball is in your court xD). The Nestedtype is in Beta in commercetools, so keep the product type sync in Beta just for that reason maybe is not enough.
I put it in Beta because even after already tested it and so on... I thought it was wise to set it as Beta just for some time until we have confidence that it actually works as the customers expect.
WDYT?
There was a problem hiding this comment.
I put it in Beta because even after already tested it and so on... I thought it was wise to set it as Beta just for some time until we have confidence that it actually works as the customers expect.
I put it in Beta because even after already tested it and so on... I thought it was wise to set it as Beta just for some time until we have confidence that it actually works as the customers expect.
|
|
||
| return fetchExistingProductTypes(keys) | ||
| .thenCompose(oldProductTypes -> syncBatch(oldProductTypes, validProductTypeDrafts)) | ||
| .thenApply((ignored) -> { |
There was a problem hiding this comment.
the parantheses around the ignored are not needed ;)
| assertThat(oldProductTypeAfter.get().getName()).isEqualTo(PRODUCT_TYPE_NAME_2); | ||
| assertThat(oldProductTypeAfter.get().getDescription()).isEqualTo(PRODUCT_TYPE_DESCRIPTION_2); | ||
| assertAttributesAreEqual(oldProductTypeAfter.get().getAttributes(), | ||
| singletonList(ATTRIBUTE_DEFINITION_DRAFT_1)); |
There was a problem hiding this comment.
instead of those 4 asserts and checking that it is not empty explicitly and doing .get() after. You can do it like this:
assertThat(oldProductTypeAfter).hasValueSatisfying(productType -> {
assertThat(productType.getName()).isEqualTo(PRODUCT_TYPE_NAME_2);
assertThat(productType.getDescription()).isEqualTo(PRODUCT_TYPE_DESCRIPTION_2);
assertAttributesAreEqual(productType.getAttributes(), singletonList(ATTRIBUTE_DEFINITION_DRAFT_1));
});There was a problem hiding this comment.
you can use it across all asserts of this class actually ;)
| } | ||
|
|
||
| @Test | ||
| public void sync_beforeUpdate_ShouldCallBeforeUpdateCallback() { |
There was a problem hiding this comment.
I am wondering why are these tests significant? Namely:
- public void sync_beforeCreate_ShouldCallBeforeCreateCallback
- public void sync_beforeCreate_ShouldNotCallBeforeUpdateCallback
- public void sync_beforeUpdate_ShouldCallBeforeUpdateCallback
- public void sync_beforeUpdate_ShouldNotCallBeforeCreateCallback
There was a problem hiding this comment.
Basically is to verify that the right callbacks are called in the correct actions.
i.e How would you verify that applyBeforeUpdateCallBack function is called when a product type is created?
There was a problem hiding this comment.
but do you think we really need an integration test for that, isn't a unit test sufficient to test that behaviour?
There was a problem hiding this comment.
@jortizsao I created an issue for it #302 you can check it later. So we don't block the release ;)
| "product_type_key_" + Integer.toString(i), | ||
| "product_type_name_" + Integer.toString(i), | ||
| "product_type_description_" + Integer.toString(i), | ||
| Arrays.asList(ATTRIBUTE_DEFINITION_DRAFT_1) |
There was a problem hiding this comment.
you can use here instead: singletonList(ATTRIBUTE_DEFINITION_DRAFT_1)
| PRODUCT_TYPE_KEY_1, | ||
| PRODUCT_TYPE_NAME_1, | ||
| PRODUCT_TYPE_DESCRIPTION_1, | ||
| Arrays.asList(ATTRIBUTE_DEFINITION_DRAFT_1) |
There was a problem hiding this comment.
singletonList(ATTRIBUTE_DEFINITION_DRAFT_1)
|
@jortizsao We are also missing benchmarks for the product Type Sync. I'll create it as an issue for now. #301 |
|
release notes entries are also missing. I will add them. |
| .of(CTP_TARGET_CLIENT) | ||
| .build(); | ||
|
|
||
| ProductTypeSyncOptions spyProductTypeSyncOptions = spy(productTypeSyncOptions); |
Summary
This PR adds the ProductTypeSync class which is responsible for creating the batches from the product type drafts, and sync them accordingly:
Description
ProductTypeSyncTodo