DPL-274 - ONT - remove existing code #745

stevieing · 2022-04-26T10:22:24Z

Changes proposed in this pull request:

...

…other code that wasn't being used.

Temporarily rollback json-api-resources to fix: ./spec/requests/v1/pacbio/libraries_spec.rb:53 ./spec/requests/v1/pacbio/pools_spec.rb:96

The update to jsonapi-resources 0.10.6 broke the loading of tubes for pacbio library and pool resources. This appears to be related to the addition of the configuration option `use_related_resource_records_for_joins` cerebris/jsonapi-resources#1381 This is failing to function correctly in this case. By specifying the relationship explicitly, we disable this behaviour, and everything behaves as expected. I still haven't quite managed to get to the bottom of the problem. It may be we need to disable the behaviour globally.

JamesGlover · 2022-04-28T14:48:55Z

Ahh bingo!

cerebris/jsonapi-resources#1308

It is applying the self.records filter from V1::Pacbio::TubeResource which is: ::Tube.by_pipeline(:pacbio) so we end up inadvertently filtering out out tubes.

JamesGlover

A few suggestions, and a couple of questions, but nothing critical.

JamesGlover · 2022-05-03T13:15:45Z

Gemfile.lock

+    rails (7.0.2.3)
+      actioncable (= 7.0.2.3)
+      actionmailbox (= 7.0.2.3)
+      actionmailer (= 7.0.2.3)
+      actionpack (= 7.0.2.3)
+      actiontext (= 7.0.2.3)
+      actionview (= 7.0.2.3)
+      activejob (= 7.0.2.3)
+      activemodel (= 7.0.2.3)
+      activerecord (= 7.0.2.3)
+      activestorage (= 7.0.2.3)
+      activesupport (= 7.0.2.3)
+      bundler (>= 1.15.0)
+      railties (= 7.0.2.3)


I wonder if its worth considering only using a subset to these?
I don't think we need:
actioncable, actionmailer, actionmailbox, actiontext, activejob, activestorage.

I'm not even certain we need actionview, although it may be that Rails routes all its erb stuff through there,

On the flip side, this is definitely simpler to maintain.

JamesGlover · 2022-05-03T13:25:00Z

app/models/concerns/nested_validation.rb

+          record.errors.add(error.attribute, error.message)
+        elsif nested_attribute == :base
+          record.errors.add(attribute, nested_error)
+        else
+          record.errors.add("#{attribute}.#{nested_attribute}", nested_error)


Suggested change

record.errors.add(error.attribute, error.message)

elsif nested_attribute == :base

record.errors.add(attribute, nested_error)

else

record.errors.add("#{attribute}.#{nested_attribute}", nested_error)

record.errors.add(error.attribute, error.message)

elsif nested_attribute == :base

record.errors.add(error.attribute, error.message)

else

record.errors.add("#{error.attribute}.#{nested_attribute}", error.message)

Although this suggests we are either only using this with keys flattened, or aren't testing the cases where we @flatten_keys is false. We could consider simplifying this instead.

JamesGlover · 2022-05-03T13:26:03Z

app/models/container_material.rb

-
-  def self.includes_args(except = nil)
-    args = []
-    args << :material unless except == :material
-    args << :container unless except == :container
-
-    args
-  end


This bit of code had me itching every time I saw it.

JamesGlover · 2022-05-03T13:42:43Z

app/models/ont/flowcell.rb

@@ -12,14 +12,11 @@ class Flowcell < ApplicationRecord
    validates :position,
              presence: true,
              uniqueness: { scope: :ont_run_id,
-                            message: 'should only appear once within run' }
+                            message: I18n.t('errors.messages.uniqueness.flowcell') }


Ahh, mostly been flagging these Todo.

I hadn't considered being explicit here, and had intended to use the default rails message lookup:; which I've always found a bit obtuse, so being explicit is nice.

However might be nice to adjust this to match one of the default lookup routes, even if you are still explicit here. They are a touch verbose, although I also found this option, which lets you be concise, but still explicit (especially if its an easily grepable string).

Oh, and as I said, obtuse, so sorry if this does already map to obe of the defaults.

JamesGlover · 2022-05-03T13:56:52Z

app/models/pacbio/library_factory.rb

+      library.errors.each do |error|
+        errors.add(error.attribute, error.message)
      end


Can dry some of these out with the nested validation, but probably better to do when already touching this code.

JamesGlover · 2022-05-03T14:13:00Z

config/initializers/01_proc.rb

-# frozen_string_literal: true
-
-# www.mattsears.com/articles/2011/11/27ruby-blocks-as-dynamic-callbacks
-class Proc
-  def callback(callable, *args)
-    self === Class.new do
-      method_name = callable.to_sym
-      define_method(method_name) { |&block| block ? block.call(*args) : true }
-      define_method("#{method_name}?") { true }
-      def method_missing(*_args)
-        super
-        false
-      end
-
-      def respond_to_missing?(*)
-        super
-        true
-      end
-    end.new
-  end
-end


I remember thinking this was a native feature I'd not seen before when I first came across it in the codebase.

JamesGlover · 2022-05-03T14:14:04Z

config/initializers/new_framework_defaults_7_0.rb

@@ -0,0 +1,118 @@
+# frozen_string_literal: true


Given you've already opted in to the 7.0 defaults, I think we can already lose this file.

JamesGlover · 2022-05-03T14:18:57Z

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.

-ActiveRecord::Schema.define(version: 2022_04_07_104659) do
+ActiveRecord::Schema[6.1].define(version: 2022_04_07_104659) do


Might be worth running rake db:schema:dump now to update this to the Rails 7 version, otherwise there will be lots of changes next time someone touches the schema for real.

JamesGlover · 2022-05-03T14:27:17Z

spec/support/read_only.rb

+    Array(klasses).each do |klass|
+      klass.define_method(:readonly?) { readonly }
+    end


Might be better using mocks here as they'll get automatically torn down, removing the need for an after block, and reducing the risk of accidentally mutating the global state.

klasses.each do |klass| allow_any_instance_of(Widget).to receive(:readonly?).and_return(readonly) end

I think rubocop-rspec will complain, but I think its still a safer option.

JamesGlover · 2022-05-03T14:31:50Z

app/models/ont/flowcell.rb

+    def readonly?
+      true
    end


I wonder if it would be safer to call readonly! in an after_initialize call? That would have the advantage that we also set the @readonly instance variable. Doesn't matter too much, as it should all be both temporary and precautionary,.

…g-code

stevieing added 2 commits April 26, 2022 11:21

Removed graphql. Removed ONT routes. Removed ONT tasks. Removed some …

29c369a

…other code that wasn't being used.

Removed constants accessor. Removed creation factories.

58c91dc

stevieing changed the title ~~Removed graphql. Removed ONT routes. Removed ONT tasks. Removed some …~~ DPL-274 - ONT - remove existing code Apr 26, 2022

stevieing linked an issue Apr 26, 2022 that may be closed by this pull request

DPL-274 - ONT - remove existing code [C=S, V=3] #707

Closed

2 tasks

stevieing and others added 7 commits April 26, 2022 15:51

Removed everything ONT. Left models and locked them.

d66b147

Moved autoloader to zeitwerk.

67b780e

Upgrade to rails 7 and fixed some tests.

f5a892c

removed deprecation warnings.

8b4e0b9

Rubocopped.

0b465f9

Fix library assignment

8ed60b9

Rollback json-api-resources

db9e226

Temporarily rollback json-api-resources to fix: ./spec/requests/v1/pacbio/libraries_spec.rb:53 ./spec/requests/v1/pacbio/pools_spec.rb:96

JamesGlover force-pushed the dpl-274-ont-remove-existing-code branch from ac14091 to db9e226 Compare April 28, 2022 13:37

stevieing added 3 commits April 28, 2022 15:52

Added vendor back in to rubocop.yml

ed659ac

Fixed rubocop errors.

681992b

Merged develop and fixed merge conflicts.

12fee32

stevieing requested a review from JamesGlover April 28, 2022 15:51

Modified readonly comment.

aa72b82

JamesGlover approved these changes May 3, 2022

View reviewed changes

stevieing added 2 commits May 5, 2022 12:32

Fixed error messages.

85732ff

Merge remote-tracking branch 'origin' into dpl-274-ont-remove-existin…

3748f66

…g-code

stevieing force-pushed the dpl-274-ont-remove-existing-code branch from 50524db to 3748f66 Compare May 5, 2022 11:35

stevieing added 3 commits May 5, 2022 14:48

Added rubocop for rspec. Modified readonly method.

88a8f96

Fixed issue with rubocop line length and specs.

18d2b2f

Modified encoding to utf8.

f0e3b22

stevieing merged commit 47bf938 into develop May 17, 2022

stevieing mentioned this pull request May 17, 2022

Revert "DPL-274 - ONT - remove existing code" #748

Merged

stevieing removed a link to an issue May 17, 2022

DPL-274 - ONT - remove existing code [C=S, V=3] #707

Closed

2 tasks

stevieing deleted the dpl-274-ont-remove-existing-code branch October 17, 2022 07:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DPL-274 - ONT - remove existing code #745

DPL-274 - ONT - remove existing code #745

stevieing commented Apr 26, 2022 •

edited

Loading

JamesGlover commented Apr 28, 2022

JamesGlover left a comment

JamesGlover May 3, 2022

JamesGlover May 3, 2022

JamesGlover May 3, 2022

JamesGlover May 3, 2022

JamesGlover May 3, 2022

JamesGlover May 3, 2022

JamesGlover May 3, 2022

JamesGlover May 3, 2022

JamesGlover May 3, 2022

JamesGlover May 3, 2022

JamesGlover May 3, 2022

JamesGlover May 3, 2022

DPL-274 - ONT - remove existing code #745

DPL-274 - ONT - remove existing code #745

Conversation

stevieing commented Apr 26, 2022 • edited Loading

JamesGlover commented Apr 28, 2022

JamesGlover left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevieing commented Apr 26, 2022 •

edited

Loading