Added logic to LanguageController for showing "Language changed" notification in the newly set language #7
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes involve modifications to the Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 0
🧹 Outside diff range and nitpick comments (2)
src/Http/Controllers/LanguageController.php (1)
Line range hint
21-41: Consider wrapping database operations in a transactionWhile the language update logic is well-structured, database operations should be wrapped in a transaction to ensure data consistency, especially when dealing with potential race conditions in concurrent requests.
Consider applying this change:
public function index(Request $request): RedirectResponse { $request->validate([ 'lang' => 'required|string', 'model' => 'required|string', 'model_id' => 'required', ]); + return DB::transaction(function () use ($request) { if (Schema::hasColumn(app($request->get('model'))->getTable(), 'lang')) { $model = app($request->get('model'))->find($request->get('model_id')); $model->lang = $request->get('lang'); $model->save(); } else { $lang = UserLanguage::query() ->where('model_type', $request->get('model')) ->where('model_id', $request->get('model_id')) ->first(); if ($lang) { $lang->lang = $request->get('lang'); $lang->save(); } else { UserLanguage::query()->create([ 'model_type' => $request->get('model'), 'model_id' => $request->get('model_id'), 'lang' => $request->get('lang'), ]); } } + });tests/src/LanguageSwitcherTest.php (1)
64-80: Consider enhancing test coverageWhile the current test is good, consider these improvements for more robust testing:
- Add more common locales (e.g., 'fr', 'es', 'ja')
- Include explicit RTL/LTR mix testing
- Add edge cases for invalid locales
- Verify additional notification attributes (success status, persistence)
Example enhancement:
})->with(['locale' => ['en', 'nl', 'ar', 'de']]); +})->with(['locale' => [ + 'common' => ['en', 'fr', 'es', 'de'], + 'rtl' => ['ar', 'he'], + 'asian' => ['ja', 'zh'], + 'edge_cases' => ['invalid-locale', ''] +]]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/Http/Controllers/LanguageController.php(1 hunks)tests/src/LanguageSwitcherTest.php(2 hunks)
🔇 Additional comments (5)
src/Http/Controllers/LanguageController.php (3)
Line range hint 13-19: LGTM: Method signature and validation are well-defined
The method properly validates all required parameters with appropriate type hints and return type.
Line range hint 49-54: LGTM: Redirection logic is well-implemented
The redirection handling properly respects configuration settings and provides flexible navigation options.
43-47: Verify locale existence before using it in translation
While the notification now uses the newly set language, it's important to verify that the locale exists to prevent potential translation issues.
Run this script to check available locales and their usage:
tests/src/LanguageSwitcherTest.php (2)
3-3: LGTM: Import statement is correctly placed
The Notification import is properly added and necessary for the new test functionality.
64-80: LGTM: Well-structured test implementation
The test case effectively verifies that notifications are displayed in the newly selected language, which directly addresses the PR's objective.
This PR includes a minor addition which applies the newly set locale to the translation function.
This way, the notification shows in the newly selected language.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests