Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis change introduces a custom Eloquent attribute cast, Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🪛 PHPMD (2.15.0)packages/core/src/Base/Casts/CouponString.php10-10: Avoid unused parameters such as '$model'. (Unused Code Rules) (UnusedFormalParameter) 10-10: Avoid unused parameters such as '$key'. (Unused Code Rules) (UnusedFormalParameter) 10-10: Avoid unused parameters such as '$attributes'. (Unused Code Rules) (UnusedFormalParameter) 15-15: Avoid unused parameters such as '$model'. (Unused Code Rules) (UnusedFormalParameter) 15-15: Avoid unused parameters such as '$key'. (Unused Code Rules) (UnusedFormalParameter) 15-15: Avoid unused parameters such as '$attributes'. (Unused Code Rules) (UnusedFormalParameter) 🔇 Additional comments (4)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/core/src/Models/Cart.php(2 hunks)packages/core/src/Models/Discount.php(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: stripe - PHP 8.3 - L12.* ↑
- GitHub Check: admin - PHP 8.4 - L12.* ↑
- GitHub Check: search - PHP 8.3 - L12.* ↑ E
- GitHub Check: shipping - PHP 8.3 - L12.* ↑ E
- GitHub Check: admin - PHP 8.4 - L12.* ↑ E
- GitHub Check: search - PHP 8.4 - L11.* ↑
- GitHub Check: core - PHP 8.4 - L11.* ↑
- GitHub Check: core - PHP 8.4 - L11.* ↑ E
- GitHub Check: core - PHP 8.3 - L12.* ↑ E
- GitHub Check: core - PHP 8.3 - L12.* ↑
- GitHub Check: admin - PHP 8.3 - L11.* ↑ E
- GitHub Check: admin - PHP 8.3 - L12.* ↑ E
- GitHub Check: core - PHP 8.3 - L11.* ↑ E
- GitHub Check: admin - PHP 8.3 - L12.* ↑
- GitHub Check: admin - PHP 8.3 - L11.* ↑
- GitHub Check: core - PHP 8.3 - L11.* ↑
🔇 Additional comments (2)
packages/core/src/Models/Cart.php (1)
8-8: LGTM!The import is correctly added and necessary for the new Attribute casting functionality.
packages/core/src/Models/Discount.php (1)
6-6: LGTM!The import is correctly added and necessary for the new Attribute casting functionality.
|
@alecritson I think this is OK, as long as we restrict coupon codes to ASCII-only. I'm guessing we don't have that in place currently. |
We don't have that in place currently afaik so it would be it's own feature if required. |
It's not really a separate feature. This PR can't be merged if we're uppercasing non-ASCII characters. |
…hp/lunar into ensure-coupon-codes-are-uppercase
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/core/Unit/Base/Casts/UppercaseAsciiTest.php (1)
102-114: Consider adding tests for potential problematic Unicode sequences.While the coverage is excellent, consider adding tests for edge cases like:
- RTL characters
- Emoji characters
- Combining diacritical marks
- Very long Unicode strings
// Additional test cases to consider: ['🎉party', 'PARTY'], // Emoji handling ['café́', 'CAFE'], // Combining diacritical marks ['متجر', 'MTR'], // RTL script (Arabic) ['お店', ''], // Non-Latin script handling
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/core/src/Base/Casts/UppercaseAscii.php(1 hunks)packages/core/src/Models/Cart.php(2 hunks)packages/core/src/Models/Discount.php(2 hunks)tests/core/Unit/Base/Casts/UppercaseAsciiTest.php(1 hunks)tests/core/Unit/DiscountTypes/AbstractDiscountTypeTest.php(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/core/Unit/DiscountTypes/AbstractDiscountTypeTest.php
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/src/Models/Discount.php
- packages/core/src/Models/Cart.php
🧰 Additional context used
🪛 PHPMD (2.15.0)
packages/core/src/Base/Casts/UppercaseAscii.php
10-10: Avoid unused parameters such as '$model'. (Unused Code Rules)
(UnusedFormalParameter)
10-10: Avoid unused parameters such as '$key'. (Unused Code Rules)
(UnusedFormalParameter)
10-10: Avoid unused parameters such as '$attributes'. (Unused Code Rules)
(UnusedFormalParameter)
15-15: Avoid unused parameters such as '$model'. (Unused Code Rules)
(UnusedFormalParameter)
15-15: Avoid unused parameters such as '$key'. (Unused Code Rules)
(UnusedFormalParameter)
15-15: Avoid unused parameters such as '$attributes'. (Unused Code Rules)
(UnusedFormalParameter)
🔇 Additional comments (5)
packages/core/src/Base/Casts/UppercaseAscii.php (4)
8-28: Excellent approach to handle non-ASCII characters - addresses PR discussion concerns.The implementation elegantly solves the Unicode character issue raised in the PR discussion by using transliteration to convert non-ASCII characters to their ASCII equivalents before uppercasing. This ensures consistent handling regardless of input character encoding.
10-18: Interface compliance requires these parameters - static analysis warnings are false positives.The unused parameter warnings from PHPMD are false positives. These parameters ($model, $key, $attributes) are required by the
CastsAttributesinterface contract and cannot be removed.
23-24: Character replacement logic handles edge cases well.The apostrophe variants and space/hyphen replacement logic is comprehensive and handles various Unicode quotation marks that could appear in coupon codes.
22-22: Intl extension requirement verified
Theext-intldependency is already declared in composer.json and documented in both docs/core/installation.md and docs/core/starter-kits.md. No further action needed.tests/core/Unit/Base/Casts/UppercaseAsciiTest.php (1)
7-115: Excellent comprehensive test coverage for Unicode transliteration.The test suite provides outstanding coverage of various Unicode character scenarios, demonstrating that the transliteration approach effectively handles the non-ASCII character concerns raised in the PR discussion.
…hp/lunar into ensure-coupon-codes-are-uppercase
|
@alecritson can you explain the recent updates for me? |
Changes were made based on your last feedback, moved to a casting class, which you can see here: https://github.com/lunarphp/lunar/pull/2254/files#diff-649fb4fd1d2680bf585ee340f8fe5d277af88153896fb1f8b5eb1773a5272718 |
I see the code, but can you explain the approach and what it's doing? |
@glennjacobs |
I think I've got this wrong. I'll do some research and come back. |
|
So initially I was concerned that uppercasing everything would fundamentally change the code. E.g. However, using E.g. Str::upper('🏆promo'); // '🏆PROMO'Sorry, I think we simply need to revert the recent changes... |
…hp/lunar into ensure-coupon-codes-are-uppercase
Reverted but kept the casting class, just renamed for future scaling if needed. |
3 participants
Should Close #2252 by ensuring coupons are stored and retrieved uppercase regardless of where they are introduced.
Summary by CodeRabbit
New Features
Bug Fixes
Chores