[PM-27018] Add 22 supported languages and fix existing language codes in Password Manager and Authenticator #2086
Conversation
|
Great job! No new security vulnerabilities introduced in this pull request |
|
🤔 Consolidating the |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2086 +/- ##
==========================================
- Coverage 85.28% 82.95% -2.34%
==========================================
Files 1695 1973 +278
Lines 144592 160801 +16209
==========================================
+ Hits 123319 133388 +10069
- Misses 21273 27413 +6140 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
vvolkgang
changed the title
vvolkgang
changed the title
vvolkgang
changed the title
|
Claude finished @vvolkgang's task —— View job Code Review CompleteSummary of ChangesThis PR successfully addresses the issue raised by @fedemkr's review comment by adding 22 missing supported languages and fixing existing language code issues across both Password Manager and Authenticator apps. The changes include:
Critical IssuesNone identified. The reviewer's concern about CFBundleLocalizations has been properly addressed in commit Suggested Improvements1. Missing Portuguese Portugal in old Password Manager Info.plistFile: The original Info.plist had an unorganized list with incorrect codes like Old Info.plist issues (now fixed)The old file had:
All of these are now corrected in the new alphabetically sorted list. 2. Code Duplication Between Password Manager and AuthenticatorFiles:
Both LanguageOption enums have identical Suggestion: This is tracked for future work and appropriately scoped out of this PR. The comment at line 20-22 in both files correctly documents the intention to dynamically fetch languages in the future. 3. Test Comment AccuracyFiles:
XCTAssertEqual(allCases.count, 63) // "default" + "en" + crowdin provided languagesThe comment is slightly misleading. The count is actually: 1 (default) + 62 language codes = 63 total. The comment suggests "en" is separate from "crowdin provided languages," but "en" is included in the 62 language codes. Suggested fix: XCTAssertEqual(allCases.count, 63) // 1 default + 62 language codes from Crowdin4. Missing DocC DocumentationFiles:
The Suggested addition: /// All supported language codes from the Crowdin localization project.
///
/// This array is manually maintained to match the languages available in Crowdin.
/// Ideally this would be dynamically fetched from `Bundle.main.localizations`, but that
/// currently doesn't return reliable results.
private static let languageCodes = [5. Info.plist Consistency Verification✅ All 5 Info.plist files that were modified have identical, alphabetically sorted
This is excellent for maintainability. Good Practices Observed
Action Items for AuthorRequiredNone - the PR addresses the reviewer's concern and is functionally complete. Optional Improvements
Architecture & Code Style Compliance✅ Architecture: Changes follow established patterns. Language handling remains in the Platform layer as appropriate. ✅ Code Style: Consistent with Swift code style guidelines. Alphabetically sorted arrays are a good practice. ✅ Testing: Test coverage is maintained with updated assertions. ✅ Security: No security implications for language selection changes. ConclusionThis is a well-executed PR that addresses missing language support and fixes existing language code issues. The reviewer's concern about CFBundleLocalizations has been properly resolved. The suggested improvements are minor and optional. The PR is ready to merge pending any final team review. Recommendation: ✅ Approve with optional minor improvements |
Let's keep that to a separate PR. Ideally, this would also be refactored to load the list based on existing files but reckon we could provide a quick win for the time being and handle that later too. |
Roger roger |
There was a problem hiding this comment.
CFBundleLocalizations => https://github.com/bitwarden/ios/blob/main/Bitwarden/Application/Support/Info.plist#L24.
@fedemkr Thanks for raising it, updated all Info.plist files! |
🎟️ Tracking
PM-27018
📔 Objective
Both iOS Password Manager and Authenticator have a list of supported languages, this list was missing 22 languages that are supported in our Crowdin Project. While analyzing that I noticed other issues with existing language codes too, this PR addresses both.
📸 Screenshots
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes