Y24-061: Send Pool XP export message #4147

sdjmchattie · 2024-06-21T14:33:48Z

Changes proposed in this pull request

Extract code for creating compound samples.
Build an endpoint and job that will create an export message.
Encode the message with Avro and publish it to RabbitMQ.

Instructions for Reviewers

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

codeclimate · 2024-06-21T14:55:33Z

app/controllers/api/v2/bioscan/export_pool_xp_to_traction_controller.rb

+
+        def create
+          Rails.logger.debug "DEBUG: In ExportPoolXpToTractionController create"
+          # TODO: do we need to check the tube barcode is valid here?


codeclimate · 2024-06-21T14:55:33Z

app/controllers/api/v2/bioscan/export_pool_xp_to_traction_controller.rb

+          Rails.logger.debug "DEBUG: In ExportPoolXpToTractionController create"
+          # TODO: do we need to check the tube barcode is valid here?
+          Delayed::Job.enqueue ExportPoolXpToTractionJob.new(barcode)
+          # TODO: check if job queuing was successful or not and return appropriate status


codeclimate · 2024-06-21T14:55:33Z

app/controllers/api/v2/bioscan/export_pool_xp_to_traction_controller.rb

+          # TODO: do we need to check the tube barcode is valid here?
+          Delayed::Job.enqueue ExportPoolXpToTractionJob.new(barcode)
+          # TODO: check if job queuing was successful or not and return appropriate status
+          # TODO: how to know if job successfully queued?


codeclimate · 2024-06-21T14:55:33Z

app/controllers/api/v2/bioscan/export_pool_xp_to_traction_controller.rb

+          Delayed::Job.enqueue ExportPoolXpToTractionJob.new(barcode)
+          # TODO: check if job queuing was successful or not and return appropriate status
+          # TODO: how to know if job successfully queued?
+          # TODO: does rendering something here send something back to Limber?


codeclimate · 2024-06-21T14:55:33Z

app/jobs/export_pool_xp_to_traction_job.rb

+
+    def connection_params
+      connection_params = {
+        host: configatron.amqp.isg.host,


ExportPoolXpToTractionJob#connection_params calls 'configatron.amqp' 5 times

app/jobs/export_pool_xp_to_traction_job.rb

app/controllers/api/v2/bioscan/export_pool_xp_to_traction_controller.rb

app/jobs/export_pool_xp_to_traction_job.rb

KatyTaylor · 2024-06-28T09:56:54Z

app/jobs/export_pool_xp_to_traction_job.rb

+          boxBarcode: "Unspecified"
+        },
+        request: {
+          costCode: project.project_cost_code,


Is it worth doing project&.project_cost_code... not sure if tube.projects is able to return nil. If so actually, would need to do tube.projects&.first as well 🤔

Similar question with tube.studies.first and study.uuid

Hmm this is a tricky one. costCode is a required field and so is the study UUID. So if they were nil there's not a lot I could do to send the message and it might be better for that to bubble up to create an error log, rather than hide it in safe-chaining.

OK gotcha. You could throw an explicit exception for it rather than let the nil pointer happen? So you could say something useful like 'expected data is not present - costcode'.

Yes fair. If we leave it as is, the exception will say something about not being able to get attribute project_cost_code on nil object. If I do the safe-chaining as you suggest and it gets to the point where it's going to encode the message using Avro, you'll get a useful message back from fastavro about the field in the message not being allowed to be null.

app/jobs/export_pool_xp_to_traction_job.rb

Co-authored-by: KatyTaylor <kt17@sanger.ac.uk>

app/jobs/export_pool_xp_to_traction_job.rb

KatyTaylor · 2024-06-28T13:20:52Z

config/config.rb

+  configatron.amqp.isg.password = 'development'
+  configatron.amqp.isg.exchange = 'sequencescape'
+
+  configatron.amqp.schemas.registry_url = 'http://redpanda.uat.psd.sanger.ac.uk/subjects/'


Do we want local config to be dependent on the UAT instance? Maybe it's not dependent...

I had considered this when I made this choice and I don't know that there are any simple and easy answers. If I make it dependent on a local instance of RedPanda, that adds dependencies to Sequencescape when we already have so many. Although thinking about it again, you would only be able to run a local instance while on the VPN when you rely on UAT's RedPanda. Thoughts?

If you don't have a connection to UAT's RedPanda, what will happen? If it breaks Sequencescape completely, I think that's not OK. If it just means this particular feature doesn't work, that's not too bad.

Similarly, I don't want a local RedPanda instance to be a requirement for developing SS locally... but if it was required for just this feature, wouldn't be too bad.

Would either of these work?

Skip the check against the schema if one is not available? (in local only)

Have a dummy schema in a file in the codebase that it uses in local only

I've added a cache to the system, which it probably should have had to start with. This means that you would need to connect to the UAT RedPanda the first time you trigger one of these jobs, but then you'd have a cached file of the schema to use in future calls. Of course, if you know the schema, you can also just dump it in the appropriately named cache file. I added some info to the README about this.

KatyTaylor · 2024-06-28T13:26:00Z

app/controllers/api/v2/bioscan/export_pool_xp_to_traction_controller.rb

+
+        def preflight_errors(barcode)
+          # Check that the tube exists
+          tube = Tube.with_barcode(barcode).first


Not important, but again could change to the following

Suggested change

tube = Tube.with_barcode(barcode).first

tube = Tube.find_by_barcode(barcode)

KatyTaylor · 2024-06-28T13:27:35Z

spec/helpers/application_helper_spec.rb

@@ -1,7 +1,6 @@
 # frozen_string_literal: true

 require 'spec_helper'
-require 'rails_helper'


What happened with these deleted 'rails_helper' lines?

I read that you only need spec_helper during specs. rails_helper is implicitly part of that require. Correct me if you know otherwise.

KatyTaylor

Finished reviewing! Only thing that needs working out I think is the schema dependency bit. Everything else was small.

app/jobs/export_pool_xp_to_traction_job.rb

codeclimate · 2024-07-01T14:25:39Z

app/controllers/api/v2/bioscan/export_pool_xp_to_traction_controller.rb

+        before_action :login_required, except: [:create]
+        skip_before_action :verify_authenticity_token
+
+        def create


Api::V2::Bioscan::ExportPoolXpToTractionController#create has approx 6 statements

app/jobs/export_pool_xp_to_traction_job.rb

KatyTaylor

Addition of caching looks good I think!
Just pointed out a typo 😆

README.md

KatyTaylor · 2024-07-02T10:16:18Z

data/avro_schema_cache/.gitignore

I didn't know you could have .gitignore files in subdirectories 🤯

Ah yeah, you can have them anywhere and they only apply to the files at the same level as them or lower.

Co-authored-by: KatyTaylor <kt17@sanger.ac.uk>

codeclimate · 2024-07-02T13:05:41Z

Code Climate has analyzed commit 2a714b4 and detected 19 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	19

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

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

View more on Code Climate.

sdjmchattie added 9 commits June 12, 2024 18:20

Set up initial endpoint controller and job

c10816e

Add dependencies needed to publish to RabbitMQ

326a86c

WIP send a message to RabbitMQ with Bunny

914dcb8

Complete message sending to RabbitMQ part

96ecbe5

Fetch schemas for message sending

6b06ac1

Create and post an Avro encoded dummy message to RabbitMQ

84c30a1

Remove unneeded insertSize value for library

d32ff2c

Extract a module for compounding samples

efd08f3

Move compound sample creation into a helper

0669964

sdjmchattie self-assigned this Jun 21, 2024

sdjmchattie marked this pull request as draft June 21, 2024 14:33

Use correctly named setter method

975ad16

codeclimate bot reviewed Jun 21, 2024

View reviewed changes

sdjmchattie added 2 commits June 21, 2024 16:14

Extract config for rabbit to a variable

33320be

Use actual values for the message contents

49740f1

codeclimate bot reviewed Jun 21, 2024

View reviewed changes

sdjmchattie added 2 commits June 24, 2024 12:02

Define a box barcode value Unspecified

9954187

Put basic checks in place for Pool XP tube

67c67b5

codeclimate bot reviewed Jun 24, 2024

View reviewed changes

sdjmchattie added 3 commits June 24, 2024 16:09

Test the export controller

93be1f7

Add test for compound sample helper

f5f7e35

Move let! to before

b125f15

codeclimate bot reviewed Jun 24, 2024

View reviewed changes

app/controllers/api/v2/bioscan/export_pool_xp_to_traction_controller.rb Show resolved Hide resolved

app/controllers/api/v2/bioscan/export_pool_xp_to_traction_controller.rb Show resolved Hide resolved

app/jobs/export_pool_xp_to_traction_job.rb Show resolved Hide resolved

sdjmchattie added 3 commits June 24, 2024 18:50

Start adding tests for export job

cd9f9d1

Add remaining tests for Export job

bb3ff50

Log errors for job execution

979c1fc

sdjmchattie requested review from andrewsparkes, KatyTaylor and BenTopping June 25, 2024 11:08

sdjmchattie marked this pull request as ready for review June 25, 2024 11:08

KatyTaylor reviewed Jun 28, 2024

View reviewed changes

app/jobs/export_pool_xp_to_traction_job.rb Outdated Show resolved Hide resolved

KatyTaylor reviewed Jun 28, 2024

View reviewed changes

app/jobs/export_pool_xp_to_traction_job.rb Outdated Show resolved Hide resolved

KatyTaylor reviewed Jun 28, 2024

View reviewed changes

app/jobs/export_pool_xp_to_traction_job.rb Outdated Show resolved Hide resolved

sdjmchattie and others added 2 commits June 28, 2024 11:02

Add missing word "be" from comments

62b6a90

Co-authored-by: KatyTaylor <kt17@sanger.ac.uk>

Prefer find_by_barcode over with_barcode().first

306124d

Co-authored-by: KatyTaylor <kt17@sanger.ac.uk>

codeclimate bot reviewed Jun 28, 2024

View reviewed changes

app/jobs/export_pool_xp_to_traction_job.rb Show resolved Hide resolved

Rename fetch method for getting HTTP responses

330d519

KatyTaylor reviewed Jun 28, 2024

View reviewed changes

app/jobs/export_pool_xp_to_traction_job.rb Show resolved Hide resolved

KatyTaylor reviewed Jun 28, 2024

View reviewed changes

Avoid nil exceptions with safe-chaining

5d91aba

KatyTaylor reviewed Jun 28, 2024

View reviewed changes

codeclimate bot reviewed Jun 28, 2024

View reviewed changes

app/jobs/export_pool_xp_to_traction_job.rb Show resolved Hide resolved

app/jobs/export_pool_xp_to_traction_job.rb Show resolved Hide resolved

sdjmchattie added 9 commits June 28, 2024 16:18

Use a development value for schemas, loading local files when it is set

dfc9d29

Make prettier happy

08a87d8

Create cache location for Avro schemas

6b9e679

Revert to using UAT RedPanda for development

9c5e8f5

Use a test subject name and version for

88c2f10

Implement caching of schemas

dd02e41

Update tests to match new functionality

707c5be

Update README

8d4cb08

Add a note about extracting common code in future jobs

a608cc8

codeclimate bot reviewed Jul 1, 2024

View reviewed changes

KatyTaylor approved these changes Jul 2, 2024

View reviewed changes

Fix typo in new README section

2a714b4

Co-authored-by: KatyTaylor <kt17@sanger.ac.uk>

sdjmchattie merged commit b0b713d into develop Jul 2, 2024
21 checks passed

sdjmchattie deleted the Y24-061-send-pool-xp-export-message branch July 2, 2024 16:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Y24-061: Send Pool XP export message #4147

Y24-061: Send Pool XP export message #4147

sdjmchattie commented Jun 21, 2024

codeclimate bot Jun 21, 2024

codeclimate bot Jun 21, 2024

codeclimate bot Jun 21, 2024

codeclimate bot Jun 21, 2024

codeclimate bot Jun 21, 2024

KatyTaylor Jun 28, 2024

sdjmchattie Jun 28, 2024

KatyTaylor Jun 28, 2024

sdjmchattie Jun 28, 2024

KatyTaylor Jun 28, 2024

sdjmchattie Jun 28, 2024

KatyTaylor Jun 28, 2024

sdjmchattie Jul 1, 2024

KatyTaylor Jun 28, 2024

KatyTaylor Jun 28, 2024

sdjmchattie Jun 28, 2024

KatyTaylor left a comment

codeclimate bot Jul 1, 2024

KatyTaylor left a comment

KatyTaylor Jul 2, 2024

sdjmchattie Jul 2, 2024

codeclimate bot commented Jul 2, 2024

	tube = Tube.with_barcode(barcode).first
	tube = Tube.find_by_barcode(barcode)

Y24-061: Send Pool XP export message #4147

Y24-061: Send Pool XP export message #4147

Conversation

sdjmchattie commented Jun 21, 2024

Changes proposed in this pull request

Instructions for Reviewers

codeclimate bot Jun 21, 2024

Choose a reason for hiding this comment

codeclimate bot Jun 21, 2024

Choose a reason for hiding this comment

codeclimate bot Jun 21, 2024

Choose a reason for hiding this comment

codeclimate bot Jun 21, 2024

Choose a reason for hiding this comment

codeclimate bot Jun 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KatyTaylor left a comment

Choose a reason for hiding this comment

codeclimate bot Jul 1, 2024

Choose a reason for hiding this comment

KatyTaylor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codeclimate bot commented Jul 2, 2024