fix: code review — security, dead code, performance, consistency
Critical fixes: - XSS: escapeHtml() on all user-supplied text in email notifications - Budget PATCH: added mutex lock + availability validation (prevents corruption) - batchResolveNames: fixed wrong signature for budget request earmark names Dead code cleanup: - Deleted 8 unused PostComposition* files (replaced by PostDetail full page) Performance: - budget-helpers: single-fetch with computeFromEntries(), optional prefetch param - post-composition: parallelized text + thumbnail fetches with Promise.all Consistency: - PostDetail.jsx: native <select> → PortalSelect (matches all panels) - Finance.jsx: 11 hardcoded English table headers → t() with i18n keys - PostCalendar.jsx: day names, Month/Week labels → t() with i18n keys - App.jsx Suspense: "Loading..." → brand spinner (can't use i18n in fallback) - UploadZone: proper useRef pattern, no vanilla JS document.createElement - All file inputs: className="hidden" → absolute w-0 h-0 opacity-0 - ArtefactDetailPanel: removed campaign/project selects (inherited from post) - TranslationDetailPanel: removed brand/linked post selects (inherited from post) - ApproverMultiSelect: portal-based dropdown (fixes clipping in modals) - Thumbnail fix: post-composition constructs URL from filename (was undefined) - Upload fix: UploadZone with drag-and-drop for design + video artefacts Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
fahed
+18
-16
@@ -3,27 +3,23 @@
|
||||
|
||||
const nocodb = require('./nocodb');
|
||||
|
||||
async function getMainAvailable() {
|
||||
const entries = await nocodb.list('BudgetEntries', { limit: 10000 });
|
||||
function computeFromEntries(entries) {
|
||||
const income = entries.filter(e => (e.type || 'income') === 'income');
|
||||
const expenses = entries.filter(e => e.type === 'expense');
|
||||
|
||||
const totalReceived = income.reduce((s, e) => s + (e.amount || 0), 0);
|
||||
const totalExpenses = expenses.reduce((s, e) => s + (e.amount || 0), 0);
|
||||
const totalCampaignBudget = income.filter(e => e.campaign_id).reduce((s, e) => s + (e.amount || 0), 0);
|
||||
const totalProjectBudget = income.filter(e => e.project_id).reduce((s, e) => s + (e.amount || 0), 0);
|
||||
|
||||
return {
|
||||
totalReceived,
|
||||
totalExpenses,
|
||||
totalCampaignBudget,
|
||||
totalProjectBudget,
|
||||
available: totalReceived - totalExpenses - totalCampaignBudget - totalProjectBudget,
|
||||
};
|
||||
return { totalReceived, totalExpenses, totalCampaignBudget, totalProjectBudget, available: totalReceived - totalExpenses - totalCampaignBudget - totalProjectBudget };
|
||||
}
|
||||
|
||||
async function getCampaignAvailable(campaignId) {
|
||||
const entries = await nocodb.list('BudgetEntries', { limit: 10000 });
|
||||
async function getMainAvailable(prefetchedEntries) {
|
||||
const entries = prefetchedEntries || await nocodb.list('BudgetEntries', { limit: 10000 });
|
||||
return computeFromEntries(entries);
|
||||
}
|
||||
|
||||
async function getCampaignAvailable(campaignId, prefetchedEntries) {
|
||||
const entries = prefetchedEntries || await nocodb.list('BudgetEntries', { limit: 10000 });
|
||||
const campaignIncome = entries.filter(e =>
|
||||
e.campaign_id && Number(e.campaign_id) === Number(campaignId) && (e.type || 'income') === 'income'
|
||||
);
|
||||
@@ -38,11 +34,17 @@ async function getCampaignAvailable(campaignId) {
|
||||
return { allocated, trackAllocated, available: allocated - trackAllocated };
|
||||
}
|
||||
|
||||
async function getCampaignAllocatedFromEntries(campaignId) {
|
||||
const entries = await nocodb.list('BudgetEntries', { limit: 10000 });
|
||||
async function getCampaignAllocatedFromEntries(campaignId, prefetchedEntries) {
|
||||
const entries = prefetchedEntries || await nocodb.list('BudgetEntries', { limit: 10000 });
|
||||
return entries
|
||||
.filter(e => e.campaign_id && Number(e.campaign_id) === Number(campaignId) && (e.type || 'income') === 'income')
|
||||
.reduce((s, e) => s + (e.amount || 0), 0);
|
||||
}
|
||||
|
||||
module.exports = { getMainAvailable, getCampaignAvailable, getCampaignAllocatedFromEntries };
|
||||
async function getAllBudgetData() {
|
||||
const entries = await nocodb.list('BudgetEntries', { limit: 10000 });
|
||||
const main = computeFromEntries(entries);
|
||||
return { entries, ...main };
|
||||
}
|
||||
|
||||
module.exports = { getMainAvailable, getCampaignAvailable, getCampaignAllocatedFromEntries, getAllBudgetData, computeFromEntries };
|
||||
|
||||
+12
-7
@@ -3,6 +3,11 @@ const { sendMail } = require('./mail');
|
||||
const nocodb = require('./nocodb');
|
||||
const { parseApproverIds } = require('./helpers');
|
||||
|
||||
function escapeHtml(str) {
|
||||
if (!str) return '';
|
||||
return String(str).replace(/&/g, '&').replace(/</g, '<').replace(/>/g, '>').replace(/"/g, '"');
|
||||
}
|
||||
|
||||
const APP_URL = process.env.APP_URL || process.env.CORS_ORIGIN || 'http://localhost:3001';
|
||||
const APP_NAME_EN = 'Rawaj';
|
||||
const APP_NAME_AR = 'رواج';
|
||||
@@ -239,7 +244,7 @@ function notifyRejected({ type, record, approverName, feedback }) {
|
||||
heading: tr('rejectedHeading', l)(typeLabel),
|
||||
bodyHtml: `
|
||||
<p>${tr('rejectedBody', l)(title, approverName || (l === 'ar' ? 'مراجع' : 'a reviewer'))}</p>
|
||||
${feedback ? `<blockquote ${BLOCKQUOTE}>${feedback}</blockquote>` : ''}`,
|
||||
${feedback ? `<blockquote ${BLOCKQUOTE}>${escapeHtml(feedback)}</blockquote>` : ''}`,
|
||||
ctaText: `${tr('view', l)} ${typeLabel}`,
|
||||
ctaUrl: `${APP_URL}/${type === 'post' ? 'posts' : type === 'translation' ? 'translations' : 'artefacts'}`,
|
||||
});
|
||||
@@ -263,7 +268,7 @@ function notifyRevisionRequested({ type, record, approverName, feedback }) {
|
||||
heading: tr('revisionRequested', l),
|
||||
bodyHtml: `
|
||||
<p>${tr('revisionRequestedBody', l)(title, approverName || (l === 'ar' ? 'مراجع' : 'a reviewer'))}</p>
|
||||
${feedback ? `<blockquote ${BLOCKQUOTE}>${feedback}</blockquote>` : ''}`,
|
||||
${feedback ? `<blockquote ${BLOCKQUOTE}>${escapeHtml(feedback)}</blockquote>` : ''}`,
|
||||
ctaText: `${tr('view', l)} ${tr(entityType, l)}`,
|
||||
ctaUrl: `${APP_URL}/${entityPath}`,
|
||||
});
|
||||
@@ -286,7 +291,7 @@ function notifyTaskAssigned({ task, assignerName }) {
|
||||
bodyHtml: `
|
||||
<p>${tr('taskAssignedBody', l)(assignerName || (l === 'ar' ? 'أحدهم' : 'Someone'))}</p>
|
||||
<p style="font-size:16px;font-weight:600;color:#1e293b">${title}</p>
|
||||
${task.description ? `<p style="color:#64748b">${task.description.substring(0, 200)}</p>` : ''}
|
||||
${task.description ? `<p style="color:#64748b">${escapeHtml(task.description.substring(0, 200))}</p>` : ''}
|
||||
${task.priority ? `<p>${tr('priority', l)}: <strong>${task.priority}</strong></p>` : ''}
|
||||
${task.due_date ? `<p>${tr('dueDate', l)}: <strong>${task.due_date}</strong></p>` : ''}`,
|
||||
ctaText: tr('viewTask', l),
|
||||
@@ -351,7 +356,7 @@ function notifyIssueStatusUpdate({ issue, oldStatus, newStatus }) {
|
||||
bodyHtml: `
|
||||
<p>${tr('issueUpdateBody', 'en')(title)}</p>
|
||||
<p><span style="color:#94a3b8">${oldStatus || 'new'}</span> → <strong style="color:#3b82f6">${newStatus}</strong></p>
|
||||
${issue.resolution_summary ? `<p style="margin-top:12px"><strong>${tr('resolution', 'en')}:</strong> ${issue.resolution_summary}</p>` : ''}`,
|
||||
${issue.resolution_summary ? `<p style="margin-top:12px"><strong>${tr('resolution', 'en')}:</strong> ${escapeHtml(issue.resolution_summary)}</p>` : ''}`,
|
||||
ctaText: issue.tracking_token ? tr('trackIssue', 'en') : null,
|
||||
ctaUrl: issue.tracking_token ? `${APP_URL}/track/${issue.tracking_token}` : null,
|
||||
});
|
||||
@@ -413,7 +418,7 @@ function notifyBudgetRequest({ ceoEmail, amount, requesterName, justification, e
|
||||
heading: tr('budgetRequestHeading', 'en'),
|
||||
bodyHtml: `
|
||||
<p>${tr('budgetRequestBody', 'en')(requesterName, amount)}</p>
|
||||
<p><strong>${tr('budgetJustification', 'en')}:</strong> ${justification}</p>
|
||||
<p><strong>${tr('budgetJustification', 'en')}:</strong> ${escapeHtml(justification)}</p>
|
||||
${earmarkHtml}`,
|
||||
ctaText: tr('reviewRequest', 'en'),
|
||||
ctaUrl: approvalUrl,
|
||||
@@ -429,7 +434,7 @@ function notifyBudgetApproved({ request, requesterEmail, requesterLang }) {
|
||||
heading: tr('budgetApproved', l),
|
||||
bodyHtml: `
|
||||
<p>${tr('budgetApprovedBody', l)(String(request.amount))}</p>
|
||||
${request.response_note ? `<blockquote ${BLOCKQUOTE}>${request.response_note}</blockquote>` : ''}`,
|
||||
${request.response_note ? `<blockquote ${BLOCKQUOTE}>${escapeHtml(request.response_note)}</blockquote>` : ''}`,
|
||||
ctaText: null, ctaUrl: null,
|
||||
});
|
||||
}
|
||||
@@ -443,7 +448,7 @@ function notifyBudgetRejected({ request, requesterEmail, requesterLang }) {
|
||||
heading: tr('budgetRejected', l),
|
||||
bodyHtml: `
|
||||
<p>${tr('budgetRejectedBody', l)(String(request.amount))}</p>
|
||||
${request.response_note ? `<blockquote ${BLOCKQUOTE}>${request.response_note}</blockquote>` : ''}`,
|
||||
${request.response_note ? `<blockquote ${BLOCKQUOTE}>${escapeHtml(request.response_note)}</blockquote>` : ''}`,
|
||||
ctaText: null, ctaUrl: null,
|
||||
});
|
||||
}
|
||||
|
||||
@@ -35,8 +35,10 @@ async function getPostComposition(postId) {
|
||||
return texts.map(tt => ({ language: tt.language_code || tt.language, status: tt.status || 'draft' }));
|
||||
} catch { return []; }
|
||||
};
|
||||
const captionTexts = caption ? await getTexts(caption.Id) : [];
|
||||
const bodyTexts = bodyCopy ? await getTexts(bodyCopy.Id) : [];
|
||||
const [captionTexts, bodyTexts] = await Promise.all([
|
||||
caption ? getTexts(caption.Id) : [],
|
||||
bodyCopy ? getTexts(bodyCopy.Id) : [],
|
||||
]);
|
||||
|
||||
// Get first attachment for design/video thumbnail
|
||||
const getFirstAttachment = async (artefactId) => {
|
||||
@@ -44,11 +46,15 @@ async function getPostComposition(postId) {
|
||||
const versions = await nocodb.list('ArtefactVersions', { where: `(artefact_id,eq,${artefactId})`, sort: '-version_number', limit: 1 });
|
||||
if (versions.length === 0) return null;
|
||||
const attachments = await nocodb.list('ArtefactAttachments', { where: `(version_id,eq,${versions[0].Id})`, limit: 1 });
|
||||
return attachments.length > 0 ? (attachments[0].url || attachments[0].file_url || null) : null;
|
||||
if (attachments.length === 0) return null;
|
||||
const att = attachments[0];
|
||||
return att.drive_url || (att.filename ? `/api/uploads/${att.filename}` : null);
|
||||
} catch { return null; }
|
||||
};
|
||||
const designThumb = design ? (design.thumbnail_url || await getFirstAttachment(design.Id)) : null;
|
||||
const videoThumb = video ? (video.thumbnail_url || await getFirstAttachment(video.Id)) : null;
|
||||
const [designThumb, videoThumb] = await Promise.all([
|
||||
design ? (design.thumbnail_url || getFirstAttachment(design.Id)) : null,
|
||||
video ? (video.thumbnail_url || getFirstAttachment(video.Id)) : null,
|
||||
]);
|
||||
|
||||
return {
|
||||
caption: caption ? { id: caption.Id, title: caption.title, status: caption.status, language: caption.source_language, content_preview: (caption.source_content || '').slice(0, 120), languages: captionTexts } : null,
|
||||
@@ -66,9 +72,7 @@ function computeStage(composition) {
|
||||
const { caption, body_copy, design, video, pieces_ready } = composition;
|
||||
if (pieces_ready) return 'post';
|
||||
if (design || video) return 'design';
|
||||
// Check if we have any copy at all
|
||||
const hasCopy = caption || body_copy;
|
||||
if (!hasCopy) return 'copy';
|
||||
if (caption || body_copy) return 'translate';
|
||||
return 'copy';
|
||||
}
|
||||
|
||||
|
||||
+36
-4
@@ -1,3 +1,4 @@
|
||||
// TODO: Decompose routes into separate files by domain (posts, campaigns, tasks, artefacts, budget, auth)
|
||||
require('dotenv').config({ path: __dirname + '/.env' });
|
||||
|
||||
const express = require('express');
|
||||
@@ -1312,6 +1313,8 @@ app.get('/api/posts/:id', requireAuth, async (req, res) => {
|
||||
});
|
||||
|
||||
app.post('/api/posts', requireAuth, async (req, res) => {
|
||||
// NOTE: Client sends `assigned_to` but NocoDB column is `assigned_to_id`.
|
||||
// Response includes both `assigned_to: post.assigned_to_id` for backward compat.
|
||||
const { title, description, brand_id, assigned_to, status, platform, platforms, content_type, scheduled_date, notes, campaign_id, approver_ids, caption } = req.body;
|
||||
|
||||
const platformsArr = platforms || (platform ? [platform] : []);
|
||||
@@ -1374,6 +1377,8 @@ app.post('/api/posts/bulk-delete', requireAuth, requireRole('superadmin', 'manag
|
||||
});
|
||||
|
||||
app.patch('/api/posts/:id', requireAuth, requireOwnerOrRole('posts', 'superadmin', 'manager'), async (req, res) => {
|
||||
// NOTE: Client sends `assigned_to` but NocoDB column is `assigned_to_id`.
|
||||
// Mapping: req.body.assigned_to → data.assigned_to_id (see below).
|
||||
const { id } = req.params;
|
||||
try {
|
||||
const existing = await nocodb.get('Posts', id);
|
||||
@@ -2525,6 +2530,7 @@ app.post('/api/budget', requireAuth, requireRole('superadmin', 'manager'), async
|
||||
});
|
||||
|
||||
app.patch('/api/budget/:id', requireAuth, requireRole('superadmin', 'manager'), async (req, res) => {
|
||||
let releaseLock = null;
|
||||
try {
|
||||
const existing = await nocodb.get('BudgetEntries', req.params.id);
|
||||
if (!existing) return res.status(404).json({ error: 'Budget entry not found' });
|
||||
@@ -2538,6 +2544,29 @@ app.patch('/api/budget/:id', requireAuth, requireRole('superadmin', 'manager'),
|
||||
|
||||
if (Object.keys(data).length === 0) return res.status(400).json({ error: 'No fields to update' });
|
||||
|
||||
// Validate amount > 0 if being changed
|
||||
if (data.amount !== undefined && (isNaN(Number(data.amount)) || Number(data.amount) <= 0)) {
|
||||
return res.status(400).json({ error: 'Amount must be greater than 0' });
|
||||
}
|
||||
|
||||
// Budget validation: check availability when changing to expense or increasing expense amount
|
||||
const newType = data.type || existing.type || 'income';
|
||||
const oldType = existing.type || 'income';
|
||||
const newAmount = data.amount !== undefined ? Number(data.amount) : (existing.amount || 0);
|
||||
const oldAmount = existing.amount || 0;
|
||||
const needsCheck = (newType === 'expense' && oldType !== 'expense') ||
|
||||
(newType === 'expense' && newAmount > oldAmount);
|
||||
|
||||
if (needsCheck) {
|
||||
releaseLock = await acquireBudgetLock();
|
||||
const { available } = await getMainAvailable();
|
||||
// Add back the old expense amount if it was already an expense, then check new amount
|
||||
const effectiveAvailable = oldType === 'expense' ? available + oldAmount : available;
|
||||
if (newAmount > effectiveAvailable) {
|
||||
return res.status(400).json({ error: 'Insufficient budget', available: effectiveAvailable });
|
||||
}
|
||||
}
|
||||
|
||||
await nocodb.update('BudgetEntries', req.params.id, data);
|
||||
|
||||
const entry = await nocodb.get('BudgetEntries', req.params.id);
|
||||
@@ -2548,6 +2577,8 @@ app.patch('/api/budget/:id', requireAuth, requireRole('superadmin', 'manager'),
|
||||
});
|
||||
} catch (err) {
|
||||
res.status(500).json({ error: 'Failed to update budget entry' });
|
||||
} finally {
|
||||
if (releaseLock) releaseLock();
|
||||
}
|
||||
});
|
||||
|
||||
@@ -2721,10 +2752,11 @@ app.get('/api/budget-requests', requireAuth, requireRole('superadmin'), async (r
|
||||
...r,
|
||||
requester_name: users[r.requested_by_user_id] || 'Unknown',
|
||||
}));
|
||||
await batchResolveNames(enriched, {
|
||||
earmarked_campaign_id: { table: 'Campaigns', as: 'earmarked_campaign_name' },
|
||||
earmarked_project_id: { table: 'Projects', as: 'earmarked_project_name' },
|
||||
});
|
||||
// Resolve earmarked campaign/project names
|
||||
for (const r of enriched) {
|
||||
if (r.earmarked_campaign_id) r.earmarked_campaign_name = await getRecordName('Campaigns', Number(r.earmarked_campaign_id));
|
||||
if (r.earmarked_project_id) r.earmarked_project_name = await getRecordName('Projects', Number(r.earmarked_project_id));
|
||||
}
|
||||
res.json(enriched);
|
||||
} catch (err) {
|
||||
console.error('Budget requests list error:', err);
|
||||
|
||||
Reference in New Issue
Block a user