View deleted Submissions and delete/undelete Submissions. #1043

sadiqkhoja · 2024-10-25T18:21:37Z

What has been done to verify that this works as intended?

Wrote bunch of tests, also manually run different scenarios.

Why is this the best possible solution? Were any other approaches considered?

Showing deleted Submissions: I have used existing odata request data resource instead of creating a separate resource because that would have required lot of duplicate functionality and re/writing of associated tests.

deletedSubmissionCount: resource is created in the submissions.js module to keep related stuff together, because of this I had to call useSubmissions() in the parent components of SubmissionList i.e. FormSubmissions and FormDraftTesting. Another approach is to create it in a separate module which wouldn't require moving of useSubmissions() to parent component.

deleted prop: is added to multiple components, which seems more suitable in this case than the provide/inject approach because this prop is used by every intermediate component rather than being passed down transparently..

Delete/Undeleted: I have followed the same pattern of deleting entity.

Spinner on delete/undelete button: This prevents double-clicking of the button, which would cause two requests to the backend—one of them would fail and error message would be shown. The awaiting response state is maintained in a Set, which causes prop re-evaluation for every row in the table, but DOM is not repainted for all of them, so it should not significantly impact performance. Additionally, we are soon going to implement pagination that will mitigate any performance hit if any.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

It's a big change but it only affects the list of Submissions. So there shouldn't be any side effects in other parts of the application.

Does this change require updates to user documentation? If so, please file an issue here and include the link below.

getodk/docs#1873

Before submitting this PR, please make sure you have:

run npm run test and npm run lint and confirmed all checks still pass OR confirm CircleCI build passes
verified that any code or assets from external sources are properly credited in comments or that everything is internally sourced

src/components/date-range-picker.vue

src/composables/disabled.js

src/components/form/submissions.vue

src/components/submission/list.vue

src/components/submission/table.vue

matthew-white

I'm still reviewing things, but I wanted to share the comments that I have so far. There are a few of these comments, but they're mostly small things. Overall things are looking great so far.

src/components/submission/delete.vue

src/components/submission/restore.vue

src/components/submission/delete.vue

src/locales/en.json5

src/components/form/submissions.vue

src/components/submission/list.vue

src/components/submission/show.vue

matthew-white

More small comments! Mostly I just need to look at tests now.

src/components/submission/metadata-row.vue

src/locales/en.json5

src/components/form/submissions.vue

test/util/http.js

src/locales/en.json5

src/components/submission/metadata-row.vue

src/components/submission/list.vue

src/components/form/submissions.vue

matthew-white

Looks great! ✨

matthew-white · 2024-11-01T21:14:35Z

test/components/submission/restore.spec.js

+    props: { state: true, checkbox: true }
+  }));
+
+describe('SubmissionRetore', () => {


Suggested change

describe('SubmissionRetore', () => {

describe('SubmissionRestore', () => {

matthew-white · 2024-11-01T21:21:13Z

test/components/submission/metadata-row.spec.js

+  describe('deleted', () => {
+    it('shows the deleted date', () => {
+      const { deletedAt } = testData.extendedSubmissions.createPast(1, { deletedAt: new Date().toISOString() }).last();
+      mountComponent({ deleted: true }).findAllComponents(DateTime)[1].props().iso.should.equal(deletedAt);


If you're going to use findAllComponents(), could you make an assertion about the length of the resulting array? If we add another <date-time> at some point, then [1] won't be the last <date-time>. Alternatively, I think you could do something like:

Suggested change

mountComponent({ deleted: true }).findAllComponents(DateTime)[1].props().iso.should.equal(deletedAt);

mountComponent({ deleted: true }).get('.state-and-actions').getComponent(DateTime).props().iso.should.equal(deletedAt);

matthew-white · 2024-11-02T04:08:44Z

test/components/submission/metadata-row.spec.js

+
+  it('does not show the delete button if user does not have submission delete permission', async () => {
+    mockLogin({ role: 'none' });
+    testData.extendedSubmissions.createPast(1, { role: 'viewer' });


I don't think testData.extendedSubmissions will use { role: 'viewer' }. It needs to be passed to testData.extendedProjects:

testData.extendedProjects.createPast(1, { role: 'viewer' }); testData.extendedSubmissions.createPast(1);

matthew-white · 2024-11-02T04:11:26Z

test/components/submission/metadata-row.spec.js

+
+  it('does not show the restore button if user does not have submission restore permission', async () => {
+    mockLogin({ role: 'none' });
+    testData.extendedSubmissions.createPast(1, { deletedAt: new Date().toISOString(), role: 'viewer' });


Similar to the comment above, { role: 'viewer' } needs to be passed to testData.extendedProjects.createPast().

matthew-white · 2024-11-02T04:16:28Z

src/components/submission/list.vue

        .finally(() => { this.refreshing = false; })
        .catch(noop);

+      // emit event to parent component to re-fetch deleted Submissions count
+      if (refresh && !this.deleted) {


Suggested change

if (refresh && !this.deleted) {

if (refresh && !this.deleted && !this.draft) {

matthew-white · 2024-11-04T05:40:25Z

test/components/submission/list.spec.js

+        });
+    });
+
+    describe('deleting after checking the checkbox', () => {


Suggested change

describe('deleting after checking the checkbox', () => {

describe('undeleting after checking the checkbox', () => {

matthew-white · 2024-11-04T05:53:59Z

src/components/submission/list.vue

+      })
+        .then(() => {
+          this.deleteModal.hide();
+          this.deletedSubmissionCount.value += 1;


If the request for deletedSubmissionCount failed, then there isn't a count to increment:

Suggested change

this.deletedSubmissionCount.value += 1;

if (this.deletedSubmissionCount.dataExists) this.deletedSubmissionCount.value += 1;

matthew-white · 2024-11-04T06:00:22Z

src/components/submission/list.vue

@@ -29,15 +30,24 @@ except according to the terms contained in the LICENSE file.
          </button>
        </form>
        <submission-download-button :form-version="formVersion"
+          :aria-disabled="deleted" v-tooltip.aria-describedby="deleted ? $t('downloadDisabled') : null"
          :filtered="odataFilter != null" @download="downloadModal.show()"/>


Suggested change

:filtered="odataFilter != null" @download="downloadModal.show()"/>

:filtered="odataFilter != null && !deleted" @download="downloadModal.show()"/>

After the user clicks .toggle-deleted-submissions, the download button should show the number of regular/non-deleted submissions, not the number of deleted submissions. Without this change, I think it will show the number of deleted submissions, since that's what odata.count will be.

After the user clicks .toggle-deleted-submissions, I think the download button also shouldn't say anything about "matching Submissions".

matthew-white · 2024-11-04T06:25:33Z

src/components/form/submissions.vue

+        <template v-if="deletedSubmissionCount.dataExists">
+          <button v-if="canUpdate && (deletedSubmissionCount.value > 0 || deleted)" type="button"
+            class="btn toggle-deleted-submissions" :class="{ 'btn-danger': deleted, 'btn-link': !deleted }"
+            @click="deleted = !deleted">


If I filter the submissions, then I click the "# deleted Submissions" button, what happens to the filters? They remain in place, right? So if I click the "# deleted Submissions" button again, the filters will start applying again? I feel like there's some potential for confusion in that interaction. Even though the user can't change the filters while viewing deleted submissions, I think they might still wonder whether the filters are being applied to what they're seeing. Maybe it'd be better to reset the filters whenever the "# deleted Submissions" button is clicked. Instead of setting deleted on this line, @click could call a method like this:

toggleDeleted() { const { path } = this.$route; this.$router.push(this.deleted ? path : `${path}?deleted=true`); }

By changing the route like this, all the filters will be reset: fromQuery will be called for each of them.

We could also wait to make this change and see what issa and the QA team think.

matthew-white · 2024-11-04T06:38:06Z

src/components/submission/activity.vue

@@ -30,6 +30,11 @@ except according to the terms contained in the LICENSE file.
            <span class="icon-pencil"></span>{{ $t('action.edit') }}
          </button>
        </template>
+        <button v-if="project.dataExists && project.permits('submission.delete')"


How about adding a test that a project viewer cannot see this button?

sadiqkhoja commented Oct 25, 2024

View reviewed changes

sadiqkhoja force-pushed the features/delete-submissions2 branch from 7809977 to a6f86ac Compare October 30, 2024 16:01

sadiqkhoja changed the title ~~Delete Submissions~~ View deleted Submissions and delete/undelete Submissions. Oct 30, 2024

sadiqkhoja marked this pull request as ready for review October 30, 2024 17:21

sadiqkhoja requested a review from matthew-white October 30, 2024 17:21

matthew-white reviewed Oct 31, 2024

View reviewed changes

Make multiselect and data range picker disableable

a48ac54

sadiqkhoja force-pushed the features/delete-submissions2 branch 5 times, most recently from 6460d16 to 8a8f747 Compare October 31, 2024 21:18

matthew-white reviewed Nov 1, 2024

View reviewed changes

sadiqkhoja force-pushed the features/delete-submissions2 branch from 8a8f747 to 27871b0 Compare November 1, 2024 16:04

sadiqkhoja requested a review from matthew-white November 1, 2024 16:43

matthew-white approved these changes Nov 4, 2024

View reviewed changes

sadiqkhoja added 5 commits November 4, 2024 09:42

Show deleted Submissions

687c811

Add Delete Submission button in the Submission list

e238598

Add delete Submission button on the Submission activity feed

f1c5415

Add undelete button in the Submission list

5808302

Add spinner to the delete/undelete buttons in the Submission list

9513fd9

sadiqkhoja force-pushed the features/delete-submissions2 branch from 27871b0 to a028868 Compare November 4, 2024 16:23

Change the color of deleted at column to red

0b74e3c

sadiqkhoja force-pushed the features/delete-submissions2 branch from a028868 to 0b74e3c Compare November 4, 2024 16:26

sadiqkhoja merged commit acd5cf6 into master Nov 4, 2024
1 check passed

sadiqkhoja deleted the features/delete-submissions2 branch November 4, 2024 16:33

This was referenced Nov 10, 2024

Disable delete button during request to delete entity getodk/central#787

Open

Use v-once and/or v-memo with submissions table #771

Closed

matthew-white mentioned this pull request Nov 19, 2024

Check submission.delete verb before listing deleted submissions #1056

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

View deleted Submissions and delete/undelete Submissions. #1043

View deleted Submissions and delete/undelete Submissions. #1043

sadiqkhoja commented Oct 25, 2024 •

edited

Loading

matthew-white left a comment

matthew-white left a comment

matthew-white left a comment

matthew-white Nov 1, 2024

matthew-white Nov 1, 2024

matthew-white Nov 2, 2024

matthew-white Nov 2, 2024

matthew-white Nov 2, 2024

matthew-white Nov 4, 2024

matthew-white Nov 4, 2024

matthew-white Nov 4, 2024

matthew-white Nov 4, 2024

matthew-white Nov 4, 2024

	describe('SubmissionRetore', () => {
	describe('SubmissionRestore', () => {

	mountComponent({ deleted: true }).findAllComponents(DateTime)[1].props().iso.should.equal(deletedAt);
	mountComponent({ deleted: true }).get('.state-and-actions').getComponent(DateTime).props().iso.should.equal(deletedAt);

	if (refresh && !this.deleted) {
	if (refresh && !this.deleted && !this.draft) {

	describe('deleting after checking the checkbox', () => {
	describe('undeleting after checking the checkbox', () => {

	this.deletedSubmissionCount.value += 1;
	if (this.deletedSubmissionCount.dataExists) this.deletedSubmissionCount.value += 1;

	:filtered="odataFilter != null" @download="downloadModal.show()"/>
	:filtered="odataFilter != null && !deleted" @download="downloadModal.show()"/>

View deleted Submissions and delete/undelete Submissions. #1043

View deleted Submissions and delete/undelete Submissions. #1043

Conversation

sadiqkhoja commented Oct 25, 2024 • edited Loading

What has been done to verify that this works as intended?

Why is this the best possible solution? Were any other approaches considered?

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Does this change require updates to user documentation? If so, please file an issue here and include the link below.

Before submitting this PR, please make sure you have:

matthew-white left a comment

Choose a reason for hiding this comment

matthew-white left a comment

Choose a reason for hiding this comment

matthew-white left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sadiqkhoja commented Oct 25, 2024 •

edited

Loading