Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[ENHC0010020] Delete multiple samples from project samples table #629

Merged
merged 68 commits into from
Jun 26, 2024
Merged

[ENHC0010020] Delete multiple samples from project samples table #629

merged 68 commits into from
Jun 26, 2024

Conversation

ChrisHuynh333
Copy link
Collaborator

@ChrisHuynh333 ChrisHuynh333 commented Jun 5, 2024 •
edited
Loading

What does this PR do and why?

This PR adds the ability to delete multiple samples from a project samples table.

In addition to this, other changes related to the changes required for this PR include:

  • Fixing Samples and Sample Files Remove functionality to update file checkboxes accordingly
  • Updating infinite_scroll_controller to use singular/plural forms based on number of selected samples

Screenshots or screen recordings

Screencast.from.2024-06-19.09.36.05.AM.webm

Multi_status message:
image

Error:
image

How to set up and validate locally

Deleting Multiple Samples:

  1. Navigate to a project samples table
  2. Select samples
    Check the following:
  • The correct number of selected samples appears in the dialog description
  • The correct samples are added to the table listing
  1. Delete the samples. Make sure the expected samples were deleted

Singular/Plural descriptions in sample dialogs:

  1. Select one and multiple samples and open the clone/transfer/delete sample dialogs. Make sure the description uses the singular and plural descriptions accordingly.

Test single removal links while the item was selected with a checkbox

  • Test remaining checked samples after a removal still behave correctly

    1. Navigate to a project samples table
    2. Check off any number of samples
    3. Use the Remove link to remove a sample
    4. Ensure the correct number of selected samples is still displayed. You can test transferring/cloning/deleting samples in this state to ensure the sample selection is still behaving properly

  • Test "Select All" checkbox and disabled button states after single removal

    1. Select all samples
    2. Remove a single sample using the Remove link
    3. Ensure the "Select All" checkbox is still checked
    4. Remove all samples using the Remove link
    5. Ensure clone/transfer/delete samples and create export buttons enter the disabled state

    Repeat the above for the sample files

PR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

@ChrisHuynh333 ChrisHuynh333 self-assigned this Jun 6, 2024
@ChrisHuynh333 ChrisHuynh333 marked this pull request as ready for review June 6, 2024 17:15
@ChrisHuynh333 ChrisHuynh333 added enhancement New feature or request ready for review Pull request is ready for review labels Jun 6, 2024
@ChrisHuynh333 ChrisHuynh333 added on hold and removed ready for review Pull request is ready for review labels Jun 7, 2024
@ChrisHuynh333 ChrisHuynh333 force-pushed the delete-multiple-samples branch 2 times, most recently from 2e2d8fc to ecabed6 Compare June 11, 2024 14:43
@ChrisHuynh333 ChrisHuynh333 added ready for review Pull request is ready for review on hold and removed on hold ready for review Pull request is ready for review labels Jun 11, 2024
deepsidhu85
deepsidhu85 requested changes Jun 18, 2024
View reviewed changes
Copy link
Contributor

@deepsidhu85 deepsidhu85 left a comment

Choose a reason for hiding this comment

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

Just have one comment to start

app/controllers/projects/samples_controller.rb Outdated Show resolved Hide resolved
@joshsadam joshsadam mentioned this pull request Jun 18, 2024
Closed
1 task
@ChrisHuynh333 ChrisHuynh333 added ready for review Pull request is ready for review and removed on hold labels Jun 19, 2024
joshsadam
joshsadam reviewed Jun 20, 2024
View reviewed changes
config/routes/project.rb Outdated Show resolved Hide resolved
joshsadam
joshsadam requested changes Jun 20, 2024
View reviewed changes
Copy link
Contributor

@joshsadam joshsadam left a comment

Choose a reason for hiding this comment

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

This looks really good. I think I just have one comment below

joshsadam
joshsadam previously approved these changes Jun 20, 2024
View reviewed changes
Copy link
Contributor

@joshsadam joshsadam left a comment

Choose a reason for hiding this comment

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

I think this looks good to me

deepsidhu85
deepsidhu85 requested changes Jun 20, 2024
View reviewed changes
Copy link
Contributor

@deepsidhu85 deepsidhu85 left a comment

Choose a reason for hiding this comment

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

This is looking good just have one more comment for now

app/controllers/projects/samples_controller.rb Outdated Show resolved Hide resolved
deepsidhu85
deepsidhu85 requested changes Jun 20, 2024
View reviewed changes
Copy link
Contributor

@deepsidhu85 deepsidhu85 left a comment

Choose a reason for hiding this comment

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

As discussed instead of having methods such as new_* lets try to move this into a deletions_controller for samples

@ChrisHuynh333
Copy link
Collaborator Author

ChrisHuynh333 commented Jun 25, 2024 •
edited
Loading

As discussed instead of having methods such as new_* lets try to move this into a deletions_controller for samples

A deletions_controller has now been added to handle the dialog confirmation and destroy actions for sample deletion

deepsidhu85
deepsidhu85 requested changes Jun 25, 2024
View reviewed changes
Copy link
Contributor

@deepsidhu85 deepsidhu85 left a comment

Choose a reason for hiding this comment

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

Just a few comments for now

app/controllers/projects/samples/deletions_controller.rb Outdated Show resolved Hide resolved
app/controllers/projects/samples/deletions_controller.rb Outdated Show resolved Hide resolved
joshsadam
joshsadam previously approved these changes Jun 25, 2024
View reviewed changes
Copy link
Contributor

@joshsadam joshsadam left a comment

Choose a reason for hiding this comment

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

This looks good to me

deepsidhu85
deepsidhu85 requested changes Jun 25, 2024
View reviewed changes
Copy link
Contributor

@deepsidhu85 deepsidhu85 left a comment

Choose a reason for hiding this comment

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

A couple of changes to remove authorizations that are already being done in the services

app/controllers/projects/samples/deletions_controller.rb Outdated Show resolved Hide resolved
JeffreyThiessen
JeffreyThiessen previously approved these changes Jun 25, 2024
View reviewed changes
Copy link
Member

@JeffreyThiessen JeffreyThiessen left a comment

Choose a reason for hiding this comment

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

Looks great! 👍

deepsidhu85
deepsidhu85 previously approved these changes Jun 25, 2024
View reviewed changes
Copy link
Contributor

@deepsidhu85 deepsidhu85 left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@github-actions GitHub Actions
Copy link

Simplecov Report

Covered Threshold
92.54% 90%

deepsidhu85
deepsidhu85 approved these changes Jun 26, 2024
View reviewed changes
Copy link
Contributor

@deepsidhu85 deepsidhu85 left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@deepsidhu85 deepsidhu85 merged commit 8085ec6 into main Jun 26, 2024
3 checks passed
@deepsidhu85 deepsidhu85 deleted the delete-multiple-samples branch June 26, 2024 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@deepsidhu85 deepsidhu85 deepsidhu85 approved these changes

@joshsadam joshsadam joshsadam left review comments

@JeffreyThiessen JeffreyThiessen JeffreyThiessen left review comments

Assignees

@ChrisHuynh333 ChrisHuynh333

Labels
enhancement New feature or request ready for review Pull request is ready for review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ChrisHuynh333 @joshsadam @JeffreyThiessen @deepsidhu85