From 4e80c87f7c63d584944e45447b5f74702be3ece1 Mon Sep 17 00:00:00 2001
From: Ashwini Sukale <ashwini.sukale@datacite.org>
Date: Wed, 3 Apr 2024 14:17:55 +0530
Subject: [PATCH] Metadata Model fixes for performance (#1141)

* Fixed metadata performance issues

* Fixed the test data

* Added more tests

* Fixed rubocop

---------

Co-authored-by: kudakwashe siziva <9620622+kaysiz@users.noreply.github.com>
---
 app/controllers/metadata_controller.rb |  2 +-
 app/models/metadata.rb                 | 24 ++++----
 spec/models/metadata_spec.rb           | 81 ++++++++++++++++++++++++++
 3 files changed, 94 insertions(+), 13 deletions(-)

diff --git a/app/controllers/metadata_controller.rb b/app/controllers/metadata_controller.rb
index da3353e61..970b0e630 100644
--- a/app/controllers/metadata_controller.rb
+++ b/app/controllers/metadata_controller.rb
@@ -7,7 +7,7 @@ class MetadataController < ApplicationController
   before_action :authenticate_user!
 
   def index
-    @doi = DataciteDoi.where(doi: params[:doi_id]).first
+    @doi = DataciteDoi.includes(:metadata).find_by(doi: params[:doi_id])
     fail ActiveRecord::RecordNotFound if @doi.blank?
 
     collection = @doi.metadata
diff --git a/app/models/metadata.rb b/app/models/metadata.rb
index 224dda991..12d6e18f8 100644
--- a/app/models/metadata.rb
+++ b/app/models/metadata.rb
@@ -28,10 +28,10 @@ def doi_id
   end
 
   def doi_id=(value)
-    r = Doi.where(doi: value).first
-    fail ActiveRecord::RecordNotFound if r.blank?
+    r = Doi.find_by(doi: value)
+    raise ActiveRecord::RecordNotFound if r.blank?
 
-    write_attribute(:dataset, r.id)
+    self.dataset = r.id
   end
 
   def client_id
@@ -41,21 +41,21 @@ def client_id
   def client_id=(value); end
 
   def metadata_must_be_valid
-    return nil if doi&.draft?
-    return nil if xml.blank?
+    return if doi&.draft? || xml.blank?
 
     doc = Nokogiri.XML(xml, nil, "UTF-8", &:noblanks)
-    return nil if doc.blank?
+    return if doc.blank?
 
-    errors.add(:xml, "XML has no namespace.") && return if namespace.blank?
+    if namespace.blank?
+      errors.add(:xml, "XML has no namespace.")
+      return
+    end
 
     # load XSD from bolognese gem
     kernel = namespace.to_s.split("/").last
-    filepath =
-      Bundler.rubygems.find_name("bolognese").first.full_gem_path +
-      "/resources/#{kernel}/metadata.xsd"
-    schema = Nokogiri::XML.Schema(open(filepath))
-    err = schema.validate(doc).map(&:to_s).unwrap
+    filepath = File.join(Gem.loaded_specs["bolognese"].full_gem_path, "resources", kernel, "metadata.xsd")
+    schema = Nokogiri::XML::Schema(File.open(filepath))
+    err = schema.validate(doc).map(&:to_s).join(", ")
     errors.add(:xml, err) if err.present?
   end
 
diff --git a/spec/models/metadata_spec.rb b/spec/models/metadata_spec.rb
index 166146b97..de5002586 100644
--- a/spec/models/metadata_spec.rb
+++ b/spec/models/metadata_spec.rb
@@ -3,11 +3,92 @@
 require "rails_helper"
 
 describe Metadata, type: :model, vcr: true do
+  let(:provider) { create(:provider, symbol: "ADMIN") }
+  let(:client) { create(:client, provider: provider) }
+  let(:doi) { create(:doi, client: client, aasm_state: "findable") }
+  let(:xml) { file_fixture("datacite.xml").read }
+
   context "validations" do
     it { should validate_presence_of(:xml) }
     it { should validate_presence_of(:namespace) }
   end
 
+  describe "associations" do
+    it { should belong_to(:doi).with_foreign_key(:dataset).inverse_of(:metadata) }
+  end
+
+  describe "#doi_id" do
+    let(:metadata) { build(:metadata, doi: doi, xml: xml) }
+
+    it "returns the DOI ID associated with the metadata" do
+      expect(metadata.doi_id).to eq(doi.doi)
+    end
+  end
+
+  describe "before_validation callbacks" do
+    let(:metadata) { build(:metadata, doi: doi, xml: xml) }
+
+    it "sets the namespace before validation" do
+      metadata.valid?
+      expect(metadata.namespace).not_to be_nil
+    end
+
+    it "sets the metadata version before validation" do
+      metadata.valid?
+      expect(metadata.metadata_version).to eq(1)
+    end
+  end
+
+  describe "#uid" do
+    let(:metadata) { Metadata.create(xml: xml, doi: doi) }
+
+    it "generates a uid for the metadata" do
+      expect(metadata.uid).not_to be_nil
+    end
+  end
+
+  describe "#client_id" do
+    let(:metadata) { build(:metadata, doi: doi, xml: xml) }
+
+    it "returns the client ID" do
+      expect(metadata.client_id).to eq(client.symbol.downcase)
+    end
+  end
+
+  describe "#metadata_must_be_valid" do
+    context "with invalid XML" do
+      let(:xml) { "invalid xml" }
+      let(:metadata) { build(:metadata, doi: doi) }
+      it "adds errors if XML is invalid" do
+        metadata.valid?
+        expect(metadata.errors[:xml]).not_to be_empty
+      end
+    end
+
+    context "with valid XML" do
+      let(:xml) { file_fixture("datacite.xml").read }
+      let(:metadata) { build(:metadata, doi: doi, xml: xml) }
+
+      it "does not add errors if XML is valid" do
+        metadata.valid?
+        expect(metadata.errors[:xml]).to be_empty
+      end
+    end
+  end
+
+  describe "#doi_id=" do
+    let(:metadata) { build(:metadata) }
+
+    it "sets the dataset attribute with the DOI id" do
+      metadata.doi_id = doi.doi
+      expect(metadata.dataset).to eq(doi.id)
+    end
+
+    it "raises ActiveRecord::RecordNotFound if DOI is not found" do
+      expect { metadata.doi_id = "invalid_doi" }.to raise_error(ActiveRecord::RecordNotFound)
+    end
+  end
+
   context "parses xml" do
     let(:provider) { create(:provider, symbol: "ADMIN") }
     let(:client) { create(:client, provider: provider) }