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

Samples table metadata fields toggle #348

Merged
merged 64 commits into from
Jan 26, 2024
Merged

Conversation

joshsadam
Copy link
Contributor

@joshsadam joshsadam commented Dec 22, 2023

What does this PR do and why?

Describe in detail what your merge request does and why.

Updated both Project > Sample and Group > Samples pages to include a toggle that, when on, will display all metadata fields for the displayed samples.

Resolves #281

Screenshots or screen recordings

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

Metadata toggle

image

Table with selected fields

image

How to set up and validate locally

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

  1. Ensure you re-seed the database as metadata fields have been added
  2. Go to a project sample page (e.g. http://localhost:3000/yersinia/yersinia-pseudotuberculosis/outbreak-2023/-/samples)
  3. Toggle the metadata, it should update the table with metadata

PR acceptance checklist

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

@joshsadam joshsadam self-assigned this Dec 22, 2023
@joshsadam joshsadam force-pushed the sample-metadata-templates branch 3 times, most recently from 5053242 to d4d11ea Compare January 12, 2024 14:41
@joshsadam joshsadam marked this pull request as ready for review January 12, 2024 15:48
@deepsidhu85
Copy link
Contributor

deepsidhu85 commented Jan 15, 2024

@joshsadam Still need to review the code but just had one comment after manually testing. If we select a metadata template and then transfer a sample, when the samples table reloads upon success, the metadata template that was applied is no longer set, and the table defaults to the columns without the metadata

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 a couple of comments below for the seed data and testing

db/seeds.rb Outdated Show resolved Hide resolved
test/system/projects/samples_test.rb Show resolved Hide resolved
deepsidhu85
deepsidhu85 previously approved these changes Jan 16, 2024
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 one comment below to move some methods into the application_controller

app/controllers/projects/samples_controller.rb Outdated Show resolved Hide resolved
@joshsadam joshsadam force-pushed the sample-metadata-templates branch 2 times, most recently from 6e569a6 to dcbb21f Compare January 24, 2024 14:17
@joshsadam joshsadam linked an issue Jan 24, 2024 that may be closed by this pull request
@joshsadam joshsadam changed the title Sample metadata templates Samples table metadata fields toggle Jan 24, 2024
@joshsadam joshsadam linked an issue Jan 24, 2024 that may be closed by this pull request
@ericenns ericenns linked an issue Jan 25, 2024 that may be closed by this pull request
@joshsadam joshsadam added ready for review Pull request is ready for review and removed on hold labels Jan 25, 2024
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 issues and left comments on them. I also found a z-index issue with viral_tooltip and the Sample column, see below:

image

app/views/groups/samples/index.turbo_stream.erb Outdated Show resolved Hide resolved
app/views/projects/samples/index.turbo_stream.erb Outdated Show resolved Hide resolved
app/views/projects/samples/_table.html.erb Outdated Show resolved Hide resolved
app/views/groups/samples/_table.html.erb Outdated Show resolved Hide resolved
app/controllers/concerns/metadata.rb Outdated Show resolved Hide resolved
Copy link

Simplecov Report

Covered Threshold
92.68% 90%

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!

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 and works as expected!

@deepsidhu85 deepsidhu85 merged commit 683ba4b into main Jan 26, 2024
2 checks passed
@deepsidhu85 deepsidhu85 deleted the sample-metadata-templates branch January 26, 2024 16:54
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.

Project Sample Metadata - Metadata views UI
3 participants