From 332fd76f94331bd9e183a39b2970228e2f7a17be Mon Sep 17 00:00:00 2001 From: John Abrahams Date: Fri, 19 Jan 2024 10:39:05 -0500 Subject: [PATCH] Delete files when removed from Draft submissions (#1248) * Delete files when removed from Draft submissions * Small test refactor --- app/adapters/file.js | 12 ++ app/components/workflow-files/index.js | 8 +- mirage/config.js | 5 +- .../components/workflow-files-test.js | 159 ++++++------------ 4 files changed, 75 insertions(+), 109 deletions(-) create mode 100644 app/adapters/file.js diff --git a/app/adapters/file.js b/app/adapters/file.js new file mode 100644 index 00000000..c2adce67 --- /dev/null +++ b/app/adapters/file.js @@ -0,0 +1,12 @@ +import ApplicationAdapter from './application'; + +export default class FileAdapter extends ApplicationAdapter { + deleteRecord(store, type, snapshot) { + console.warn('File deletion cannot be rolled back'); + // Can't use this.buildURL, as that will append the ember-data File path, which we don't want + const url = `/${this.namespace}${snapshot.attr('uri')}`; + return fetch(url, { + method: 'DELETE', + }).then(() => super.deleteRecord(store, type, snapshot)); + } +} diff --git a/app/components/workflow-files/index.js b/app/components/workflow-files/index.js index 8efac96f..90007f30 100644 --- a/app/components/workflow-files/index.js +++ b/app/components/workflow-files/index.js @@ -115,6 +115,7 @@ export default class WorkflowFiles extends Component { this.args.newFiles.pushObject(newFile); } catch (error) { FileUpload.file.state = 'aborted'; + console.error(error); } return true; @@ -129,9 +130,10 @@ export default class WorkflowFiles extends Component { return; } - file.set('submission', undefined); - await file.save(); - file.unloadRecord(); + // Delete record without a chance to roll back + // This will automatically remove the uploaded bytes of the original file + // then delete the File model record + await file.destroyRecord(); } @action diff --git a/mirage/config.js b/mirage/config.js index 8a11a275..803ccf39 100644 --- a/mirage/config.js +++ b/mirage/config.js @@ -1,6 +1,6 @@ import { camelize } from '@ember/string'; import { discoverEmberDataModels } from 'ember-cli-mirage'; -import { createServer, JSONAPISerializer } from 'miragejs'; +import { createServer, JSONAPISerializer, Response } from 'miragejs'; import doiJournals from './custom-fixtures/nih-submission/doi-journals'; import schemas from './routes/schemas'; import ENV from 'pass-ui/config/environment'; @@ -111,6 +111,9 @@ export default function (config) { return schema.create('file', attrs); }); + this.delete('/data/file/:id/:name', (_schema, _request) => { + return new Response(200); + }); /** Schema Service */ schemas(this); diff --git a/tests/integration/components/workflow-files-test.js b/tests/integration/components/workflow-files-test.js index faf32031..c901336f 100644 --- a/tests/integration/components/workflow-files-test.js +++ b/tests/integration/components/workflow-files-test.js @@ -6,59 +6,49 @@ import EmberObject, { set } from '@ember/object'; import { setupRenderingTest } from 'ember-qunit'; import hbs from 'htmlbars-inline-precompile'; import { module, test } from 'qunit'; -import { run } from '@ember/runloop'; -import { click, render, triggerEvent, waitFor, getContext } from '@ember/test-helpers'; +import { click, render } from '@ember/test-helpers'; import setupMirage from 'ember-cli-mirage/test-support/setup-mirage'; +import sinon from 'sinon'; module('Integration | Component | workflow files', (hooks) => { setupRenderingTest(hooks); setupMirage(hooks); hooks.beforeEach(function () { - let submission = EmberObject.create({ - repositories: [], + const store = this.owner.lookup('service:store'); + this.submission = store.createRecord('submission', { + repositoriers: [], grants: [], }); - let files = [EmberObject.create({})]; - let newFiles = A([]); - set(this, 'submission', submission); - set(this, 'files', files); - set(this, 'newFiles', newFiles); - set(this, 'loadPrevious', (actual) => {}); - set(this, 'loadNext', (actual) => {}); - - const mockStaticConfig = Service.extend({ - getStaticConfig: () => - Promise.resolve({ - branding: { - stylesheet: '', - pages: {}, - }, - }), - addCss: () => {}, - }); + this.files = [{}]; + this.newFiles = []; + + this.loadPrevious = sinon.fake(); + this.loadNext = sinon.fake(); + + // Bogus action so component actions don't complain + this.fakeAction = sinon.fake(); + + const staticConfig = this.owner.lookup('service:app-static-config'); + sinon.replace( + staticConfig, + 'getStaticConfig', + sinon.fake.returns(Promise.resolve({ branding: { stylesheet: '', returns: {} } })) + ); - this.owner.register('service:app-static-config', mockStaticConfig); + this.owner.register('service:app-static-config', staticConfig); this.owner.register( 'service:workflow', - Service.extend({ - getDoiInfo: () => ({ DOI: 'moo-doi' }), - }) + sinon.stub(this.owner.lookup('service:workflow'), 'getDoiInfo').returns({ DOI: 'moo-doi' }) ); - this.owner.register( - 'service:oa-manuscript-service', - Service.extend({ - lookup: () => - Promise.resolve([ - { - name: 'This is a moo', - url: 'http://example.com/moo.pdf', - }, - ]), - }) + this.msServiceFake = sinon.replace( + this.owner.lookup('service:oa-manuscript-service'), + 'lookup', + sinon.fake.returns(Promise.resolve([{ name: 'This is a moo', url: 'http://example.com/moo.pdf' }])) ); + this.owner.register('service:oa-manuscript-service', this.msServiceFake); // Inline configure mirage to respond to File saves this.server.post( @@ -75,45 +65,28 @@ module('Integration | Component | workflow files', (hooks) => { * First upload a file, then click the 'Remove' button */ test('Files removed from UI should no longer reference submission', async function (assert) { - assert.expect(6); - const submission = EmberObject.create({}); this.set('submission', submission); - this.set( - 'previouslyUploadedFiles', - A([ - EmberObject.create({ - name: 'File-for-test', - fileRole: 'manuscript', - submission, - // TODO: bring this back when file service is implemented - save() { - // Should be called when "deleted" to persist changes - assert.ok(true); - return Promise.resolve(); - }, - unloadRecord() { - assert.ok(true); - return Promise.resolve(); - }, - }), - ]) - ); - - this.set('newFiles', []); + const deleteStub = sinon.stub().returns(() => Promise.resolve()); + const file = { + name: 'File-for-test', + fileRole: 'manuscript', + submission, + destroyRecord: deleteStub, + }; - // Bogus action so component actions don't complain - this.set('moo', () => {}); + this.previouslyUploadedFiles = [file]; + this.newFiles = []; await render(hbs` `); @@ -129,6 +102,8 @@ module('Integration | Component | workflow files', (hooks) => { const workflowFiles = this.previouslyUploadedFiles; assert.strictEqual(workflowFiles.length, 0, 'Should have 0 files tracked'); + + assert.ok(deleteStub.calledOnce, 'File destroyRecord() should be called'); }); /** @@ -139,34 +114,22 @@ module('Integration | Component | workflow files', (hooks) => { */ test("Can't select oa mss when manuscript already attached to submission", async function (assert) { this.store = this.owner.lookup('service:store'); - const submission = this.store.createRecord('submission'); + this.submission = this.store.createRecord('submission'); - const ms = EmberObject.create({ + const ms = { name: 'This is the first moo', fileRole: 'manuscript', - }); + }; - set(this, 'submission', submission); - set(this, 'previouslyUploadedFiles', A([ms])); - set(this, 'moo', () => {}); - - this.owner.register( - 'service:submission-handler', - Service.extend({ - uploadFile(submission, file) { - assert.ok(submission, 'No submission found'); - assert.ok(file, 'No file specified for upload'); - }, - }) - ); + this.previouslyUploadedFiles = [ms]; await render(hbs``); assert.dom('[data-test-foundmss-component]').doesNotExist(); @@ -182,32 +145,18 @@ module('Integration | Component | workflow files', (hooks) => { }); test('Manually uploading a MS should hide FoundManuscript component', async function (assert) { - this.store = this.owner.lookup('service:store'); - const submission = this.store.createRecord('submission'); - - set(this, 'moo', () => {}); - set(this, 'submission', submission); - set(this, 'previouslyUploadedFiles', A([])); - - this.owner.register( - 'service:submission-handler', - Service.extend({ - uploadFile(submission, file) { - assert.ok(submission, 'No submission found'); - assert.ok(file, 'No file specified for upload'); - }, - }) - ); + this.previouslyUploadedFiles = []; await render(hbs``); + assert.ok(this.msServiceFake.calledOnce, 'Manuscript Service should be called once'); assert.dom('[data-test-added-manuscript-row]').doesNotExist(); assert.dom('#file-multiple-input').exists(); @@ -215,7 +164,7 @@ module('Integration | Component | workflow files', (hooks) => { submissionFile.name = 'my-submission.pdf'; await selectFiles('input[type=file]', submissionFile); - assert.dom('[data-test-added-manuscript-row]').exists(); + assert.dom('[data-test-added-manuscript-row]').exists({ count: 1 }); assert.dom('[data-test-added-manuscript-row]').includesText('my-submission.pdf'); }); });