286 product type attributes enum actions#295
Conversation
* Add PlainEnumValue to attribute definition update action * Change order of PlainEnumValue update action * Remove PlainEnumValues from attribute definition update action * Change plain enum value label update action * Add LocalizedEnumValue to attribute definition update action * Change order of LocalizedEnumValue update action * Remove LocalizedEnumValues from attribute definition update action * Change localized enum value label update action
# Conflicts: # src/main/java/com/commercetools/sync/producttypes/utils/AttributeDefinitionUpdateActionUtils.java # src/main/java/com/commercetools/sync/producttypes/utils/ProductTypeUpdateActionUtils.java
Codecov Report
@@ Coverage Diff @@
## master #295 +/- ##
============================================
+ Coverage 95.15% 95.28% +0.13%
- Complexity 967 1011 +44
============================================
Files 88 94 +6
Lines 2415 2546 +131
Branches 120 123 +3
============================================
+ Hits 2298 2426 +128
- Misses 99 102 +3
Partials 18 18
Continue to review full report at Codecov.
|
lojzatran
left a comment
There was a problem hiding this comment.
I cannot finish the review now because there are some other urgent tasks. So here's what I saw so far.
|
|
||
| import javax.annotation.Nonnull; | ||
|
|
||
| public class DifferentTypeException extends RuntimeException { |
There was a problem hiding this comment.
Can you write a short comment when is this exception used?
| throws DuplicateKeyException, DifferentTypeException { | ||
|
|
||
| return Stream | ||
| final List<UpdateAction<ProductType>> updateActions = Stream |
There was a problem hiding this comment.
This should be only inside if (haveSameAttributeType(oldAttributeDefinition, newAttributeDefinitionDraft)) {.
| .collect(toList()); | ||
|
|
||
| if (haveSameAttributeType(oldAttributeDefinition, newAttributeDefinitionDraft)) { | ||
| if (isPlainEnumAttribute(oldAttributeDefinition)) { |
There was a problem hiding this comment.
Other attribute types will be done later?
There was a problem hiding this comment.
Only PlainEnumValues and LocalizedEnum values because you can't apply update actions on the other attribute types according to the commercetools API: https://docs.commercetools.com/http-api-projects-productTypes.html
There was a problem hiding this comment.
There's also update of isSearchable etc., but I understand now.
| * | ||
| * @return true if both attribute definitions have the same attribute type, false otherwise. | ||
| */ | ||
| private static final boolean haveSameAttributeType( |
There was a problem hiding this comment.
No need for final as method is private.
| * @param oldAttributeDefinitions the old list of attribute definitions. | ||
| * @param newAttributeDefinitionsDrafts the new list of attribute definitions drafts. | ||
| * | ||
| * @param oldAttributeDefinitions the old list of attribute definitions. |
There was a problem hiding this comment.
Maybe you should sync the formatting rules with @heshamMassoud so you don't have these formatting changes here.
There was a problem hiding this comment.
I already did it :). That's why the changes on the format in this file. At the beginning I had the default formatting rules.
| final List<UpdateAction<ProductType>> updateActions = | ||
| buildRemoveAttributeDefinitionOrAttributeDefinitionUpdateActions( | ||
| oldAttributeDefinitions, | ||
| removedAttributesDefinitionsNames, |
There was a problem hiding this comment.
This variable got me confused. It's not used afterwards at all. Can you check?
There was a problem hiding this comment.
mmm... there is in line 118 and
updateActions.addAll(
buildAddAttributeDefinitionUpdateActions(
newAttributeDefinitionsDrafts,
oldAttributesDefinitionsNameMap
)
);
it is also used in line 125, by finally returning the updateActions in line 131:
There was a problem hiding this comment.
Sorry I was not clear, this attribute is not used: removedAttributesDefinitionsNames
There was a problem hiding this comment.
it's used in line 112 in
final List<UpdateAction<ProductType>> updateActions =
buildRemoveAttributeDefinitionOrAttributeDefinitionUpdateActions(
oldAttributeDefinitions,
removedAttributesDefinitionsNames,
newAttributesDefinitionsDraftsNameMap
);
There was a problem hiding this comment.
Yes, I see, but what do you do with it in the method? You have this in the method:
removedAttributeDefinitionNames.add(oldAttributeDefinitionName);
but then you don't use it anymore. So I still don't understand why do you need it.
| return updateActions; | ||
| return updateActions; | ||
|
|
||
| } catch (final DuplicateNameException dne) { |
There was a problem hiding this comment.
You can use } catch (final DuplicateNameException | DuplicateKeyException | DifferentTypeException dne) {
lojzatran
left a comment
There was a problem hiding this comment.
I added my second part of review. Now I reviewed everything.
| * Otherwise, if the enum values are identical, an empty optional is returned. | ||
| */ | ||
| @Nonnull | ||
| public static <T extends WithKey> Optional<UpdateAction<ProductType>> buildRemoveEnumValuesUpdateActions( |
There was a problem hiding this comment.
What about this:
@Nonnull
public static <T extends WithKey> Optional<UpdateAction<ProductType>> buildRemoveEnumValuesUpdateActions(
@Nonnull final String attributeDefinitionName,
@Nonnull final List<T> oldEnumValues,
@Nullable final List<T> newEnumValues) {
final Map<String, T> newEnumValuesKeyMap = getEnumValuesKeyMapWithKeyValidation(
attributeDefinitionName,
Optional.ofNullable(newEnumValues).orElse(Collections.emptyList())
);
final List<String> keysToRemove = oldEnumValues
.stream()
.map(WithKey::getKey)
.filter(oldEnumValueKey -> !newEnumValuesKeyMap.containsKey(oldEnumValueKey))
.collect(Collectors.toList());
if (!keysToRemove.isEmpty()) {
return of(RemoveEnumValues.of(attributeDefinitionName, keysToRemove));
} else {
return empty();
}
}You don't have to do if/else because you will receive newEnumValuesKeyMap empty anyway. And return of(RemoveEnumValues.of(attributeDefinitionName, keysToRemove)); is better because it can never be null.
There was a problem hiding this comment.
Better use specific type RemoveEnumValues instead of Optional<UpdateAction<ProductType>>?
There was a problem hiding this comment.
Better use specific type RemoveEnumValues instead of Optional<UpdateAction>?
I can treat all update action as a more generic UpdateAction type, so then all update actions applied to the product type are the same and easier to manipulate in more abstract methods used at higher levels of the product type sync
| * Otherwise, if the enum values order is identical, an empty optional is returned. | ||
| */ | ||
| @Nonnull | ||
| public static <T extends WithKey> Optional<UpdateAction<ProductType>> buildChangeEnumValuesOrderUpdateAction( |
There was a problem hiding this comment.
I don't see the order comparision in this method. I see only existence check. Can you clarify?
There was a problem hiding this comment.
The array of new enum values drafts are either existing enum values or new enum values that are added and didn’t exist before.
The addition of new enum values are added at the end of the array (commercetools doesn’t allow to add a new enum value in a particular position of the array).
So concatenating the sorted keys of the existing enum values with the keys of the new ones, we would have the “natural” order if there wasn’t a change on the order.
Thus, if we compare this concatenated array containing the keys of the “natural” order against the array of keys of the enum values coming as a parameter, we can know that if the order is different it means that has been a change in the order.
Note: the method buildUpdateAction compares the first two parameters and returns the function applied to both if they are different
| @Nonnull final List<T> newEnumValues, | ||
| @Nonnull final BiFunction<String, T, UpdateAction<ProductType>> addEnumCallback) { | ||
|
|
||
| final Map<String, T> oldEnumValuesKeyMap = getEnumValuesKeyMap(oldEnumValues); |
There was a problem hiding this comment.
Why not calling getEnumValuesKeyMapWithKeyValidation() also in this case?
|
|
||
| return oldEnumValues | ||
| .stream() | ||
| .map(oldEnumValue -> |
There was a problem hiding this comment.
A bit more consistent with the previous method calls:
return oldEnumValues
.stream()
.filter(oldEnumValue -> newEnumValuesKeyMap.containsKey(oldEnumValue.getKey()))
.map(oldEnumValue -> matchingEnumCallback.apply(attributeDefinitionName, oldEnumValue, newEnumValuesKeyMap.get(oldEnumValue.getKey())))
.flatMap(Collection::stream)
.collect(Collectors.toList());
| @Nonnull final String attributeDefinitionName, | ||
| @Nonnull final List<T> oldEnumValues, | ||
| @Nonnull final List<T> newEnumValues, | ||
| @Nonnull final BiFunction<String, T, UpdateAction<ProductType>> addEnumCallback) { |
There was a problem hiding this comment.
Better use specific type AddEnumValue instead of UpdateAction<ProductType>?
There was a problem hiding this comment.
It has to be the parent class because the action can be AddEnumValue or AddLocalizedEnumValue depending on the addEnumCallback will return one or the other.
Another thing is that I can treat all update action as a more generic UpdateAction<ProductType> type, so then all update actions applied to the product type are the same and easier to manipulate in more abstract methods used at higher levels of the product type sync
| @Nonnull final String attributeDefinitionName, | ||
| @Nonnull final List<T> oldEnumValues, | ||
| @Nonnull final List<T> newEnumValues, | ||
| @Nonnull final BiFunction<String, List<T>, UpdateAction<ProductType>> changeOrderEnumCallback) { |
There was a problem hiding this comment.
Better use specific type ChangeEnumValueOrder instead of UpdateAction<ProductType>?
There was a problem hiding this comment.
It has to be the parent class because the action can be ChangeEnumValueOrder or ChangeLocalizedEnumValueOrder depending on the changeOrderEnumCallback will return one or the other
Another thing is that I can treat all update action as a more generic UpdateAction<ProductType> type, so then all update actions applied to the product type are the same and easier to manipulate in more abstract methods used at higher levels of the product type sync
| import static com.commercetools.sync.producttypes.utils.ProductTypeUpdateEnumActionsUtils.buildChangeEnumValuesOrderUpdateAction; | ||
| import static java.util.Collections.emptyList; | ||
|
|
||
| public final class ProductTypeUpdateLocalizedEnumActionUtils { |
There was a problem hiding this comment.
The code is almost identical with class ProductTypeUpdatePlainEnumActionUtils. What about creating a new abstract parent class for both of them and put the common methods there?
There was a problem hiding this comment.
The ProductTypeUpdateLocalizedEnumActionUtils and ProductTypeUpdatePlainEnumActionUtils classes are similar in the buildUpdateAction method and someone could argue that I could also reuse the code in the ProductTypeUpdateEnumActionsUtils class, the same as it's done with other methods.
I did it that way first but the code ended up more tight and difficult to understand since all three particular methods for adding, removing and change order of the enums have to be passed as parameters in the common method placed at ProductTypeUpdateEnumActionsUtils, making this method to have 6 parameters where some of them are BiFunction, TriFunction... (long and "complex" signature)
So I finally decided to repeat some code in ProductTypeUpdateLocalizedEnumActionUtils and ProductTypeUpdateLocalizedEnumActionUtils in order to clarify and decouple the buildUpdateAction method of both classes.
| } | ||
|
|
||
| @Test | ||
| public void buildActions_WithNewDifferentValues_ShouldReturnAction() { |
There was a problem hiding this comment.
buildActions_WithNewDifferentValues_ShouldReturnAction -> buildActions_WithNewDifferentValues_ShouldReturnActions
heshamMassoud
left a comment
There was a problem hiding this comment.
Looks good but please check the comments @jortizsao :)
| @Nonnull final String attributeDefinitionName, | ||
| @Nonnull final List<EnumValue> oldEnumValues, | ||
| @Nullable final List<EnumValue> newEnumValues) | ||
| throws DuplicateKeyException { |
There was a problem hiding this comment.
The throws DuplicateKeyException here is redundant since it's a runtime exception
| .map(Optional::get) | ||
| .collect(toList()); | ||
| @Nonnull final AttributeDefinitionDraft newAttributeDefinitionDraft) | ||
| throws DuplicateKeyException, DifferentTypeException { |
There was a problem hiding this comment.
The throws DuplicateKeyException here is redundant since it's a runtime exception
| )); | ||
| } | ||
| } else { | ||
| throw new DifferentTypeException(format("The attribute type of the attribute definitions are different. " |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
don't you think is more safer to throw an error, and if the user actually wants to change the type, he can follow a two step process of deleting the attribute in a first run and add the new one with the new type in a second run?
Removing is always dangerous but if you think that we should't notify the user and remove the attribute right away, it is ok :)
There was a problem hiding this comment.
I've changed my mind :). let's help the user and avoid unnecessary steps ;)
| * | ||
| * @return true if the attribute definition is a plain enum value, false otherwise. | ||
| */ | ||
| private static boolean isPlainEnumAttribute(@Nonnull final AttributeDefinition attributeDefiniton) { |
There was a problem hiding this comment.
attributeDefiniton -> attributeDefinition in the variable name and in the javadoc.
| * | ||
| * @return true if the attribute definition is a localized enum value, false otherwise. | ||
| */ | ||
| private static boolean isLocalizedEnumAttribute(@Nonnull final AttributeDefinition attributeDefiniton) { |
There was a problem hiding this comment.
also here: attributeDefiniton -> attributeDefinition
| .collect(Collectors.toList()); | ||
|
|
||
| if (!keysToRemove.isEmpty()) { | ||
| return ofNullable(RemoveEnumValues.of(attributeDefinitionName, keysToRemove)); |
There was a problem hiding this comment.
RemoveEnumValues.of(attributeDefinitionName, keysToRemove) can never be null, so we should use Optional#of instead of Optional#ofNullable.
Also instead of if else, you can use conditional operator like that:
return keysToRemove.isEmpty() ? of(RemoveEnumValues.of(attributeDefinitionName, keysToRemove)) : empty();|
|
||
| final Map<String, T> newEnumValuesKeyMap = getEnumValuesKeyMapWithKeyValidation( | ||
| attributeDefinitionName, | ||
| Optional.ofNullable(newEnumValues).orElse(Collections.emptyList()) |
There was a problem hiding this comment.
I see down there you already are statically importing the Optional#ofNullable, so would be good if here too for consistent style
There was a problem hiding this comment.
you could also statically import Collections#emptyList() ;)
| @Nonnull | ||
| public static <T extends WithKey> Map<String, T> getEnumValuesKeyMapWithKeyValidation( | ||
| @Nonnull final String attributeDefinitionName, | ||
| @Nonnull final List<T> enumValues) throws DuplicateKeyException { |
There was a problem hiding this comment.
The throws DuplicateKeyException here is redundant since it's a runtime exception
| @Nonnull final EnumValue oldEnumValue, | ||
| @Nonnull final EnumValue newEnumValue) { | ||
|
|
||
| return Stream |
There was a problem hiding this comment.
(just a question) don't you think it looks better this way:
return Stream.of(buildChangeLabelAction(attributeDefinitionName, oldEnumValue, newEnumValue))
.filter(Optional::isPresent)
.map(Optional::get)
.collect(toList());| ) | ||
|
|
||
| ).collect(Collectors.toList()); | ||
| } |
There was a problem hiding this comment.
This method looks exactly the same as ProductTypeUpdateLocalizedEnumActionUtils#buildActions. Why don't you make a generic implementation and reuse. Like this:
@Nonnull
private static <T extends WithKey> List<UpdateAction<ProductType>> buildUpdateActions(
@Nonnull final String attributeDefinitionName,
@Nonnull final List<T> oldEnumValues,
@Nonnull final List<T> newEnumValues,
@Nonnull final TriFunction<String, T, T, List<UpdateAction<ProductType>>> matchingEnumActionBuilder,
@Nonnull final BiFunction<String, T, UpdateAction<ProductType>> addEnumActionBuilder,
@Nonnull final BiFunction<String, List<T>, UpdateAction<ProductType>> changeOrderActionBuilder){
final List<UpdateAction<ProductType>> removeEnumValuesUpdateActions = buildRemoveEnumValuesUpdateActions(
attributeDefinitionName, oldEnumValues, newEnumValues)
.map(Collections::singletonList)
.orElse(emptyList());
final List<UpdateAction<ProductType>> matchingEnumValuesUpdateActions =
buildMatchingEnumValuesUpdateActions(attributeDefinitionName, oldEnumValues, newEnumValues,
matchingEnumActionBuilder);
final List<UpdateAction<ProductType>> addEnumValuesUpdateActions = buildAddEnumValuesUpdateActions(
attributeDefinitionName, oldEnumValues, newEnumValues, addEnumActionBuilder);
final List<UpdateAction<ProductType>> changeEnumValuesOrderUpdateActions =
buildChangeEnumValuesOrderUpdateAction(attributeDefinitionName, oldEnumValues, newEnumValues,
changeOrderActionBuilder)
.map(Collections::singletonList)
.orElse(emptyList());
return Stream.concat(
Stream.concat(removeEnumValuesUpdateActions.stream(), matchingEnumValuesUpdateActions.stream()),
Stream.concat(addEnumValuesUpdateActions.stream(), changeEnumValuesOrderUpdateActions.stream()))
.collect(Collectors.toList());
}and then reuse it for both by doing:
for ProductTypeUpdatePlainEnumActionUtils
buildUpdateActions(attributeDefinitionName, oldEnumValues, newEnumValues, PlainEnumUpdateActionsUtils::buildActions, AddEnumValue::of, ChangeEnumValueOrder::of);and for ProductTypeUpdateLocalizedEnumActionUtils:
buildUpdateActions(attributeDefinitionName, oldEnumValues, newEnumValues, LocalizedEnumUpdateActionsUtils::buildActions, AddLocalizedEnumValue::of, ChangeLocalizedEnumValueOrder::of);There was a problem hiding this comment.
@heshamMassoud I tried your way first and I explained the reason of "copying" to Lam. This is my previous comment
"The ProductTypeUpdateLocalizedEnumActionUtils and ProductTypeUpdatePlainEnumActionUtils classes are similar in the buildUpdateAction method and someone could argue that I could also reuse the code in the ProductTypeUpdateEnumActionsUtils class, the same as it's done with other methods.
I did it that way first but the code ended up more tight and difficult to understand since all three particular methods for adding, removing and change order of the enums have to be passed as parameters in the common method placed at ProductTypeUpdateEnumActionsUtils, making this method to have 6 parameters where some of them are BiFunction, TriFunction... (long and "complex" signature)
So I finally decided to repeat some code in ProductTypeUpdateLocalizedEnumActionUtils and ProductTypeUpdateLocalizedEnumActionUtils in order to clarify and decouple the buildUpdateAction method of both classes."
In my opinion, that function is too tight and coupled (it's ad-hoc).
First: Passing 6 parameters looks too many parameters for a function (it is a coupling indicator)
Second: It's going to be difficult to extend or modify in the future if a particular change only applies to one of the enum values (either plain or localized).
Third: In some cases is better to "copy" and explicitly show what's going on behind scenes rather than hide and add too much "magic"
What do you think? if you find your way more understandable for future maintainers, I change it :)
There was a problem hiding this comment.
I think it's ok to keep them copied also, I agree with your arguments. It was just an indicator for reuse right away and taking advantage of generics when they look the same. So we don't have to have the exact same code twice. But your arguments are also valid enough, It's fine for now, since it's only going to be reused by only these two. From experience we had some generic methods which maybe had many params but were very helpful to use by any sync process implementation and saved a lot of time (and also the dev didn't need to know about how it's working inside), this was the custom update action builder util method - it was used easily by almost every sync process implementation for example latley in the price custom type sync.
We can leave it like this for now, and see what happens in the future, if we see that we are changing always in both the same things, then we can generify the method.
Summary
This PR adds the attribute definition update actions for plain enum values and localized enum values
Description
AttributeDefinitionUpdateActionUtilsTodo