Update settings OBO uniqueness validations

app/controllers/api_controller.rb

@@ -7,7 +7,6 @@ class ApiController < ActionController::API
     if error.message == 'Nil JSON web token'
       render_bad_request(RuntimeError.new(:no_credentials))
     else
-      # key = error.message == 'obo' ? "obo : :credentials
       render_unauthorized(error.message)
     end
   end

app/models/box.rb

@@ -37,8 +37,6 @@ class Box < ApplicationRecord
   validates_presence_of :name, :short_name, :uri
   validate :validate_box_with_api_connection
 
-  store_accessor :settings, :obo, prefix: true # TODO: move to Govbox::Box superclass?
-
   def self.create_with_api_connection!(params)
     raise NotImplementedError
   end

app/models/govbox/api_connection.rb

@@ -31,6 +31,6 @@ def validate_box(box)
   private
 
   def invalid_obo?(box)
-    box.settings && box.settings["obo"].present?
+    box.settings && box.settings_obo.present?
   end
 end

app/models/govbox/api_connection_with_obo_support.rb

@@ -27,6 +27,6 @@ def destroy_with_box?
 
   def validate_box(box)
     box.errors.add(:settings_obo, :not_allowed) if obo.present?
-    box.errors.add(:settings_obo, :invalid) if boxes.where.not(id: box.id).where("settings ->> 'obo' = ?", box.settings_obo).exists?
+    box.errors.add(:settings_obo, :invalid) if boxes.where.not(id: box.id).where("settings @> ?", { obo: box.settings_obo }.to_json).exists?
   end
 end

app/models/sk_api/api_connection_with_obo_support.rb

@@ -27,7 +27,7 @@ def destroy_with_box?
 
   def validate_box(box)
     box.errors.add(:settings_obo, :not_allowed) if invalid_obo?(box)
-    box.errors.add(:settings_obo, :invalid) if boxes.where.not(id: box.id).where("settings ->> 'obo' = ?", box.settings_obo).exists?
+    box.errors.add(:settings_obo, :invalid) if boxes.where.not(id: box.id).where("settings @> ?", { obo: box.settings_obo }.to_json).exists?
   end
 
   private

app/models/upvs/box.rb

@@ -20,9 +20,16 @@ def self.policy_class
     BoxPolicy
   end
 
+  store_accessor :settings, :obo, prefix: true
+
   validates_uniqueness_of :name, :short_name, :uri, scope: :tenant_id
 
-  store_accessor :settings, :obo, prefix: true
+  validate :validate_settings_obo
+
+  normalizes :settings, with: -> (settings) {
+    settings['obo'] = settings['obo'].presence
+    settings
+  }
 
   def self.create_with_api_connection!(params)
     if params[:api_connection]
@@ -38,4 +45,11 @@ def self.create_with_api_connection!(params)
   def sync
     Govbox::SyncBoxJob.perform_later(self)
   end
+
+  private
+
+  def validate_settings_obo
+    return unless settings_obo.present?
+    errors.add(:settings_obo, "OBO must be in UUID format") unless settings_obo.match?(Utils::UUID_PATTERN)
+  end
 end

db/migrate/20240726131027_update_empty_strings_boxes_settings_obo.rb

@@ -0,0 +1,8 @@
+class UpdateEmptyStringsBoxesSettingsObo < ActiveRecord::Migration[7.1]
+  def up
+    Upvs::Box.find_each do |box|
+      box.normalize_attribute(:settings)
+      box.save
+    end
+  end
+end

test/fixtures/api_connections.yml

@@ -40,6 +40,12 @@ govbox_api_api_connection2:
   api_token_private_key: private_key
   type: 'Govbox::ApiConnection'
 
+govbox_api_api_connection3:
+  sub: sub2
+  obo: <%= SecureRandom.uuid %>
+  api_token_private_key: private_key
+  type: 'Govbox::ApiConnection'
+
 govbox_api_api_connection_with_obo_support:
   sub: sub3
   api_token_private_key: private_key

test/fixtures/boxes.yml

@@ -7,6 +7,8 @@ ssd_main:
   short_name: MY1
   api_connection: govbox_api_api_connection1
   type: 'Upvs::Box'
+  settings:
+    obo:
 
 ssd_other:
   name: SSD other
@@ -15,14 +17,18 @@ ssd_other:
   short_name: MY2
   api_connection: govbox_api_api_connection1
   type: 'Upvs::Box'
+  settings:
+    obo: cf703e1a-cc4a-4445-9c76-08eae349505f
 
 solver_main:
   name: Solver main
   uri: SolverMainURI
   tenant: solver
   short_name: MY1
-  api_connection: govbox_api_api_connection1
+  api_connection: govbox_api_api_connection3
   type: 'Upvs::Box'
+  settings:
+    obo:
 
 google_box_with_govbox_api_connection:
   name: Box with GovBox API connection
@@ -31,15 +37,18 @@ google_box_with_govbox_api_connection:
   short_name: box2
   api_connection: govbox_api_api_connection2
   type: 'Upvs::Box'
+  settings:
+    obo:
 
 google_box_with_govbox_api_connection_with_obo_support:
   name: Box with GovBox API connection with OBO support
   uri: google_box_with_govbox_api_connection_with_obo_support_uri
   tenant: google
   short_name: box3
   api_connection: govbox_api_api_connection_with_obo_support
-  settings: {"obo": 1123-1123}
   type: 'Upvs::Box'
+  settings:
+    obo: cf703e1a-cc4a-4445-9c76-08eae349505f
 
 google_box_with_sk_api_api_connection_with_obo_support:
   name: Box with SkAPi API connection with OBO support
@@ -48,6 +57,18 @@ google_box_with_sk_api_api_connection_with_obo_support:
   short_name: box4
   api_connection: sk_api_api_connection_with_obo_support
   type: 'Upvs::Box'
+  settings:
+    obo:
+
+google_box_with_govbox_api_connection_with_obo_support_without_obo_value:
+  name: Box with GovBox API connection with OBO support without OBO value
+  uri: google_box_with_govbox_api_connection_with_obo_support_without_obo_value_uri
+  tenant: google
+  short_name: box5
+  api_connection: govbox_api_api_connection_with_obo_support
+  type: 'Upvs::Box'
+  settings:
+    obo:
 
 fs_accountants:
   name: Accountants main FS

test/models/box_test.rb

@@ -38,6 +38,37 @@ class BoxTest < ActiveSupport::TestCase
     assert_enqueued_jobs Upvs::Box.where(syncable: true).count
   end
 
+  test "should not be valid if same obo value present in other boxes within connection" do
+    box = boxes(:google_box_with_govbox_api_connection_with_obo_support)
+
+    new_box = Upvs::Box.create(
+      name: SecureRandom.hex,
+      short_name: SecureRandom.hex,
+      uri: SecureRandom.hex,
+      tenant: box.tenant,
+      api_connection: box.api_connection,
+      settings_obo: box.settings_obo
+    )
+
+    assert_not new_box.valid?
+    assert_equal :settings_obo, new_box.errors.first.attribute
+  end
+
+  test "should not be valid if no obo value already present in other boxes within connection" do
+    box = boxes(:google_box_with_govbox_api_connection_with_obo_support_without_obo_value)
+
+    new_box = Upvs::Box.create(
+      name: SecureRandom.hex,
+      short_name: SecureRandom.hex,
+      uri: SecureRandom.hex,
+      tenant: box.tenant,
+      api_connection: box.api_connection
+    )
+
+    assert_not new_box.valid?
+    assert_equal :settings_obo, new_box.errors.first.attribute
+  end
+
   test "after_destroy callback destroys api_connection if Govbox::ApiConnection without any boxes" do
     box = boxes(:google_box_with_govbox_api_connection)
     api_connection = box.api_connection
@@ -47,6 +78,15 @@ class BoxTest < ActiveSupport::TestCase
     assert api_connection.destroyed?
   end
 
+  test "before_save callback normalizes settings_obo attribute" do
+    box = boxes(:google_box_with_govbox_api_connection).dup
+    box.settings_obo = ''
+
+    box.save
+
+    assert_nil box.settings_obo
+  end
+
   test "after_destroy callback does not destroy api_connection if Govbox::ApiConnectionWithOboSupport" do
     box = boxes(:google_box_with_govbox_api_connection_with_obo_support)
     api_connection = box.api_connection