Fixed: replaced AnyAscii.transliterate with I18n.transliterate#58
Fixed: replaced AnyAscii.transliterate with I18n.transliterate#58damianlegawiec merged 6 commits intomainfrom
AnyAscii.transliterate with I18n.transliterate#58Conversation
`any_ascii` gem was removed from spree dependencies
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughSwitches transliteration to I18n and trims the result; bumps gem VERSION to 1.6.0.beta; raises minimum Spree dependency to >= 5.4.0.beta; adds two gems to the Gemfile; adds corresponding rails generator calls to the Rakefile; removes one synchronous test example. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/presenters/spree_stripe/statement_descriptor_suffix_presenter.rb (1)
27-31:⚠️ Potential issue | 🟠 MajorAdd explicit locale and replacement parameters to
I18n.transliterate.Without an explicit
locale:parameter,I18n.transliterateuses the current request locale, which can produce inconsistent output. Without areplacement:parameter, characters without ASCII approximations fall back to?—non-Latin scripts like Japanese, Arabic, and Chinese become chains of question marks that won't be stripped by the allowed-character filter. For payment descriptors, this should be deterministic and graceful.Suggested change
- I18n.transliterate(order_description) + I18n.transliterate(order_description, locale: :en, replacement: '') .gsub(STATEMENT_DESCRIPTOR_NOT_ALLOWED_CHARS, '') .strip .upcase[0...STATEMENT_DESCRIPTOR_MAX_CHARS] .stripAdd a regression spec for non-Latin input (e.g., a Japanese order description) to verify it produces a valid descriptor instead of
?characters.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/presenters/spree_stripe/statement_descriptor_suffix_presenter.rb` around lines 27 - 31, The transliteration call should be made deterministic and not produce question marks for non-Latin scripts: change the I18n.transliterate(order_description) invocation in the presenter to include explicit locale and replacement options (e.g., I18n.transliterate(order_description, locale: :en, replacement: '') ) so unknown characters are dropped instead of becoming '?' before applying STATEMENT_DESCRIPTOR_NOT_ALLOWED_CHARS and truncating to STATEMENT_DESCRIPTOR_MAX_CHARS; then add a regression spec that passes a Japanese (or other non-Latin) order description to the presenter to assert the resulting descriptor contains only allowed characters (no question marks) and respects the max length.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@app/presenters/spree_stripe/statement_descriptor_suffix_presenter.rb`:
- Around line 27-31: The transliteration call should be made deterministic and
not produce question marks for non-Latin scripts: change the
I18n.transliterate(order_description) invocation in the presenter to include
explicit locale and replacement options (e.g.,
I18n.transliterate(order_description, locale: :en, replacement: '') ) so unknown
characters are dropped instead of becoming '?' before applying
STATEMENT_DESCRIPTOR_NOT_ALLOWED_CHARS and truncating to
STATEMENT_DESCRIPTOR_MAX_CHARS; then add a regression spec that passes a
Japanese (or other non-Latin) order description to the presenter to assert the
resulting descriptor contains only allowed characters (no question marks) and
respects the max length.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cdacadd5-65e1-451d-bc05-09e8cfe675d5
📒 Files selected for processing (3)
app/presenters/spree_stripe/statement_descriptor_suffix_presenter.rblib/spree_stripe/version.rbspree_stripe.gemspec
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Gemfile`:
- Around line 18-19: The Gemfile now includes an unrelated dependency gem
'spree_posts' (the line gem 'spree_posts', github: 'spree/spree-posts', branch:
'main') which appears out of scope for the transliteration change; either remove
that gem line from the Gemfile to keep the PR focused, or if it's intentional,
add a short explanation to the PR description describing why 'spree_posts' is
required and how it relates to the AnyAscii -> I18n.transliterate change so
reviewers understand the rationale.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 51ba0ce8-145c-4bfd-bac9-a14c722df3f3
📒 Files selected for processing (2)
Gemfilelib/spree_stripe/version.rb
| gem 'spree_posts', github: 'spree/spree-posts', branch: 'main' | ||
|
|
There was a problem hiding this comment.
Unrelated change: clarify the reason for adding spree_posts.
The PR objective is to replace AnyAscii.transliterate with I18n.transliterate, but this adds an unrelated spree_posts gem dependency. This appears to be scope creep or an accidental inclusion.
If this is intentional, please update the PR description to explain why this dependency is needed. Otherwise, consider removing it to keep the PR focused.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Gemfile` around lines 18 - 19, The Gemfile now includes an unrelated
dependency gem 'spree_posts' (the line gem 'spree_posts', github:
'spree/spree-posts', branch: 'main') which appears out of scope for the
transliteration change; either remove that gem line from the Gemfile to keep the
PR focused, or if it's intentional, add a short explanation to the PR
description describing why 'spree_posts' is required and how it relates to the
AnyAscii -> I18n.transliterate change so reviewers understand the rationale.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Gemfile`:
- Around line 18-19: Reorder the two gem entries so the Gemfile section is
alphabetized: move the gem 'spree_legacy_product_properties', github:
'spree/spree_legacy_product_properties', branch: 'main' line to appear before
gem 'spree_posts', github: 'spree/spree-posts', branch: 'main' to satisfy
Bundler/OrderedGems and avoid lint churn.
In `@Rakefile`:
- Around line 24-26: The three system(...) calls invoking the generators (the
commands 'rails g spree_posts:install', 'rails g spree_legacy_api_v2:install',
and 'rails g spree_legacy_product_properties:install') currently ignore exit
status; change them to fail fast by either replacing them with Rake's sh (which
raises on non‑zero) or by checking the boolean return and aborting/raising on
failure (e.g., raise or abort with a clear message if system(...) returns false)
so the Rake task stops immediately when any generator exits non‑zero.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
1 participant
any_asciigem was removed from spree dependenciesSummary by CodeRabbit
Bug Fixes
Dependencies
Chores
Tests