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

Y24-396: Refactor Automatic State Changers #2044

Open
wants to merge 18 commits into
base: y24-088-tuberacks-epic
Choose a base branch
from

Conversation

StephenHulme
Copy link
Contributor

@StephenHulme StephenHulme commented Nov 1, 2024

Closes #2002

Changes proposed in this pull request

  • Refactor labware state changers to be specialised by labware and use mixins for alternative behaviour
    • Solves bug where a tube thinks it's a plate and calls the contents_for method of DefaultStateChanger which was originally designed for plates and not tubes
  • Refactor and add state changer unit tests

Instructions for Reviewers

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

@StephenHulme
Copy link
Contributor Author

Will likely conflict with #2034, probably best to let that be merged in first, then refactor as required.

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.

Think this needs discussion re: tube aggregation
Plus I'm also touching this file in the tuberacks story Y24-099 to add a tube rack state changer.


api.work_completion.create!(submissions: in_prog_submissions, target: v2_labware.uuid, user: user_uuid)
def labware
raise 'Tubes are not supported by API V1'
Copy link
Member

Choose a reason for hiding this comment

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

That can't be true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not a path that appears to be used in the current iteration - I'm hoping it will be removed with @sdjmchattie's v1 API removal changes

spec/models/state_changers_spec.rb Outdated Show resolved Hide resolved
@StephenHulme StephenHulme changed the base branch from develop to y24-088-tuberacks-epic November 29, 2024 14:30
Copy link

codecov bot commented Dec 2, 2024

Codecov Report

Attention: Patch coverage is 87.50000% with 5 lines in your changes missing coverage. Please review.

Project coverage is 78.54%. Comparing base (6cbcc3f) to head (4d724e1).
Report is 55 commits behind head on y24-088-tuberacks-epic.

Files with missing lines Patch % Lines
app/models/state_changers.rb 87.17% 5 Missing ⚠️
Additional details and impacted files
@@                    Coverage Diff                     @@
##           y24-088-tuberacks-epic    #2044      +/-   ##
==========================================================
+ Coverage                   78.11%   78.54%   +0.43%     
==========================================================
  Files                         480      483       +3     
  Lines                       18155    18554     +399     
  Branches                      262      308      +46     
==========================================================
+ Hits                        14182    14574     +392     
- Misses                       3971     3978       +7     
  Partials                        2        2              
Flag Coverage Δ
javascript 70.76% <ø> (+0.86%) ⬆️
pull_request 80.44% <87.50%> (?)
push 78.11% <87.50%> (-0.01%) ⬇️
ruby 91.17% <87.50%> (+0.01%) ⬆️

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.

def v2_labware
@v2_labware ||= Sequencescape::Api::V2.tube_for_completion(labware_uuid)
@v2_labware ||= Sequencescape::Api::V2.tube_rack_for_completion(labware_uuid)
end

def labware
Copy link
Member

Choose a reason for hiding this comment

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

this labware vs v2_labware nonsense will hopefully get refactored out in a later story!

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.

Looks really good, I just don't like the tube aggregation references

@StephenHulme
Copy link
Contributor Author

Further refactoring required after #2099 is merged in

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Y24-396 - [BUG] Cancelling a PBMC tube
2 participants