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

Show both parent and child relationships on Labware #1641

Merged
merged 32 commits into from
Sep 4, 2024

Conversation

StephenHulme
Copy link
Contributor

@StephenHulme StephenHulme commented Mar 20, 2024

Closes a long outstanding bug-bear.
Possibly improves #645.
Dependent on sanger/General-Backlog-Items#405

Changes proposed in this pull request

  • Renames Children tab to Relatives
  • Displays all immediate parents and childre with purpose, barcode, and state
  • Provides easy navigation between relatives
  • Removes code that was part of the previous solution
  • Removes erroneously copied data-plate-view attribute

Here is a good example in UAT

Before:

Screenshot 2024-03-20 at 17 25 29

After:

Screenshot 2024-03-20 at 17 24 44

Instructions for Reviewers

See https://gitlab.internal.sanger.ac.uk/psd/integration-suite/-/merge_requests/154 for Integration Suite changes

[All PRs] - Confirm PR template filled
[Feature Branches] - Review code
[Production Merges to main]
    - Check story numbers included
    - Check for debug code
    - Check version

Copy link

codecov bot commented Mar 20, 2024

Codecov Report

Attention: Patch coverage is 35.71429% with 9 lines in your changes missing coverage. Please review.

Project coverage is 74.90%. Comparing base (0ee4b40) to head (94d3402).
Report is 33 commits behind head on develop.

Files with missing lines Patch % Lines
app/frontend/javascript/legacy_scripts_a.js 0.00% 5 Missing ⚠️
...p/models/labware_creators/final_tube_from_plate.rb 0.00% 1 Missing ⚠️
...s/labware_creators/multi_stamp_library_splitter.rb 0.00% 1 Missing ⚠️
...dels/labware_creators/plate_split_to_tube_racks.rb 0.00% 1 Missing ⚠️
...pp/models/labware_creators/quadrant_split_plate.rb 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1641      +/-   ##
===========================================
- Coverage    74.95%   74.90%   -0.06%     
===========================================
  Files          420      420              
  Lines        14300    14297       -3     
===========================================
- Hits         10719    10709      -10     
- Misses        3581     3588       +7     
Flag Coverage Δ
javascript 60.23% <0.00%> (-0.05%) ⬇️
pull_request 74.90% <35.71%> (?)
push 74.90% <35.71%> (-0.06%) ⬇️
ruby 91.15% <55.55%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@StephenHulme StephenHulme self-assigned this Mar 21, 2024
@StephenHulme StephenHulme added Enhancement New feature or request Size: S Small - low effort & risk Value: 4 Value to the insitute is high labels Mar 21, 2024
Copy link
Contributor

@sdjmchattie sdjmchattie 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. I don't know the logic well enough to comment on the validity of it, but nothing jumps out as bad. Nice work!

Copy link
Member

@andrewsparkes andrewsparkes 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 great but can we please test it for something where there are a LOT of parents. e.g. in Bioscan the LBSN-384 PCR 2 Pool tube will have 24 parent 384-well plates.
e.g. in Cardinal the LCA Blood Array plate could have 24 parent tubes
e.g. in scRNA Core the LRC Blood Bank could have many parent tubes

@StephenHulme
Copy link
Contributor Author

Good point @andrewsparkes, here's the updated screenshot for a LCA Blood Array plate showing how the 96 parents are handled. I have limited the heights of the parent and children boxes to 40% of the view-port height and added a scrollbar. There is now a number displayed to make it easy to count too.

Screenshot 2024-03-22 at 12 52 08

And here's one for the children on a LRC PBMC Bank plate.

Screenshot 2024-03-22 at 12 55 06

@SujitDey2022 SujitDey2022 added the Could've MoSCoW Prioritization - Could/Good to Have Capability/Feature label Apr 4, 2024
@KatyTaylor
Copy link
Contributor

Was this one paused because it broke the integration suite selenium tests? Or was that another one? I would advocate for merging this in if we're happy it works well.

@StephenHulme
Copy link
Contributor Author

Was this one paused because it broke the integration suite selenium tests? Or was that another one?

Yes, that's right.

Integration suite broke quite badly because it was relying on the undelying .tube-list and .plate-list classes which was refactored out of this PR.

If there is value in fixing IntSuite to try find and parse the barcode names/links themselves, then I'm happy to look at this again.

@KatyTaylor
Copy link
Contributor

Was this one paused because it broke the integration suite selenium tests? Or was that another one?

Yes, that's right.

Integration suite broke quite badly because it was relying on the undelying .tube-list and .plate-list classes which was refactored out of this PR.

If there is value in fixing IntSuite to try find and parse the barcode names/links themselves, then I'm happy to look at this again.

I think there's a lot of value in this PR. Is there an issue in the backlog for this? If not, we could make one and put it forward for prioritisation? Just so that work is tracked, as it may not be trivial!

@StephenHulme
Copy link
Contributor Author

Updated and ready for review, Integration Suite changes can be found here

Copy link
Contributor

@BenTopping BenTopping left a comment

Choose a reason for hiding this comment

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

Works well 👌

app/views/labware/_basic_relative.html.erb Outdated Show resolved Hide resolved
app/frontend/javascript/legacy_scripts_a.js Outdated Show resolved Hide resolved
Copy link

codeclimate bot commented Sep 2, 2024

Code Climate has analyzed commit 94d3402 and detected 2 issues on this pull request.

Here's the issue category breakdown:

Category Count
Duplication 2

The test coverage on the diff in this pull request is 55.5% (50% is the threshold).

This pull request will bring the total coverage in the repository to 91.1% (0.0% change).

View more on Code Climate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Could've MoSCoW Prioritization - Could/Good to Have Capability/Feature Enhancement New feature or request Size: S Small - low effort & risk Value: 4 Value to the insitute is high
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants