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.

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

Groups/samples_count #783

Merged
merged 41 commits into from
Oct 29, 2024
Merged

Groups/samples_count #783

merged 41 commits into from
Oct 29, 2024

Conversation

malchua
Copy link
Contributor

@malchua malchua commented Sep 25, 2024

What does this PR do and why?

Describe in detail what your merge request does and why.

This PR adds a samples_count column to Groups to show the total number of samples contained within a group. This also includes all samples in projects within subgroups.

Screenshots or screen recordings

Screenshots are required for UI changes, and strongly recommended for all other pull requests.
image

How to set up and validate locally

Numbered steps to set up and validate the change are strongly suggested.

  1. Sign in
  2. Click on "Groups" on the sidebar
  3. Find a group where you are the owner and take note of the number of samples listed (number next to the beaker icon)
  4. Select the group
  5. Look through "Subgroups and projects" and check that the number of samples is accumulating correctly as you move up from projects to subgroups or subgroups to the parent group
  6. Compare the number of samples listed in step 3 and see that it equals the accumulated number from step 5
  7. Select a project within that group
  8. Click on "Samples" on the sidebar
  9. Add a new sample to the project
  10. Go back to the parent group and check to see that the number of samples increased by one
  11. Repeat steps 8-10 but delete samples
  12. Repeat steps 8-10 but clone samples
  13. Repeat steps 8-10 but transfer samples
  14. Click on "Projects" on the sidebar
  15. Select a project where you are the owner
  16. Select "Settings" -> "General" in the sidebar
  17. Go to the Transfer project section and transfer the project to a different parent group
  18. Click on "Projects" on the sidebar
  19. Find the parent group and check that the sample counts are correct.
  20. Go back into the same project and delete the project
  21. Click on "Projects" on the sidebar
  22. Find the parent group of the deleted project
  23. Check that the sample counts are correct
  24. Repeat steps 14-23 but transfer and delete a group instead

PR acceptance checklist

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

@malchua malchua force-pushed the groups/samples_count branch from bbb1ca7 to 62f2e53 Compare October 1, 2024 23:44

This comment has been minimized.

@malchua malchua force-pushed the groups/samples_count branch from ccd7ee7 to a09d761 Compare October 3, 2024 14:52

This comment has been minimized.

@malchua malchua marked this pull request as ready for review October 3, 2024 15:14
@malchua
Copy link
Contributor Author

malchua commented Oct 3, 2024

I found a bug where deleting a project doesn't properly update the samples_count for the parent groups. Currently working on it.

@malchua
Copy link
Contributor Author

malchua commented Oct 3, 2024

I found a bug where deleting a project doesn't properly update the samples_count for the parent groups. Currently working on it.

Fixed in 5eb4621

@malchua malchua force-pushed the groups/samples_count branch from 5eb4621 to 59cde47 Compare October 3, 2024 18:35

This comment has been minimized.

@malchua malchua added the ready for review Pull request is ready for review label Oct 3, 2024
Copy link
Collaborator

@ChrisHuynh333 ChrisHuynh333 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 comments at first glance.

You'll want to add UI tests as well for each of the actions (ie: after deleting a sample, return to the group index page and verify the sample count in the group's table row is accurate).

@malchua malchua force-pushed the groups/samples_count branch from ac91d50 to 6e93c79 Compare October 16, 2024 08:17

This comment has been minimized.

@deepsidhu85
Copy link
Contributor

@malchua I was testing out your PR and came across this:

image

The number of samples is not being displayed for the projects on the Subgroups and projects tab when a group is expanded

This comment has been minimized.

@malchua malchua removed the ready for review Pull request is ready for review label Oct 17, 2024
@malchua malchua force-pushed the groups/samples_count branch from 935e539 to dcc4e5d Compare October 17, 2024 21:44

This comment has been minimized.

@malchua
Copy link
Contributor Author

malchua commented Oct 17, 2024

@malchua I was testing out your PR and came across this:

image

The number of samples is not being displayed for the projects on the Subgroups and projects tab when a group is expanded

Great catch! Should be fixed in 884c4b0

@malchua
Copy link
Contributor Author

malchua commented Oct 17, 2024

After talking to Eric, he recommended that I add the missing UI tests for the project samples_counters in a different PR. I will work on that PR after this one.

@malchua malchua added the ready for review Pull request is ready for review label Oct 17, 2024
Copy link
Collaborator

@ChrisHuynh333 ChrisHuynh333 left a comment

Choose a reason for hiding this comment

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

@ericenns @deepsidhu85 Feel free to chime in if you guys don't feel the same about the following.

I think you'll have to add group.samples_count manually to all groups like you have for projects.

In addition, I'd add a test to each of group and project models to loop through all projects/groups and check the current fixture samples_count to the actual samples_count (retrieve the real samples_count from Project.reset_counters / your group scope). This will help keep everyone accountable to update the sample_counts as we continue to add samples to our fixtures and keep things accurate. Add a comment to each test with something like If you've added samples to fixtures, update the sample_count to the corresponding project/group as the project/group won't be specified when the test fails.

test/controllers/groups_controller_test.rb Outdated Show resolved Hide resolved
test/controllers/groups_controller_test.rb Outdated Show resolved Hide resolved
test/models/group_test.rb Outdated Show resolved Hide resolved
test/services/samples/destroy_service_test.rb Outdated Show resolved Hide resolved
test/system/groups_test.rb Outdated Show resolved Hide resolved
app/services/groups/destroy_service.rb Outdated Show resolved Hide resolved
test/services/groups/destroy_service_test.rb Outdated Show resolved Hide resolved
Copy link
Member

@ericenns ericenns left a comment

Choose a reason for hiding this comment

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

I found a few edge cases where samples_count is not properly updated.

app/services/projects/transfer_service.rb Outdated Show resolved Hide resolved
app/services/samples/transfer_service.rb Outdated Show resolved Hide resolved
@malchua malchua force-pushed the groups/samples_count branch from 2dc414b to 3777cbb Compare October 21, 2024 19:33
@malchua malchua removed the ready for review Pull request is ready for review label Oct 24, 2024
@malchua malchua force-pushed the groups/samples_count branch from 3777cbb to 59587ac Compare October 28, 2024 16:58
@malchua malchua added the ready for review Pull request is ready for review label Oct 28, 2024
Copy link
Collaborator

@ChrisHuynh333 ChrisHuynh333 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!

Copy link
Member

@ericenns ericenns 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!

@ericenns ericenns merged commit a83d024 into main Oct 29, 2024
3 checks passed
@ericenns ericenns deleted the groups/samples_count branch October 29, 2024 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review Pull request is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants