[TM-2739] add pull push button translation forms #463
Conversation
pachonjcl
changed the title
| return await this.formsService.addFullDto(buildJsonApi<FormFullDto>(FormFullDto), form, false); | ||
| } | ||
|
|
||
| @Post(":uuid/translate") |
There was a problem hiding this comment.
When you're making a REST creation endpoint, you're creating a resource. This reads like an action (you don't create a translate, you create a translation), so this should likely be ":uuid/translation"
| } | ||
|
|
||
| @Post(":uuid/translate") | ||
| @ApiOperation({ operationId: "formTranslate", description: "Translate a form" }) |
There was a problem hiding this comment.
This endpoint doesn't actually do the translation, it triggers the translation process, so this should probably reflect that.
| const form = await this.formsService.findOne(uuid); | ||
| await this.policyService.authorize("update", form); | ||
| await this.localizationService.pushTranslationByForm(form); | ||
| return await this.formsService.addFullDto(buildJsonApi<FormFullDto>(FormFullDto), form, false); |
There was a problem hiding this comment.
Specifying the generic DTO type explicitly should not be needed here:
return await this.formsService.addFullDto(buildJsonApi(FormFullDto), form, false);
I also think it's a little weird for this endpoint to return the form DTO. As mentioned above, you're creating a translation, not a form. Doing the work to generate the full form DTO doesn't really seem appropriate. Perhaps there should be a FormTranslation DTO to make the semantics and resource based nature of this endpoint a bit cleaner.
| return await this.formsService.addFullDto(buildJsonApi<FormFullDto>(FormFullDto), form, false); | ||
| } | ||
|
|
||
| @Get(":uuid/translate") |
There was a problem hiding this comment.
This endpoint should match the one above in terms of the name of the endpoint, description and DTO. For one thing, this endpoint currently doesn't return anything, which is a bit odd and would be new for this codebase. I think that when you create a form translation DTO for the endpoint above (I expect it'll be tiny, but I'm not sure what's really needed in it), it should be returned here as well.
I haven't looked at how the FE implementation works yet, but for one thing, not returning anything at all is likely to cause looping requests in the connection system.
| type TranslationParamsType = string | number; | ||
|
|
||
| @Injectable() | ||
| export class LocalizationFormService { |
There was a problem hiding this comment.
Is there a good reason to make a whole new service instead of simply including these methods in the existing form service?
| ) { | ||
| const filterParamsArray = Array.isArray(filterParams) ? filterParams : [filterParams]; | ||
| const i18nFields = this.getI18nTranslationEntityFields(model); | ||
| // @ts-expect-error - entity is a model class |
There was a problem hiding this comment.
Your TS errors will probably go away and the code will be cleaner (for one thing, the return type will be defined so the error you're hiding in getI18nIdsForForm should go away) if you make this method generic:
private async processTranslationEntity<M extends TranslationModelType>(model: M, property: string, filterParams: TranslationParamsType | TranslationParamsType[]) {
Task Link