Skip to content

Commit

Permalink
Switch to find_or_create_by after code review
Browse files Browse the repository at this point in the history
Jeremy pointed out during code review that it might be worth using
find_or_create_by in the Term methods, rather than a combination of the
create method and a guard method in the Detection model.

This makes that change, adopting find_or_create_by inside Term and
removing the check_for_current method from the Detection model.

Along the way we update the specific assertion being looked for in the
Detection tests. Please note that there are tests in the Term model that
confirm the record_* methods an be called repeatedly without either
creating duplicate records, or throwing errors.
  • Loading branch information
matt-bernhardt committed Sep 19, 2024
1 parent 48ef85b commit aa746f6
Show file tree
Hide file tree
Showing 3 changed files with 6 additions and 15 deletions.
11 changes: 1 addition & 10 deletions app/models/detection.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ class Detection < ApplicationRecord
belongs_to :detector

# We use the before_create hook to prevent needing to override the initialize method, which Rails frowns upon.
before_create :set_defaults, :check_for_current
before_create :set_defaults

# These scopes allow for easy filtering of Detection records by a single parameter.
scope :current, -> { where(detector_version: ENV.fetch('DETECTOR_VERSION', 'unset')) }
Expand All @@ -34,15 +34,6 @@ class Detection < ApplicationRecord

private

# This leverages three scopes to confirm, before creating a new Detection, whether that combination of Term, Detector,
# and Version has already been registered. If one is found, it aborts the creation.
def check_for_current
return unless Detection.for_term(term).for_detector(detector).current.exists?

errors.add(:detector_version, 'already exists')
throw(:abort)
end

# This looks up the current Detector Version from the environment, storing the value as part of the record which is
# about to be saved. This prevents the rest of the application from having to worry about this value, while also
# providing a mechanism to prevent duplicate records from being created.
Expand Down
6 changes: 3 additions & 3 deletions app/models/term.rb
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ def record_patterns
si = Detector::StandardIdentifiers.new(phrase)

si.identifiers.each_key do |k|
Detection.create(
Detection.find_or_create_by(
term: self,
detector: Detector.where(name: k.to_s.upcase).first
)
Expand All @@ -51,7 +51,7 @@ def record_journals
result = Detector::Journal.full_term_match(phrase)
return unless result.any?

Detection.create(
Detection.find_or_create_by(
term: self,
detector: Detector.where("name = 'Journal'").first
)
Expand All @@ -69,7 +69,7 @@ def record_suggested_resources
result = Detector::SuggestedResource.full_term_match(phrase)
return unless result.any?

Detection.create(
Detection.find_or_create_by(
term: self,
detector: Detector.where("name = 'SuggestedResource'").first
)
Expand Down
4 changes: 2 additions & 2 deletions test/models/detection_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ class DetectionTest < ActiveSupport::TestCase

assert_equal(initial_count + 1, post_count)

assert_raises(ActiveRecord::RecordNotSaved) do
assert_raises(ActiveRecord::RecordNotUnique) do
Detection.create!(sample)
end

Expand All @@ -48,7 +48,7 @@ class DetectionTest < ActiveSupport::TestCase
}

# A purely duplicate record fails to save...
assert_raises(ActiveRecord::RecordNotSaved) do
assert_raises(ActiveRecord::RecordNotUnique) do
Detection.create!(new_sample)
end

Expand Down

0 comments on commit aa746f6

Please sign in to comment.