From c55ce37cfb6150fe6f8bfd289b554f493704bf9c Mon Sep 17 00:00:00 2001 From: John Abrahams Date: Wed, 21 Feb 2024 14:26:37 -0500 Subject: [PATCH 1/4] Update submission deletion, tests --- app/services/submission-handler.js | 38 +++- .../unit/services/submission-handler-test.js | 165 ++++++++++++++++-- 2 files changed, 181 insertions(+), 22 deletions(-) diff --git a/app/services/submission-handler.js b/app/services/submission-handler.js index 05778e9a..85ad470e 100644 --- a/app/services/submission-handler.js +++ b/app/services/submission-handler.js @@ -4,6 +4,7 @@ import Service, { inject as service } from '@ember/service'; import ENV from 'pass-ui/config/environment'; import { task } from 'ember-concurrency-decorators'; import { get } from '@ember/object'; +import SubmissionModel from '../models/submission'; /** * Service to manage submissions. @@ -230,18 +231,43 @@ export default class SubmissionHandlerService extends Service { } /** - * Simply set submission.submissionStatus to 'cancelled'. This operation is - * distinct from `#cancelSubmission` because this does not create a - * SubmissionEvent + * Delete a draft submission. Any attached files will be deleted and the attached + * Publication will be deleted if it is not referenced by any other submissions. * - * All other objects associated with the submission (Publication, Files, etc) - * will remain intact. + * - First ensure submission source is pass and submission status is draft + * - Delete Files + * - Delete linked Publication only if no other submissions reference it + * - Delete the Submission only if all of the above succeed + * + * Will reject operation if the specified submission is NOT in 'draft' status + * + * Will pass errors from all of the network interactions: + * - File deletions + * - Publication deletion + * - Submission deletion * * @param {Submission} submission submission to delete * @returns {Promise} that returns once the submission deletion is persisted */ async deleteSubmission(submission) { - submission.set('submissionStatus', 'cancelled'); + if (submission.source !== 'pass' || submission.submissionStatus !== 'draft') { + return Promise.reject(`Non-DRAFT submissions cannot be deleted`); + } + + // Get submissions for this file + const files = await this.store.query('file', { filter: { submission: submission.id } }); + await Promise.all(files.map((file) => file.destroyRecord())); + + const publication = await submission.publication; + + // Search for Submissions that reference this publication + const subsWithThisPublication = await this.store.query('submission', { filter: { publication: publication.id } }); + if (subsWithThisPublication.length === 1) { + publication.deleteRecord(); + await publication.save(); + } + + submission.deleteRecord(); return submission.save(); } } diff --git a/tests/unit/services/submission-handler-test.js b/tests/unit/services/submission-handler-test.js index a4cb7257..a3ac6bc9 100644 --- a/tests/unit/services/submission-handler-test.js +++ b/tests/unit/services/submission-handler-test.js @@ -3,6 +3,7 @@ import { A } from '@ember/array'; import EmberObject from '@ember/object'; import { module, test } from 'qunit'; import { setupTest } from 'ember-qunit'; +import Sinon from 'sinon'; module('Unit | Service | submission-handler', (hooks) => { setupTest(hooks); @@ -313,26 +314,158 @@ module('Unit | Service | submission-handler', (hooks) => { }); }); - /** - * #deleteSubmission should set submissionStatus to 'cancelled' while leaving associated - * objects alone - */ - test('delete submission', function (assert) { - assert.expect(3); + test('cannot delete non-draft submissions', function (assert) { + const submission = { + submissionStatus: 'not-draft', + publication: { title: 'Moo title' }, + save: () => Promise.resolve(), + }; + + const service = this.owner.lookup('service:submission-handler'); + + service + .deleteSubmission(submission) + .then((_) => assert.fail('Deletion should throw an error')) + .catch((_e) => assert.ok(true)); + }); - const submission = EmberObject.create({ + test('File deletion failure kills the submission deletion process', async function (assert) { + const service = this.owner.lookup('service:submission-handler'); + const store = this.owner.lookup('service:store'); + + const submission = { + source: 'pass', submissionStatus: 'draft', - publication: EmberObject.create({ title: 'Moo title' }), - save: () => Promise.resolve(), - }); + publication: { + title: 'Moo Title', + save: Sinon.fake.resolves(), + deleteRecord: Sinon.fake.resolves(), + }, + save: Sinon.fake.resolves(), + deleteRecord: Sinon.fake.resolves(), + }; + + const query = (model, _filter) => { + switch (model) { + case 'file': + // Get files for the submission + return Promise.resolve([ + { name: 'file1', submission: 0, destroyRecord: Sinon.fake.rejects() }, + { name: 'file2', submission: 0, destroyRecord: Sinon.fake.rejects() }, + ]); + case 'submission': + return Promise.resolve([submission]); // Get submissions that share the same Publication + default: + return Promise.reject(); + } + }; + const storeQueryFake = Sinon.replace(store, 'query', Sinon.spy(query)); + this.owner.register('service:store', store); + + await service + .deleteSubmission(submission) + // Fail test on positive return + .then(() => assert.ok(false, 'Delete submission is expected to not succeed')) + .catch((_e) => assert.ok(true)); + + assert.ok(storeQueryFake.calledOnce, 'Store query should be called only once'); + assert.equal(submission.deleteRecord.callCount, 0, 'Submission delete should not be called'); + assert.equal(submission.save.callCount, 0, 'Submission should not be saved'); + }); + test('Publication not deleted if multiple submissions reference it', async function (assert) { const service = this.owner.lookup('service:submission-handler'); - const result = service.deleteSubmission(submission); + const store = this.owner.lookup('service:store'); + + const publication = { + title: 'Moo Title', + save: Sinon.fake.resolves(), + deleteRecord: Sinon.fake.resolves(), + }; + const submission = { + id: 0, + source: 'pass', + submissionStatus: 'draft', + publication, + save: Sinon.fake.resolves(), + deleteRecord: Sinon.fake.resolves(), + }; + + const query = (model, _filter) => { + switch (model) { + case 'file': + // Get files for the submission + return Promise.resolve([ + { name: 'file1', submission: 0, destroyRecord: Sinon.fake.resolves() }, + { name: 'file2', submission: 0, destroyRecord: Sinon.fake.resolves() }, + ]); + case 'submission': + // 2 submissions use the same Publication + return Promise.resolve([submission, { id: 1, publication }]); + default: + return Promise.reject(); + } + }; + + const storeQueryFake = Sinon.replace(store, 'query', Sinon.fake(query)); + this.owner.register('service:store', store); + + await service.deleteSubmission(submission); + + assert.equal(storeQueryFake.callCount, 2, 'Store query should be called twice'); + assert.ok(submission.deleteRecord.calledOnce, 'Submission delete should be called'); + assert.ok(submission.save.calledOnce, 'Submission should be saved'); + assert.equal(publication.deleteRecord.callCount, 0, 'Publication should not be deleted'); + assert.equal(publication.save.callCount, 0, 'Publication should not be re-persisted'); + }); - assert.ok(result, 'No result found'); - result.then(() => { - assert.strictEqual(submission.get('submissionStatus'), 'cancelled', 'Unexpected status'); - assert.ok(submission.get('publication'), 'No publication found'); - }); + test('Submission, publication, and files are deleted', async function (assert) { + const service = this.owner.lookup('service:submission-handler'); + const store = this.owner.lookup('service:store'); + + const publication = { + title: 'Moo Title', + save: Sinon.fake.resolves(), + deleteRecord: Sinon.fake.resolves(), + }; + const submission = { + id: 0, + source: 'pass', + submissionStatus: 'draft', + publication, + save: Sinon.fake.resolves(), + deleteRecord: Sinon.fake.resolves(), + }; + const files = [ + { name: 'file1', submission, destroyRecord: Sinon.fake.resolves() }, + { name: 'file2', submission, destroyRecord: Sinon.fake.resolves() }, + ]; + + const query = (model, _filter) => { + switch (model) { + case 'file': + // Get files for the submission + return Promise.resolve(files); + case 'submission': + return Promise.resolve([submission]); + default: + return Promise.reject(); + } + }; + + const storeQueryFake = Sinon.replace(store, 'query', Sinon.fake(query)); + this.owner.register('service:store', store); + + await service.deleteSubmission(submission); + + assert.equal(storeQueryFake.callCount, 2, 'Store query should be called twice'); + assert.ok(submission.deleteRecord.calledOnce, 'Submission delete should be called'); + assert.ok(submission.save.calledOnce, 'Submission should be saved'); + assert.ok(publication.deleteRecord.calledOnce, 'Publication should be deleted'); + assert.ok(publication.save.calledOnce, 'Publication deletion should be persisted'); + assert.ok( + files.map((f) => f.destroyRecord).every((func) => func.calledOnce), + 'All files should be destroyed' + ); }); }); From a0174c2b31feac7b3df937ca3b41d0833c57535f Mon Sep 17 00:00:00 2001 From: John Abrahams Date: Thu, 22 Feb 2024 10:06:26 -0500 Subject: [PATCH 2/4] Update another submission delete button --- .../submission-action-cell/index.js | 27 +++---- .../components/submission-action-cell-test.js | 72 ++++++++++++++----- 2 files changed, 70 insertions(+), 29 deletions(-) diff --git a/app/components/submission-action-cell/index.js b/app/components/submission-action-cell/index.js index 603bd93a..690cfebd 100644 --- a/app/components/submission-action-cell/index.js +++ b/app/components/submission-action-cell/index.js @@ -7,6 +7,7 @@ import swal from 'sweetalert2'; export default class SubmissionActionCell extends Component { @service currentUser; @service submissionHandler; + @service flashMessages; get isPreparer() { let userId = this.currentUser.user.id; @@ -23,32 +24,32 @@ export default class SubmissionActionCell extends Component { } /** - * Delete the specified submission record from Fedora. - * - * Note: `ember-fedora-adapter#deleteRecord` behaves like `ember-data#destroyRecord` - * in that the deletion is pushed to the back end automatically, such that a - * subsequent 'save()' will fail. + * Delete the specified submission record. This is done via #destroyRecord + * so is immediately persisted * * @param {object} submission model object to be removed */ @action deleteSubmission(submission) { swal({ - // title: 'Are you sure?', text: 'Are you sure you want to delete this draft submission? This cannot be undone.', confirmButtonText: 'Delete', confirmButtonColor: '#DC3545', showCancelButton: true, - // buttonsStyling: false, - // confirmButtonClass: 'btn btn-danger', - // cancelButtonText: 'No', - // customClass: { - // confirmButton: 'btn btn-danger' - // } }).then((result) => { if (result.value) { - this.submissionHandler.deleteSubmission(submission); + this.do_delete(submission); } }); } + + async do_delete(submission) { + try { + await this.submissionHandler.deleteSubmission(submission); + } catch (e) { + this.flashMessages.danger( + 'We encountered an error deleting this draft submission. Please try again later or contact your administrator' + ); + } + } } diff --git a/tests/integration/components/submission-action-cell-test.js b/tests/integration/components/submission-action-cell-test.js index 8689f1ae..93e17062 100644 --- a/tests/integration/components/submission-action-cell-test.js +++ b/tests/integration/components/submission-action-cell-test.js @@ -1,12 +1,13 @@ /* eslint-disable ember/no-classic-classes, ember/avoid-leaking-state-in-ember-objects, ember/no-settled-after-test-helper, ember/no-get */ -import EmberObject, { get } from '@ember/object'; +import EmberObject from '@ember/object'; import { A } from '@ember/array'; import Service from '@ember/service'; import { setupRenderingTest } from 'ember-qunit'; import hbs from 'htmlbars-inline-precompile'; import { module, test } from 'qunit'; -import { render, click, settled } from '@ember/test-helpers'; +import { render, click, settled, waitFor } from '@ember/test-helpers'; import { run } from '@ember/runloop'; +import Sinon from 'sinon'; module('Integration | Component | submission action cell', (hooks) => { setupRenderingTest(hooks); @@ -61,30 +62,69 @@ module('Integration | Component | submission action cell', (hooks) => { * supplied submission, then `#save()` should be called */ test('should delete and persist submission', async function (assert) { - assert.expect(2); - - const record = EmberObject.create({ - preparers: A(), + const record = { + preparers: [], isDraft: true, - save() { - assert.ok(true); - return Promise.resolve(); - }, - submitter: { - id: 1, - }, - }); + save: Sinon.fake.resolves(), + submitter: { id: 1 }, + }; this.set('record', record); + const submissionHandler = this.owner.lookup('service:submission-handler'); + const handlerFake = Sinon.replace(submissionHandler, 'deleteSubmission', Sinon.fake.resolves()); + this.owner.register('service:submission-handler', submissionHandler); + await render(hbs``); await click('a.delete-button'); await click(document.querySelector('.swal2-confirm')); await settled(); - const status = get(this, 'record.submissionStatus'); - assert.strictEqual(status, 'cancelled'); + assert.ok(handlerFake.calledOnce, 'Submission handler delete should be called'); + }); + + test('Should show error message on submission deletion error', async function (assert) { + const record = { preparers: [], isDraft: true, save: Sinon.fake.resolves(), submitter: { id: 1 } }; + this.record = record; + + const submissionHandler = this.owner.lookup('service:submission-handler'); + // Make sure submissionHandler#deleteSubmission fails + const handlerFake = Sinon.replace(submissionHandler, 'deleteSubmission', Sinon.fake.throws()); + this.owner.register('service:submission-handler', submissionHandler); + + // Need to make sure the flash message service is initialized + this.flashMessages = this.owner.lookup('service:flash-messages'); + // Need harness for flash messages + // In the real app, this is done at the application level and will always be available + // but needs to be injected for this isolated render test + await render(hbs` + {{#each this.flashMessages.queue as |flash|}} +
+ +
+ {{flash.message}} + + x + +
+ +
+ {{/each}} + + `); + + assert.dom('a.delete-button').exists(); + assert.dom('a.btn').containsText('Edit'); + + await click('a.delete-button'); + await click(document.querySelector('.swal2-confirm')); + await settled(); + + await waitFor('.flash-message.alert-danger'); + assert.dom('.flash-message.alert-danger').containsText('We encountered an error deleting this draft submission'); + + assert.ok(handlerFake.calledOnce, 'Submission handler delete should be called'); }); test('Draft submissions should display Edit and Delete buttons', async function (assert) { From da3163dc0063a8035195f88d4f746315b33bd02b Mon Sep 17 00:00:00 2001 From: John Abrahams Date: Thu, 22 Feb 2024 11:16:05 -0500 Subject: [PATCH 3/4] Add flash message to submission details page for submission deletion --- app/controllers/submissions/detail.js | 15 ++++++++--- .../controllers/submissions/detail-test.js | 27 +++++++++++++++++++ 2 files changed, 38 insertions(+), 4 deletions(-) diff --git a/app/controllers/submissions/detail.js b/app/controllers/submissions/detail.js index 458d9dd1..7220c22c 100644 --- a/app/controllers/submissions/detail.js +++ b/app/controllers/submissions/detail.js @@ -474,10 +474,17 @@ export default class SubmissionsDetail extends Controller { if (result.value) { const ignoreList = this.searchHelper; - await this.submissionHandler.deleteSubmission(submission); - ignoreList.clearIgnore(); - ignoreList.ignore(submission.get('id')); - this.transitionToRoute('submissions'); + try { + await this.submissionHandler.deleteSubmission(submission); + + ignoreList.clearIgnore(); + ignoreList.ignore(submission.get('id')); + this.transitionToRoute('submissions'); + } catch (e) { + this.flashMessages.danger( + 'We encountered an error deleting this draft submission. Please try again later or contact your administrator' + ); + } } } } diff --git a/tests/unit/controllers/submissions/detail-test.js b/tests/unit/controllers/submissions/detail-test.js index 90d8f087..0c48a753 100644 --- a/tests/unit/controllers/submissions/detail-test.js +++ b/tests/unit/controllers/submissions/detail-test.js @@ -1,6 +1,7 @@ import EmberObject from '@ember/object'; import { module, test } from 'qunit'; import { setupTest } from 'ember-qunit'; +import Sinon from 'sinon'; module('Unit | Controller | submissions/detail', (hooks) => { setupTest(hooks); @@ -38,4 +39,30 @@ module('Unit | Controller | submissions/detail', (hooks) => { controller.send('deleteSubmission', submission); }); + + test('error message shown on submission deletion error', async function (assert) { + const submission = {}; + + // Mock global SweetAlert. Mocks a user clicking OK on the popup + swal = Sinon.fake.resolves({ value: true }); + + const controller = this.owner.lookup('controller:submissions/detail'); + const transitionFake = Sinon.replace(controller, 'transitionToRoute', Sinon.fake()); + + controller.submissionHandler = this.owner.lookup('service:submission-handler'); + const deleteFake = Sinon.replace(controller.submissionHandler, 'deleteSubmission', Sinon.fake.rejects()); + + controller.flashMessages = this.owner.lookup('service:flash-messages'); + const flashFake = Sinon.replace(controller.flashMessages, 'danger', Sinon.fake()); + + // Note: using controller.send resolves immediately + // making subsequent assertions evaluate before the controller action fires + // Can't really use Sinon in a nice way unless we call the controller + // method directly + await controller.deleteSubmission(submission); + + assert.ok(deleteFake.calledOnce, 'Submission handler delete should be called'); + assert.ok(flashFake.calledOnce, 'Flash message should be called'); + assert.equal(transitionFake.callCount, 0, 'Transition should not be called'); + }); }); From 53e83e0a707bab6dff51404ee826329fc4cfed3d Mon Sep 17 00:00:00 2001 From: John Abrahams Date: Mon, 26 Feb 2024 10:45:19 -0500 Subject: [PATCH 4/4] Fix search queries, consolidate file queries --- app/routes/submissions/new.js | 9 ++------- app/services/submission-handler.js | 14 +++++++------ app/util/paginated-query.js | 20 +++++++++++++++++++ .../unit/services/submission-handler-test.js | 2 +- 4 files changed, 31 insertions(+), 14 deletions(-) diff --git a/app/routes/submissions/new.js b/app/routes/submissions/new.js index 53e0eba3..b4908bff 100644 --- a/app/routes/submissions/new.js +++ b/app/routes/submissions/new.js @@ -2,6 +2,7 @@ import CheckSessionRoute from '../check-session-route'; import { inject as service } from '@ember/service'; import { set } from '@ember/object'; import { hash } from 'rsvp'; +import { fileForSubmissionQuery } from '../../util/paginated-query'; export default class NewRoute extends CheckSessionRoute { @service('workflow') @@ -64,13 +65,7 @@ export default class NewRoute extends CheckSessionRoute { sort: '+performedDate', }); - files = this.store - .query('file', { - filter: { - file: `submission.id==${newSubmission.get('id')}`, - }, - }) - .then((files) => [...files.toArray()]); + files = this.store.query('file', fileForSubmissionQuery(newSubmission.id)).then((files) => [...files.toArray()]); // Also seed workflow.doiInfo with metadata from the Submission const metadata = newSubmission.get('metadata'); diff --git a/app/services/submission-handler.js b/app/services/submission-handler.js index 85ad470e..5d538a79 100644 --- a/app/services/submission-handler.js +++ b/app/services/submission-handler.js @@ -5,6 +5,7 @@ import ENV from 'pass-ui/config/environment'; import { task } from 'ember-concurrency-decorators'; import { get } from '@ember/object'; import SubmissionModel from '../models/submission'; +import { fileForSubmissionQuery, submissionsWithPublicationQuery } from '../util/paginated-query'; /** * Service to manage submissions. @@ -255,19 +256,20 @@ export default class SubmissionHandlerService extends Service { } // Get submissions for this file - const files = await this.store.query('file', { filter: { submission: submission.id } }); + const files = await this.store.query('file', fileForSubmissionQuery(submission.id)); await Promise.all(files.map((file) => file.destroyRecord())); const publication = await submission.publication; // Search for Submissions that reference this publication - const subsWithThisPublication = await this.store.query('submission', { filter: { publication: publication.id } }); - if (subsWithThisPublication.length === 1) { + const submissionId = submission.id; + submission.deleteRecord(); + await submission.save(); + + const subsWithThisPublication = await this.store.query('submission', submissionsWithPublicationQuery(publication)); + if (subsWithThisPublication.length === 0) { publication.deleteRecord(); await publication.save(); } - - submission.deleteRecord(); - return submission.save(); } } diff --git a/app/util/paginated-query.js b/app/util/paginated-query.js index bf18c752..56268ad3 100644 --- a/app/util/paginated-query.js +++ b/app/util/paginated-query.js @@ -1,3 +1,7 @@ +/** + * Centralizing JSON-API queries are formatted using RSQL, which can be a bit awkward to write. + */ + /** * Paginated query for the submissions/index route * @@ -105,6 +109,22 @@ export function grantsIndexSubmissionQuery(user) { }; } +export function fileForSubmissionQuery(submissionId) { + return { + filter: { + file: `submission.id==${submissionId}`, + }, + }; +} + +export function submissionsWithPublicationQuery(publication) { + return { + filter: { + submission: `publication.id==${publication.id}`, + }, + }; +} + function filter(value, ...props) { if (!value) { return ''; diff --git a/tests/unit/services/submission-handler-test.js b/tests/unit/services/submission-handler-test.js index a3ac6bc9..d5b5265f 100644 --- a/tests/unit/services/submission-handler-test.js +++ b/tests/unit/services/submission-handler-test.js @@ -447,7 +447,7 @@ module('Unit | Service | submission-handler', (hooks) => { // Get files for the submission return Promise.resolve(files); case 'submission': - return Promise.resolve([submission]); + return Promise.resolve([]); default: return Promise.reject(); }