fix(logo-customization): fix bad use of upload providers

packages/core/admin/server/services/__tests__/project-settings.test.js

@@ -1,6 +1,5 @@
 'use strict';
 
-const { unlinkSync } = require('fs');
 const {
   parseFilesData,
   getProjectSettings,
@@ -10,18 +9,24 @@ const {
 
 jest.mock('fs', () => ({
   createReadStream: () => null,
-  unlinkSync: jest.fn(),
 }));
 
 const storeSet = jest.fn();
+const providerDelete = jest.fn();
 global.strapi = {
   dirs: {
     public: 'publicDir',
   },
+  config: {
+    get: () => ({ provider: 'local' }),
+  },
   plugins: {
     upload: {
       provider: {
-        uploadStream: jest.fn(),
+        async uploadStream(file) {
+          file.url = `/uploads/${file.hash}`;
+        },
+        delete: providerDelete,
       },
       services: {
         upload: {
@@ -45,12 +50,12 @@ global.strapi = {
     get: () => ({
       menuLogo: {
         name: 'name',
-        path: 'path/to/file',
         url: 'file/url',
         width: 100,
         height: 100,
         ext: 'png',
         size: 123,
+        provider: 'local',
       },
     }),
     set: storeSet,
@@ -79,13 +84,12 @@ describe('Project setting', () => {
           hash: 'filename_123',
           ext: '.png',
           mime: 'image/png',
+          provider: 'local',
           size: 123,
           stream: null,
           width: 100,
           height: 100,
-          path: 'publicDir/uploads/filename_123.png',
           tmpPath: '/tmp/filename_123',
-          url: '/uploads/filename_123.png',
         },
       };
 
@@ -131,24 +135,25 @@ describe('Project setting', () => {
       const newSettings = {
         menuLogo: {
           size: 24085,
-          path: '/tmp/filename_123',
           name: 'file.png',
           type: 'image/png',
+          url: 'file/url',
         },
       };
 
       await deleteOldFiles({ previousSettings, newSettings });
 
-      expect(unlinkSync).not.toBeCalled();
+      expect(providerDelete).not.toBeCalled();
     });
 
     it('Does not delete when there is no new file uploaded', async () => {
       const previousSettings = {
         menuLogo: {
           size: 24085,
-          path: '/tmp/filename_123',
           name: 'file.png',
           type: 'image/png',
+          provider: 'local',
+          url: 'file/url',
         },
       };
 
@@ -156,16 +161,17 @@ describe('Project setting', () => {
 
       await deleteOldFiles({ previousSettings, newSettings });
 
-      expect(unlinkSync).not.toBeCalled();
+      expect(providerDelete).not.toBeCalled();
     });
 
     it('Deletes when inputs are explicitely set to null', async () => {
       const previousSettings = {
         menuLogo: {
           size: 24085,
-          path: '/tmp/filename_123',
           name: 'file.png',
           type: 'image/png',
+          provider: 'local',
+          url: 'file/url',
         },
       };
 
@@ -173,31 +179,30 @@ describe('Project setting', () => {
 
       await deleteOldFiles({ previousSettings, newSettings });
 
-      expect(unlinkSync).toBeCalledTimes(1);
+      expect(providerDelete).toBeCalledTimes(1);
-      expect(unlinkSync).toBeCalledWith(previousSettings.menuLogo.path);
     });
 
     it('Deletes when new files are uploaded', async () => {
       const previousSettings = {
         menuLogo: {
           size: 24085,
-          path: '/tmp/filename_123',
           name: 'file.png',
           type: 'image/png',
+          provider: 'local',
+          url: 'file/url',
         },
       };
 
       const newSettings = {
         menuLogo: {
           ...previousSettings.menuLogo,
-          path: 'another/path',
+          url: 'file/new',
         },
       };
 
       await deleteOldFiles({ previousSettings, newSettings });
 
-      expect(unlinkSync).toBeCalledTimes(1);
+      expect(providerDelete).toBeCalledTimes(1);
-      expect(unlinkSync).toBeCalledWith(previousSettings.menuLogo.path);
     });
   });
 
@@ -216,7 +221,6 @@ describe('Project setting', () => {
           stream: null,
           width: 100,
           height: 100,
-          path: 'publicDir/uploads/filename_123.png',
           tmpPath: '/tmp/filename_123',
           url: '/uploads/filename_123.png',
         },
@@ -225,11 +229,11 @@ describe('Project setting', () => {
       const expectedOutput = {
         menuLogo: {
           name: 'filename.png',
-          path: 'publicDir/uploads/filename_123.png',
+          hash: 'filename_123',
-          url: '/uploads/filename_123.png',
+          url: '/uploads/filename_123',
           width: 100,
           height: 100,
-          ext: 'png',
+          ext: '.png',
           size: 123,
         },
       };
@@ -267,12 +271,12 @@ describe('Project setting', () => {
       const expectedOutput = {
         menuLogo: {
           name: 'name',
-          path: 'path/to/file',
           url: 'file/url',
           width: 100,
           height: 100,
           ext: 'png',
           size: 123,
+          provider: 'local',
         },
       };
 

packages/core/admin/server/services/project-settings.js

@@ -1,7 +1,6 @@
 'use strict';
 
 const fs = require('fs');
-const path = require('path');
 const { pick } = require('lodash');
 
 const PROJECT_SETTINGS_FILE_INPUTS = ['menuLogo'];
@@ -37,13 +36,11 @@ const parseFilesData = async files => {
           .getDimensions({ getStream })
       );
 
-      // Add file path, url and stream
+      // Add file path, and stream
-      const url = `/uploads/${formatedFilesData[inputName].hash}${formatedFilesData[inputName].ext}`;
       Object.assign(formatedFilesData[inputName], {
-        path: path.join(strapi.dirs.public, url),
         stream: getStream(),
         tmpPath: files[inputName].path,
-        url,
+        provider: strapi.config.get('plugin.upload').provider,
       });
     })
   );
@@ -87,14 +84,19 @@ const deleteOldFiles = async ({ previousSettings, newSettings }) => {
       // Skip if the file was not changed
       if (
         newSettings[inputName] &&
-        previousSettings[inputName].path === newSettings[inputName].path
+        previousSettings[inputName].url === newSettings[inputName].url
       ) {
         return;
       }
 
+      // Skip if the file was not uploaded with the current provider
+      if (strapi.config.get('plugin.upload').provider !== previousSettings[inputName].provider) {
+        return;
+      }
+
       // There was a previous file and an new file was uploaded
       // Remove the previous file
-      fs.unlinkSync(previousSettings[inputName].path);
+      strapi.plugin('upload').provider.delete(previousSettings[inputName]);
     })
   );
 };
@@ -103,6 +105,8 @@ const updateProjectSettings = async ({ body, files }) => {
   const store = await strapi.store({ type: 'core', name: 'admin' });
   const previousSettings = await store.get({ key: 'project-settings' });
 
+  await uploadFiles(files);
+
   const newSettings = {
     ...body,
     ...files,
@@ -119,18 +123,18 @@ const updateProjectSettings = async ({ body, files }) => {
       // Update the file
       newSettings[inputName] = {
         name: newSettings[inputName].name,
-        path: newSettings[inputName].path,
+        hash: newSettings[inputName].hash,
         url: newSettings[inputName].url,
         width: newSettings[inputName].width,
         height: newSettings[inputName].height,
-        ext: newSettings[inputName].ext.replace('.', ''),
+        ext: newSettings[inputName].ext,
         size: newSettings[inputName].size,
+        provider: newSettings[inputName].provider,
       };
     }
   });
 
   // No await to proceed asynchronously
-  uploadFiles(files);
   deleteOldFiles({ previousSettings, newSettings });
 
   await store.set({