SectionOne
commented
6 months ago Em sembla una bona proposta sempre que s'optimitzi el codi tal com suggereix en @elpupas en un comentari anterior.
Open FranEnLaNube opened 6 months ago
SectionOne
commented
6 months ago Em sembla una bona proposta sempre que s'optimitzi el codi tal com suggereix en @elpupas en un comentari anterior.
sonarcloud[bot]
commented
5 months ago 
Description
This is the continuity of @androsrivas #98 to resolve #94 .
This PR replaces #109 , which is practically the same.
I've also brought changes made by @androsrivas on #101
The main idea is to separate the language of the application set in "Localization" from Laravel-lang to the language to is pretend to save the FAQ. That's is achieved through a language query parameter.
The app can return the FAQ already translated in a specific language or the FAQ with all its available translations, depending if the parameter is present or not.
I've bought the commits from @Andrea PR ESTE PR where she's fixed many tests to follow the multilanguage changes.
This PR adds changes to Work with different languages on FAQ, but
Changes
Swagger annotations:
- **IMPORTANT**: to generate the documentation after that edition, as is said in the [documentation](https://github.com/DarkaOnLine/L5-Swagger/wiki/Installation-&-Configuration), it's needed to run ``` php artisan l5-swagger:generate ``` Following the instructions from #64 of @GabrielaMaureira , I changed the comments in dummy functions in the folder `App\Annotations\OpenApi` as implemented by @androsrivas in #91 - Changes on AppsAnnotations came from ANDREA PR #101 https://github.com/IT-Academy-BCN/ita-landing-backend/pull/101 - Particularly from [this](https://github.com/IT-Academy-BCN/ita-landing-backend/pull/98/commits/6beff41a366429b26c397237c17096fad0a7483d) commit ### `app\Annotations\OpenApi\AnnotationsInfo.php` - Added English to the available languages. - Added the possibility to use the language in parameter to save FAQs in different language. - [ ] We should add APPs when they're ready. - Add the production URL `@OA\Server` - [ ] L17. The localhost URL is set there. Should we change it? - [ ] It's also possible to add a second one, to set one for local and another for production if it's wanted. ### `app\Annotations\OpenApi\AnnotationsTags.php` - Add Collaborators tag. ### `\controllersAnnotations\appAnnotations\AnnotationsApps.php` - Add translatable fields on `@OA\Property` for `store()` and `update()` - Changes from #101 ### `\controllersAnnotations\usersAnnotations\AnnotationsAuth.php` - Edited DNI example to a valid one to make it usable on Swagger online endpoint tester. - Completed description on response 200. ### `\controllersAnnotations\faqsAnnotations\AnnotationsFaqs.php` - New block comments for `show()` with two endpoints. This is possible by adding two `@OA Get` statements for the same dummy method `show()` in this case. - `/faqs/{id}`to show the JSON with all the available translations. - `/faqs/{id}?language` to show the FAQ already translated in a specific language, if available, by using the 'language' query parameter. - Edited the rest of the block comments to follow the `FaqController` changes. ### `\controllersAnnotations\usersAnnotations\AnnotationsUsers.php` - Changed DNI example to a valid one. - Add `code` field. ### `\modelsAnnotations\appAnnotations\AnnotationsApp.php` - add GitHub field on `@OA\Property` - Changes from #101, commit https://github.com/IT-Academy-BCN/ita-landing-backend/commit/004d456c95e4c2fe106f3b1cd23b94d28bbd9cd6 ### `\modelsAnnotations\faqAnnotations\AnnotationsFaq.php` - Changed the order of the items in the output array for `AnnotationsFaq {}`. - Added the available translations in the example. - Add a new class `AnnotationsFaqTranslation {}` to have the FAQ translations fields. - Add a new class `AnnotationsFaqStored {}` to have the Schema for when a FAQ is stored. - [ ] Check if this is the proper way or place to add Schemas. Mor info [here](https://stackoverflow.com/questions/64387159/l5-swagger-how-to-add-examples-):Controllers:
### `AppController.php` - Changes are from previous PR when the old way to approach multi-language Apps where implemented. - [ ] We need to work on this. Update it following the idea implemented on FAQs. ### `AuthController.php` - Improved syntax and added a type annotation to avoid error messages on IDE - [ ] There is a mistake, a duplicated code on L18. We need to fix it on next commit. ### `CodeController.php` and `CollaboratorsController.php` - Added `__()` helper on responses to get them on the language set on the `Accept-Language` header. (Catalan if it's missing) - [ ] Some responses are missing this change. - Syntax changes made to avoid SonarCloud messages. ### `FaqController.php` #### `index()` - Add status code to response. - [ ] We can add pagination here. #### `show()` - If we pass the $language as a query parameter in the URL it shows the FAQ already translated in that language. - If not, returns the FAQ with all its available translations. - The main FAQ is in the language set in the"Accept-Language" header, if empty defaults Catalan ('ca'). - [ ] This behavior needs to be checked. - Returns error messages if - The translation is not available 406. [Source](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Accept-Language) - The language key is not in the available languages set in 'supported_locales' 422. - The FAQ is not in database 404. - Added status codes to the response outputs. - Deleted `use Astrotomic\Translatable\Validation\RuleFactory;` - We don't need it any more because it's not necessary to validate all the languages together. - Added call to `use \Astrotomic\Translatable\Locales`. - To get the available locales/languages configured in the app. #### `store()` - Before, to add a new FAQ, Translatable admitted an array with all the languages as mandatory, like this one: ``` { "ca": { "title": "FAQ title in Catalan", "description": "FAQ description in catalan" }, "es": { "title": "FAQ title in Spanish", "description": "FAQ description in Spanish" }, "en": { "title": "FAQ title in English", "description": "FAQ description in English" } } ``` - Deleting the `RuleFactory` we are able to send one like this. `RuleFactory` is used to validate all the language inputs in one shot, we don't need it any more. ``` { "ca": { "title": "FAQ title", "description": "FAQ description" } } ``` - To make it easier, I set the function to receive the language from the parameters, so the Json now needs to be: ``` { "title": "FAQ title", "description": "FAQ description" } ``` - Then, that array is merge with the $language value coming from the URL `$dataWithLocaleKey = [$language => $validatedData];` And using the result array as input in the `$faq = Faq::create($dataWithLocaleKey)->setDefaultLocale($locale);` - `setDefaultLocale()` is used because Translatable needs to be set in the same language as the FAQ that we are trying to save to work properly. - More info [here](https://docs.astrotomic.info/laravel-translatable/package/fallback-locale) - Added validation to make the FAQ title unique `'title' => ['required','string', 'max:255', 'unique:faq_translations,title'],` - [ ] We should add this restriction to the migration as well. - **Important**: to add a new translation of a FAQ, we need to do it from the **uptate** route, otherwise is gonna create another instance of FAQ and the translations are not gonna be related. #### `update()` - Receives the language that we want to update from the URL query parameter. - Now, to update, we need to send the same array as in `create()` method. - If the parameter either is missing from the URL, is not available in the available locales, or the input doesn't pass the validation it throws an error. - Despite `update()` method in Laravel doesn't update if we leave the field empty, I added the restriction to get an error in that case. - Also took the `RuleFactory` off from here. #### `destroy()` - If we pass the parameter in the URL deletes the FAQ translation for that language, if not, deletes the FAQ completely. - If the last translation is deleted, the whole FAQ it's gonna be also deleted. ### `UserController.php` - Deleted hard coded validation error messages and replaced by `ValidationException` ones. - Added `__()` helper on responses to get them on the language set on the `Accept-Language` header. (Catalan if it's missing)Models:
- `App.php` and `Faq.php` models have been changed to set the Astronomic/Translatable package. - `AppTranslation.php` and `FaqTranslation.php` were created following the same objective.Other files:
### `setLocale.php` - I reckon the line with `App::setLocale(config('app.locale'));` inside the `else {}` in L26 it's not necessary because a fallback is configured on the `config/app.php` file. I don't know if same happens with `Session::put('locale', config('app.locale'));` which doesn't have a default value. ### `config\l5-swagger.php` - Set the title of the application dynamically in L8. `'title' => env('APP_NAME', 'ITA-Landing-API')` - Set the constant 'L5_SWAGGER_CONST_HOST' => env('L5_SWAGGER_CONST_HOST', env('APP_URL', 'http://127.0.0.1:8000').'/api'),` which can be used in annotations. - If it's not set in the `.env` it'll take the URL set on it, if neither is set, it'll take LocalHost and concatenate /api to it. - [ ] This need to be checked - [ ] **In this file it's possible to configure many things** , for instance the order of the methods in L257 `'operations_sort' => env('L5_SWAGGER_OPERATIONS_SORT', null),` ### `database\factories\FaqFactory.php` - Attempt to create the new method following the new changes. - [ ] Once this factory is done we should uncomment it from `database\seeders\DatabaseSeeder.php` ### `setApiLocale.php` - Deleted, it wasn't used, only `setLocale.php` is used instead. ### `config/app.php` - Changed `'locale' => 'ca'`, it was in `'en'` - `'fallback_locale' => 'ca'`, Set to Catalan, also was in `'en'` - Is the language returned when the asked one is not available. - Added 'en' to the array `'supported_locales' => ['ca', 'es', 'en'],` - These are the languages supported by Laravel Localization in our app. - `'faker_locale' => 'ca',` - In case of generate fake data it does it with information related to `'ca'`. - [ ] I'm not sure if 'ca' is supported. ### `config\translatable.php` - L13 locale added `'en'` to the available languages (locales). - L80 `'fallback_locale' => false,` - It was in `'en'`. Changing it to false make that in case the asked translation is not available it returns anything. - If a language is set, i.e. `'ca'` returns that translation. - If is set in null returns the first available in the array `'locales'` - [ ] We should check with the client which is the wished performance. - L137: `'format' => \Astrotomic\Translatable\Validation\RuleFactory::FORMAT_ARRAY,` - [ ] If we are not going to use it RuleFactory any more, this is not needed. ### `database/factories/FaqFactory.php` - Change the `definition()` method to create a FAQ in all the languages set in the app in one shot. ### `resources\lang\ca\api.php` - Added translations for `translation_not_found`, `translation_key_not_available`, `faq_translation_updated` and `faq_translation_deleted` - In that file and also for `'es'` y `'en'`. ### `routes/api.php` - Deleted call to `use Illuminate\Http\Request;` - Deleted call to middleware SetLocale `Route::middleware(['SetLocale'])->group(function () {}` because `\App\Http\Middleware\SetLocale::class`, is already set in `app/Http/Kernel.php` , L39: `protected $middlewareGroups = []` so it's not necessary to have it in this file. - This was in this the previous changes made on this "branch", it wasn't in "develop" - [Source](https://laraveldaily.com/post/set-laravel-user-locale-middleware) - Applied route model binding, [source](https://blog.keytech.dev/mastering-laravel-routing-a-comprehensive-guide-with-examples) - Add `->prefix('faqs')` and `->prefix('apps')` as it was asked in #58. - Group routes by controller like this example below. - [ ]Later we should do teh same with `AppController`: ``` Route::middleware(['auth:api'])->prefix('apps')->controller(AppController::class)->group(function () { Route::get('/{id}', 'show')->name('app.show'); Route::post('/', 'store')->name('app.store'); Route::put('/{id}', [AppController::class, 'update'])->name('app.update'); Route::delete('/{id}', [AppController::class, 'destroy'])->name('app.destroy'); }); ```To Do, pending tasks:
- [ ] I reckon should be a better way to manage the errors responses on controllers. SonarCloud is saying that each method has too many returns. - [ ] Should be a constant anywhere with the available locales for validations in methods `store` and `update` of `FaqController`. - [ ] `AuthController.php` We should refactor and translate the responses for `login()`. - [ ] There is a specific test for translations already done but it fails, we should fix it when this changes are applied on `AppController`. - [ ] We need to improve consistency in our status responses, in some methods we are returning a Boolean value, in others the status code and in others any of those, just the response. - There is a [method](https://developer.mozilla.org/es/docs/Web/API/Response/ok) or function to use in front codes that checks if the status is between 200 and 299 and returns true, so, I reckon we should just send the status code. - [ ] There is an error when I fetch an endpoint who demands validation and I'm not, it sends me to the login endpoint but keeping the `GET` method, so it fails, because login needs to be a `POST` one. - I fixed it sending the header `Accept = Application/json` on the call, but still needs to be checked. - Source: [SOF](https://stackoverflow.com/questions/57006647/how-to-fix-the-get-method-is-not-supported-for-this-route-supported-methods-p) - [ ] We need to add the `Accept-language` header on swagger App` endpoints . - [ ] We should apply the advices given by @elpupas [here](https://github.com/IT-Academy-BCN/ita-landing-backend/pull/109#pullrequestreview-1786761062) in the previous PR #109Some more interesting info:
- #75 is the issue where @Edvenan implemented Multi-Language. - #80 @levifvy add Laravel-Lang - #81 @Edvenan added useful comments about localization and the implementation of Translatable Package - The VSC extension [Swagger-PHP Annotation](https://marketplace.visualstudio.com/items?itemName=qvtec3.swagger-php-annotation) it's a good to have. - [This](https://www.pavelzanek.com/en/laravel/posts/factory-astrotomic-laravel-translatable) article gives some information to make a factory when Translatable is used.