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

fix: consider sendToAll settings in routingForms response #18226

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
2 changes: 1 addition & 1 deletion apps/api/v2/src/ee/platform-endpoints-module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ import { SchedulesModule_2024_04_15 } from "@/ee/schedules/schedules_2024_04_15/
import { SchedulesModule_2024_06_11 } from "@/ee/schedules/schedules_2024_06_11/schedules.module";
import { SlotsModule } from "@/modules/slots/slots.module";
import { TeamsEventTypesModule } from "@/modules/teams/event-types/teams-event-types.module";
import { TeamsModule } from "@/modules/teams/teams/teams.module";
import { TeamsMembershipsModule } from "@/modules/teams/memberships/teams-memberships.module";
import { TeamsModule } from "@/modules/teams/teams/teams.module";
import type { MiddlewareConsumer, NestModule } from "@nestjs/common";
import { Module } from "@nestjs/common";

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@ import { EventTypesService_2024_06_14 } from "@/ee/event-types/event-types_2024_
import { AtomsRepository } from "@/modules/atoms/atoms.repository";
import { CredentialsRepository } from "@/modules/credentials/credentials.repository";
import { MembershipsRepository } from "@/modules/memberships/memberships.repository";
import { TeamsEventTypesService } from "@/modules/teams/event-types/services/teams-event-types.service";
import { PrismaReadService } from "@/modules/prisma/prisma-read.service";
import { PrismaWriteService } from "@/modules/prisma/prisma-write.service";
import { TeamsEventTypesService } from "@/modules/teams/event-types/services/teams-event-types.service";
import { UsersService } from "@/modules/users/services/users.service";
import { UserWithProfile } from "@/modules/users/users.repository";
import { Injectable, NotFoundException, ForbiddenException } from "@nestjs/common";
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
import { CreateTeamInput } from "@/modules/teams/teams/inputs/create-team.input";

export class CreateOrgTeamDto extends CreateTeamInput {}
export class CreateOrgTeamDto extends CreateTeamInput {}
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
import { UpdateTeamDto } from "@/modules/teams/teams/inputs/update-team.input";

export class UpdateOrgTeamDto extends UpdateTeamDto {}
export class UpdateOrgTeamDto extends UpdateTeamDto {}
116 changes: 114 additions & 2 deletions packages/app-store/routing-forms/playwright/tests/basic.e2e.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,11 @@ import type { Locator, Page } from "@playwright/test";
import { expect } from "@playwright/test";

import { prisma } from "@calcom/prisma";
import { AttributeType, SchedulingType } from "@calcom/prisma/enums";
import { AttributeType, MembershipRole, SchedulingType } from "@calcom/prisma/enums";
import type { Fixtures } from "@calcom/web/playwright/lib/fixtures";
import { test } from "@calcom/web/playwright/lib/fixtures";
import { selectInteractions } from "@calcom/web/playwright/lib/pageObject";
import { gotoRoutingLink } from "@calcom/web/playwright/lib/testUtils";
import { getEmailsReceivedByUser, gotoRoutingLink } from "@calcom/web/playwright/lib/testUtils";

import {
addForm,
Expand Down Expand Up @@ -614,6 +614,118 @@ test.describe("Routing Forms", () => {
})();
});
});

test.describe("Team Routing Form", () => {
test.afterEach(async ({ users }) => {
await users.deleteAll();
});

const createUserAndTeamRoutingForm = async ({
users,
page,
}: {
users: Fixtures["users"];
page: Page;
}) => {
const owner = await users.create(
{ username: "routing-forms" },
{
hasTeam: true,
isOrg: true,
hasSubteam: true,
}
);
const { team } = await owner.getFirstTeamMembership();
await owner.apiLogin();
const formId = await addForm(page, {
forTeam: true,
});
await addShortTextFieldAndSaveForm({
page,
formId,
});
await page.click('[href*="/route-builder/"]');
await selectNewRoute(page);
await selectOption({
selector: {
selector: ".data-testid-select-routing-action",
nth: 0,
},
option: 2,
page,
});
await page.fill("[name=externalRedirectUrl]", "https://cal.com");
await saveCurrentForm(page);
return {
formId,
teamId: team.id,
owner,
};
};

const selectSendMailToAllMembers = async ({ page, formId }: { page: Page; formId: string }) => {
await page.goto(`apps/routing-forms/form-edit/${formId}`);
await page.click('[data-testid="assign-all-team-members-toggle"]');
await saveCurrentForm(page);
};

const addNewUserToTeam = async ({ users, teamId }: { users: Fixtures["users"]; teamId: number }) => {
const newUser = await users.create();
await prisma.membership.create({
data: {
teamId: teamId,
userId: newUser.id,
role: MembershipRole.MEMBER,
accepted: true,
},
});
return newUser;
};

const goToRoutingLinkAndSubmit = async ({ page, formId }: { page: Page; formId: string }) => {
await gotoRoutingLink({ page, formId });
await page.fill('[data-testid="form-field-short-text"]', "test");
page.click('button[type="submit"]');
await page.waitForURL((url) => {
return url.hostname.includes("cal.com");
});
};

test("newly added team member gets mail updates, when `Add all team members, including future members` switch is selected", async ({
page,
emails,
users,
}) => {
const { formId, teamId, owner } = await createUserAndTeamRoutingForm({ users, page });
await selectSendMailToAllMembers({ page, formId });
const newUser = await addNewUserToTeam({ users, teamId });
await goToRoutingLinkAndSubmit({ page, formId });

// eslint-disable-next-line playwright/no-wait-for-timeout
await page.waitForTimeout(2000);
const receivedEmailsOwner = await getEmailsReceivedByUser({ emails, userEmail: owner.email });
expect(receivedEmailsOwner?.total).toBe(1);
const receivedEmailsNewUser = await getEmailsReceivedByUser({ emails, userEmail: newUser.email });
expect(receivedEmailsNewUser?.total).toBe(1);
});

test("newly added team member Not gets mail updates, when `Add all team members, including future members` switch is Not selected", async ({
page,
emails,
users,
}) => {
const { formId, teamId, owner } = await createUserAndTeamRoutingForm({ users, page });
const newUser = await addNewUserToTeam({ users, teamId });
await goToRoutingLinkAndSubmit({ page, formId });

// eslint-disable-next-line playwright/no-wait-for-timeout
await page.waitForTimeout(2000);
const receivedEmailsOwner = await getEmailsReceivedByUser({ emails, userEmail: owner.email });
expect(receivedEmailsOwner?.total).toBe(1);
const receivedEmailsNewUser = await getEmailsReceivedByUser({ emails, userEmail: newUser.email });
expect(receivedEmailsNewUser?.total).toBe(0);
});
});
});

async function disableForm(page: Page) {
Expand Down
13 changes: 6 additions & 7 deletions packages/app-store/routing-forms/trpc/response.handler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -99,14 +99,13 @@ export const responseHandler = async ({ ctx, input }: ResponseHandlerOptions) =>

const settings = RoutingFormSettings.parse(form.settings);
let userWithEmails: string[] = [];
if (form.teamId && settings?.sendUpdatesTo?.length) {
if (form.teamId && (settings?.sendToAll || settings?.sendUpdatesTo?.length)) {
Praashh marked this conversation as resolved.
Show resolved Hide resolved
const whereClause: Prisma.MembershipWhereInput = { teamId: form.teamId };
if (!settings?.sendToAll) {
whereClause.userId = { in: settings.sendUpdatesTo };
}
Comment on lines +103 to +106
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of doing this, shouldn't we find and update sendUpdatesTo to include future members? Why are we currently not adding future members to it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just wanted to avoid having an prisma update request on App_RoutingForms_Form table every time there is a response on the routing Form. (in cases of sendToAll)

If we take UI approach to update sendUpdatesTo when switch is selected in RoutingForm, we may miss on the scenario where - new team member is added after the switch in RoutingForm is selected and the sendUpdatesTo wont have future members until the switch is re-toggled.

Copy link
Contributor Author

@vijayraghav-io vijayraghav-io Dec 19, 2024

Choose a reason for hiding this comment

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

IMO , even if we keep update sendUpdatesTo in sync, there is no advantage from this. Because any how, here in response.handler.ts we need to check for latest new members added if any.
I understand. that's the reason we have an additional variable sendToAll.

In other case, we will be making an unnecessary update operation for each response.

const userEmails = await prisma.membership.findMany({
where: {
teamId: form.teamId,
userId: {
in: settings.sendUpdatesTo,
},
},
where: whereClause,
select: {
user: {
select: {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { ApiProperty as DocsProperty, ApiPropertyOptional } from "@nestjs/swagger";
import { ApiPropertyOptional } from "@nestjs/swagger";
import { Expose } from "class-transformer";
import { IsInt, IsOptional } from "class-validator";

Expand Down
Loading