Skip to content

feature(attachments): add bulk deleting attachments using the row selector in table with mock api TASK-1516 TASK-1710 #5626

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 28 commits into from
Apr 14, 2025

Conversation

duvld
Copy link
Member

@duvld duvld commented Mar 27, 2025

πŸ—’οΈ Checklist

  1. run linter locally
  2. update all related docs (API, README, inline, etc.), if any
  3. draft PR with a title <type>(<scope>)<!>: <title> TASK-1234
  4. tag PR: at least frontend or backend unless it's global
  5. fill in the template below and delete template comments
  6. review thyself: read the diff and repro the preview as written
  7. open PR & confirm that CI passes
  8. request reviewers, if needed
  9. delete this section before merging

πŸ“£ Summary

Adds a way to bulk delete attachments for submissions through the table view bulk options.

πŸ‘€ Preview steps

Standard workflow:

  1. Enable the removingAttachmentsEnabled feature flag
  2. Have a survey with every media type as questions (image, video, audio, file)
  3. Submit responses to the survey with some combination of the media types. I think the following is the simplest way to test all permutations:
  • Submit 1 response for each media type
  • Submit 1 response that has every media type
  1. Select some combination of responses via the bulk selection checkbox
  2. Click "Delete media files only"
  3. 🟒 Notice that the list of media files in the modal accurately represents the available media files that can be deleted
  4. 🟒 Notice that the "Delete" button is disabled until the user clicks the acknowledgement checkbox

Some deleted files workflow:

  1. Copy steps 1-3 above
  2. Mock some deleted attachments
    • To mock it, you can override submissions object in submissionsActions.getSubmissions.listen from jsapp/js/actions/submissions.ts file, like so:
      response.results.forEach((submission) => {
        submission._attachments.forEach((attachment) => {
          attachment.is_deleted = true
        })
      })
      
  • Specifying only some responses via response.results[<index>]._attachments is useful to test how many attachments/submissions are displayed in the modal
  1. Select a combination of responses that include ones with all their attachments deleted (as done above)
  2. 🟒 Notice that the list of media types in the modal only show those that are not deleted or have the is_deleted tag
  3. 🟒 Notice that if the user selects a submission (or a set of submissions) with no valid attachments then the "Delete media files only" does not appear
  4. 🟒 Notice that submissions that don't have any valid attachments aren't counted towards the deleted submissions

@duvld duvld added the Front end label Apr 4, 2025
@duvld duvld marked this pull request as ready for review April 4, 2025 19:24
@duvld duvld requested a review from magicznyleszek as a code owner April 4, 2025 19:24

interface BulkDeleteMediaFilesProps {
selectedSubmissions: SubmissionResponse[]
selectedRowIds: string[] // an array of the selected submission UIDs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't fully understand these two. One of these is a list of submission responses, and the other is a list of submission ids? So like first is a raw data of all possible submissions with attachments, and the second is a list of selected submissions?

Could we do this differently?

One idea would be to only have one prop, which is SubmissionResponse[] but only of selected rows and only ones with attachments - this would require parent of the component to have some logic

Other idea would be to move that getSelectedSubmissionsWithAttachments logic here. So we would pass whole unfiltered SubmissionResponse[] (so it would factually be selectedSubmissions, and not selectedSubmissionsButOnlyOnesWithAttachments). Then you don't need selectedRowIds, because you will get that from selectedSubmissions, right?

Copy link
Member Author

@duvld duvld Apr 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The prop containing a row of Ids was a leftover so I got rid of it. The idea is to implement the first option (which I think is successful after I fix the comment previous to this).

I originally wanted to do the second option because I don't want the parent component to have extra logic but it lead to a dependency loop that I didn't want to spend budget time to fix for the MVP. I think in general we'd want to go with less logic for the parent components if reasonable, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in general we'd want to go with less logic for the parent components if reasonable, right?

Yes :)

{getMediaCount(props.selectedSubmissions)}
</Text>
}
onClick={() => setWarningAcknowledged(!warningAcknowledged)}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should also have checked={warningAcknowledged}

</Button>

<Button
disabled={!warningAcknowledged || isDeletePending}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have found a bug:

  1. Open this bulk delete modal
  2. 🟒 checkbox is unchecked, and "Delete" button is disabled
  3. Check the checkbox
  4. Close the modal with "x"
  5. Open modal again
  6. πŸ› checkbox is unchecked, and "Delete" button is not disabled

This is happening because warningAcknowledged is not being reset to false when modal is being closed.

@duvld duvld merged commit 504bed7 into main Apr 14, 2025
5 checks passed
@duvld duvld deleted the anji/1516-bulk-delete-api branch April 14, 2025 19:05
@duvld duvld restored the anji/1516-bulk-delete-api branch April 14, 2025 19:05
@duvld duvld deleted the anji/1516-bulk-delete-api branch April 14, 2025 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants