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

API v2 Milestone 3 #2099

Open
wants to merge 141 commits into
base: develop
Choose a base branch
from
Open

API v2 Milestone 3 #2099

wants to merge 141 commits into from

Conversation

sdjmchattie
Copy link
Contributor

Closes #1818

Changes proposed in this pull request

All has been reviewed before, but this is a last once over before going into develop.

  • Update usage of v2 endpoints on Sequencescape:
    • bulk_transfers
    • plates
    • plate_conversions
    • plate_creations
    • qc_files
    • submission_pools
    • transfer_request_collections
    • transfers
    • tubes
    • tube_from_tube_creations
  • Update tests accordingly.

Instructions for Reviewers

[All PRs] - Confirm PR template filled
[Feature Branches] - Review code

when preparing transfer_requests_attributes
spec/features/pooling_multiple_tubes_into_one_tube_spec.rb
spec/models/labware_creators/donor_pooling_plate_spec.rb
spec/models/labware_creators/plate_split_to_tube_racks_spec.rb
spec/models/labware_creators/pooled_wells_by_sample_in_groups_spec.rb
to be fair, this was a good suggestion!
@sdjmchattie sdjmchattie self-assigned this Dec 2, 2024
Copy link

codecov bot commented Dec 2, 2024

Codecov Report

Attention: Patch coverage is 97.22814% with 13 lines in your changes missing coverage. Please review.

Project coverage is 80.65%. Comparing base (2fbce2e) to head (f2b18a1).
Report is 4 commits behind head on develop.

Files with missing lines Patch % Lines
app/models/concerns/presenters/extended_csv.rb 0.00% 7 Missing ⚠️
...pp/models/labware_creators/cardinal_pools_plate.rb 33.33% 2 Missing ⚠️
.../models/labware_creators/pooled_tubes_by_sample.rb 0.00% 2 Missing ⚠️
...s/labware_creators/multi_stamp_library_splitter.rb 0.00% 1 Missing ⚠️
app/models/labware_creators/ten_stamp.rb 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2099      +/-   ##
===========================================
+ Coverage    80.43%   80.65%   +0.21%     
===========================================
  Files          471      477       +6     
  Lines        17932    18146     +214     
  Branches       268      268              
===========================================
+ Hits         14424    14636     +212     
- Misses        3506     3508       +2     
  Partials         2        2              
Flag Coverage Δ
javascript 74.05% <100.00%> (+0.44%) ⬆️
pull_request 80.65% <97.22%> (+0.21%) ⬆️
push 80.65% <97.22%> (+0.21%) ⬆️
ruby 91.28% <90.84%> (+<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.

response = api.qc_file.find(params[:id]).retrieve
filename = /filename="([^"]*)"/.match(response['Content-Disposition'])[1] || 'unnamed_file'
send_data(response.body, filename: filename, type: 'sequencescape/qc_file')
qc_file = Sequencescape::Api::V2::QcFile.find(uuid: params[:id]).first
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way we can hide the long winded Sequencescape::Api::V2:: naming all over the codebase? for api 1 we seem to do the much shorter api naming

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could (in a separate piece of work) create an alias for the Sequencescape::Api::V2 module. Maybe something like SSAPIv2 so that these would become SSAPIv2::QcFile etc. I don't think we should name the real module differently, but an alias would be a reasonable approach.

This allows us to get the stock_plate and its attributes without making requests repeatedly over the API
stock_plates.order(id: :asc).last
# Note that it's only when we call last that we get an actual object to cache.
# If we cache the query to the API, then it will still be made every time we call stock_plate.
@stock_plate ||= fetch_stock_plate_ancestors.order(id: :asc).last
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@andrewsparkes and @StephenHulme This is the fix for the repeated requests for stock plates. I've added a comment to make it clearer to future maintainers, but basically we were caching the query object back in the stock_plates method and not the stock plate itself. The query object is cheap to create and hasn't made a request to the API at the time we were caching it. Instead we need to cache the response which is only obtained after you call last on this line. This gives us the speed up we need AFAIK.


it 'returns the stock plates because that must be cool' do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think someone was having a bad day when they chose these test names.

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-190-3: Use SS API v2 endpoints for Labware Creators
4 participants