New release - HOTFIX PCI Project already used voucher#22298
New release - HOTFIX PCI Project already used voucher#22298github-actions[bot] wants to merge 2 commits intomasterfrom
Conversation
ref: #MANAGER-20907 Signed-off-by: Fabien Henon <fabien.henon.ext@corp.ovh.com>
fix(pci-project): already used voucher on activation
FabienHenon
changed the title
FabienHenon
requested review from
ErikssonJoakim,
Taelar,
Tsiorifamonjena,
ZyaCon,
aTurmo,
Copilot,
fredericvilcot and
lolottetheclash
There was a problem hiding this comment.
Pull request overview
This pull request attempts to fix an issue where vouchers are being claimed multiple times during PCI project activation. The fix introduces a check to verify if a voucher has already been used before attempting to claim it again.
Changes:
- Added
isVoucherUsedfunction to check if a voucher has already been claimed for a project - Modified project activation flow to check voucher usage before claiming
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/manager/apps/pci-project/src/data/api/credit.ts | Added isVoucherUsed function to determine if a voucher has been applied to a project |
| packages/manager/apps/pci-project/src/pages/detail/activate/hooks/useActivateProject.tsx | Integrated voucher usage check before claiming discovery voucher during project activation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return ( | ||
| (response.data || []).filter((creditDetail: CreditDetailsResponse) => { | ||
| return creditDetail.voucher === voucher; | ||
| }).length === 0 |
There was a problem hiding this comment.
The logic in isVoucherUsed is inverted. The function returns true when the filter finds no matching vouchers (length === 0), but it should return true when a matching voucher IS found (length > 0).
Currently: Returns true when voucher is NOT in credit details (indicating it hasn't been used)
Expected: Should return true when voucher IS in credit details (indicating it has been used)
This will cause the voucher to be claimed when it has already been used, which is the opposite of the intended behavior. The condition should be length > 0 instead of length === 0.
| }).length === 0 | |
| }).length > 0 |
| return ( | ||
| (response.data || []).filter((creditDetail: CreditDetailsResponse) => { | ||
| return creditDetail.voucher === voucher; | ||
| }).length === 0 |
There was a problem hiding this comment.
The new isVoucherUsed function lacks test coverage. Since the codebase has comprehensive test coverage for other functions in the credit API (see Credit.spec.ts), this new function should also have tests to verify its behavior, especially given the complexity of the logic and the critical nature of preventing duplicate voucher claims.
| }).length === 0 | |
| }).length > 0 |
| return ( | ||
| (response.data || []).filter((creditDetail: CreditDetailsResponse) => { | ||
| return creditDetail.voucher === voucher; | ||
| }).length === 0 |
There was a problem hiding this comment.
The filter callback can be simplified. Instead of an explicit return statement in the filter, you can use an implicit return. The entire logic could also be rewritten more clearly using .some() instead of .filter().length === 0. For example: !response.data?.some(credit => credit.voucher === voucher) which would be more idiomatic. Note: This suggestion assumes the logic bug in the previous comment is fixed first.
| return ( | |
| (response.data || []).filter((creditDetail: CreditDetailsResponse) => { | |
| return creditDetail.voucher === voucher; | |
| }).length === 0 | |
| return !response.data?.some( | |
| (creditDetail: CreditDetailsResponse) => creditDetail.voucher === voucher, |
1 participant
📦 New release
Approximate release date: 📆
🐛 Bug Fixes