add number_format_ja(Japan Format) for number_format#1485
add number_format_ja(Japan Format) for number_format#1485kusanaginoturugi wants to merge 5 commits intoInvoicePlane:prep/v172from
Conversation
📝 WalkthroughWalkthroughA new Japanese number format configuration has been added to the application's number formats settings. The configuration introduces a new public entry with label "number_format_ja" and comma thousands separator without modifying existing format definitions. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can disable poems in the walkthrough.Disable the |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@application/config/number_formats.php`:
- Around line 53-56: The new number_format_ja array entry is missing the
required 'decimal_point' key so code in Settings.php that does
$number_formats[$value]['decimal_point'] will throw an undefined index error;
update the 'number_format_ja' configuration (the 'number_format_ja' array) to
include a 'decimal_point' key with an explicit value (for example '.' or an
empty string if you prefer no decimals) so the lookup in Settings.php and any
consumers of $number_formats always find the key.
- Around line 53-56: Add a translation entry for the new config key
'number_format_ja' so the settings dropdown (which uses _trans($value['label']))
shows a human-friendly label; update the language files to include a mapping for
'number_format_ja' following the existing pattern used by other number formats
(e.g., the same style as other entries in the language arrays) so the label is
translated instead of showing the raw key.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 359d5ef1-4843-4612-9924-aac1b412c1e7
📒 Files selected for processing (1)
application/config/number_formats.php
| 'number_format_ja' => [ | ||
| 'label' => 'number_format_ja', | ||
| 'thousands_separator' => ',', | ||
| ], |
There was a problem hiding this comment.
Missing decimal_point key will cause runtime error.
The new number_format_ja entry is missing the decimal_point key that all other format entries have. When this format is selected, application/modules/settings/controllers/Settings.php (lines 101-105) directly accesses $number_formats[$value]['decimal_point'] without an isset() check, which will cause an "Undefined index" error.
Even if Japanese Yen doesn't use decimals, a value must be provided for the application to function correctly.
🐛 Proposed fix to add the missing decimal_point key
'number_format_ja' => [
'label' => 'number_format_ja',
+ 'decimal_point' => '.',
'thousands_separator' => ',',
],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@application/config/number_formats.php` around lines 53 - 56, The new
number_format_ja array entry is missing the required 'decimal_point' key so code
in Settings.php that does $number_formats[$value]['decimal_point'] will throw an
undefined index error; update the 'number_format_ja' configuration (the
'number_format_ja' array) to include a 'decimal_point' key with an explicit
value (for example '.' or an empty string if you prefer no decimals) so the
lookup in Settings.php and any consumers of $number_formats always find the key.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for number_format_ja translation key in language files
# Search for the translation key in all language files
rg -n "number_format_ja" --glob "*_lang.php"Repository: InvoicePlane/InvoicePlane
Length of output: 51
🏁 Script executed:
# Check if number_format_ja is referenced/used anywhere in the codebase
rg -n "number_format_ja" --type php --type htmlRepository: InvoicePlane/InvoicePlane
Length of output: 231
🏁 Script executed:
# Examine the referenced language file to see the existing format translations
head -400 application/language/english/ip_lang.php | tail -30Repository: InvoicePlane/InvoicePlane
Length of output: 2641
🏁 Script executed:
# Search for how number_format labels are used in views/templates
rg -n "number_format" --glob "*.html" --glob "*.php" -A 2 | head -50Repository: InvoicePlane/InvoicePlane
Length of output: 5585
Add missing translation key for the new number format.
The label 'number_format_ja' lacks a corresponding translation entry in the language files. When displayed in the settings dropdown (via _trans($value['label']) at application/modules/settings/views/partial_settings_general.php:217), it will show the raw key "number_format_ja" instead of a human-readable label. Follow the existing pattern from other number formats:
'number_format_ja' => '1,000,000 (Japanese format)',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@application/config/number_formats.php` around lines 53 - 56, Add a
translation entry for the new config key 'number_format_ja' so the settings
dropdown (which uses _trans($value['label'])) shows a human-friendly label;
update the language files to include a mapping for 'number_format_ja' following
the existing pattern used by other number formats (e.g., the same style as other
entries in the language arrays) so the label is translated instead of showing
the raw key.
There was a problem hiding this comment.
Pull request overview
This PR adds a Japanese number format option (1,000,000 — no decimal places) to InvoicePlane's number format configuration. It also includes an unrelated fix to the release workflow's language directory check and removes an old analysis document.
Changes:
- Added
number_format_jaentry to the number formats config - Updated the release workflow to check
application/language/englishinstead ofresources/lang/en - Deleted the
.junie/PR-1441-security-dry-analysis.mddocument
Reviewed changes
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| application/config/number_formats.php | Adds Japanese number format with thousands separator but missing decimal_point key |
| .github/workflows/release.yml | Fixes language directory path check from Laravel-style to CodeIgniter-style path |
| .junie/PR-1441-security-dry-analysis.md | Removes old security/DRY analysis document for a previous PR |
| 'thousands_separator' => '', | ||
| ], | ||
| 'number_format_ja' => [ | ||
| 'label' => 'number_format_ja', |
|
@kusanaginoturugi thanks for the PR, man!
You might want to consider this format: It's the US/UK format. |
Pull Request Checklist
Please check the following steps before submitting your PR. If any items are incomplete, consider marking it as
[WIP](Work in Progress).Checklist
Description
Provide a brief description of the changes made in this pull request.
Related Issue(s)
List any related issues or discussions. If applicable, link to an accompanying thread on the forums.
Fixes # (example)
Motivation and Context
Why was this change necessary? Does it solve a problem or improve an existing feature? If this PR fixes an open issue, link to it here.
Issue Type (Check one or more)
Screenshots (If Applicable)
Attach relevant screenshots that demonstrate your changes.
Thank you for your contribution to InvoicePlane! We appreciate your time and effort.
Summary by CodeRabbit
Release Notes