fix: Disallow creating duplicate prices #7866
Conversation
sradevski
requested review from
riqwan,
fPolic and
olivermrbl
and removed request for
a team
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Ignored Deployments
|
|
| const pricesToCreate = data | ||
| .map((addPrice) => | ||
| this.normalizePrices( | ||
| addPrice.prices as any, |
There was a problem hiding this comment.
The typings can be improve a lot more, but I decided not to do it as part of this PR
sradevski
force-pushed
the
fix/rule-validations
branch
from
e99a5ac
to
83902e5
Compare
sradevski
force-pushed
the
fix/rule-validations
branch
2 times, most recently
from
fb1f400
to
9aaf160
Compare
| @@ -163,6 +163,7 @@ export class PricingRepository | |||
| pl_rules_count: "price.pl_rules_count", | |||
| price_list_type: "price.pl_type", | |||
| price_list_id: "price.price_list_id", | |||
| all_rules_count: knex.raw("price.rules_count + price.pl_rules_count"), | |||
There was a problem hiding this comment.
don't we need a COALESCE here? "pl_rules_count" could be NULL no?
There was a problem hiding this comment.
Both rules count are non-nullable and default to 0, so we should be good
There was a problem hiding this comment.
At a first glance price.pl_rules_count apparently comes from pl_rules_count: "pl.rules_count", and that is a left join with price_list.
that why I asked if that can be null.
There was a problem hiding this comment.
it seams that the pl_price_count is indeed retrieved through a left join and it means by default that the right side is optional and therefore if you dont have a matching pl tuple the value will not be present which is why carlos is suggesting to use a coalescing (either in the original sub query or before using it, i would suggest to coalesce in the original select to be always defaulted) as it can be null. The only time it cant be null is when the left join as a match and therefore the default value at least will take place or if you were in the case of an inner join because both side requires to be present to be returned.
Feel free to let us know if we are wrong but could you run a little test query (just one select with one left join) where you select a price that does have a matching price list and let us know what the select returns you, i believe the count will be null 🙏
Maybe we are missing some context and in that case feel free to complement.
.../modules/pricing/integration-tests/__tests__/services/pricing-module/calculate-price.spec.ts
Show resolved
Hide resolved
| entry.prices = this.normalizePrices( | ||
| priceListData.prices, | ||
| [], | ||
| priceListData.prices.price_set_id |
There was a problem hiding this comment.
The price_set_id should come from individual prices in a price list as they can be from multiple price sets. This might not be needed here.
There was a problem hiding this comment.
Yeah you are right, let me try to clean this up a bit more 👍
sradevski
force-pushed
the
feat/revamp-pricing-module
branch
from
0d714dc
to
a59892f
Compare
sradevski
force-pushed
the
fix/rule-validations
branch
from
9aaf160
to
4e8bd61
Compare
sradevski
force-pushed
the
feat/revamp-pricing-module
branch
from
a59892f
to
dbcd313
Compare
sradevski
force-pushed
the
fix/rule-validations
branch
from
4e8bd61
to
9a46b1c
Compare
sradevski
force-pushed
the
fix/rule-validations
branch
from
9a46b1c
to
e31a2d3
Compare
This PR is targeting the pricing revamp as it depends on the cleanup there. Will change base branch to develop once that is merged.
We do two things here: