Skip to content
This repository was archived by the owner on Aug 19, 2025. It is now read-only.

Conversation

Babanila
Copy link
Contributor

@Babanila Babanila commented Mar 13, 2019

Support reference number

SUPPORT-4604

Summary

Add tiered prices sync support for product Import/Export

Description

This PR adds tier to prices and allow the tiered prices to be synchronized with both the product import and export.
Tier is an array of priceTier.

To import tier with prices in csv, the tier is denoted from the base price by % symbol.

Example 1
Assuming the base price is EUR 50
For tier with single priceTier.

  • minimum quantity of 100, the price is EUR 45
EUR 50%EUR 45 @100

Example 2
For tier with multiple priceTier.

  • minimum quantity of 100, the price is EUR 45
  • minimum quantity of 200, the price is EUR 40
  • minimum quantity of 300, the price is EUR 35
EUR 50%EUR 45 @100%EUR 40 @200%EUR 35 @300

@Babanila Babanila self-assigned this Mar 13, 2019
@Babanila Babanila requested review from daern91 and junajan March 13, 2019 14:45
@coveralls
Copy link

coveralls commented Mar 13, 2019

Coverage Status

Coverage increased (+0.08%) to 78.415% when pulling 7daae9c on 242-use-sphere-product-csv-sync-to-sync-tier-prices into 4c24c1d on master.

@daern91 daern91 requested a review from butenkor March 13, 2019 15:28
README.md Outdated
In the `prices` column you can define a list of prices for each variant separated by `;`:
```
CH-EUR 999 B2B;EUR 899|745;USD 19900 #retailerA;DE-EUR 1000 B2C#wareHouse1;GB-GBP 999$2001-09-11T14:00:00.000Z~2015-10-12T14:00:00.000Z
CH-EUR 999 B2B;EUR 899|745;USD 19900 #retailerA;DE-EUR 1000 B2C#wareHouse1;GB-GBP 999$2001-09-11T14:00:00.000Z~2015-10-12T14:00:00.000Z;EUR 500%EUR 450@1000
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add tiers example to docs.

}],
"validFrom": "2001-09-11T14:00:00.000Z",
"validUntil": "2015-09-11T14:00:00.000Z",
"tiers": [
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add tiers to sample data.

ATTRIBUTE_CONSTRAINT_SAME_FOR_ALL: 'SameForAll'

REGEX_PRICE: new RegExp /^(([A-Za-z]{2})-|)([A-Z]{3}) (-?\d+)(-?\|(\d+)|)( ([^~\$#]*)|)(#([^~\$]*)|)(\$([^~]*)|)(~(.*)|)$/
REGEX_PRICE: new RegExp /^(([A-Za-z]{2})-|)([A-Z]{3}) (-?\d+)(-?\|(\d+)|)( ([^~\$#%]*)|)(#([^~\$%]*)|)(\$([^~%]*)|)(~([^%]*)|)(%([A-Z]{3} -?\d+ @\d+(.*))|)$/
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add tiers format to the price regex and defined the symbol (%) to be used for tiers.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woah, this is insane 😄

Copy link
Contributor

@daern91 daern91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks fine 👍 I just have one question

_mapTiers: (tiers) ->
_.reduce(tiers, (acc, priceTier, index) ->
acc += GLOBALS.DELIM_TIERS_MULTI_VALUE unless index is 0
acc + "#{priceTier.value.currencyCode} #{priceTier.value.centAmount} @#{priceTier.minimumQuantity}"
Copy link
Contributor

@daern91 daern91 Mar 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, are we sure that priceTier.value only contains centAmount and currenctyCode? Can't it be both Money and HighPrecisionMoney? 🤔

https://docs.commercetools.com/http-api-projects-products#pricetier

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The priceTier is always the same as the base price and by default, the base price is in centPrecision(Money).

PriceTier can either be centPrecision(Money) or highPrecision(HighPrecisionMoney) but not both at the same time.

For csv export and import for a price with tiers, only the centAmount and currenctyCode are needed. If highPrecision is required it can be set in the productType attributes of the price.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, ok. So what happens if I try to import or export a highPrecision price with highPrecision in priceTier?

Copy link
Contributor Author

@Babanila Babanila Mar 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it works once the base price type is set to highPrecision.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this doesn't make sense to me. How can it work? Did you test it? HighPrecisionMoney got more fields, e.g. preciseAmount

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works for only the default price type which is centPrecision.

Copy link
Contributor

@daern91 daern91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had another look at the code now and it looks really good already 💯 I would just like to expand on the docs so it's easier to understand how to use it. Added some more comments and questions below.

Also, could we please add a test for the import as well? Just so we fully test both imports and exports of this feature 👍

README.md Outdated
In the `prices` column you can define a list of prices for each variant separated by `;`:
```
CH-EUR 999 B2B;EUR 899|745;USD 19900 #retailerA;DE-EUR 1000 B2C#wareHouse1;GB-GBP 999$2001-09-11T14:00:00.000Z~2015-10-12T14:00:00.000Z
CH-EUR 999 B2B;EUR 899|745;USD 19900 #retailerA;DE-EUR 1000 B2C#wareHouse1;GB-GBP 999$2001-09-11T14:00:00.000Z~2015-10-12T14:00:00.000Z;EUR 500%EUR 450@1000
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: according to your tests, like this one, there should be a space between the amount and the @:

EUR 500%EUR 450@1000
EUR 500%EUR 450 @1000

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had another look at the code now and it looks really good already 💯 I would just like to expand on the docs so it's easier to understand how to use it. Added some more comments and questions below.

Also, could we please add a test for the import as well? Just so we fully test both imports and exports of this feature 👍

Tiers import test added.

`<country>-<currenyCode> <centAmount>|<discountedCentAmount> <customerGroupName>#<channelKey>$<validFrom>~<validUntil>%<tiers>`

Date values `validFrom` and `validUntils` has to be in [ISO 8601 format](http://dev.commercetools.com/http-api-types.html#datetime).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should add a line here on how to structure <tiers>. It's not self-evident which fields are required and in what order they should be.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

ATTRIBUTE_CONSTRAINT_SAME_FOR_ALL: 'SameForAll'

REGEX_PRICE: new RegExp /^(([A-Za-z]{2})-|)([A-Z]{3}) (-?\d+)(-?\|(\d+)|)( ([^~\$#]*)|)(#([^~\$]*)|)(\$([^~]*)|)(~(.*)|)$/
REGEX_PRICE: new RegExp /^(([A-Za-z]{2})-|)([A-Z]{3}) (-?\d+)(-?\|(\d+)|)( ([^~\$#%]*)|)(#([^~\$%]*)|)(\$([^~%]*)|)(~([^%]*)|)(%([A-Z]{3} -?\d+ @\d+(.*))|)$/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woah, this is insane 😄

@daern91 daern91 removed the request for review from butenkor March 25, 2019 16:41
@Babanila
Copy link
Contributor Author

Babanila commented Mar 26, 2019

I had another look at the code now and it looks really good already 💯 I would just like to expand on the docs so it's easier to understand how to use it. Added some more comments and questions below.

Also, could we please add a test for the import as well? Just so we fully test both imports and exports of this feature 👍

Yes, tiers import test added.

@Babanila Babanila requested a review from daern91 March 26, 2019 14:03
Copy link
Contributor

@daern91 daern91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me now, thanks @Babanila! Let's also await review from @junajan before merging.

Copy link
Contributor

@junajan junajan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM .. just added two nitpicks

formattedTier =
value:
currencyCode: matchedPriceTier[0]
centAmount: parseInt matchedPriceTier[1]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
centAmount: parseInt matchedPriceTier[1]
centAmount: parseInt matchedPriceTier[1], 10

it is safer to use parseInt(number, 10) so it is always parsed with decimal number base

value:
currencyCode: matchedPriceTier[0]
centAmount: parseInt matchedPriceTier[1]
minimumQuantity: parseInt matchedPriceTier[3]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
minimumQuantity: parseInt matchedPriceTier[3]
minimumQuantity: parseInt matchedPriceTier[3], 10

@Babanila Babanila merged commit bd25263 into master Apr 3, 2019
@Babanila Babanila deleted the 242-use-sphere-product-csv-sync-to-sync-tier-prices branch April 3, 2019 09:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants