Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New Proposal for FAQ Multilingual request and response #94 #109

Closed
wants to merge 33 commits into from

Conversation

FranEnLaNube
Copy link
Collaborator

@FranEnLaNube FranEnLaNube commented Dec 14, 2023

Description

Changes

Swagger annotations

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

  • IMPORTANT: to generate the documentation after that edition it's needed to run php artisan l5-swagger:generate as is said in the documentation

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 . It's also possible to add a second one.
      • L17. The localhost URL is set there. Should we change it?

app\Annotations\OpenApi\AnnotationsTags.php

  • Add Collaborators tag.

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.
  • Edit the rest of the block comments to follow the FaqController changes.

app\Annotations\OpenApi\controllersAnnotations\usersAnnotations\AnnotationsUsers.php

  • Deleted useless trailing whitespaces at the end of some lines.

app\Annotations\OpenApi\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:

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').
    • Returns error messages if
      • The translation is not available 406. Source
      • 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.
      • If we decide to also add this feature we should keep it.
  • Added call to use \Astrotomic\Translatable\Locales;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 need 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);
    • Translatable need to be set in the same language as the faq we are trying to save to work properly, that's the reason of setDefaultLocale()
  • 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 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.
    • I don't know if that is a wished performance.
  • 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.

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).
  • L83 '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.
  • L136: 'format' => \Astrotomic\Translatable\Validation\RuleFactory::FORMAT_ARRAY,
    • If we are not going to use it RuleFactory any more this is not needed.

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;
  • L22: 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.
  • Added {language?} to FAQ show, store , update and destroy methods to be able do call a particular language or not depending on the parameter is present or not.
  • Add ->prefix('faqs') and ->prefix('apps') as it was asked in [BE] Apps Crud #54 #58.
    • We still could improve route's order by grouping by controller for example. Like this:
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. 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.
  • UserController.php We should refactor and translate the responses for login.
    • Delete variable $validatedData
  • We should change the tests, at least the faqTests, if later we add a user with admin role.
    • There is also a test specific for translations already done, take care when making FaqTests.
    • This article gives some information to make a factory when Translatable is used.
  • 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 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 reckon that can be fixed in app\Http\Middleware\Authenticate.php file.

Some more interesting info

levifvy and others added 30 commits July 22, 2023 20:24
@FranEnLaNube FranEnLaNube self-assigned this Dec 14, 2023
Copy link

sonarcloud bot commented Dec 14, 2023

Quality Gate Failed Quality Gate failed

Failed conditions

22.2% Duplication on New Code (required ≤ 3%)
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

idea Catch issues before they fail your Quality Gate with our IDE extension SonarLint SonarLint

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the App model namespace is duplicated

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved

Copy link
Collaborator

@pupadevs pupadevs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent work, @FranEnLaNube To reduce the amount of 'return' in your functions, I recommend taking advantage of the various error handling classes that Laravel offers. Instead of multiple 'return', you can use different classes like Response, JsonResponse and other built-in methods to handle and respond to specific errors in a more structured way. This simplifies the flow of your code and makes it easier to maintain.

- Using HttpResponseException to return an HTTP response in case of an error:
throw new HttpResponseException(response()->json(['error' => 'Something went wrong'], 400));

- Using ValidationException to handle validation errors:
throw ValidationException::withMessages(['field' => 'Error validating field']);

- Using ModelNotFoundException to handle exceptions when a model is not found:
throw new ModelNotFoundException('The requested model was not found');


Tests: the multilanguage tests do not pass, I don't know if you have the settings for the tests to pass locally.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the trycatch block lacks a catch or finally, then there are 2 returns in a row, only the first return will be executed, the second one will be ignored

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh! that's true! I didn't realize that! Cause I didn't touch those files. The changes come from the freaking pint ...
I think this branch needs a biiig analysis...

Copy link
Collaborator

@pupadevs pupadevs Dec 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, that branch would have to pull from the develop branch or create a new branch from the develop branch and paste your changes. Although now I have noticed that it has no conflicts

@FranEnLaNube
Copy link
Collaborator Author

I've created a new PR, #110 , so I'll close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BE] Multilanguage API Calls explained to Swagger
5 participants