From 0d6231a02e920164bf5683f11463f1d2d1ea6dc1 Mon Sep 17 00:00:00 2001 From: Horacio Daniel Ros Date: Tue, 6 Oct 2026 11:52:24 -0300 Subject: [PATCH] =?UTF-8?q?feat:=20agregar=20l=C3=B3gica=20para=20evitar?= =?UTF-8?q?=20duplicados=20de=20clientes=20por=20email=20y=20vincular=20us?= =?UTF-8?q?uarios=20existentes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../fixes/consulta clientes duplicados.txt | 8 + .../Clients/Clients.Adapter.Mongoose.ts | 8 + server/src/Models/Clients/Clients.ts | 62 +++- .../Clients.createClientByUser.test.ts | 284 ++++++++++++++++++ 4 files changed, 351 insertions(+), 11 deletions(-) create mode 100644 resources/fixes/consulta clientes duplicados.txt create mode 100644 server/src/Models/Clients/__tests__/Clients.createClientByUser.test.ts diff --git a/resources/fixes/consulta clientes duplicados.txt b/resources/fixes/consulta clientes duplicados.txt new file mode 100644 index 0000000..23724eb --- /dev/null +++ b/resources/fixes/consulta clientes duplicados.txt @@ -0,0 +1,8 @@ +db.clients.aggregate([ + { $group: { + _id: { companyId: "$companyId", email: { $toLower: "$email" } }, + count: { $sum: 1 }, + emails: { $addToSet: "$email" } + } }, + { $match: { count: { $gt: 1 }, $expr: { $gt: [{ $size: "$emails" }, 1] } } } +]) \ No newline at end of file diff --git a/server/src/Models/Clients/Clients.Adapter.Mongoose.ts b/server/src/Models/Clients/Clients.Adapter.Mongoose.ts index 96fa20c..8e0638e 100644 --- a/server/src/Models/Clients/Clients.Adapter.Mongoose.ts +++ b/server/src/Models/Clients/Clients.Adapter.Mongoose.ts @@ -32,6 +32,14 @@ export class ClientsAdapterMongoose implements IClientsAdapter { banned: { type: Boolean, required: false, default: false }, }); + //Unicidad por email dentro de la organización. La unicidad por usuario es + //parcial: excluye los clientes cuyo userId fue removido con $unset. + this.schema.index({ companyId: 1, email: 1 }, { unique: true }); + this.schema.index( + { companyId: 1, userId: 1 }, + { unique: true, partialFilterExpression: { userId: { $type: "oid" } } } + ); + this.clientList = model("Client", this.schema); } diff --git a/server/src/Models/Clients/Clients.ts b/server/src/Models/Clients/Clients.ts index b3d607b..c8b8002 100644 --- a/server/src/Models/Clients/Clients.ts +++ b/server/src/Models/Clients/Clients.ts @@ -266,6 +266,27 @@ class ClientManager implements IClientsManager { throw new Error("El cliente ya pertenece a la compañia"); } + //Si la organización ya tiene un cliente con el email del usuario, se + //vincula el usuario a ese cliente en lugar de crear un duplicado. + const existingByEmail = await this.clients.findOne({ + companyId: data.companyId, + email: userCheck.email, + }); + + if (existingByEmail) { + existingByEmail.userId = data.userId; + await existingByEmail.save(); + + await NotificationsManager.sendSystemNotification({ + userId: String(userCheck._id), + subject: "Ha sido dado de alta como cliente", + message: `Su usuario ha sido dado de alta como cliente en la organización ${companyCheck.name}`, + type: NotificationType.BILLING + }); + + return existingByEmail; + } + const canAdd = await MetricsList.canAddClient(companyCheck.ownerId); if (!canAdd) { @@ -274,17 +295,36 @@ class ClientManager implements IClientsManager { ); } - const newClient = await this.clients.create({ - ...data, - ...{ - email: userCheck.email, - firstName: userCheck.firstName, - lastName: userCheck.lastName, - phoneAreaCode: userCheck.phoneAreaCode, - phoneCountryCode: userCheck.phoneCountryCode, - phoneNumber: userCheck.phoneNumber, - }, - }); + let newClient: IClientDocument; + + try { + newClient = await this.clients.create({ + ...data, + ...{ + email: userCheck.email, + firstName: userCheck.firstName, + lastName: userCheck.lastName, + phoneAreaCode: userCheck.phoneAreaCode, + phoneCountryCode: userCheck.phoneCountryCode, + phoneNumber: userCheck.phoneNumber, + }, + }); + } catch (error) { + //Carrera de concurrencia: el índice único rechazó el duplicado y la + //petición ganadora ya creó el cliente, lo contabilizó y notificó. + if ((error as { code?: number })?.code === 11000) { + const racedClient = await this.clients.findOne({ + companyId: data.companyId, + userId: data.userId, + }); + + if (racedClient) { + return racedClient; + } + } + + throw error; + } await MetricsList.addClient({ userId: String(companyCheck.ownerId), diff --git a/server/src/Models/Clients/__tests__/Clients.createClientByUser.test.ts b/server/src/Models/Clients/__tests__/Clients.createClientByUser.test.ts new file mode 100644 index 0000000..4ee1569 --- /dev/null +++ b/server/src/Models/Clients/__tests__/Clients.createClientByUser.test.ts @@ -0,0 +1,284 @@ +// Unit tests for ClientManager.createClientByUser: adopt-by-email path, create +// path, the existing userId guard, and the E11000 concurrent-create race. + +// Mock all external dependencies before importing the module under test +jest.mock("../../Users/Users", () => ({ + __esModule: true, + default: { + users: { + findOne: jest.fn(), + }, + }, +})); + +jest.mock("../../Companies/Companies", () => ({ + __esModule: true, + default: { + companies: { + findOne: jest.fn(), + }, + }, +})); + +jest.mock("../../Employees/Employee", () => ({ + __esModule: true, + default: { + checkPermission: jest.fn(), + }, +})); + +jest.mock("../../Metrics/Metrics", () => ({ + __esModule: true, + default: { + canAddClient: jest.fn(), + addClient: jest.fn(), + }, +})); + +jest.mock("../../Notifications/Notifications", () => ({ + __esModule: true, + NotificationsManager: { + sendSystemNotification: jest.fn(), + }, +})); + +jest.mock("../../Appointments/Appointments", () => ({ + __esModule: true, + default: { + Appointments: { + find: jest.fn().mockResolvedValue([]), + findOne: jest.fn(), + delete: jest.fn(), + }, + }, +})); + +jest.mock("../../Repeats/Repeats", () => ({ + __esModule: true, + default: { + deleteRepeatsByClient: jest.fn(), + }, +})); + +jest.mock("../../ClientAccounts/ClientAccount", () => ({ + __esModule: true, + default: { + getAvailableAmount: jest.fn(), + deleteClientAccount: jest.fn(), + }, +})); + +jest.mock("../../Payments/Payments", () => ({ + __esModule: true, + default: { + deletePaymentsByClient: jest.fn(), + }, +})); + +import ClientList from "../Clients"; +import UsersManager from "../../Users/Users"; +import CompaniesManager from "../../Companies/Companies"; +import MetricsList from "../../Metrics/Metrics"; +import { NotificationsManager } from "../../Notifications/Notifications"; +import { NotificationType } from "../../SystemNotifications/SystemNotification.Interface"; + +const request = { + companyId: "company-001", + userId: "user-001", + sessionUser: "session-user", +}; + +const sessionUser = { _id: "session-user" }; + +const company = { + _id: "company-001", + ownerId: "owner-user", + name: "Acme", +}; + +const user = { + _id: "user-001", + email: "client@example.com", + firstName: "John", + lastName: "Doe", + phoneAreaCode: "11", + phoneCountryCode: "54", + phoneNumber: "12345678", +}; + +// Helper to build a mock client document with save() +function buildClientDoc(overrides: Record = {}): Record { + return { + _id: "client-001", + id: "client-001", + companyId: "company-001", + email: "client@example.com", + firstName: "Admin Entered Name", + lastName: "Admin Entered Lastname", + save: jest.fn().mockResolvedValue(true), + ...overrides, + }; +} + +describe("createClientByUser", () => { + // Keep the real adapter to restore it after each test and to inspect indexes + const realAdapter = ClientList.clients; + let mockFindOne: jest.Mock; + let mockCreate: jest.Mock; + + beforeEach(() => { + jest.clearAllMocks(); + + mockFindOne = jest.fn(); + mockCreate = jest.fn(); + + (ClientList as any).clients = { + findOne: mockFindOne, + create: mockCreate, + }; + + // First call: session user validation. Second call: target user lookup. + (UsersManager.users.findOne as jest.Mock) + .mockResolvedValueOnce(sessionUser) + .mockResolvedValueOnce(user); + + (CompaniesManager.companies.findOne as jest.Mock).mockResolvedValue(company); + }); + + afterEach(() => { + (ClientList as any).clients = realAdapter; + }); + + // --- Adopt path: existing client by email --- + + it("adopts the existing client found by email instead of creating a duplicate", async () => { + const existingClient = buildClientDoc(); + + mockFindOne + .mockResolvedValueOnce(null) // guard by companyId + userId + .mockResolvedValueOnce(existingClient); // lookup by companyId + email + + const result = await ClientList.createClientByUser(request); + + expect(mockCreate).not.toHaveBeenCalled(); + expect(existingClient.userId).toBe("user-001"); + expect(existingClient.save).toHaveBeenCalled(); + // Admin-entered data must not be overwritten by the adopt path + expect(existingClient.firstName).toBe("Admin Entered Name"); + expect(existingClient.lastName).toBe("Admin Entered Lastname"); + // The client was already counted, so metrics are untouched + expect(MetricsList.canAddClient).not.toHaveBeenCalled(); + expect(MetricsList.addClient).not.toHaveBeenCalled(); + expect(NotificationsManager.sendSystemNotification).toHaveBeenCalledWith({ + userId: "user-001", + subject: "Ha sido dado de alta como cliente", + message: "Su usuario ha sido dado de alta como cliente en la organización Acme", + type: NotificationType.BILLING, + }); + expect(result).toBe(existingClient); + }); + + // --- Create path: no client by userId nor email --- + + it("creates a new client when no client exists by userId nor email", async () => { + const newClient = buildClientDoc({ userId: "user-001" }); + + mockFindOne + .mockResolvedValueOnce(null) // guard by companyId + userId + .mockResolvedValueOnce(null); // lookup by companyId + email + + (MetricsList.canAddClient as jest.Mock).mockResolvedValue(true); + mockCreate.mockResolvedValue(newClient); + + const result = await ClientList.createClientByUser(request); + + expect(mockCreate).toHaveBeenCalledWith({ + companyId: "company-001", + userId: "user-001", + sessionUser: "session-user", + email: user.email, + firstName: user.firstName, + lastName: user.lastName, + phoneAreaCode: user.phoneAreaCode, + phoneCountryCode: user.phoneCountryCode, + phoneNumber: user.phoneNumber, + }); + expect(MetricsList.addClient).toHaveBeenCalledWith({ + userId: "owner-user", + quantity: 1, + }); + expect(NotificationsManager.sendSystemNotification).toHaveBeenCalledWith({ + userId: "user-001", + subject: "Ha sido dado de alta como cliente", + message: "Su usuario ha sido dado de alta como cliente en la organización Acme", + type: NotificationType.BILLING, + }); + expect(result).toBe(newClient); + }); + + // --- Existing guard preserved --- + + it("throws when the client already belongs to the company", async () => { + mockFindOne.mockResolvedValueOnce(buildClientDoc()); // guard by companyId + userId + + await expect(ClientList.createClientByUser(request)).rejects.toThrow( + /ya pertenece a la compan/ + ); + + expect(mockCreate).not.toHaveBeenCalled(); + expect(NotificationsManager.sendSystemNotification).not.toHaveBeenCalled(); + }); + + // --- E11000 concurrent-create race --- + + it("returns the winning client when create hits the unique index (E11000)", async () => { + const racedClient = buildClientDoc({ userId: "user-001" }); + + mockFindOne + .mockResolvedValueOnce(null) // guard by companyId + userId + .mockResolvedValueOnce(null) // lookup by companyId + email + .mockResolvedValueOnce(racedClient); // re-fetch after E11000 + + (MetricsList.canAddClient as jest.Mock).mockResolvedValue(true); + mockCreate.mockRejectedValue({ code: 11000 }); + + const result = await ClientList.createClientByUser(request); + + expect(result).toBe(racedClient); + // The winning request already counted and notified + expect(MetricsList.addClient).not.toHaveBeenCalled(); + expect(NotificationsManager.sendSystemNotification).not.toHaveBeenCalled(); + }); + + it("rethrows non-duplicate-key errors from create", async () => { + mockFindOne + .mockResolvedValueOnce(null) // guard by companyId + userId + .mockResolvedValueOnce(null); // lookup by companyId + email + + (MetricsList.canAddClient as jest.Mock).mockResolvedValue(true); + mockCreate.mockRejectedValue(new Error("database down")); + + await expect(ClientList.createClientByUser(request)).rejects.toThrow("database down"); + }); + + // --- Schema indexes --- + + it("declares unique indexes for companyId+email and companyId+userId", () => { + const indexes = realAdapter.schema.indexes(); + + const emailIndex = indexes.find( + ([fields]) => fields.companyId === 1 && fields.email === 1 + ); + expect(emailIndex).toBeDefined(); + expect(emailIndex?.[1]).toMatchObject({ unique: true }); + + const userIdIndex = indexes.find( + ([fields]) => fields.companyId === 1 && fields.userId === 1 + ); + expect(userIdIndex).toBeDefined(); + expect(userIdIndex?.[1]).toMatchObject({ + unique: true, + partialFilterExpression: { userId: { $type: "oid" } }, + }); + }); +});