Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion apps/entity-service/src/forms/forms.controller.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { PolicyService } from "@terramatch-microservices/common";
import { BadRequestException } from "@nestjs/common/exceptions/bad-request.exception";
import { FormFactory, FormQuestionFactory, FormSectionFactory } from "@terramatch-microservices/database/factories";
import { StoreFormAttributes } from "./dto/form.dto";
import { LocalizationService } from "@terramatch-microservices/common/localization/localization.service";

describe("FormsController", () => {
let controller: FormsController;
Expand All @@ -20,7 +21,8 @@ describe("FormsController", () => {
controllers: [FormsController],
providers: [
{ provide: FormsService, useValue: (service = createMock<FormsService>()) },
{ provide: PolicyService, useValue: (policyService = createMock<PolicyService>()) }
{ provide: PolicyService, useValue: (policyService = createMock<PolicyService>()) },
{ provide: LocalizationService, useValue: createMock<LocalizationService>() }
]
}).compile();

Expand Down
30 changes: 29 additions & 1 deletion apps/entity-service/src/forms/forms.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,16 @@ import { FormGetQueryDto, FormIndexQueryDto } from "./dto/form-query.dto";
import { JsonApiDeletedResponse } from "@terramatch-microservices/common/decorators/json-api-response.decorator";
import { PolicyService } from "@terramatch-microservices/common";
import { Form } from "@terramatch-microservices/database/entities";
import { LocalizationService } from "@terramatch-microservices/common/localization/localization.service";

@Controller("forms/v3/forms")
@ApiExtraModels(Forms)
export class FormsController {
constructor(private readonly formsService: FormsService, private readonly policyService: PolicyService) {}
constructor(
private readonly formsService: FormsService,
private readonly policyService: PolicyService,
private readonly localizationService: LocalizationService
) {}

@Get()
@ApiOperation({
Expand Down Expand Up @@ -100,4 +105,27 @@ export class FormsController {
await this.formsService.store(payload.data.attributes, form);
return await this.formsService.addFullDto(buildJsonApi<FormFullDto>(FormFullDto), form, false);
}

@Post(":uuid/translate")
Copy link
Collaborator

Choose a reason for hiding this comment

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

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"

@ApiOperation({ operationId: "formTranslate", description: "Translate a form" })
Copy link
Collaborator

Choose a reason for hiding this comment

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

This endpoint doesn't actually do the translation, it triggers the translation process, so this should probably reflect that.

@ExceptionResponse(UnauthorizedException, { description: "Form translation not allowed." })
@ExceptionResponse(BadRequestException, { description: "Form payload malformed." })
@ExceptionResponse(NotFoundException, { description: "Form not found." })
async translate(@Param("uuid") uuid: string) {
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);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

}

@Get(":uuid/translate")
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

@ApiOperation({ operationId: "formPullTranslations", description: "Pull translations for a form" })
@ExceptionResponse(UnauthorizedException, { description: "Form translation not allowed." })
@ExceptionResponse(BadRequestException, { description: "Form payload malformed." })
@ExceptionResponse(NotFoundException, { description: "Form not found." })
async pullTranslations(@Param("uuid") uuid: string) {
const form = await this.formsService.findOne(uuid);
await this.policyService.authorize("update", form);
await this.localizationService.pullTranslations({ filterTags: form.uuid });
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import { NotFoundException } from "@nestjs/common";
import { LocalizationService } from "@terramatch-microservices/common/localization/localization.service";
import { createMock } from "@golevelup/ts-jest";
import { ConfigService } from "@nestjs/config";
import { LocalizationFormService } from "@terramatch-microservices/common/localization/localization-form.service";

const mockLocale = (locale: ValidLocale) => {
jest.spyOn(User, "findOne").mockResolvedValue({ locale } as User);
Expand All @@ -25,7 +26,11 @@ describe("OptionsLabelsController", () => {
beforeEach(async () => {
const module = await Test.createTestingModule({
controllers: [OptionLabelsController],
providers: [LocalizationService, { provide: ConfigService, useValue: createMock<ConfigService>() }]
providers: [
LocalizationService,
LocalizationFormService,
{ provide: ConfigService, useValue: createMock<ConfigService>() }
]
}).compile();

controller = module.get(OptionLabelsController);
Expand Down
3 changes: 3 additions & 0 deletions libs/common/src/lib/common.module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { JwtModule } from "@nestjs/jwt";
import { ConfigModule, ConfigService } from "@nestjs/config";
import { PolicyService } from "./policies/policy.service";
import { LocalizationService } from "./localization/localization.service";
import { LocalizationFormService } from "./localization/localization-form.service";
import { EmailService } from "./email/email.service";
import { MediaService } from "./media/media.service";
import { SlackService } from "./slack/slack.service";
Expand Down Expand Up @@ -60,6 +61,7 @@ export const QUEUES = ["email", "analytics"];
{ provide: APP_GUARD, useClass: AuthGuard },
EmailService,
LocalizationService,
LocalizationFormService,
MediaService,
TemplateService,
SlackService,
Expand All @@ -74,6 +76,7 @@ export const QUEUES = ["email", "analytics"];
JwtModule,
EmailService,
LocalizationService,
LocalizationFormService,
MediaService,
TemplateService,
SlackService,
Expand Down
37 changes: 37 additions & 0 deletions libs/common/src/lib/localization/localization-form.service.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
import { Test } from "@nestjs/testing";
import { LocalizationFormService } from "./localization-form.service";
import {
FormFactory,
FormQuestionFactory,
FormSectionFactory,
I18nItemFactory
} from "@terramatch-microservices/database/factories";

describe("LocalizationFormService", () => {
let service: LocalizationFormService;

beforeEach(async () => {
const module = await Test.createTestingModule({
providers: [LocalizationFormService]
}).compile();

service = module.get<LocalizationFormService>(LocalizationFormService);
});

afterEach(async () => {
jest.restoreAllMocks();
});

describe("getI18nIdsForForm", () => {
it("should return the I18n IDs for a form", async () => {
const i18nItem = await I18nItemFactory.create();
const form = await FormFactory.create({ titleId: i18nItem.id });
const formSection1 = await FormSectionFactory.create({ formId: form.uuid });
const formSection2 = await FormSectionFactory.create({ formId: form.uuid });
await FormQuestionFactory.create({ formSectionId: formSection1.id });
await FormQuestionFactory.create({ formSectionId: formSection2.id });
const i18nIds = await service.getI18nIdsForForm(form);
expect(i18nIds).toBeDefined();
});
});
});
98 changes: 98 additions & 0 deletions libs/common/src/lib/localization/localization-form.service.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
import { Injectable } from "@nestjs/common";
import {
Form,
FormOptionList,
FormOptionListOption,
FormQuestion,
FormQuestionOption,
FormSection,
FormTableHeader,
FundingProgramme,
LocalizationKey
} from "@terramatch-microservices/database/entities";
import { intersection } from "lodash";
import { Model, Op } from "sequelize";

type TranslationModelType =
| typeof Form
| typeof FormSection
| typeof FormQuestion
| typeof FormQuestionOption
| typeof FormTableHeader
| typeof FormOptionList
| typeof FundingProgramme
| typeof LocalizationKey
| typeof FormOptionListOption;

type TranslationParamsType = string | number;

@Injectable()
export class LocalizationFormService {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a good reason to make a whole new service instead of simply including these methods in the existing form service?

private extraFields: string[] = ["id", "optionsList"];

private i18nIdsToBePushed: number[] = [];

private getI18nTranslationEntityFields(translationEntity: TranslationModelType) {
return translationEntity.I18N_FIELDS.map(field => `${field}Id`);
}

private async processTranslationEntity(
model: TranslationModelType,
property: string,
filterParams: TranslationParamsType | TranslationParamsType[]
) {
const filterParamsArray = Array.isArray(filterParams) ? filterParams : [filterParams];
const i18nFields = this.getI18nTranslationEntityFields(model);
// @ts-expect-error - entity is a model class
Copy link
Collaborator

Choose a reason for hiding this comment

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

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[]) {

const entities = await model.findAll({
where: {
[property]: {
[Op.in]: filterParamsArray
}
},
// @ts-expect-error - model is a model class
attributes: intersection(Object.keys(model.getAttributes()), [...i18nFields, ...this.extraFields])
});

entities.forEach(entity => {
this.pushI18nIds(entity, i18nFields);
});

return entities;
}

private pushI18nIds(entity: Model, i18nFields: string[]) {
Object.entries(entity.dataValues).forEach(([key, value]) => {
if (i18nFields.includes(key) && value != null) {
this.i18nIdsToBePushed.push(value as number);
}
});
}

async getI18nIdsForForm(form: Form) {
this.pushI18nIds(form, this.getI18nTranslationEntityFields(Form));
const formSections = await this.processTranslationEntity(FormSection, "formId", [form.uuid]);
const formQuestions = await this.processTranslationEntity(
FormQuestion,
"formSectionId",
formSections.map(section => section.id)
);
await this.processTranslationEntity(
FormTableHeader,
"formQuestionId",
formQuestions.map(question => question.id)
);
const optionsListParams = formQuestions
// @ts-expect-error - optionsList is a field of FormQuestion
.map(question => question.optionsList)
.filter(optionsList => optionsList != null)
.filter(optionsList => optionsList != "0");
const formOptionsLists = await this.processTranslationEntity(FormOptionList, "key", optionsListParams);
await this.processTranslationEntity(
FormOptionListOption,
"formOptionListId",
formOptionsLists.map(list => list.id)
);
return this.i18nIdsToBePushed;
}
}
29 changes: 24 additions & 5 deletions libs/common/src/lib/localization/localization.service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
import { Test, TestingModule } from "@nestjs/testing";
import { LocalizationService } from "./localization.service";
import {
Form,
FormOptionListOption,
FormQuestion,
FormQuestionOption,
Expand All @@ -20,7 +21,8 @@ import { I18nTranslationFactory } from "@terramatch-microservices/database/facto
import { LocalizationKeyFactory } from "@terramatch-microservices/database/factories/localization-key.factory";
import { Dictionary, trim } from "lodash";
import { NotFoundException } from "@nestjs/common";
import { FormQuestionFactory, I18nItemFactory } from "@terramatch-microservices/database/factories";
import { FormFactory, FormQuestionFactory, I18nItemFactory } from "@terramatch-microservices/database/factories";
import { LocalizationFormService } from "./localization-form.service";

jest.mock("@transifex/native", () => ({
tx: {
Expand All @@ -46,7 +48,11 @@ describe("LocalizationService", () => {

beforeEach(async () => {
const module: TestingModule = await Test.createTestingModule({
providers: [LocalizationService, { provide: ConfigService, useValue: createMock<ConfigService>() }]
providers: [
LocalizationService,
LocalizationFormService,
{ provide: ConfigService, useValue: createMock<ConfigService>() }
]
}).compile();

service = module.get<LocalizationService>(LocalizationService);
Expand Down Expand Up @@ -228,7 +234,19 @@ describe("LocalizationService", () => {
(createNativeInstance as jest.Mock).mockImplementation(() => ({
pushSource: pushSouceMock
}));
await service.pushTranslations();
await service.pushNewTranslations();
expect(pushSouceMock).toHaveBeenCalled();
});
});

describe("pushTranslationByForm", () => {
it("should push translations by form", async () => {
const form = await FormFactory.create();
const pushSouceMock = jest.fn();
(createNativeInstance as jest.Mock).mockImplementation(() => ({
pushSource: pushSouceMock
}));
await service.pushTranslationByForm(form);
expect(pushSouceMock).toHaveBeenCalled();
});
});
Expand All @@ -238,11 +256,12 @@ describe("LocalizationService", () => {
// Cleaning up the database
const entities = [
I18nItem,
FormOptionListOption,
Form,
FormSection,
FormQuestion,
FormQuestionOption,
FormSection,
FormTableHeader,
FormOptionListOption,
LocalizationKey,
FundingProgramme
];
Expand Down
Loading