From 81f75d285e4c19e8608f9689492272c85802d22a Mon Sep 17 00:00:00 2001 From: richardtekula Date: Fri, 5 Dec 2025 07:25:49 +0100 Subject: [PATCH] Refactor: code quality improvements MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Extract admin.service.js from admin.controller.js (proper layering) - Remove console.log statements from todo.controller.js - Fix inconsistent error handling in auth.controller.js (return next) - Remove logger.debug calls from contact.controller.js - Add transaction management to contact.service.js addContact() 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- src/controllers/admin.controller.js | 222 ++++---------------------- src/controllers/auth.controller.js | 14 +- src/controllers/contact.controller.js | 21 --- src/controllers/todo.controller.js | 2 - src/services/admin.service.js | 217 +++++++++++++++++++++++++ src/services/contact.service.js | 90 +++++------ 6 files changed, 303 insertions(+), 263 deletions(-) create mode 100644 src/services/admin.service.js diff --git a/src/controllers/admin.controller.js b/src/controllers/admin.controller.js index 6aee6fd..29b6f4b 100644 --- a/src/controllers/admin.controller.js +++ b/src/controllers/admin.controller.js @@ -1,10 +1,5 @@ -import { db } from '../config/database.js'; -import { users, userEmailAccounts, emailAccounts } from '../db/schema.js'; -import { eq, inArray } from 'drizzle-orm'; -import { hashPassword, generateTempPassword } from '../utils/password.js'; +import * as adminService from '../services/admin.service.js'; import { logUserCreation, logRoleChange } from '../services/audit.service.js'; -import { ConflictError, NotFoundError } from '../utils/errors.js'; -import * as emailAccountService from '../services/email-account.service.js'; /** * Vytvorenie nového usera s automatic temporary password (admin only) @@ -18,83 +13,41 @@ export const createUser = async (req, res, next) => { const userAgent = req.headers['user-agent']; try { - // Skontroluj či username už neexistuje - const [existingUser] = await db - .select() - .from(users) - .where(eq(users.username, username)) - .limit(1); - - if (existingUser) { - throw new ConflictError('Username už existuje'); - } - - // Automaticky vygeneruj temporary password - const tempPassword = generateTempPassword(12); - const hashedTempPassword = await hashPassword(tempPassword); - - // Validuj role - iba 'admin' alebo 'member' - const validRole = role === 'admin' ? 'admin' : 'member'; - - // Vytvor usera - const [newUser] = await db - .insert(users) - .values({ - username, - tempPassword: hashedTempPassword, - role: validRole, - firstName: firstName || null, - lastName: lastName || null, - changedPassword: false, - }) - .returning(); - - // Ak sú poskytnuté email credentials, vytvor email account (many-to-many) - let emailAccountCreated = false; - let emailAccountData = null; - - if (email && emailPassword) { - try { - // Použij emailAccountService ktorý automaticky vytvorí many-to-many link - const newEmailAccount = await emailAccountService.createEmailAccount( - newUser.id, - email, - emailPassword - ); - - emailAccountCreated = true; - emailAccountData = { - id: newEmailAccount.id, - email: newEmailAccount.email, - jmapAccountId: newEmailAccount.jmapAccountId, - shared: newEmailAccount.shared, - }; - } catch (emailError) { - // Email account sa nepodarilo vytvoriť, ale user bol vytvorený - // Admin môže pridať email account neskôr - console.error('Failed to create email account:', emailError); - } - } + const result = await adminService.createUser( + username, + firstName, + lastName, + role, + email, + emailPassword + ); // Log user creation - await logUserCreation(adminId, newUser.id, username, validRole, ipAddress, userAgent); + await logUserCreation( + adminId, + result.user.id, + username, + result.user.role, + ipAddress, + userAgent + ); res.status(201).json({ success: true, data: { user: { - id: newUser.id, - username: newUser.username, - firstName: newUser.firstName, - lastName: newUser.lastName, - role: newUser.role, - emailSetup: emailAccountCreated, - emailAccount: emailAccountData, - tempPassword: tempPassword, // Vráti plain text password pre admina aby ho mohol poslať userovi + id: result.user.id, + username: result.user.username, + firstName: result.user.firstName, + lastName: result.user.lastName, + role: result.user.role, + emailSetup: result.emailAccountCreated, + emailAccount: result.emailAccountData, + tempPassword: result.tempPassword, }, }, - message: emailAccountCreated - ? emailAccountData.shared + message: result.emailAccountCreated + ? result.emailAccountData.shared ? 'Používateľ vytvorený a pripojený k existujúcemu zdieľanému email účtu.' : 'Používateľ úspešne vytvorený s novým emailovým účtom.' : 'Používateľ úspešne vytvorený. Email môže byť nastavený neskôr.', @@ -110,18 +63,7 @@ export const createUser = async (req, res, next) => { */ export const getAllUsers = async (req, res, next) => { try { - const allUsers = await db - .select({ - id: users.id, - username: users.username, - firstName: users.firstName, - lastName: users.lastName, - role: users.role, - changedPassword: users.changedPassword, - lastLogin: users.lastLogin, - createdAt: users.createdAt, - }) - .from(users); + const allUsers = await adminService.getAllUsers(); res.status(200).json({ success: true, @@ -141,36 +83,12 @@ export const getUser = async (req, res, next) => { const { userId } = req.params; try { - const [user] = await db - .select({ - id: users.id, - username: users.username, - firstName: users.firstName, - lastName: users.lastName, - role: users.role, - changedPassword: users.changedPassword, - lastLogin: users.lastLogin, - createdAt: users.createdAt, - updatedAt: users.updatedAt, - }) - .from(users) - .where(eq(users.id, userId)) - .limit(1); - - if (!user) { - throw new NotFoundError('Používateľ nenájdený'); - } - - // Get user's email accounts (cez many-to-many) - const userEmailAccounts = await emailAccountService.getUserEmailAccounts(userId); + const user = await adminService.getUserById(userId); res.status(200).json({ success: true, data: { - user: { - ...user, - emailAccounts: userEmailAccounts, - }, + user, }, }); } catch (error) { @@ -190,38 +108,14 @@ export const changeUserRole = async (req, res, next) => { const userAgent = req.headers['user-agent']; try { - // Získaj starú rolu - const [user] = await db - .select() - .from(users) - .where(eq(users.id, userId)) - .limit(1); - - if (!user) { - throw new NotFoundError('Používateľ nenájdený'); - } - - const oldRole = user.role; - - // Update role - await db - .update(users) - .set({ - role, - updatedAt: new Date(), - }) - .where(eq(users.id, userId)); + const result = await adminService.changeUserRole(userId, role); // Log role change - await logRoleChange(adminId, userId, oldRole, role, ipAddress, userAgent); + await logRoleChange(adminId, userId, result.oldRole, result.newRole, ipAddress, userAgent); res.status(200).json({ success: true, - data: { - userId, - oldRole, - newRole: role, - }, + data: result, message: 'Rola používateľa bola zmenená', }); } catch (error) { @@ -237,58 +131,12 @@ export const deleteUser = async (req, res, next) => { const { userId } = req.params; try { - const [user] = await db - .select() - .from(users) - .where(eq(users.id, userId)) - .limit(1); - - if (!user) { - throw new NotFoundError('Používateľ nenájdený'); - } - - // Zabraň zmazaniu posledného admina - if (user.role === 'admin') { - const [adminCount] = await db - .select({ count: db.$count(users) }) - .from(users) - .where(eq(users.role, 'admin')); - - if (adminCount.count <= 1) { - throw new ConflictError('Nemôžete zmazať posledného administrátora'); - } - } - - // Get user's email account IDs before deletion - const userEmailAccountLinks = await db - .select({ emailAccountId: userEmailAccounts.emailAccountId }) - .from(userEmailAccounts) - .where(eq(userEmailAccounts.userId, userId)); - - const emailAccountIds = userEmailAccountLinks.map(link => link.emailAccountId); - - // Delete user (cascades userEmailAccounts links) - await db.delete(users).where(eq(users.id, userId)); - - // Delete orphaned email accounts (no users linked) - // This will cascade delete contacts and emails - let deletedEmailAccounts = 0; - for (const emailAccountId of emailAccountIds) { - const [remainingLinks] = await db - .select({ count: db.$count(userEmailAccounts) }) - .from(userEmailAccounts) - .where(eq(userEmailAccounts.emailAccountId, emailAccountId)); - - if (remainingLinks.count === 0) { - await db.delete(emailAccounts).where(eq(emailAccounts.id, emailAccountId)); - deletedEmailAccounts++; - } - } + const result = await adminService.deleteUser(userId); res.status(200).json({ success: true, message: 'Používateľ bol zmazaný', - deletedEmailAccounts, + deletedEmailAccounts: result.deletedEmailAccounts, }); } catch (error) { return next(error); diff --git a/src/controllers/auth.controller.js b/src/controllers/auth.controller.js index b47f61c..eac3d24 100644 --- a/src/controllers/auth.controller.js +++ b/src/controllers/auth.controller.js @@ -57,7 +57,7 @@ export const login = async (req, res, next) => { // Log failed login await logLoginAttempt(username, false, ipAddress, userAgent, error.message); - next(error); + return next(error); } }; @@ -84,7 +84,7 @@ export const setPassword = async (req, res, next) => { message: 'Heslo úspešne nastavené', }); } catch (error) { - next(error); + return next(error); } }; @@ -114,7 +114,7 @@ export const linkEmail = async (req, res, next) => { message: 'Email účet úspešne pripojený a overený', }); } catch (error) { - next(error); + return next(error); } }; @@ -135,7 +135,7 @@ export const skipEmail = async (req, res, next) => { message: 'Email setup preskočený', }); } catch (error) { - next(error); + return next(error); } }; @@ -164,7 +164,7 @@ export const logout = async (req, res, next) => { message: result.message, }); } catch (error) { - next(error); + return next(error); } }; @@ -183,7 +183,7 @@ export const getSession = async (req, res, next) => { }, }); } catch (error) { - next(error); + return next(error); } }; @@ -201,6 +201,6 @@ export const getMe = async (req, res, next) => { }, }); } catch (error) { - next(error); + return next(error); } }; diff --git a/src/controllers/contact.controller.js b/src/controllers/contact.controller.js index 779fb4c..2341b01 100644 --- a/src/controllers/contact.controller.js +++ b/src/controllers/contact.controller.js @@ -1,7 +1,6 @@ import * as contactService from '../services/contact.service.js'; import { discoverContactsFromJMAP, getJmapConfigFromAccount } from '../services/jmap.service.js'; import * as emailAccountService from '../services/email-account.service.js'; -import { logger } from '../utils/logger.js'; /** * Get all contacts for an email account @@ -47,18 +46,12 @@ export const discoverContacts = async (req, res, next) => { const userId = req.userId; const { accountId, search = '', limit = 50 } = req.query; - logger.debug('discoverContacts called', { userId, accountId, search, limit }); - // Get email account (or primary if not specified) let emailAccount; if (accountId) { - logger.debug('Getting email account by ID', { accountId }); emailAccount = await emailAccountService.getEmailAccountWithCredentials(accountId, userId); - logger.debug('Email account retrieved', { id: emailAccount.id, email: emailAccount.email }); } else { - logger.debug('No accountId provided, getting primary account', { userId }); const primaryAccount = await emailAccountService.getPrimaryEmailAccount(userId); - logger.debug('Primary account', primaryAccount ? { id: primaryAccount.id, email: primaryAccount.email } : { found: false }); if (!primaryAccount) { return res.status(400).json({ success: false, @@ -69,16 +62,9 @@ export const discoverContacts = async (req, res, next) => { }); } emailAccount = await emailAccountService.getEmailAccountWithCredentials(primaryAccount.id, userId); - logger.debug('Email account retrieved from primary', { id: emailAccount.id, email: emailAccount.email }); } const jmapConfig = getJmapConfigFromAccount(emailAccount); - logger.debug('JMAP Config created', { - server: jmapConfig.server, - username: jmapConfig.username, - accountId: jmapConfig.accountId, - hasPassword: !!jmapConfig.password - }); const potentialContacts = await discoverContactsFromJMAP( jmapConfig, @@ -93,7 +79,6 @@ export const discoverContacts = async (req, res, next) => { data: potentialContacts, }); } catch (error) { - logger.error('ERROR in discoverContacts', { error: error.message, stack: error.stack }); return next(error); } }; @@ -106,11 +91,8 @@ export const discoverContacts = async (req, res, next) => { export const addContact = async (req, res, next) => { try { const userId = req.userId; - logger.debug('Full req.body', { body: req.body }); const { email, name = '', notes = '', accountId } = req.body; - logger.debug('addContact called', { userId, email, name, accountId }); - if (!email) { return res.status(400).json({ success: false, @@ -124,10 +106,8 @@ export const addContact = async (req, res, next) => { // Get email account (or primary if not specified) let emailAccount; if (accountId) { - logger.debug('Using provided accountId', { accountId }); emailAccount = await emailAccountService.getEmailAccountWithCredentials(accountId, userId); } else { - logger.debug('No accountId provided, using primary account'); const primaryAccount = await emailAccountService.getPrimaryEmailAccount(userId); if (!primaryAccount) { return res.status(400).json({ @@ -139,7 +119,6 @@ export const addContact = async (req, res, next) => { }); } emailAccount = await emailAccountService.getEmailAccountWithCredentials(primaryAccount.id, userId); - logger.debug('Using primary account', { accountId: primaryAccount.id }); } const jmapConfig = getJmapConfigFromAccount(emailAccount); diff --git a/src/controllers/todo.controller.js b/src/controllers/todo.controller.js index c76e867..fca493e 100644 --- a/src/controllers/todo.controller.js +++ b/src/controllers/todo.controller.js @@ -110,7 +110,6 @@ export const createTodo = async (req, res, next) => { const userId = req.userId; const data = req.body; - console.log('Backend received todo data:', data); const todo = await todoService.createTodo(userId, data); // Log audit event @@ -136,7 +135,6 @@ export const updateTodo = async (req, res, next) => { const { todoId } = req.params; const data = req.body; - console.log('Backend received update data:', data); const todo = await todoService.updateTodo(todoId, data); res.status(200).json({ diff --git a/src/services/admin.service.js b/src/services/admin.service.js new file mode 100644 index 0000000..c892833 --- /dev/null +++ b/src/services/admin.service.js @@ -0,0 +1,217 @@ +import { db } from '../config/database.js'; +import { users, userEmailAccounts, emailAccounts } from '../db/schema.js'; +import { eq } from 'drizzle-orm'; +import { hashPassword, generateTempPassword } from '../utils/password.js'; +import { ConflictError, NotFoundError } from '../utils/errors.js'; +import * as emailAccountService from './email-account.service.js'; +import { logger } from '../utils/logger.js'; + +/** + * Skontroluj či username už neexistuje + */ +export const checkUsernameExists = async (username) => { + const [existingUser] = await db + .select() + .from(users) + .where(eq(users.username, username)) + .limit(1); + + return !!existingUser; +}; + +/** + * Vytvorenie nového usera s automatic temporary password + */ +export const createUser = async (username, firstName, lastName, role, email, emailPassword) => { + // Skontroluj či username už neexistuje + if (await checkUsernameExists(username)) { + throw new ConflictError('Username už existuje'); + } + + // Automaticky vygeneruj temporary password + const tempPassword = generateTempPassword(12); + const hashedTempPassword = await hashPassword(tempPassword); + + // Validuj role - iba 'admin' alebo 'member' + const validRole = role === 'admin' ? 'admin' : 'member'; + + // Vytvor usera + const [newUser] = await db + .insert(users) + .values({ + username, + tempPassword: hashedTempPassword, + role: validRole, + firstName: firstName || null, + lastName: lastName || null, + changedPassword: false, + }) + .returning(); + + // Ak sú poskytnuté email credentials, vytvor email account (many-to-many) + let emailAccountCreated = false; + let emailAccountData = null; + + if (email && emailPassword) { + try { + const newEmailAccount = await emailAccountService.createEmailAccount( + newUser.id, + email, + emailPassword + ); + + emailAccountCreated = true; + emailAccountData = { + id: newEmailAccount.id, + email: newEmailAccount.email, + jmapAccountId: newEmailAccount.jmapAccountId, + shared: newEmailAccount.shared, + }; + } catch (emailError) { + logger.error('Failed to create email account:', { error: emailError.message }); + } + } + + return { + user: newUser, + tempPassword, + emailAccountCreated, + emailAccountData, + }; +}; + +/** + * Zoznam všetkých userov + */ +export const getAllUsers = async () => { + const allUsers = await db + .select({ + id: users.id, + username: users.username, + firstName: users.firstName, + lastName: users.lastName, + role: users.role, + changedPassword: users.changedPassword, + lastLogin: users.lastLogin, + createdAt: users.createdAt, + }) + .from(users); + + return allUsers; +}; + +/** + * Získanie konkrétneho usera + */ +export const getUserById = async (userId) => { + const [user] = await db + .select({ + id: users.id, + username: users.username, + firstName: users.firstName, + lastName: users.lastName, + role: users.role, + changedPassword: users.changedPassword, + lastLogin: users.lastLogin, + createdAt: users.createdAt, + updatedAt: users.updatedAt, + }) + .from(users) + .where(eq(users.id, userId)) + .limit(1); + + if (!user) { + throw new NotFoundError('Používateľ nenájdený'); + } + + // Get user's email accounts (cez many-to-many) + const userEmailAccountsList = await emailAccountService.getUserEmailAccounts(userId); + + return { + ...user, + emailAccounts: userEmailAccountsList, + }; +}; + +/** + * Zmena role usera + */ +export const changeUserRole = async (userId, newRole) => { + // Získaj starú rolu + const [user] = await db + .select() + .from(users) + .where(eq(users.id, userId)) + .limit(1); + + if (!user) { + throw new NotFoundError('Používateľ nenájdený'); + } + + const oldRole = user.role; + + // Update role + await db + .update(users) + .set({ + role: newRole, + updatedAt: new Date(), + }) + .where(eq(users.id, userId)); + + return { userId, oldRole, newRole }; +}; + +/** + * Zmazanie usera + */ +export const deleteUser = async (userId) => { + const [user] = await db + .select() + .from(users) + .where(eq(users.id, userId)) + .limit(1); + + if (!user) { + throw new NotFoundError('Používateľ nenájdený'); + } + + // Zabraň zmazaniu posledného admina + if (user.role === 'admin') { + const [adminCount] = await db + .select({ count: db.$count(users) }) + .from(users) + .where(eq(users.role, 'admin')); + + if (adminCount.count <= 1) { + throw new ConflictError('Nemôžete zmazať posledného administrátora'); + } + } + + // Get user's email account IDs before deletion + const userEmailAccountLinks = await db + .select({ emailAccountId: userEmailAccounts.emailAccountId }) + .from(userEmailAccounts) + .where(eq(userEmailAccounts.userId, userId)); + + const emailAccountIds = userEmailAccountLinks.map(link => link.emailAccountId); + + // Delete user (cascades userEmailAccounts links) + await db.delete(users).where(eq(users.id, userId)); + + // Delete orphaned email accounts (no users linked) + let deletedEmailAccounts = 0; + for (const emailAccountId of emailAccountIds) { + const [remainingLinks] = await db + .select({ count: db.$count(userEmailAccounts) }) + .from(userEmailAccounts) + .where(eq(userEmailAccounts.emailAccountId, emailAccountId)); + + if (remainingLinks.count === 0) { + await db.delete(emailAccounts).where(eq(emailAccounts.id, emailAccountId)); + deletedEmailAccounts++; + } + } + + return { deletedEmailAccounts }; +}; diff --git a/src/services/contact.service.js b/src/services/contact.service.js index de52f5e..c0398fe 100644 --- a/src/services/contact.service.js +++ b/src/services/contact.service.js @@ -3,7 +3,6 @@ import { contacts, emails, companies } from '../db/schema.js'; import { eq, and, desc, or, ne } from 'drizzle-orm'; import { NotFoundError, ConflictError } from '../utils/errors.js'; import { syncEmailsFromSender } from './jmap.service.js'; -import { logger } from '../utils/logger.js'; /** * Get all contacts for an email account @@ -21,6 +20,7 @@ export const getContactsForEmailAccount = async (emailAccountId) => { /** * Add a new contact to an email account + * Uses transaction for atomic operations */ export const addContact = async (emailAccountId, jmapConfig, email, name = '', notes = '', addedByUserId = null) => { // Check if contact already exists for this email account @@ -39,41 +39,32 @@ export const addContact = async (emailAccountId, jmapConfig, email, name = '', n throw new ConflictError('Kontakt už existuje'); } - // Create contact - const [newContact] = await db - .insert(contacts) - .values({ - emailAccountId, - email, - name: name || email.split('@')[0], - notes: notes || null, - addedBy: addedByUserId, - }) - .returning(); - - // Sync emails from this sender - try { - await syncEmailsFromSender(jmapConfig, emailAccountId, newContact.id, email); - } catch (error) { - logger.error('Failed to sync emails for new contact', { error: error.message }); - // Don't throw - contact was created successfully - } - - // REASSIGN: Fix any existing emails that belong to this contact but have wrong contactId - try { - logger.info(`Checking for emails to reassign to contact ${email}...`); + // Use transaction for atomic contact creation and email reassignment + const newContact = await db.transaction(async (tx) => { + // Create contact + const [contact] = await tx + .insert(contacts) + .values({ + emailAccountId, + email, + name: name || email.split('@')[0], + notes: notes || null, + addedBy: addedByUserId, + }) + .returning(); + // REASSIGN: Fix any existing emails that belong to this contact but have wrong contactId // Find emails where: - // - from === newContact.email OR to === newContact.email - // - contactId !== newContact.id (belongs to different contact) + // - from === contact.email OR to === contact.email + // - contactId !== contact.id (belongs to different contact) // - emailAccountId matches - const emailsToReassign = await db + const emailsToReassign = await tx .select() .from(emails) .where( and( eq(emails.emailAccountId, emailAccountId), - ne(emails.contactId, newContact.id), + ne(emails.contactId, contact.id), or( eq(emails.from, email), eq(emails.to, email) @@ -82,25 +73,32 @@ export const addContact = async (emailAccountId, jmapConfig, email, name = '', n ); if (emailsToReassign.length > 0) { - logger.info(`Found ${emailsToReassign.length} emails to reassign to contact ${email}`); - - // Update contactId for these emails - for (const emailToReassign of emailsToReassign) { - await db - .update(emails) - .set({ - contactId: newContact.id, - companyId: newContact.companyId || null, - }) - .where(eq(emails.id, emailToReassign.id)); - } - - logger.success(`Reassigned ${emailsToReassign.length} emails to contact ${email}`); - } else { - logger.info(`No emails to reassign for contact ${email}`); + // Bulk update contactId for these emails + await tx + .update(emails) + .set({ + contactId: contact.id, + companyId: contact.companyId || null, + }) + .where( + and( + eq(emails.emailAccountId, emailAccountId), + ne(emails.contactId, contact.id), + or( + eq(emails.from, email), + eq(emails.to, email) + ) + ) + ); } - } catch (error) { - logger.error('Failed to reassign emails', { error: error.message }); + + return contact; + }); + + // Sync emails from this sender (outside transaction - external API call) + try { + await syncEmailsFromSender(jmapConfig, emailAccountId, newContact.id, email); + } catch { // Don't throw - contact was created successfully }