From 81494ea473393b57be410d5dfa810358d3d47a9d Mon Sep 17 00:00:00 2001
From: Bryan Hockey <bhock@umich.edu>
Date: Fri, 11 Nov 2016 13:26:34 -0600
Subject: [PATCH 01/14] Model scopes on associations expect model objects,
 instead of ids. Add scope specs for new (as in Bag.new) models.

---
 app/models/bag.rb                        |  6 ++---
 app/models/fixity_check.rb               |  4 +--
 app/models/ingest.rb                     |  4 +--
 app/models/message_digest.rb             |  2 +-
 app/models/replication_transfer.rb       |  6 ++---
 app/models/restore_transfer.rb           |  6 ++---
 spec/models/bag_spec.rb                  | 27 ++++++++++++-------
 spec/models/replication_transfer_spec.rb | 33 ++++++++++++++++++++++++
 spec/models/restore_transfer_spec.rb     | 33 ++++++++++++++++++++++++
 9 files changed, 98 insertions(+), 23 deletions(-)

diff --git a/app/models/bag.rb b/app/models/bag.rb
index 7a8dff33..3e11145d 100644
--- a/app/models/bag.rb
+++ b/app/models/bag.rb
@@ -80,9 +80,9 @@ def self.find_fields
   ### Scopes
   scope :updated_before, ->(time) { where("updated_at < ?", time) unless time.blank? }
   scope :updated_after, ->(time) { where("updated_at > ?", time) unless time.blank? }
-  scope :with_admin_node_id, ->(id) { where(admin_node_id: id) unless id.blank? }
-  scope :with_ingest_node_id, ->(id) { where(ingest_node_id: id) unless id.blank? }
-  scope :with_member_id, ->(id) { where(member_id: id) unless id.blank? }
+  scope :with_admin_node, ->(node) { where(admin_node: node) unless node.new_record? }
+  scope :with_ingest_node, ->(node) { where(ingest_node: node) unless node.new_record? }
+  scope :with_member, ->(member) { where(member: member) unless member.new_record? }
   scope :with_bag_type, ->(bag_type) { where(type: bag_type) unless bag_type.blank? }
   scope :replicated_by, ->(nodes) {
     unless nodes.empty?
diff --git a/app/models/fixity_check.rb b/app/models/fixity_check.rb
index ebb71e5f..67141288 100644
--- a/app/models/fixity_check.rb
+++ b/app/models/fixity_check.rb
@@ -36,8 +36,8 @@ def self.find_fields
   scope :created_after, ->(time) { where("created_at > ?", time) unless time.blank? }
   scope :created_before, ->(time) { where("created_at < ?", time) unless time.blank? }
   scope :with_success, ->(success) { where(success: success) unless success.blank? }
-  scope :with_bag_id, ->(id) { where(bag_id: id) unless id.blank? }
-  scope :with_node_id, ->(id) { where(node_id: id) unless id.blank? }
+  scope :with_bag, ->(bag) { where(bag: bag) unless bag.new_record? }
+  scope :with_node, ->(node) { where(node: node) unless node.new_record? }
   scope :latest_only, ->(flag) do
     unless flag.blank?
       joins("INNER JOIN (
diff --git a/app/models/ingest.rb b/app/models/ingest.rb
index fc59b430..3c3696d1 100644
--- a/app/models/ingest.rb
+++ b/app/models/ingest.rb
@@ -11,7 +11,7 @@ def self.find_fields
   end
   
   belongs_to :bag, inverse_of: :ingests
-  
+
   has_many :node_ingests, inverse_of: :ingest, dependent: :destroy,
     before_add: :fail_unless_new,
     before_remove: :fail_unless_new
@@ -33,7 +33,7 @@ def self.find_fields
   ### Scopes
   scope :created_after, ->(time) { where("created_at > ?", time) unless time.blank? }
   scope :created_before, ->(time) { where("created_at < ?", time) unless time.blank? }
-  scope :with_bag_id, ->(id) { where(bag_id: id) unless id.blank? }
+  scope :with_bag, ->(bag) { where(bag: bag) unless bag.new_record? }
   scope :with_ingested, ->(ingested) { where(ingested: ingested) if [true,false].include?(ingested) }
   scope :latest_only, ->(flag) do
     unless flag.blank?
diff --git a/app/models/message_digest.rb b/app/models/message_digest.rb
index 2a8f6209..eea05903 100644
--- a/app/models/message_digest.rb
+++ b/app/models/message_digest.rb
@@ -31,7 +31,7 @@ def self.find_fields
   ### Scopes
   scope :created_after, ->(time) { where("created_at > ?", time) unless time.blank? }
   scope :created_before, ->(time) { where("created_at < ?", time) unless time.blank? }
-  scope :with_bag_id, ->(id) { where(bag_id: id) unless id.blank? }
+  scope :with_bag, ->(bag) { where(bag: bag) unless bag.new_record? }
 
 
 end
\ No newline at end of file
diff --git a/app/models/replication_transfer.rb b/app/models/replication_transfer.rb
index 03a742d2..d2e5c722 100644
--- a/app/models/replication_transfer.rb
+++ b/app/models/replication_transfer.rb
@@ -87,9 +87,9 @@ def cancel!(reason, detail)
   ### Scopes
   scope :updated_after, ->(time) { where("updated_at > ?", time) unless time.blank? }
   scope :updated_before, ->(time) { where("updated_at < ?", time) unless time.blank? }
-  scope :with_bag_id, ->(id) { where(bag_id: id) unless id.blank? }
-  scope :with_from_node_id, ->(id) { where(from_node_id: id) unless id.blank? }
-  scope :with_to_node_id, ->(id) { where(to_node_id: id) unless id.blank? }
+  scope :with_bag, ->(bag) { where(bag: bag) unless bag.new_record? }
+  scope :with_from_node, ->(node) { where(from_node: node) unless node.new_record? }
+  scope :with_to_node, ->(node) { where(to_node: node) unless node.new_record? }
   scope :with_store_requested, ->(v){ where(store_requested: v) if [true,false].include?(v) }
   scope :with_stored, ->(v){ where(stored: v) if [true,false].include?(v) }
   scope :with_cancelled, ->(v){ where(cancelled: v) if [true,false].include?(v) }
diff --git a/app/models/restore_transfer.rb b/app/models/restore_transfer.rb
index 9198dd5b..0d908e75 100644
--- a/app/models/restore_transfer.rb
+++ b/app/models/restore_transfer.rb
@@ -69,9 +69,9 @@ def cancel!(reason, detail)
   ### Scopes
   scope :updated_after, ->(time) { where("updated_at > ?", time) unless time.blank? }
   scope :updated_before, ->(time) { where("updated_at < ?", time) unless time.blank? }
-  scope :with_bag_id, ->(id) { where(bag_id: id) unless id.blank? }
-  scope :with_from_node_id, ->(id) { where(from_node_id: id) unless id.blank? }
-  scope :with_to_node_id, ->(id) { where(to_node_id: id) unless id.blank? }
+  scope :with_bag, ->(bag) { where(bag: bag) unless bag.new_record? }
+  scope :with_from_node, ->(node) { where(from_node: node) unless node.new_record? }
+  scope :with_to_node, ->(node) { where(to_node: node) unless node.new_record? }
   scope :with_accepted, ->(v){ where(accepted: v) if [true,false].include?(v) }
   scope :with_finished, ->(v){ where(finished: v) if [true,false].include?(v) }
   scope :with_cancelled, ->(v){ where(cancelled: v) if [true,false].include?(v) }
diff --git a/spec/models/bag_spec.rb b/spec/models/bag_spec.rb
index fe79b3b4..ed1cd1dd 100644
--- a/spec/models/bag_spec.rb
+++ b/spec/models/bag_spec.rb
@@ -145,27 +145,36 @@
 
   it_behaves_like "it has temporal scopes for", :updated_at
 
-  describe "scope with_admin_node_id" do
+  describe "scope with_admin_node" do
     let!(:bag) { Fabricate(:bag) }
+    let!(:other_bag) { Fabricate(:bag) }
     it "includes matching only" do
-      Fabricate(:bag)
-      expect(Bag.with_admin_node_id(bag.admin_node_id)).to match_array [bag]
+      expect(Bag.with_admin_node(bag.admin_node)).to match_array [bag]
+    end
+    it "does not filter given a new record" do
+      expect(Bag.with_admin_node(Fabricate.build(:node))).to contain_exactly(bag, other_bag)
     end
   end
 
-  describe "scope with_ingest_node_id" do
+  describe "scope with_ingest_node" do
     let!(:bag) { Fabricate(:bag) }
+    let!(:other_bag) { Fabricate(:bag) }
     it "includes matching only" do
-      Fabricate(:bag)
-      expect(Bag.with_ingest_node_id(bag.ingest_node_id)).to match_array [bag]
+      expect(Bag.with_ingest_node(bag.ingest_node)).to match_array [bag]
+    end
+    it "does not filter given a new record" do
+      expect(Bag.with_ingest_node(Fabricate.build(:node))).to contain_exactly(bag, other_bag)
     end
   end
 
-  describe "scope with_member_id" do
+  describe "scope with_member" do
     let!(:bag) { Fabricate(:bag) }
+    let!(:other_bag) { Fabricate(:bag) }
     it "includes matching only" do
-      Fabricate(:bag)
-      expect(Bag.with_member_id(bag.member_id)).to match_array [bag]
+      expect(Bag.with_member(bag.member)).to match_array [bag]
+    end
+    it "does not filter given a new record" do
+      expect(Bag.with_member(Fabricate.build(:member))).to contain_exactly(bag, other_bag)
     end
   end
 
diff --git a/spec/models/replication_transfer_spec.rb b/spec/models/replication_transfer_spec.rb
index b73a398d..94ca0474 100644
--- a/spec/models/replication_transfer_spec.rb
+++ b/spec/models/replication_transfer_spec.rb
@@ -215,6 +215,39 @@
 
   it_behaves_like "it has temporal scopes for", :updated_at
 
+  describe "scope with_bag" do
+    let!(:transfer) { Fabricate(:replication_transfer) }
+    let!(:other_transfer) { Fabricate(:replication_transfer) }
+    it "includes matching only" do
+      expect(ReplicationTransfer.with_bag(transfer.bag)).to match_array [transfer]
+    end
+    it "does not filter given a new record" do
+      expect(ReplicationTransfer.with_bag(Fabricate.build(:bag)))
+        .to contain_exactly(transfer, other_transfer)
+    end
+  end
+  describe "scope with_from_node" do
+    let!(:transfer) { Fabricate(:replication_transfer) }
+    let!(:other_transfer) { Fabricate(:replication_transfer) }
+    it "includes matching only" do
+      expect(ReplicationTransfer.with_from_node(transfer.from_node)).to match_array [transfer]
+    end
+    it "does not filter given a new record" do
+      expect(ReplicationTransfer.with_from_node(Fabricate.build(:node)))
+        .to contain_exactly(transfer, other_transfer)
+    end
+  end
+  describe "scope with_to_node" do
+    let!(:transfer) { Fabricate(:replication_transfer) }
+    let!(:other_transfer) { Fabricate(:replication_transfer) }
+    it "includes matching only" do
+      expect(ReplicationTransfer.with_to_node(transfer.to_node)).to match_array [transfer]
+    end
+    it "does not filter given a new record" do
+      expect(ReplicationTransfer.with_to_node(Fabricate.build(:node)))
+        .to contain_exactly(transfer, other_transfer)
+    end
+  end
   describe "scope with_store_requested" do
     it_behaves_like "a boolean filter" do
       let(:scope_name) { :with_store_requested }
diff --git a/spec/models/restore_transfer_spec.rb b/spec/models/restore_transfer_spec.rb
index d53821f7..f2b5bfac 100644
--- a/spec/models/restore_transfer_spec.rb
+++ b/spec/models/restore_transfer_spec.rb
@@ -173,6 +173,39 @@
 
   it_behaves_like "it has temporal scopes for", :updated_at
 
+  describe "scope with_bag" do
+    let!(:transfer) { Fabricate(:restore_transfer) }
+    let!(:other_transfer) { Fabricate(:restore_transfer) }
+    it "includes matching only" do
+      expect(RestoreTransfer.with_bag(transfer.bag)).to match_array [transfer]
+    end
+    it "does not filter given a new record" do
+      expect(RestoreTransfer.with_bag(Fabricate.build(:bag)))
+        .to contain_exactly(transfer, other_transfer)
+    end
+  end
+  describe "scope with_from_node" do
+    let!(:transfer) { Fabricate(:restore_transfer) }
+    let!(:other_transfer) { Fabricate(:restore_transfer) }
+    it "includes matching only" do
+      expect(RestoreTransfer.with_from_node(transfer.from_node)).to match_array [transfer]
+    end
+    it "does not filter given a new record" do
+      expect(RestoreTransfer.with_from_node(Fabricate.build(:node)))
+        .to contain_exactly(transfer, other_transfer)
+    end
+  end
+  describe "scope with_to_node" do
+    let!(:transfer) { Fabricate(:restore_transfer) }
+    let!(:other_transfer) { Fabricate(:restore_transfer) }
+    it "includes matching only" do
+      expect(RestoreTransfer.with_to_node(transfer.to_node)).to match_array [transfer]
+    end
+    it "does not filter given a new record" do
+      expect(RestoreTransfer.with_to_node(Fabricate.build(:node)))
+        .to contain_exactly(transfer, other_transfer)
+    end
+  end
   describe "scope with_accepted" do
     it_behaves_like "a boolean filter" do
       let(:scope_name) { :with_accepted }

From d57f884986990171f9af1a44cf1aaf43dbe205fd Mon Sep 17 00:00:00 2001
From: Bryan Hockey <bhock@umich.edu>
Date: Fri, 11 Nov 2016 15:52:13 -0600
Subject: [PATCH 02/14] Adapter map_belongs_to now maps models, not the
 associated model's id.

---
 app/adapters/adapter.rb                            |  7 +++----
 spec/adapters/bag_adapter_spec.rb                  |  6 +++---
 spec/adapters/fixity_check_adapter_spec.rb         |  4 ++--
 spec/adapters/ingest_adapter_spec.rb               |  2 +-
 spec/adapters/message_digest_adapter_spec.rb       |  6 +++---
 spec/adapters/replication_transfer_adapter_spec.rb | 10 +++++-----
 spec/adapters/restore_transfer_adapter_spec.rb     |  8 ++++----
 7 files changed, 21 insertions(+), 22 deletions(-)

diff --git a/app/adapters/adapter.rb b/app/adapters/adapter.rb
index 9d7b3f71..b7c32883 100644
--- a/app/adapters/adapter.rb
+++ b/app/adapters/adapter.rb
@@ -66,18 +66,17 @@ def hidden_field(model_field)
   def map_belongs_to(model_field, public_field, options = {})
     model_class = options[:model_class] || model_field.to_s.classify.constantize
     sub_method = options[:sub_method] || public_field
-    model_field_id = :"#{model_field}_id"
 
     unless options[:only] == :to
       map_from_public public_field do |value|
         record = model_class.send(:"find_by_#{sub_method}", value)
-        {model_field_id => record ? record.id : nil}
+        {model_field => record ? record : model_class.new(sub_method => value)}
       end
     end
 
     unless options[:only] == :from
-      map_to_public model_field_id do |id|
-        {public_field => model_class.find_by(id: id).send(sub_method)}
+      map_to_public model_field do |record|
+        {public_field => record.send(sub_method)}
       end
     end
   end
diff --git a/spec/adapters/bag_adapter_spec.rb b/spec/adapters/bag_adapter_spec.rb
index 0169558b..4000bdcf 100644
--- a/spec/adapters/bag_adapter_spec.rb
+++ b/spec/adapters/bag_adapter_spec.rb
@@ -61,9 +61,9 @@
       size: 10101,
       version: 1,
       version_family: @model.version_family,
-      ingest_node_id: @model.ingest_node_id,
-      admin_node_id: @model.admin_node_id,
-      member_id: @model.member_id,
+      ingest_node: @model.ingest_node,
+      admin_node: @model.admin_node,
+      member: @model.member,
       created_at: time_from_string(created_at),
       updated_at: time_from_string(updated_at),
       interpretive_bags: @model.interpretive_bags,
diff --git a/spec/adapters/fixity_check_adapter_spec.rb b/spec/adapters/fixity_check_adapter_spec.rb
index 7bc75c46..6ca21887 100644
--- a/spec/adapters/fixity_check_adapter_spec.rb
+++ b/spec/adapters/fixity_check_adapter_spec.rb
@@ -34,8 +34,8 @@
 
     @model_hash = {
       fixity_check_id: fixity_check_id,
-      bag_id: @model.bag_id,
-      node_id: @model.node_id,
+      bag: @model.bag,
+      node: @model.node,
       success: success,
       fixity_at: time_from_string("2015-02-25T15:27:40Z"),
       created_at: time_from_string("2015-02-25T15:27:40Z")
diff --git a/spec/adapters/ingest_adapter_spec.rb b/spec/adapters/ingest_adapter_spec.rb
index 9294d5fb..ac23ef19 100644
--- a/spec/adapters/ingest_adapter_spec.rb
+++ b/spec/adapters/ingest_adapter_spec.rb
@@ -33,7 +33,7 @@
 
     @model_hash = {
       ingest_id: ingest_id,
-      bag_id: @model.bag_id,
+      bag: @model.bag,
       ingested: ingested,
       nodes: @model.nodes,
       created_at: time_from_string(created_at)
diff --git a/spec/adapters/message_digest_adapter_spec.rb b/spec/adapters/message_digest_adapter_spec.rb
index 02ccb986..56691ace 100644
--- a/spec/adapters/message_digest_adapter_spec.rb
+++ b/spec/adapters/message_digest_adapter_spec.rb
@@ -31,9 +31,9 @@
     }
 
     @model_hash = {
-      bag_id: @model.bag_id,
-      node_id: @model.node_id,
-      fixity_alg_id: @model.fixity_alg_id,
+      bag: @model.bag,
+      node: @model.node,
+      fixity_alg: @model.fixity_alg,
       value: value,
       created_at: time_from_string("2015-02-25T15:27:40Z")
     }
diff --git a/spec/adapters/replication_transfer_adapter_spec.rb b/spec/adapters/replication_transfer_adapter_spec.rb
index 3bfd0c4b..3af2dffb 100644
--- a/spec/adapters/replication_transfer_adapter_spec.rb
+++ b/spec/adapters/replication_transfer_adapter_spec.rb
@@ -62,12 +62,12 @@
 
     @model_hash = {
       replication_id: replication_id,
-      from_node_id: @model.from_node_id,
-      to_node_id: @model.to_node_id,
-      bag_id: @model.bag_id,
-      protocol_id: @model.protocol_id,
+      from_node: @model.from_node,
+      to_node: @model.to_node,
+      bag: @model.bag,
+      protocol: @model.protocol,
       link: link,
-      fixity_alg_id: @model.fixity_alg_id,
+      fixity_alg: @model.fixity_alg,
       fixity_nonce: nil,
       fixity_value: fixity_value,
       store_requested: true,
diff --git a/spec/adapters/restore_transfer_adapter_spec.rb b/spec/adapters/restore_transfer_adapter_spec.rb
index 00a5c8d3..4e3c1a8a 100644
--- a/spec/adapters/restore_transfer_adapter_spec.rb
+++ b/spec/adapters/restore_transfer_adapter_spec.rb
@@ -56,10 +56,10 @@
 
     @model_hash = {
       restore_id: restore_id,
-      from_node_id: @model.from_node_id,
-      to_node_id: @model.to_node_id,
-      bag_id: @model.bag_id,
-      protocol_id: @model.protocol_id,
+      from_node: @model.from_node,
+      to_node: @model.to_node,
+      bag: @model.bag,
+      protocol: @model.protocol,
       link: link,
       accepted: accepted,
       finished: finished,

From 044ec2042f5eef0c69184bbcbf9f7a4a205ef597 Mon Sep 17 00:00:00 2001
From: Bryan Hockey <bhock@umich.edu>
Date: Mon, 14 Nov 2016 12:51:29 -0600
Subject: [PATCH 03/14] validates_associated on Ingest

---
 app/models/ingest.rb | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/app/models/ingest.rb b/app/models/ingest.rb
index 3c3696d1..a8b8f113 100644
--- a/app/models/ingest.rb
+++ b/app/models/ingest.rb
@@ -11,10 +11,12 @@ def self.find_fields
   end
   
   belongs_to :bag, inverse_of: :ingests
+  validates_associated :bag
 
   has_many :node_ingests, inverse_of: :ingest, dependent: :destroy,
     before_add: :fail_unless_new,
     before_remove: :fail_unless_new
+
   has_many :nodes, through: :node_ingests, source: :node
 
   ### ActiveModel::Dirty Validations

From 9ce6e768570886773172aad08367ff057bfaca3f Mon Sep 17 00:00:00 2001
From: Bryan Hockey <bhock@umich.edu>
Date: Mon, 14 Nov 2016 12:45:47 -0600
Subject: [PATCH 04/14] validates_associated on Bag model
 Bag#update_with_associations now expects model objects, not ids

---
 app/models/bag.rb | 28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/app/models/bag.rb b/app/models/bag.rb
index 3e11145d..e87a5334 100644
--- a/app/models/bag.rb
+++ b/app/models/bag.rb
@@ -34,23 +34,24 @@ def self.find_fields
   ### Associations
   belongs_to :ingest_node, :foreign_key => "ingest_node_id", :class_name => "Node",
              autosave: true, inverse_of: :ingest_bags
+  validates_associated :ingest_node
+
   belongs_to :admin_node, :foreign_key => "admin_node_id", :class_name => "Node",
              autosave: true, inverse_of: :admin_bags
+  validates_associated :admin_node
+
   belongs_to :member, :foreign_key => "member_id", :class_name => "Member",
              autosave: true, inverse_of: :bags
-
-  has_many :message_digests, autosave: true, dependent: :destroy, inverse_of: :bag
-  validates_associated :message_digests
-  
-  has_many :fixity_checks, inverse_of: :bag
-  has_many :ingests, inverse_of: :bag
+  validates_associated :member
 
   belongs_to :version_family, :inverse_of => :bags, autosave: true
   validates_associated :version_family
 
+  has_many :message_digests, autosave: true, dependent: :destroy, inverse_of: :bag
+  has_many :fixity_checks, inverse_of: :bag
+  has_many :ingests, inverse_of: :bag
   has_many :replication_transfers, autosave: true, inverse_of: :bag
   has_many :restore_transfers, autosave: true, inverse_of: :bag
-
   has_many :bag_nodes, inverse_of: :bag
   has_many :replicating_nodes, through: :bag_nodes, source: :node
   
@@ -110,9 +111,18 @@ def self_legal_if_first_version?
   
   def set_attributes_with_associations(new_attributes, &block)
     new_attributes = new_attributes.with_indifferent_access
-    self.attributes = new_attributes.slice(*attribute_names)
-    self.replicating_nodes = new_attributes[:replicating_nodes]
+    self.uuid           = new_attributes[:uuid]
+    self.local_id       = new_attributes[:local_id]
+    self.size           = new_attributes[:size]
+    self.version        = new_attributes[:version]
+    self.type           = new_attributes[:type]
+    self.created_at     = new_attributes[:created_at]
+    self.updated_at     = new_attributes[:updated_at]
+    self.member         = new_attributes[:member]
+    self.ingest_node    = new_attributes[:ingest_node]
+    self.admin_node     = new_attributes[:admin_node]
     self.version_family = new_attributes[:version_family]
+    self.replicating_nodes = new_attributes[:replicating_nodes]
     if self.is_a?(DataBag)
       self.rights_bags = new_attributes[:rights_bags]
       self.interpretive_bags = new_attributes[:interpretive_bags]

From f45089003715b1715ff617e61e1a48aaf1e57c69 Mon Sep 17 00:00:00 2001
From: Bryan Hockey <bhock@umich.edu>
Date: Mon, 14 Nov 2016 12:53:15 -0600
Subject: [PATCH 05/14] validates_associated on FixityCheck

---
 app/models/fixity_check.rb | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/app/models/fixity_check.rb b/app/models/fixity_check.rb
index 67141288..ea9c13c4 100644
--- a/app/models/fixity_check.rb
+++ b/app/models/fixity_check.rb
@@ -12,6 +12,8 @@ def self.find_fields
   
   belongs_to :node, inverse_of: :fixity_checks
   belongs_to :bag, inverse_of: :fixity_checks
+  validates_associated :node
+  validates_associated :bag
 
   ### ActiveModel::Dirty Validations
   validates :fixity_check_id, read_only: true, on: :update

From 354b55fb83cd1ac0f676bc9efedad0013ad2bde5 Mon Sep 17 00:00:00 2001
From: Bryan Hockey <bhock@umich.edu>
Date: Mon, 14 Nov 2016 12:54:45 -0600
Subject: [PATCH 06/14] validates_associated on RestoreTransfer

---
 app/models/restore_transfer.rb | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/app/models/restore_transfer.rb b/app/models/restore_transfer.rb
index 0d908e75..d7c469f3 100644
--- a/app/models/restore_transfer.rb
+++ b/app/models/restore_transfer.rb
@@ -33,6 +33,10 @@ def cancel!(reason, detail)
     inverse_of: :restore_transfers_to
   belongs_to :bag
   belongs_to :protocol
+  validates_associated :from_node
+  validates_associated :to_node
+  validates_associated :bag
+  validates_associated :protocol
 
 
   ### Static Validations

From ed2855fe9a95530016c6d0723fb084f924ee4028 Mon Sep 17 00:00:00 2001
From: Bryan Hockey <bhock@umich.edu>
Date: Mon, 14 Nov 2016 12:55:34 -0600
Subject: [PATCH 07/14] validates_associated on ReplicationTransfer

---
 app/models/replication_transfer.rb | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/app/models/replication_transfer.rb b/app/models/replication_transfer.rb
index d2e5c722..60958e2b 100644
--- a/app/models/replication_transfer.rb
+++ b/app/models/replication_transfer.rb
@@ -42,6 +42,11 @@ def cancel!(reason, detail)
   belongs_to :protocol
   has_one :bag_man_request, inverse_of: :replication_transfer
 
+  validates_associated :from_node
+  validates_associated :to_node
+  validates_associated :bag
+  validates_associated :fixity_alg
+  validates_associated :protocol
 
   ### Callbacks
   after_create :add_request_if_needed

From f03a65fa77a179ecb157605e45d23eb144ac49fe Mon Sep 17 00:00:00 2001
From: Bryan Hockey <bhock@umich.edu>
Date: Fri, 11 Nov 2016 15:59:55 -0600
Subject: [PATCH 08/14] fix scopes, fix strong params on BagsController (via
 removal)

---
 app/controllers/bags_controller.rb | 27 +++++----------------------
 1 file changed, 5 insertions(+), 22 deletions(-)

diff --git a/app/controllers/bags_controller.rb b/app/controllers/bags_controller.rb
index 78eea8c0..1f26a3bc 100644
--- a/app/controllers/bags_controller.rb
+++ b/app/controllers/bags_controller.rb
@@ -15,9 +15,9 @@ class BagsController < ApplicationController
   def index
     @bags = Bag.updated_after(params[:after])
       .updated_before(params[:before])
-      .with_admin_node_id(params[:admin_node_id])
-      .with_ingest_node_id(params[:ingest_node_id])
-      .with_member_id(params[:member_id])
+      .with_admin_node(params[:admin_node])
+      .with_ingest_node(params[:ingest_node])
+      .with_member(params[:member])
       .with_bag_type(params[:type])
       .replicated_by(params[:replicating_nodes])
       .order(parse_ordering(params[:order_by]))
@@ -48,7 +48,7 @@ def create
       else
         Bag.new
       end
-      if @bag.update_with_associations(create_params(params))
+      if @bag.update_with_associations(params)
         render "shared/create", status: 201
       else
         render "shared/errors", status: 400
@@ -60,7 +60,7 @@ def create
   def update
     @bag = Bag.find_by_uuid!(params[:uuid])
 
-    if @bag.update_with_associations(update_params(params))
+    if @bag.update_with_associations(params)
       render "shared/update", status: 200
     else
       render "shared/errors", status: 400
@@ -74,21 +74,4 @@ def destroy
     render nothing: true, status: 204
   end
 
-
-  private
-
-
-  def create_params(params)
-    new_params = params.permit(Bag.attribute_names)
-    new_params.merge! params.slice(
-      :replicating_nodes, :version_family,
-      :rights_bags, :interpretive_bags)
-    return new_params
-  end
-
-  def update_params(params)
-    create_params(params)
-  end
-
-
 end

From 80ad281b0640ec43cea3e071a8fd8a1f89dc0f33 Mon Sep 17 00:00:00 2001
From: Bryan Hockey <bhock@umich.edu>
Date: Fri, 11 Nov 2016 16:00:43 -0600
Subject: [PATCH 09/14] fix scopes, fix strong params on FixityChecksController

---
 app/controllers/fixity_checks_controller.rb | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/app/controllers/fixity_checks_controller.rb b/app/controllers/fixity_checks_controller.rb
index 0e709424..9cad40ec 100644
--- a/app/controllers/fixity_checks_controller.rb
+++ b/app/controllers/fixity_checks_controller.rb
@@ -16,8 +16,8 @@ def index
     @fixity_checks = FixityCheck.created_after(params[:after])
       .created_before(params[:before])
       .with_success(params[:success])
-      .with_node_id(params[:node_id])
-      .with_bag_id(params[:bag_id])
+      .with_node(params[:node])
+      .with_bag(params[:bag])
       .latest_only(convert_bool(params[:latest]))
       .page(@page)
       .per(@page_size)
@@ -41,7 +41,9 @@ def create
 
   private
   def create_params(params)
-    params.permit(FixityCheck.attribute_names)
+    params
+      .permit(:fixity_check_id, :fixity_at, :success, :created_at)
+      .merge(params.slice(:bag, :node))
   end
 
 end

From c81d87197fb55e764f9c6f01cbd6188b35bf7148 Mon Sep 17 00:00:00 2001
From: Bryan Hockey <bhock@umich.edu>
Date: Fri, 11 Nov 2016 16:11:29 -0600
Subject: [PATCH 10/14] fix scopes, fix strong params on
 ReplicationTransfersController

---
 .../replication_transfers_controller.rb       | 22 +++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/app/controllers/replication_transfers_controller.rb b/app/controllers/replication_transfers_controller.rb
index b5da5818..6f1343db 100644
--- a/app/controllers/replication_transfers_controller.rb
+++ b/app/controllers/replication_transfers_controller.rb
@@ -16,9 +16,9 @@ class ReplicationTransfersController < ApplicationController
   def index
     @replication_transfers = ReplicationTransfer.updated_after(params[:after])
       .updated_before(params[:before])
-      .with_bag_id(params[:bag_id])
-      .with_to_node_id(params[:to_node_id])
-      .with_from_node_id(params[:from_node_id])
+      .with_bag(params[:bag])
+      .with_to_node(params[:to_node])
+      .with_from_node(params[:from_node])
       .with_store_requested(params[:store_requested])
       .with_stored(params[:stored])
       .with_cancelled(params[:cancelled])
@@ -77,8 +77,22 @@ def destroy
 
 
   private
+
+  SCALAR_PARAMS = [
+    :link, :fixity_nonce, :fixity_value,
+    :created_at, :updated_at,
+    :replication_id, :store_requested, :stored,
+    :cancelled, :cancel_reason, :cancel_reason_detail
+  ]
+  ASSOCIATED_PARAMS = [
+    :bag, :from_node, :to_node, :protocol, :fixity_alg
+  ]
+
+
   def create_params(params)
-    params.permit(ReplicationTransfer.attribute_names)
+    params
+      .permit(SCALAR_PARAMS)
+      .merge(params.slice(*ASSOCIATED_PARAMS))
   end
 
 

From d9ddfbc13650c1938a2b4c4692a74458bbee267c Mon Sep 17 00:00:00 2001
From: Bryan Hockey <bhock@umich.edu>
Date: Fri, 11 Nov 2016 16:14:14 -0600
Subject: [PATCH 11/14] fix scopes, fix strong params on
 RestoreTransfersController

---
 .../restore_transfers_controller.rb           | 20 +++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/app/controllers/restore_transfers_controller.rb b/app/controllers/restore_transfers_controller.rb
index 9c8cf9e6..87cb0a19 100644
--- a/app/controllers/restore_transfers_controller.rb
+++ b/app/controllers/restore_transfers_controller.rb
@@ -16,9 +16,9 @@ class RestoreTransfersController < ApplicationController
   def index
     @restore_transfers = RestoreTransfer.updated_after(params[:after])
       .updated_before(params[:before])
-      .with_bag_id(params[:bag_id])
-      .with_to_node_id(params[:to_node_id])
-      .with_from_node_id(params[:from_node_id])
+      .with_bag(params[:bag])
+      .with_to_node(params[:to_node])
+      .with_from_node(params[:from_node])
       .with_accepted(params[:accepted])
       .with_finished(params[:finished])
       .with_cancelled(params[:cancelled])
@@ -74,8 +74,20 @@ def destroy
   end
 
   private
+
+  SCALAR_PARAMS = [
+    :link, :created_at, :updated_at,
+    :restore_id, :accepted, :finished,
+    :cancelled, :cancel_reason, :cancel_reason_detail
+  ]
+  ASSOCIATED_PARAMS = [
+    :bag, :from_node, :to_node, :protocol
+  ]
+
   def create_params(params)
-    params.permit(RestoreTransfer.attribute_names + [:requester])
+    params
+      .permit(SCALAR_PARAMS)
+      .merge(params.slice(*ASSOCIATED_PARAMS))
   end
 
 

From be105bbdc240b27b6408bd7c17c214e410abef84 Mon Sep 17 00:00:00 2001
From: Bryan Hockey <bhock@umich.edu>
Date: Fri, 11 Nov 2016 16:22:17 -0600
Subject: [PATCH 12/14] fix scopes, fix strong params on IngestsController

---
 app/controllers/ingests_controller.rb | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/app/controllers/ingests_controller.rb b/app/controllers/ingests_controller.rb
index 3095f90f..a847c690 100644
--- a/app/controllers/ingests_controller.rb
+++ b/app/controllers/ingests_controller.rb
@@ -15,7 +15,7 @@ class IngestsController < ApplicationController
   def index
     @ingests = Ingest.created_after(params[:after])
       .created_before(params[:before])
-      .with_bag_id(params[:bag_id])
+      .with_bag(params[:bag])
       .with_ingested(params[:ingested])
       .latest_only(convert_bool(params[:latest]))
       .page(@page)
@@ -39,8 +39,13 @@ def create
   
   
   private
+  SCALAR_PARAMS = [:ingest_id, :ingested, :created_at]
+  ASSOCIATED_PARAMS = [:bag, :nodes]
+
   def create_params(params)
-    params.permit(Ingest.attribute_names)
+    params
+      .permit(SCALAR_PARAMS)
+      .merge(params.slice(*ASSOCIATED_PARAMS))
   end
 
 end
\ No newline at end of file

From 961b6805089b8ca431f195e586933a126ab128eb Mon Sep 17 00:00:00 2001
From: Bryan Hockey <bhock@umich.edu>
Date: Fri, 11 Nov 2016 16:22:28 -0600
Subject: [PATCH 13/14] fix scopes, fix strong params on
 MessageDigestsController

---
 app/controllers/message_digests_controller.rb | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/app/controllers/message_digests_controller.rb b/app/controllers/message_digests_controller.rb
index 8cc57df8..c6b2dec1 100644
--- a/app/controllers/message_digests_controller.rb
+++ b/app/controllers/message_digests_controller.rb
@@ -15,7 +15,7 @@ class MessageDigestsController < ApplicationController
   def index
     @message_digests = MessageDigest.created_after(params[:after])
       .created_before(params[:before])
-      .with_bag_id(params[:bag_id])
+      .with_bag(params[:bag])
       .order(parse_ordering(params[:order_by]))
       .page(@page)
       .per(@page_size)
@@ -26,15 +26,15 @@ def index
   
   def show
     @message_digest = MessageDigest.find_by!(
-      bag_id: params[:bag_id],
-      fixity_alg_id: params[:fixity_alg_id])
+      bag_id: params[:bag]&.id,
+      fixity_alg_id: params[:fixity_alg]&.id)
     render "shared/show", status: 200
   end
   
   def create
     existing = MessageDigest.find_by(
-      bag_id: params[:bag_id],
-      fixity_alg_id: params[:fixity_alg_id]
+      bag_id: params[:bag]&.id,
+      fixity_alg_id: params[:fixity_alg]&.id
     )
     if existing
       render nothing: true, status: 409 and return
@@ -50,9 +50,13 @@ def create
   
   private
 
+  SCALAR_PARAMS = [:value, :created_at]
+  ASSOCIATED_PARAMS = [:bag, :node, :fixity_alg]
 
   def create_params(params)
-    params.permit(MessageDigest.attribute_names)
+    params
+      .permit(SCALAR_PARAMS)
+      .merge(params.slice(*ASSOCIATED_PARAMS))
   end
   
 end
\ No newline at end of file

From 02b6e9770fabfce4942ee82fa486d310e2914be8 Mon Sep 17 00:00:00 2001
From: Bryan Hockey <bhock@umich.edu>
Date: Mon, 12 Dec 2016 15:37:56 -0600
Subject: [PATCH 14/14] create and use shared example group for 'with_bag' and
 similar scopes.

---
 spec/models/bag_spec.rb                  | 32 +++++-----------
 spec/models/replication_transfer_spec.rb | 33 +++++-----------
 spec/models/restore_transfer_spec.rb     | 48 +++++++++++++-----------
 spec/support/examples/a_with_filter.rb   | 16 ++++++++
 4 files changed, 60 insertions(+), 69 deletions(-)
 create mode 100644 spec/support/examples/a_with_filter.rb

diff --git a/spec/models/bag_spec.rb b/spec/models/bag_spec.rb
index ed1cd1dd..09c6ddbc 100644
--- a/spec/models/bag_spec.rb
+++ b/spec/models/bag_spec.rb
@@ -146,35 +146,21 @@
   it_behaves_like "it has temporal scopes for", :updated_at
 
   describe "scope with_admin_node" do
-    let!(:bag) { Fabricate(:bag) }
-    let!(:other_bag) { Fabricate(:bag) }
-    it "includes matching only" do
-      expect(Bag.with_admin_node(bag.admin_node)).to match_array [bag]
-    end
-    it "does not filter given a new record" do
-      expect(Bag.with_admin_node(Fabricate.build(:node))).to contain_exactly(bag, other_bag)
+    it_behaves_like "a 'with' filter" do
+      let(:field_name) { :admin_node }
+      let(:field_factory) { :node }
     end
   end
-
   describe "scope with_ingest_node" do
-    let!(:bag) { Fabricate(:bag) }
-    let!(:other_bag) { Fabricate(:bag) }
-    it "includes matching only" do
-      expect(Bag.with_ingest_node(bag.ingest_node)).to match_array [bag]
-    end
-    it "does not filter given a new record" do
-      expect(Bag.with_ingest_node(Fabricate.build(:node))).to contain_exactly(bag, other_bag)
+    it_behaves_like "a 'with' filter" do
+      let(:field_name) { :admin_node }
+      let(:field_factory) { :node }
     end
   end
-
   describe "scope with_member" do
-    let!(:bag) { Fabricate(:bag) }
-    let!(:other_bag) { Fabricate(:bag) }
-    it "includes matching only" do
-      expect(Bag.with_member(bag.member)).to match_array [bag]
-    end
-    it "does not filter given a new record" do
-      expect(Bag.with_member(Fabricate.build(:member))).to contain_exactly(bag, other_bag)
+    it_behaves_like "a 'with' filter" do
+      let(:field_name) { :member }
+      let(:field_factory) { :member }
     end
   end
 
diff --git a/spec/models/replication_transfer_spec.rb b/spec/models/replication_transfer_spec.rb
index 94ca0474..343d7860 100644
--- a/spec/models/replication_transfer_spec.rb
+++ b/spec/models/replication_transfer_spec.rb
@@ -216,36 +216,21 @@
   it_behaves_like "it has temporal scopes for", :updated_at
 
   describe "scope with_bag" do
-    let!(:transfer) { Fabricate(:replication_transfer) }
-    let!(:other_transfer) { Fabricate(:replication_transfer) }
-    it "includes matching only" do
-      expect(ReplicationTransfer.with_bag(transfer.bag)).to match_array [transfer]
-    end
-    it "does not filter given a new record" do
-      expect(ReplicationTransfer.with_bag(Fabricate.build(:bag)))
-        .to contain_exactly(transfer, other_transfer)
+    it_behaves_like "a 'with' filter" do
+      let(:field_name) { :bag }
+      let(:field_factory) { :bag }
     end
   end
   describe "scope with_from_node" do
-    let!(:transfer) { Fabricate(:replication_transfer) }
-    let!(:other_transfer) { Fabricate(:replication_transfer) }
-    it "includes matching only" do
-      expect(ReplicationTransfer.with_from_node(transfer.from_node)).to match_array [transfer]
-    end
-    it "does not filter given a new record" do
-      expect(ReplicationTransfer.with_from_node(Fabricate.build(:node)))
-        .to contain_exactly(transfer, other_transfer)
+    it_behaves_like "a 'with' filter" do
+      let(:field_name) { :from_node }
+      let(:field_factory) { :node }
     end
   end
   describe "scope with_to_node" do
-    let!(:transfer) { Fabricate(:replication_transfer) }
-    let!(:other_transfer) { Fabricate(:replication_transfer) }
-    it "includes matching only" do
-      expect(ReplicationTransfer.with_to_node(transfer.to_node)).to match_array [transfer]
-    end
-    it "does not filter given a new record" do
-      expect(ReplicationTransfer.with_to_node(Fabricate.build(:node)))
-        .to contain_exactly(transfer, other_transfer)
+    it_behaves_like "a 'with' filter" do
+      let(:field_name) { :to_node }
+      let(:field_factory) { :node }
     end
   end
   describe "scope with_store_requested" do
diff --git a/spec/models/restore_transfer_spec.rb b/spec/models/restore_transfer_spec.rb
index f2b5bfac..29b74988 100644
--- a/spec/models/restore_transfer_spec.rb
+++ b/spec/models/restore_transfer_spec.rb
@@ -173,37 +173,41 @@
 
   it_behaves_like "it has temporal scopes for", :updated_at
 
-  describe "scope with_bag" do
-    let!(:transfer) { Fabricate(:restore_transfer) }
-    let!(:other_transfer) { Fabricate(:restore_transfer) }
+
+  # let(:field_name) Symbol of the field under test, without "with_"
+  # let(:existing_record) Record of the described class already saved to the db
+  # let(:unsaved_field_obj) Unsaved record appropriate for the scope under test
+  shared_examples "a with scope" do
+    scope_name = :"with_#{field_name}"
     it "includes matching only" do
-      expect(RestoreTransfer.with_bag(transfer.bag)).to match_array [transfer]
+      field_obj_on_existing = existing_record.public_send(field_name)
+      expect(described_class.public_send(scope_name, field_obj_on_existing))
+        .to match_array [existing_record]
     end
     it "does not filter given a new record" do
-      expect(RestoreTransfer.with_bag(Fabricate.build(:bag)))
-        .to contain_exactly(transfer, other_transfer)
+      expect(described_class.public_send(scope_name, new_field_obj))
+        .to contain_exactly()
     end
   end
-  describe "scope with_from_node" do
-    let!(:transfer) { Fabricate(:restore_transfer) }
-    let!(:other_transfer) { Fabricate(:restore_transfer) }
-    it "includes matching only" do
-      expect(RestoreTransfer.with_from_node(transfer.from_node)).to match_array [transfer]
+
+
+
+  describe "scope with_bag" do
+    it_behaves_like "a 'with' filter" do
+      let(:field_name) { :bag }
+      let(:field_factory) { :bag }
     end
-    it "does not filter given a new record" do
-      expect(RestoreTransfer.with_from_node(Fabricate.build(:node)))
-        .to contain_exactly(transfer, other_transfer)
+  end
+  describe "scope with_from_node" do
+    it_behaves_like "a 'with' filter" do
+      let(:field_name) { :from_node }
+      let(:field_factory) { :node }
     end
   end
   describe "scope with_to_node" do
-    let!(:transfer) { Fabricate(:restore_transfer) }
-    let!(:other_transfer) { Fabricate(:restore_transfer) }
-    it "includes matching only" do
-      expect(RestoreTransfer.with_to_node(transfer.to_node)).to match_array [transfer]
-    end
-    it "does not filter given a new record" do
-      expect(RestoreTransfer.with_to_node(Fabricate.build(:node)))
-        .to contain_exactly(transfer, other_transfer)
+    it_behaves_like "a 'with' filter" do
+      let(:field_name) { :to_node }
+      let(:field_factory) { :node }
     end
   end
   describe "scope with_accepted" do
diff --git a/spec/support/examples/a_with_filter.rb b/spec/support/examples/a_with_filter.rb
new file mode 100644
index 00000000..d78e4120
--- /dev/null
+++ b/spec/support/examples/a_with_filter.rb
@@ -0,0 +1,16 @@
+# @param field_name [Symbol] field name to test, without "with_"
+# @param field_factory [Symbol] factory of the class the scope pertains to
+shared_examples "a 'with' filter" do
+  factory = described_class.to_s.underscore.to_sym
+  let!(:record) { Fabricate(factory) }
+  let!(:other_record) { Fabricate(factory) }
+  let(:scope_name) { :"with_#{field_name}" }
+  it "includes matching only" do
+    expect(described_class.public_send(scope_name, record.public_send(field_name)))
+      .to match_array [record]
+  end
+  it "does not filter given anew record" do
+    expect(described_class.public_send(scope_name, Fabricate.build(field_factory)))
+      .to contain_exactly(record, other_record)
+  end
+end
\ No newline at end of file