Improve loading of uploaded files in draft field

opencollective · Jan 13, 2025 · 78d7331 · 78d7331
1 parent 8d491b0
commit 78d7331
Show file tree

Hide file tree

Showing 6 changed files with 41 additions and 22 deletions.
diff --git a/server/controllers/files.ts b/server/controllers/files.ts
@@ -48,6 +48,14 @@ export async function getFile(req: Request, res: Response) {
 
   const actualUrl = uploadedFile.getDataValue('url');
 
+  if (uploadedFile.isPublicFile()) {
+    if (isJsonAccepted) {
+      return res.send({ url: actualUrl });
+    } else {
+      return res.redirect(307, actualUrl);
+    }
+  }
+
   if (
     !(await hasUploadedFilePermission(req, uploadedFile, {
       expenseId: expenseId ? idDecode(expenseId as string, IDENTIFIER_TYPES.EXPENSE) : null,

diff --git a/server/graphql/common/expenses.ts b/server/graphql/common/expenses.ts
@@ -1508,14 +1508,14 @@ export async function prepareAttachedFiles(req: Request, attachedFiles: ExpenseD
     return [];
   }
 
-  const mapItemUrlToUploadedFile: Record<string, string> = {};
+  const mapItemUrlToUploadedFileUrl: Record<string, string> = {};
   for (const item of attachedFiles) {
     if (!item.url) {
       continue;
     }
 
     if (!UploadedFile.isUploadedFileURL(item.url)) {
-      mapItemUrlToUploadedFile[item.url] = item.url;
+      mapItemUrlToUploadedFileUrl[item.url] = item.url;
       continue;
     }
 
@@ -1525,12 +1525,12 @@ export async function prepareAttachedFiles(req: Request, attachedFiles: ExpenseD
 
     const uploadedFile = await UploadedFile.getFromURL(item.url);
 
-    mapItemUrlToUploadedFile[item.url] = uploadedFile.getDataValue('url');
+    mapItemUrlToUploadedFileUrl[item.url] = uploadedFile.getDataValue('url');
   }
 
   return attachedFiles.map(file => ({
     ...file,
-    url: mapItemUrlToUploadedFile[file.url],
+    url: mapItemUrlToUploadedFileUrl[file.url],
   }));
 }
 
@@ -1568,14 +1568,14 @@ export const prepareExpenseItemInputs = async (
     );
   }
 
-  const mapItemUrlToUploadedFile: Record<string, string> = {};
+  const mapItemUrlToUploadedFileUrl: Record<string, string> = {};
   for (const item of itemsInput) {
     if (!item.url) {
       continue;
     }
 
     if (!UploadedFile.isUploadedFileURL(item.url)) {
-      mapItemUrlToUploadedFile[item.url] = item.url;
+      mapItemUrlToUploadedFileUrl[item.url] = item.url;
       continue;
     }
 
@@ -1584,7 +1584,7 @@ export const prepareExpenseItemInputs = async (
     }
 
     const uploadedFile = await UploadedFile.getFromURL(item.url);
-    mapItemUrlToUploadedFile[item.url] = uploadedFile.getDataValue('url');
+    mapItemUrlToUploadedFileUrl[item.url] = uploadedFile.getDataValue('url');
   }
 
   // Prepare items
@@ -1593,7 +1593,7 @@ export const prepareExpenseItemInputs = async (
     const values: Partial<ExpenseItem> = pick(itemInput, fieldsToPick);
 
     if (values.url) {
-      values.url = mapItemUrlToUploadedFile[values.url];
+      values.url = mapItemUrlToUploadedFileUrl[values.url];
     }
 
     if (itemInput['amount'] && itemInput['amountV2']) {

diff --git a/server/graphql/common/uploaded-file.ts b/server/graphql/common/uploaded-file.ts
@@ -73,13 +73,11 @@ export async function hasUploadedFilePermission(
         include: { model: Expense, include: [{ association: 'fromCollective' }] },
       });
 
-      const expense = expenseItem?.Expense;
-
-      if (!expense) {
+      if (!expenseItem) {
         return req.remoteUser?.id === uploadedFile.CreatedByUserId;
       }
 
-      if (await canSeeExpenseAttachments(req, expense)) {
+      if (await canSeeExpenseAttachments(req, expenseItem?.Expense)) {
         return true;
       }
       break;
@@ -92,13 +90,11 @@ export async function hasUploadedFilePermission(
         include: { model: Expense, include: [{ model: Collective, as: 'fromCollective' }] },
       });
 
-      const expense = expenseAttachedFile?.Expense;
-
-      if (!expense) {
+      if (!expenseAttachedFile) {
         return req.remoteUser?.id === uploadedFile.CreatedByUserId;
       }
 
-      if (await canSeeExpenseAttachments(req, expense)) {
+      if (await canSeeExpenseAttachments(req, expenseAttachedFile?.Expense)) {
         return true;
       }
 

diff --git a/server/graphql/v2/object/Expense.ts b/server/graphql/v2/object/Expense.ts
@@ -9,7 +9,7 @@ import {
   GraphQLString,
 } from 'graphql';
 import { GraphQLDateTime, GraphQLJSON } from 'graphql-scalars';
-import { findLast, pick, round, takeRightWhile, toString, uniq } from 'lodash';
+import { findLast, keyBy, pick, round, takeRightWhile, toString, uniq } from 'lodash';
 import { WhereOptions } from 'sequelize';
 
 import ActivityTypes from '../../../constants/activities';
@@ -554,9 +554,19 @@ export const GraphQLExpense = new GraphQLObjectType<ExpenseModel, express.Reques
 
             const draftData = pick(expense.data, draftFields);
             const items = ((expense.data.items as { url?: string }[]) || []).map(item => pick(item, itemsFields));
+            const attachedFiles = (draftData.attachedFiles as { url?: string }[]) || [];
+
+            const uploadedFiles = keyBy(
+              (await req.loaders.UploadedFile.byUrl.loadMany([
+                ...items.filter(item => item.url).map(item => item.url),
+                ...attachedFiles.filter(attachedFile => attachedFile.url),
+              ])) as UploadedFile[],
+              uploadedFile => uploadedFile.getDataValue('url'),
+            );
+
             for (const item of items) {
               if (item.url) {
-                const uploadedFile = await req.loaders.UploadedFile.byUrl.load(item.url);
+                const uploadedFile = uploadedFiles[item.url];
                 item.url = uploadedFile
                   ? UploadedFile.getProtectedURLFromOpenCollectiveS3Bucket(uploadedFile, {
                       expenseId: expense.id,
@@ -570,10 +580,9 @@ export const GraphQLExpense = new GraphQLObjectType<ExpenseModel, express.Reques
               draftData.items = items;
             }
 
-            const attachedFiles = (draftData.attachedFiles as { url?: string }[]) || [];
             for (const attachedFile of attachedFiles) {
               if (attachedFile.url) {
-                const uploadedFile = await req.loaders.UploadedFile.byUrl.load(attachedFile.url);
+                const uploadedFile = uploadedFiles[attachedFile.url];
 
                 attachedFile.url = uploadedFile
                   ? UploadedFile.getProtectedURLFromOpenCollectiveS3Bucket(uploadedFile, {

diff --git a/server/models/UploadedFile.ts b/server/models/UploadedFile.ts
@@ -377,6 +377,12 @@ class UploadedFile extends Model<InferAttributes<UploadedFile>, InferCreationAtt
     // S3 limits file names to 1024 characters. We're using 900 to be safe and give some room for the kind + uuid + extension.
     return `${parsedFileName.name.slice(0, 900)}${expectedExtension}`;
   }
+
+  public isPublicFile(): boolean {
+    return ['ACCOUNT_AVATAR', 'ACCOUNT_BANNER', 'ACCOUNT_LONG_DESCRIPTION', 'TIER_LONG_DESCRIPTION'].includes(
+      this.kind,
+    );
+  }
 }
 
 UploadedFile.init(

diff --git a/test/test-helpers/fake-data.ts b/test/test-helpers/fake-data.ts
@@ -398,9 +398,9 @@ export const fakeUploadedFile = async (fileData: Partial<InferCreationAttributes
   const fileType = sample(SUPPORTED_FILE_TYPES);
   const extension = SUPPORTED_FILE_EXTENSIONS[fileType];
   const fileName = `${randStr()}${extension}`;
-  const kind = sample(SUPPORTED_FILE_KINDS);
+  const kind = fileData?.kind || sample(SUPPORTED_FILE_KINDS);
   return models.UploadedFile.create({
-    url: fakeUploadedFileURL(fileData?.kind || kind, fileName),
+    url: fakeUploadedFileURL(kind, fileName),
     kind,
     fileSize: randNumber(100, 100000),
     fileName,