From ef6da3c757b49e74050c3ff4b65e1eff97c74702 Mon Sep 17 00:00:00 2001 From: Kevin Musiorski Date: Thu, 17 May 2018 18:05:40 -0500 Subject: [PATCH 01/16] rm'ing .rspec-local from git --- .rspec-local | 1 - 1 file changed, 1 deletion(-) delete mode 100644 .rspec-local diff --git a/.rspec-local b/.rspec-local deleted file mode 100644 index 49d5710b..00000000 --- a/.rspec-local +++ /dev/null @@ -1 +0,0 @@ ---format documentation From 37c1f4ad547d170c18b4f25614a2ae1984f913a9 Mon Sep 17 00:00:00 2001 From: Kevin Musiorski <1021154+RudyOnRails@users.noreply.github.com> Date: Mon, 21 May 2018 23:12:16 -0500 Subject: [PATCH 02/16] AUTOMATED TESTING: don't delete tmp dir on LakeshoreTesting.restore if files were created from memoized helper method #479 LakeshoreTesting.restore tweak to keep tmp files that are memoized in rspec let (#479) --- .../lakeshore/ingest_controller/create_spec.rb | 9 +-------- spec/support/lakeshore_testing.rb | 4 ++-- 2 files changed, 3 insertions(+), 10 deletions(-) diff --git a/spec/controllers/lakeshore/ingest_controller/create_spec.rb b/spec/controllers/lakeshore/ingest_controller/create_spec.rb index bd99e321..9cb135f1 100644 --- a/spec/controllers/lakeshore/ingest_controller/create_spec.rb +++ b/spec/controllers/lakeshore/ingest_controller/create_spec.rb @@ -12,7 +12,6 @@ end before do - LakeshoreTesting.reset_uploads sign_in_basic(apiuser) end @@ -21,7 +20,6 @@ end context "when uploading a file" do - before { LakeshoreTesting.restore } it "successfully adds the fileset to the work" do expect(Lakeshore::AttachFilesToWorkJob).to receive(:perform_later) post :create, asset_type: "StillImage", content: { intermediate: image_asset }, metadata: metadata @@ -43,8 +41,6 @@ end let(:asset) { GenericWork.where(uid: "SI-99").first } - before { LakeshoreTesting.restore } - it "successfully creates the the work" do expect(Lakeshore::AttachFilesToWorkJob).to receive(:perform_later) post :create, asset_type: "StillImage", content: { intermediate: image_asset }, metadata: metadata @@ -156,9 +152,8 @@ } end - before { LakeshoreTesting.restore } - it "updates the non_asset with the preferred representation" do + LakeshoreTesting.restore(reset_tmp_files: false) expect(Lakeshore::AttachFilesToWorkJob).to receive(:perform_later) expect(non_asset.preferred_representation_uri).to be_nil post :create, asset_type: "StillImage", content: { intermediate: image_asset }, metadata: metadata @@ -203,8 +198,6 @@ } end - before { LakeshoreTesting.restore } - it "updates the non_asset with the new preferred representation" do expect(Lakeshore::AttachFilesToWorkJob).to receive(:perform_later) expect(non_asset.preferred_representation_uri).to eq(asset.uri) diff --git a/spec/support/lakeshore_testing.rb b/spec/support/lakeshore_testing.rb index 4b8755d7..a7c6ed65 100644 --- a/spec/support/lakeshore_testing.rb +++ b/spec/support/lakeshore_testing.rb @@ -3,11 +3,11 @@ class LakeshoreTesting class << self # Removes all resources from Fedora and Solr and restores # the repository to a minimal testing state - def restore + def restore(reset_tmp_files: true) ActiveFedora::Cleaner.clean! cleanout_redis reset_derivatives - reset_uploads + reset_uploads if reset_tmp_files create_minimal_resources ListManager.new(File.join(Rails.root, "config/lists/status.yml")).create ActiveFedora::Base.all.map(&:update_index) From 2bc74b9cef36b8a6a671b479643355c8997989f3 Mon Sep 17 00:00:00 2001 From: Kevin Musiorski Date: Tue, 22 May 2018 11:22:35 -0500 Subject: [PATCH 03/16] adding format documentation to .rspec so I can search travis build logs to find specific example execution --- .rspec | 1 + 1 file changed, 1 insertion(+) diff --git a/.rspec b/.rspec index 253ba544..b3354e98 100644 --- a/.rspec +++ b/.rspec @@ -1,3 +1,4 @@ --color --profile +--format documentation --format progress From 987606f215b6ffb49efebad779a16e818f7f66d4 Mon Sep 17 00:00:00 2001 From: Kevin Musiorski <1021154+RudyOnRails@users.noreply.github.com> Date: Tue, 22 May 2018 16:26:24 -0500 Subject: [PATCH 04/16] 2943 (part II): non-memoize file creation processes so they get invoked even when LakeshoreTesting.restore'ing #480 https://cits.artic.edu/issues/2943 * Revert "AUTOMATED TESTING: don't delete tmp dir on LakeshoreTesting.restore if files were created from memoized helper method #479" * non-memoize test file creations via let-bang so that file gets recreated after LakeshoreTesting.restore's are used --- .rspec | 1 - .../lakeshore/ingest_controller/create_spec.rb | 13 +++++++++++-- .../lakeshore/ingest_controller/update_spec.rb | 8 ++++++-- spec/models/sufia/uploaded_file_spec.rb | 4 +++- spec/support/lakeshore_testing.rb | 4 ++-- 5 files changed, 22 insertions(+), 8 deletions(-) diff --git a/.rspec b/.rspec index b3354e98..c2e29743 100644 --- a/.rspec +++ b/.rspec @@ -1,4 +1,3 @@ --color --profile --format documentation ---format progress diff --git a/spec/controllers/lakeshore/ingest_controller/create_spec.rb b/spec/controllers/lakeshore/ingest_controller/create_spec.rb index 9cb135f1..0f62e96a 100644 --- a/spec/controllers/lakeshore/ingest_controller/create_spec.rb +++ b/spec/controllers/lakeshore/ingest_controller/create_spec.rb @@ -5,13 +5,16 @@ let(:apiuser) { create(:apiuser) } let(:user) { create(:user1) } - let(:image_asset) do + # bang this so it is not memoized and gets invoked even after tmp dir is wiped via LakeshoreTesting + # https://cits.artic.edu/issues/2943 + let!(:image_asset) do ActionDispatch::Http::UploadedFile.new(filename: "sun.png", content_type: "image/png", tempfile: File.new(File.join(fixture_path, "sun.png"))) end before do + LakeshoreTesting.reset_uploads sign_in_basic(apiuser) end @@ -20,6 +23,7 @@ end context "when uploading a file" do + before { LakeshoreTesting.restore } it "successfully adds the fileset to the work" do expect(Lakeshore::AttachFilesToWorkJob).to receive(:perform_later) post :create, asset_type: "StillImage", content: { intermediate: image_asset }, metadata: metadata @@ -41,6 +45,8 @@ end let(:asset) { GenericWork.where(uid: "SI-99").first } + before { LakeshoreTesting.restore } + it "successfully creates the the work" do expect(Lakeshore::AttachFilesToWorkJob).to receive(:perform_later) post :create, asset_type: "StillImage", content: { intermediate: image_asset }, metadata: metadata @@ -152,8 +158,9 @@ } end + before { LakeshoreTesting.restore } + it "updates the non_asset with the preferred representation" do - LakeshoreTesting.restore(reset_tmp_files: false) expect(Lakeshore::AttachFilesToWorkJob).to receive(:perform_later) expect(non_asset.preferred_representation_uri).to be_nil post :create, asset_type: "StillImage", content: { intermediate: image_asset }, metadata: metadata @@ -198,6 +205,8 @@ } end + before { LakeshoreTesting.restore } + it "updates the non_asset with the new preferred representation" do expect(Lakeshore::AttachFilesToWorkJob).to receive(:perform_later) expect(non_asset.preferred_representation_uri).to eq(asset.uri) diff --git a/spec/controllers/lakeshore/ingest_controller/update_spec.rb b/spec/controllers/lakeshore/ingest_controller/update_spec.rb index 77dc471c..6e102cd4 100644 --- a/spec/controllers/lakeshore/ingest_controller/update_spec.rb +++ b/spec/controllers/lakeshore/ingest_controller/update_spec.rb @@ -5,13 +5,17 @@ let(:apiuser) { create(:apiuser) } let(:user) { create(:user1) } - let(:image_asset) do + # bang this so it is not memoized and gets invoked even after tmp dir is wiped via LakeshoreTesting + # https://cits.artic.edu/issues/2943 + let!(:image_asset) do ActionDispatch::Http::UploadedFile.new(filename: "sun.png", content_type: "image/png", tempfile: File.new(File.join(fixture_path, "sun.png"))) end - let(:replacement_asset) do + # bang this so it is not memoized and gets invoked even after tmp dir is wiped via LakeshoreTesting + # https://cits.artic.edu/issues/2943 + let!(:replacement_asset) do ActionDispatch::Http::UploadedFile.new(filename: "tardis.png", content_type: "image/png", tempfile: File.new(File.join(fixture_path, "tardis.png"))) diff --git a/spec/models/sufia/uploaded_file_spec.rb b/spec/models/sufia/uploaded_file_spec.rb index 28273fb1..0c183635 100644 --- a/spec/models/sufia/uploaded_file_spec.rb +++ b/spec/models/sufia/uploaded_file_spec.rb @@ -2,7 +2,9 @@ require 'rails_helper' describe Sufia::UploadedFile do - let(:file) do + # bang this so it is not memoized and gets invoked even after tmp dir is wiped via LakeshoreTesting + # https://cits.artic.edu/issues/2943 + let!(:file) do ActionDispatch::Http::UploadedFile.new(filename: "sun.png", content_type: "image/png", tempfile: File.new(File.join(fixture_path, "sun.png"))) diff --git a/spec/support/lakeshore_testing.rb b/spec/support/lakeshore_testing.rb index a7c6ed65..4b8755d7 100644 --- a/spec/support/lakeshore_testing.rb +++ b/spec/support/lakeshore_testing.rb @@ -3,11 +3,11 @@ class LakeshoreTesting class << self # Removes all resources from Fedora and Solr and restores # the repository to a minimal testing state - def restore(reset_tmp_files: true) + def restore ActiveFedora::Cleaner.clean! cleanout_redis reset_derivatives - reset_uploads if reset_tmp_files + reset_uploads create_minimal_resources ListManager.new(File.join(Rails.root, "config/lists/status.yml")).create ActiveFedora::Base.all.map(&:update_index) From 03b95361bba0c365daa5a2ec7d41a9c7a16eb3da Mon Sep 17 00:00:00 2001 From: Kevin Musiorski Date: Wed, 23 May 2018 15:18:05 -0500 Subject: [PATCH 05/16] rm'ing some .DS_Store file that got in --- spec/.DS_Store | Bin 6148 -> 0 bytes 1 file changed, 0 insertions(+), 0 deletions(-) delete mode 100644 spec/.DS_Store diff --git a/spec/.DS_Store b/spec/.DS_Store deleted file mode 100644 index da5154159fe9d6a913fe1ced93bdfb4abc0eb8b8..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 6148 zcmeHKJ8DBQ5S)!&JV@ixrLK@02;-a}7YK=M2m}mq3@KmbbLD86{S+}T?wrQV!fK?| zj)bOo`xbz$j++Ny0brmz;^f2BeBXUycNH-roo5{Jfh`WZ&Ea*D{dd5*19o`B7e0UU zU-tWV-;_xKDIf);fE17dS5=_O>u`V7Q+1dWkOH@$fPWtv-LV&riSg;+5G??4&Tts# z(Mu4U2Z+6JOk{*+NhK!Ls>QIRGu|q%7mkTZhgI`ob+c86VsSgqZ;=k`i5jJV6gXF4 zmdl0L|3~_T{{Nh$l@yQyH>H5h*H7yepH#JV@;I-xjs8scoNu}t=Rx5R<(L@dm Date: Tue, 29 May 2018 22:47:49 -0500 Subject: [PATCH 06/16] change rspec to run `--order defined` (vs random seed) #483 --- .rspec | 1 + 1 file changed, 1 insertion(+) diff --git a/.rspec b/.rspec index c2e29743..80ba7126 100644 --- a/.rspec +++ b/.rspec @@ -1,3 +1,4 @@ +--order defined --color --profile --format documentation From f75f7dde0a8d6bea50ad4f4a91a5d97680a80998 Mon Sep 17 00:00:00 2001 From: Kevin Musiorski <1021154+RudyOnRails@users.noreply.github.com> Date: Wed, 30 May 2018 11:20:10 -0500 Subject: [PATCH 07/16] remove resetting of tmp uploads and see if upsets anything (#481) --- spec/support/lakeshore_testing.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/support/lakeshore_testing.rb b/spec/support/lakeshore_testing.rb index 4b8755d7..67f762d0 100644 --- a/spec/support/lakeshore_testing.rb +++ b/spec/support/lakeshore_testing.rb @@ -7,7 +7,7 @@ def restore ActiveFedora::Cleaner.clean! cleanout_redis reset_derivatives - reset_uploads + # reset_uploads create_minimal_resources ListManager.new(File.join(Rails.root, "config/lists/status.yml")).create ActiveFedora::Base.all.map(&:update_index) From 13f068a75cf30017498dcd1c057e3499b85f5018 Mon Sep 17 00:00:00 2001 From: Kevin Musiorski <1021154+RudyOnRails@users.noreply.github.com> Date: Wed, 30 May 2018 13:49:34 -0500 Subject: [PATCH 08/16] Bug #1686: GUI: improve duplicate file checking (rapid GUI batches, and dupes in same batch) (#445) https://cits.artic.edu/issues/1686 --- .travis.yml | 4 ++ .../sufia/batch_uploads_controller.rb | 7 +++ app/controllers/sufia/uploads_controller.rb | 13 +++++- app/models/concerns/uploaded_file.rb | 36 ++++++++++++++++ app/models/uploaded_batch.rb | 3 ++ app/services/checksum_service.rb | 12 ++++++ app/validators/checksum_validator.rb | 43 +++++++++++++++++++ app/views/sufia/batch_uploads/_form.html.erb | 1 + config/application.rb | 4 ++ config/locales/lakeshore.en.yml | 2 + .../20180511222000_create_uploaded_batches.rb | 8 ++++ ...add_uploaded_batch_id_to_uploaded_files.rb | 5 +++ ...0517141303_add_status_to_uploaded_files.rb | 5 +++ ...17143221_add_checksum_to_uploaded_files.rb | 6 +++ db/schema.rb | 16 +++++-- lib/tasks/citi_notifications.rake | 11 ----- spec/actors/replace_file_actor_spec.rb | 6 ++- .../ingest_controller/update_spec.rb | 2 +- .../sufia/batch_uploads_controller_spec.rb | 13 ++++++ .../sufia/uploads_controller_spec.rb | 8 +++- spec/factories/files.rb | 38 ++++++++++++++++ spec/factories/uploaded_files.rb | 13 ++++++ spec/features/batch_upload_spec.rb | 19 ++++++++ spec/jobs/attach_files_to_work_job_spec.rb | 11 +++-- spec/jobs/batch_asset_create_job_spec.rb | 2 + spec/jobs/create_asset_job_spec.rb | 18 ++++---- spec/models/lakeshore/ingest_spec.rb | 18 +++++--- spec/models/sufia/uploaded_file_spec.rb | 30 ++++++++++++- spec/models/uploaded_batch_spec.rb | 8 ++++ ...licate_upload_verification_service_spec.rb | 4 +- spec/support/lakeshore_testing.rb | 2 +- 31 files changed, 327 insertions(+), 41 deletions(-) create mode 100644 app/models/concerns/uploaded_file.rb create mode 100644 app/models/uploaded_batch.rb create mode 100644 app/services/checksum_service.rb create mode 100644 app/validators/checksum_validator.rb create mode 100644 db/migrate/20180511222000_create_uploaded_batches.rb create mode 100644 db/migrate/20180511225722_add_uploaded_batch_id_to_uploaded_files.rb create mode 100644 db/migrate/20180517141303_add_status_to_uploaded_files.rb create mode 100644 db/migrate/20180517143221_add_checksum_to_uploaded_files.rb create mode 100644 spec/factories/files.rb create mode 100644 spec/factories/uploaded_files.rb create mode 100644 spec/models/uploaded_batch_spec.rb diff --git a/.travis.yml b/.travis.yml index ddac0dbb..c2c14761 100644 --- a/.travis.yml +++ b/.travis.yml @@ -38,6 +38,10 @@ matrix: allow_failures: - env: TEST_SUITE=jasmine +after_script: + - "bundle exec rake db:version" + - "bundle exec rake db:migrate:status" + script: - "bundle exec rake ci:$TEST_SUITE" diff --git a/app/controllers/sufia/batch_uploads_controller.rb b/app/controllers/sufia/batch_uploads_controller.rb index fe44095f..9f4bee55 100644 --- a/app/controllers/sufia/batch_uploads_controller.rb +++ b/app/controllers/sufia/batch_uploads_controller.rb @@ -2,6 +2,8 @@ class Sufia::BatchUploadsController < ApplicationController include Sufia::BatchUploadsControllerBehavior + after_action :set_uploaded_file_status, only: [:create] + def self.form_class BatchUploadForm end @@ -23,4 +25,9 @@ def build_form @form.current_ability = current_ability @form.parameterized_relationships = ParameterizedRelationships.new(params) end + + def set_uploaded_file_status + uploaded_file_ids = params["uploaded_files"] + Sufia::UploadedFile.change_status(uploaded_file_ids, "begun_ingestion") + end end diff --git a/app/controllers/sufia/uploads_controller.rb b/app/controllers/sufia/uploads_controller.rb index 4a76ee82..df91520c 100644 --- a/app/controllers/sufia/uploads_controller.rb +++ b/app/controllers/sufia/uploads_controller.rb @@ -5,8 +5,13 @@ class UploadsController < ApplicationController before_action :validate_asset_type, :validate_duplicate_upload, only: [:create] def create - @upload.attributes = { file: uploaded_file, user: current_user, use_uri: use_uri } - @upload.save! + @upload.attributes = { file: uploaded_file, user: current_user, use_uri: use_uri, uploaded_batch_id: uploaded_batch_id } + + if @upload.save + render status: :ok + else + render json: { files: [@upload.errors.messages[:checksum].first] } + end end def destroy @@ -40,6 +45,10 @@ def uploaded_file params[:files].first end + def uploaded_batch_id + asset_attributes.fetch(:uploaded_batch_id, nil) + end + def asset_type asset_attributes.fetch(:asset_type) end diff --git a/app/models/concerns/uploaded_file.rb b/app/models/concerns/uploaded_file.rb new file mode 100644 index 00000000..ff3742fb --- /dev/null +++ b/app/models/concerns/uploaded_file.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true +module UploadedFile + include ChecksumService + extend ActiveSupport::Concern + + included do + extend ClassMethods + belongs_to :batch_upload + after_initialize :set_status, if: "status.nil?" + before_validation :calculate_checksum + + validates :checksum, presence: true, checksum: true, on: :create + + scope :begun_ingestion, -> { where(status: "begun_ingestion") } + + attr_accessor :uploaded_file + end + + module ClassMethods + # @param [Array] uploaded_files_ids array of ids in batch upload + # @param [String] new string that status should be updated to + def change_status(uploaded_files_ids, str) + where(id: uploaded_files_ids).update_all(status: str) + end + end + + private + + def calculate_checksum + self.checksum = fedora_shasum + end + + def set_status + self.status = "new" + end +end diff --git a/app/models/uploaded_batch.rb b/app/models/uploaded_batch.rb new file mode 100644 index 00000000..ac79ccd8 --- /dev/null +++ b/app/models/uploaded_batch.rb @@ -0,0 +1,3 @@ +# frozen_string_literal: true +class UploadedBatch < ActiveRecord::Base +end diff --git a/app/services/checksum_service.rb b/app/services/checksum_service.rb new file mode 100644 index 00000000..d282c131 --- /dev/null +++ b/app/services/checksum_service.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true +module ChecksumService + # Calculate the SHA1 checksum and format it like Fedora does + def fedora_shasum + "urn:sha1:#{Digest::SHA1.file(file_path)}" + end + + def file_path + return file.path if file.respond_to?(:path) + file.file.file.path + end +end diff --git a/app/validators/checksum_validator.rb b/app/validators/checksum_validator.rb new file mode 100644 index 00000000..b6c527d9 --- /dev/null +++ b/app/validators/checksum_validator.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true +class ChecksumValidator < ActiveModel::Validator + include Rails.application.routes.url_helpers + + attr_reader :record + + def validate(record) + @record = record + if !verification_service.duplicates.empty? + record.errors.add(:checksum, in_solr_error_message) + elsif Sufia::UploadedFile.begun_ingestion.map(&:checksum).include?(record.checksum) + record.errors.add(:checksum, begun_ingestion_error_message) + elsif Sufia::UploadedFile.where(uploaded_batch_id: record.uploaded_batch_id).map(&:checksum).include?(record.checksum) + record.errors.add(:checksum, already_in_batch_error_message) + end + end + + private + + def in_solr_error_message + { + error: I18n.t('lakeshore.upload.errors.duplicate', name: @record.file.filename), + duplicate_path: polymorphic_path(verification_service.duplicates.first), + duplicate_name: verification_service.duplicates.first.to_s + } + end + + def begun_ingestion_error_message + { + error: I18n.t('lakeshore.upload.errors.begun_ingestion', name: @record.file.filename) + } + end + + def already_in_batch_error_message + { + error: I18n.t('lakeshore.upload.errors.already_in_batch', name: @record.file.filename) + } + end + + def verification_service + @verification_service ||= DuplicateUploadVerificationService.new(record.file) + end +end diff --git a/app/views/sufia/batch_uploads/_form.html.erb b/app/views/sufia/batch_uploads/_form.html.erb index 57aed6c5..fe6c85ba 100644 --- a/app/views/sufia/batch_uploads/_form.html.erb +++ b/app/views/sufia/batch_uploads/_form.html.erb @@ -6,6 +6,7 @@ <%= render 'curation_concerns/base/guts4form', f: f, tabs: %w[files metadata relationships share] do %> <% end %> <%= f.input :use_uri, as: :hidden %> + <%= f.hidden_field :uploaded_batch_id, value: UploadedBatch.create.id %> <% end %> <%= render "lakeshore_js_template" %> diff --git a/config/application.rb b/config/application.rb index be0b9691..e8e42f0f 100644 --- a/config/application.rb +++ b/config/application.rb @@ -38,6 +38,10 @@ class Application < Rails::Application # Do not swallow errors in after_commit/after_rollback callbacks. config.active_record.raise_in_transactional_callbacks = true + + config.to_prepare do + Sufia::UploadedFile.include UploadedFile + end end class DuplicateAssetError < StandardError; end diff --git a/config/locales/lakeshore.en.yml b/config/locales/lakeshore.en.yml index 7faf55ce..e450cc21 100644 --- a/config/locales/lakeshore.en.yml +++ b/config/locales/lakeshore.en.yml @@ -13,6 +13,8 @@ en: errors: invalid: "Incorrect asset type. %{name} is not a type of %{type}" duplicate: "%{name} is a duplicate of:" + begun_ingestion: "%{name} has already begun ingestion." + already_in_batch: "%{name} is already in this batch." relationship: add_assets: "Add New Assets to this CITI " diff --git a/db/migrate/20180511222000_create_uploaded_batches.rb b/db/migrate/20180511222000_create_uploaded_batches.rb new file mode 100644 index 00000000..45a6afdf --- /dev/null +++ b/db/migrate/20180511222000_create_uploaded_batches.rb @@ -0,0 +1,8 @@ +class CreateUploadedBatches < ActiveRecord::Migration + def change + create_table :uploaded_batches do |t| + + t.timestamps null: false + end + end +end \ No newline at end of file diff --git a/db/migrate/20180511225722_add_uploaded_batch_id_to_uploaded_files.rb b/db/migrate/20180511225722_add_uploaded_batch_id_to_uploaded_files.rb new file mode 100644 index 00000000..f869e102 --- /dev/null +++ b/db/migrate/20180511225722_add_uploaded_batch_id_to_uploaded_files.rb @@ -0,0 +1,5 @@ +class AddUploadedBatchIdToUploadedFiles < ActiveRecord::Migration + def change + add_reference :uploaded_files, :uploaded_batch, index: true, foreign_key: true + end +end \ No newline at end of file diff --git a/db/migrate/20180517141303_add_status_to_uploaded_files.rb b/db/migrate/20180517141303_add_status_to_uploaded_files.rb new file mode 100644 index 00000000..d051a23e --- /dev/null +++ b/db/migrate/20180517141303_add_status_to_uploaded_files.rb @@ -0,0 +1,5 @@ +class AddStatusToUploadedFiles < ActiveRecord::Migration + def change + add_column :uploaded_files, :status, :string + end +end \ No newline at end of file diff --git a/db/migrate/20180517143221_add_checksum_to_uploaded_files.rb b/db/migrate/20180517143221_add_checksum_to_uploaded_files.rb new file mode 100644 index 00000000..ba7a451f --- /dev/null +++ b/db/migrate/20180517143221_add_checksum_to_uploaded_files.rb @@ -0,0 +1,6 @@ +class AddChecksumToUploadedFiles < ActiveRecord::Migration + def change + add_column :uploaded_files, :checksum, :string + add_index :uploaded_files, :checksum + end +end \ No newline at end of file diff --git a/db/schema.rb b/db/schema.rb index 767b3746..cc52e3d4 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20161031025602) do +ActiveRecord::Schema.define(version: 20180517143221) do create_table "bookmarks", force: :cascade do |t| t.integer "user_id", null: false @@ -259,16 +259,26 @@ t.datetime "updated_at", null: false end + create_table "uploaded_batches", force: :cascade do |t| + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + end + create_table "uploaded_files", force: :cascade do |t| t.string "file" t.integer "user_id" t.string "file_set_uri" - t.datetime "created_at", null: false - t.datetime "updated_at", null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false t.string "use_uri" + t.integer "uploaded_batch_id" + t.string "status" + t.string "checksum" end + add_index "uploaded_files", ["checksum"], name: "index_uploaded_files_on_checksum" add_index "uploaded_files", ["file_set_uri"], name: "index_uploaded_files_on_file_set_uri" + add_index "uploaded_files", ["uploaded_batch_id"], name: "index_uploaded_files_on_uploaded_batch_id" add_index "uploaded_files", ["use_uri"], name: "index_uploaded_files_on_use_uri" add_index "uploaded_files", ["user_id"], name: "index_uploaded_files_on_user_id" diff --git a/lib/tasks/citi_notifications.rake b/lib/tasks/citi_notifications.rake index de89a003..792c9d3a 100644 --- a/lib/tasks/citi_notifications.rake +++ b/lib/tasks/citi_notifications.rake @@ -1,15 +1,4 @@ # frozen_string_literal: true -require 'benchmark' - -class Rake::Task - def execute_with_benchmark(*args) - bm = Benchmark.measure { execute_without_benchmark(*args) } - puts " #{name} --> #{bm}" - end - - alias execute_without_benchmark execute - alias execute execute_with_benchmark -end namespace :citi_notifications do desc "Counts the number assets that have been modified after a certain date 'YYYY-MM-DD', are preferred reps, and have an imaging uid" diff --git a/spec/actors/replace_file_actor_spec.rb b/spec/actors/replace_file_actor_spec.rb index a513283a..56a20f18 100644 --- a/spec/actors/replace_file_actor_spec.rb +++ b/spec/actors/replace_file_actor_spec.rb @@ -1,4 +1,4 @@ -# frozen_string_literal: true +# frozen_string_literal: false require 'rails_helper' describe ReplaceFileActor do @@ -7,6 +7,8 @@ let(:file) { File.open(File.join(fixture_path, "sun.png")) } let(:original_file) { Sufia::UploadedFile.create(file: file, user: user, use_uri: AICType.OriginalFileSet) } + before { LakeshoreTesting.restore } + describe "an intermediate file" do context "when it is present" do let(:asset) { create(:asset, :with_intermediate_file_set) } @@ -24,7 +26,7 @@ it "replaces the content of the existing file set" do expect(IngestFileJob).to receive(:perform_later).with( asset.intermediate_file_set.first, - end_with("tmp/test-uploads/sufia/uploaded_file/file/1/sun.png"), + end_with("uploads/sufia/uploaded_file/file/1/sun.png"), user, mime_type: "image/png", relation: "original_file") actor.update(attributes) diff --git a/spec/controllers/lakeshore/ingest_controller/update_spec.rb b/spec/controllers/lakeshore/ingest_controller/update_spec.rb index 6e102cd4..c4a71141 100644 --- a/spec/controllers/lakeshore/ingest_controller/update_spec.rb +++ b/spec/controllers/lakeshore/ingest_controller/update_spec.rb @@ -22,8 +22,8 @@ end before do - LakeshoreTesting.reset_uploads sign_in_basic(apiuser) + LakeshoreTesting.restore end describe "replacing files" do diff --git a/spec/controllers/sufia/batch_uploads_controller_spec.rb b/spec/controllers/sufia/batch_uploads_controller_spec.rb index 370d9e35..e37570ff 100644 --- a/spec/controllers/sufia/batch_uploads_controller_spec.rb +++ b/spec/controllers/sufia/batch_uploads_controller_spec.rb @@ -2,7 +2,20 @@ require 'rails_helper' describe Sufia::BatchUploadsController do + routes { Sufia::Engine.routes } + include_context "authenticated saml user" + subject { described_class } + let(:uploaded_file) { create(:uploaded_file) } + let(:work_attributes) { { "asset_type" => AICType.StillImage.to_s, "use_uri" => AICType.IntermediateFileSet.to_s, "uploaded_batch_id" => "999" } } its(:form_class) { is_expected.to eq(BatchUploadForm) } + + it "switches the status of Sufia Uploaded File" do + ActiveJob::Base.queue_adapter = :test + expect(uploaded_file.status).to eq "new" + post :create, uploaded_files: [uploaded_file.id.to_s], batch_upload_item: work_attributes + expect(uploaded_file.reload.status).to eq "begun_ingestion" + ActiveJob::Base.queue_adapter = :inline + end end diff --git a/spec/controllers/sufia/uploads_controller_spec.rb b/spec/controllers/sufia/uploads_controller_spec.rb index b096bbed..4a47bb25 100644 --- a/spec/controllers/sufia/uploads_controller_spec.rb +++ b/spec/controllers/sufia/uploads_controller_spec.rb @@ -6,7 +6,10 @@ include_context "authenticated saml user" let(:duplicates) { [] } - before { allow(controller).to receive(:duplicate_upload).and_return(duplicates) } + before do + LakeshoreTesting.restore + allow(controller).to receive(:duplicate_upload).and_return(duplicates) + end describe "uploading a single asset" do before { post :create, files: [file], generic_work: work_attributes, format: 'json' } @@ -54,11 +57,12 @@ context "with a valid still image" do let(:file) { fixture_file_upload('sun.png', 'image/png') } - let(:work_attributes) { { "asset_type" => AICType.StillImage.to_s, "use_uri" => AICType.IntermediateFileSet.to_s } } + let(:work_attributes) { { "asset_type" => AICType.StillImage.to_s, "use_uri" => AICType.IntermediateFileSet.to_s, "uploaded_batch_id" => "999" } } it "successfully uploads the file" do expect(response).to be_success expect(assigns(:upload)).to be_kind_of Sufia::UploadedFile + expect(assigns(:upload).uploaded_batch_id).to eq 999 expect(assigns(:upload)).to be_persisted end end diff --git a/spec/factories/files.rb b/spec/factories/files.rb new file mode 100644 index 00000000..1dbf71ad --- /dev/null +++ b/spec/factories/files.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: false +FactoryGirl.define do + factory :file, class: ActionDispatch::Http::UploadedFile do + skip_create + + factory :image_file do + tempfile File.new("#{Rails.root}/spec/fixtures/sun.png") + filename "sun.png" + content_type "image/png" + + factory :intermediate_upload do + tempfile File.new("#{Rails.root}/spec/fixtures/tardis.png") + end + + factory :original_upload do + tempfile File.new("#{Rails.root}/spec/fixtures/tardis2.png") + end + + factory :presevation_master_upload do + tempfile File.new("#{Rails.root}/spec/fixtures/text.png") + end + end + + factory :text_file do + tempfile File.new("#{Rails.root}/spec/fixtures/text.txt") + filename "text.txt" + content_type "text/plain" + end + + factory :oddball_file2 do + tempfile File.new("#{Rails.root}/spec/fixtures/api_409.json") + filename "409.json" + content_type "image/png" + end + + initialize_with { new(attributes) } + end +end diff --git a/spec/factories/uploaded_files.rb b/spec/factories/uploaded_files.rb new file mode 100644 index 00000000..0064db9d --- /dev/null +++ b/spec/factories/uploaded_files.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: false +FactoryGirl.define do + factory :uploaded_file, class: Sufia::UploadedFile do + association :file, factory: :image_file + association :user, factory: :default_user + use_uri "http://file.use.uri" + uploaded_batch_id "1" + + factory :begun_ingestion do + status "begun_ingestion" + end + end +end diff --git a/spec/features/batch_upload_spec.rb b/spec/features/batch_upload_spec.rb index 714f2d19..061bd0b7 100644 --- a/spec/features/batch_upload_spec.rb +++ b/spec/features/batch_upload_spec.rb @@ -9,6 +9,20 @@ describe "uploading a new asset;" do before { LakeshoreTesting.restore } + context "when the file/binary is already in the current batch" do + before do + visit("/batch_uploads/new") + select("Still Image", from: "asset_type_select") + attach_file('files[]', File.join(fixture_path, "sun.png"), visible: false) + attach_file('files[]', File.join(fixture_path, "sun.png"), visible: false) + end + + it "you get an error informing you of what's wrong" do + expect(page).to have_content "already in this batch" + expect(page).to have_button('Save', disabled: true) + end + end + context "signed in as a non-admin;" do it "enforces a workflow to ensure the asset is correctly ingested" do visit("/batch_uploads/new") @@ -95,6 +109,11 @@ visit("/batch_uploads/new#metadata") expect(page).not_to have_field("Imaging UID") end + + it "contains a hidden input for uploaded_batch_id" do + visit("/batch_uploads/new") + expect(page).to have_selector("input#batch_upload_item_uploaded_batch_id", visible: false) + end end context "signed in as an admin;" do diff --git a/spec/jobs/attach_files_to_work_job_spec.rb b/spec/jobs/attach_files_to_work_job_spec.rb index a5c675c2..9cdd019c 100644 --- a/spec/jobs/attach_files_to_work_job_spec.rb +++ b/spec/jobs/attach_files_to_work_job_spec.rb @@ -4,12 +4,17 @@ describe AttachFilesToWorkJob do let(:user) { create(:user1) } let(:file) { File.open(File.join(fixture_path, "sun.png")) } + let(:file0) { File.open(File.join(fixture_path, "tardis.png")) } + let(:file1) { File.open(File.join(fixture_path, "tardis2.png")) } + let(:file2) { File.open(File.join(fixture_path, "text.png")) } let(:original_file) { Sufia::UploadedFile.create(file: file, user: user, use_uri: AICType.OriginalFileSet) } - let(:intermediate_file) { Sufia::UploadedFile.create(file: file, user: user, use_uri: AICType.IntermediateFileSet) } - let(:master_file) { Sufia::UploadedFile.create(file: file, user: user, use_uri: AICType.PreservationMasterFileSet) } - let(:plain_file) { Sufia::UploadedFile.create(file: file, user: user) } + let(:intermediate_file) { Sufia::UploadedFile.create(file: file0, user: user, use_uri: AICType.IntermediateFileSet) } + let(:master_file) { Sufia::UploadedFile.create(file: file1, user: user, use_uri: AICType.PreservationMasterFileSet) } + let(:plain_file) { Sufia::UploadedFile.create(file: file2, user: user) } let(:asset) { create(:department_asset) } + before { LakeshoreTesting.restore } + context "with each of the different use files" do let(:types) { asset.file_sets.map(&:type).map { |set| set.map(&:to_s) }.flatten.uniq } it "attaches the files to the work and updates them" do diff --git a/spec/jobs/batch_asset_create_job_spec.rb b/spec/jobs/batch_asset_create_job_spec.rb index 7c896022..ff3ad11d 100644 --- a/spec/jobs/batch_asset_create_job_spec.rb +++ b/spec/jobs/batch_asset_create_job_spec.rb @@ -34,6 +34,8 @@ ) end + before { LakeshoreTesting.restore } + it "creates two assets" do expect(CurationConcerns::CurationConcern).to receive(:actor).and_return(actor).twice expect(actor).to receive(:create).with(asset_type: AICType.StillImage.to_s, pref_label: 'File One', uploaded_files: ['1']).and_return(true) diff --git a/spec/jobs/create_asset_job_spec.rb b/spec/jobs/create_asset_job_spec.rb index 41bd7d65..9ca5fca5 100644 --- a/spec/jobs/create_asset_job_spec.rb +++ b/spec/jobs/create_asset_job_spec.rb @@ -2,14 +2,16 @@ require 'rails_helper' describe CreateAssetJob do - let(:user) { create(:user1) } - let(:file) { Sufia::UploadedFile.create } - let(:attributes) { { uploaded_files: [file.id] } } - let(:log) { Sufia::BatchCreateOperation.new } - let(:actor) { double } - let(:service) { double } + let(:file) { File.open(File.join(fixture_path, "sun.png")) } + let(:user) { create(:user1) } + let(:uploaded_file) { Sufia::UploadedFile.create(file: file, user: user, use_uri: AICType.OriginalFileSet) } + let(:attributes) { { uploaded_files: [uploaded_file.id] } } + let(:log) { Sufia::BatchCreateOperation.new } + let(:actor) { double } + let(:service) { double } before do + LakeshoreTesting.restore allow(CurationConcerns::CurationConcern).to receive(:actor) .with(instance_of(GenericWork), user) .and_return(actor) @@ -19,7 +21,7 @@ before { allow(actor).to receive(:create).and_return(true) } it "performs the job successfully" do - expect(DuplicateUploadVerificationService).to receive(:unique?).with(file).and_return(true) + expect(DuplicateUploadVerificationService).to receive(:unique?).with(uploaded_file).and_return(true) described_class.perform_now(user, "GenericWork", attributes, log) expect(log.status).to eq("success") end @@ -28,7 +30,7 @@ context "with a duplicate file" do it "halts the job and raises an error" do expect(actor).not_to receive(:create) - expect(DuplicateUploadVerificationService).to receive(:unique?).with(file).and_return(false) + expect(DuplicateUploadVerificationService).to receive(:unique?).with(uploaded_file).and_return(false) expect { described_class.perform_now(user, "GenericWork", attributes, log) }.to raise_error(Lakeshore::DuplicateAssetError) diff --git a/spec/models/lakeshore/ingest_spec.rb b/spec/models/lakeshore/ingest_spec.rb index 94aae807..d8bcd49f 100644 --- a/spec/models/lakeshore/ingest_spec.rb +++ b/spec/models/lakeshore/ingest_spec.rb @@ -5,6 +5,12 @@ let(:user) { create(:user1) } let(:controller_params) { ActionController::Parameters.new(params) } let(:ingest) { described_class.new(controller_params) } + let(:file) { File.open(File.join(fixture_path, "sun.png")) } + let(:file0) { File.open(File.join(fixture_path, "tardis.png")) } + let(:file1) { File.open(File.join(fixture_path, "tardis2.png")) } + let(:file2) { File.open(File.join(fixture_path, "text.png")) } + + before { LakeshoreTesting.restore } describe "#valid?" do subject { ingest } @@ -82,13 +88,13 @@ end context "with only an intermediate file" do - let(:content) { { intermediate: "intermediate file_set" } } + let(:content) { { intermediate: file } } it { is_expected.to contain_exactly(AICType.IntermediateFileSet) } end context "with original, intermediate, legacy, and preservation files" do let(:content) do - { original: "original file_set", pres_master: "preservation master file_set", intermediate: "intermediate file_set", legacy: "legacy file_set" } + { original: file, pres_master: file0, intermediate: file1, legacy: file2 } end it { is_expected.to contain_exactly(AICType.OriginalFileSet, AICType.IntermediateFileSet, @@ -98,7 +104,7 @@ context "with assorted other files" do let(:content) do - { "intermediate" => "intermediate file_set", "0" => "oddball 0", "1" => "oddball 1", "other" => "other" } + { "intermediate" => file, "odd0" => file0, "odd1" => file1, "odd2" => file2 } end it { is_expected.to contain_exactly(AICType.IntermediateFileSet, nil, nil, nil) } @@ -112,7 +118,7 @@ let(:params) do { asset_type: "StillImage", - content: { intermediate: "file_set" }, + content: { intermediate: file }, metadata: { document_type_uri: "doc_type", depositor: user.email } } end @@ -128,7 +134,7 @@ let(:params) do { asset_type: "StillImage", - content: { intermediate: "file_set" }, + content: { intermediate: file }, metadata: { document_type_uri: "doc_type", depositor: user.email }, sharing: '[{ "type" : "group", "name" : "112", "access" : "read" }, {"type" : "person", "name" : "jdoe99", "access" : "edit"}]' } @@ -149,7 +155,7 @@ let(:params) do { asset_type: "StillImage", - content: { intermediate: "file_set" }, + content: { intermediate: file }, metadata: { document_type_uri: "doc_type", depositor: user.email }, sharing: '[{ "type" : "group", "name" : "112", "access" : "read" }, {"type" : "person", "name" : "' + user.email + '", "access" : "edit"}]' } diff --git a/spec/models/sufia/uploaded_file_spec.rb b/spec/models/sufia/uploaded_file_spec.rb index 0c183635..d0393783 100644 --- a/spec/models/sufia/uploaded_file_spec.rb +++ b/spec/models/sufia/uploaded_file_spec.rb @@ -10,11 +10,39 @@ tempfile: File.new(File.join(fixture_path, "sun.png"))) end let(:user) { create(:user1) } + let(:use) { "http://file.use.uri" } subject { described_class.new(file: file, user: user, use_uri: use) } + before { LakeshoreTesting.restore } + describe "#use_uri" do - let(:use) { "http://file.use.uri" } its(:use_uri) { is_expected.to eq(use) } end + + describe "duplicate checking in same batch" do + context "when no other duplicate exists" do + it "checksum is valid" do + expect(described_class.where(checksum: subject.checksum)).to be_empty + expect(subject).to be_valid + end + end + + context "when a duplicate exists" do + before { described_class.create(file: file, user: user, use_uri: use) } + + it "checksum is not valid" do + expect(subject).not_to be_valid + end + end + end + + context "when the file has already begun ingestion" do + before { described_class.create(file: file, user: user, use_uri: use, status: "begun_ingestion") } + + it "the checksum is invalid" do + expect(subject).not_to be_valid + expect(subject.errors.messages[:checksum]).to eq([error: "sun.png has already begun ingestion."]) + end + end end diff --git a/spec/models/uploaded_batch_spec.rb b/spec/models/uploaded_batch_spec.rb new file mode 100644 index 00000000..d89e8ec5 --- /dev/null +++ b/spec/models/uploaded_batch_spec.rb @@ -0,0 +1,8 @@ +# frozen_string_literal: true +require 'rails_helper' + +describe "UploadedBatch" do + it "model exisits" do + expect { UploadedBatch.create.id }.not_to raise_error + end +end diff --git a/spec/services/duplicate_upload_verification_service_spec.rb b/spec/services/duplicate_upload_verification_service_spec.rb index fad6c7e5..e3fdd6cf 100644 --- a/spec/services/duplicate_upload_verification_service_spec.rb +++ b/spec/services/duplicate_upload_verification_service_spec.rb @@ -10,13 +10,15 @@ subject { service.duplicates } + before { LakeshoreTesting.restore } + context "when no duplicates exist" do before { allow(FileSet).to receive(:where).with(digest_ssim: file_digest).and_return([]) } it { is_expected.to be_empty } end context "when duplicates exist" do - let(:asset) { create(:asset) } + let(:asset) { create(:asset, :with_doctype_metadata) } let(:duplicate_file) { double } before do allow(duplicate_file).to receive(:parent).and_return(asset) diff --git a/spec/support/lakeshore_testing.rb b/spec/support/lakeshore_testing.rb index 67f762d0..2355db76 100644 --- a/spec/support/lakeshore_testing.rb +++ b/spec/support/lakeshore_testing.rb @@ -7,7 +7,7 @@ def restore ActiveFedora::Cleaner.clean! cleanout_redis reset_derivatives - # reset_uploads + # reset_uploads # removed in https://cits.artic.edu/issues/1686, feature build green's just fine create_minimal_resources ListManager.new(File.join(Rails.root, "config/lists/status.yml")).create ActiveFedora::Base.all.map(&:update_index) From b27d7b00202436ed398d778a7aa16eceaf1e3e17 Mon Sep 17 00:00:00 2001 From: Kevin Musiorski <1021154+RudyOnRails@users.noreply.github.com> Date: Wed, 30 May 2018 15:54:02 -0500 Subject: [PATCH 09/16] 2907: (API) new fileset update endpoint (#484) https://cits.artic.edu/issues/2907 --- app/controllers/application_controller.rb | 12 ++++++++++-- app/controllers/lakeshore/file_sets_controller.rb | 14 ++++++++++++++ config/routes.rb | 1 + spec/routing/api_routes_spec.rb | 14 +++++++++++++- 4 files changed, 38 insertions(+), 3 deletions(-) create mode 100644 app/controllers/lakeshore/file_sets_controller.rb diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 49f4664e..9b263886 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -12,9 +12,13 @@ class ApplicationController < ActionController::Base # Prevent CSRF attacks by raising an exception. # For APIs, you may want to use :null_session instead. - protect_from_forgery with: :exception + # protect_from_forgery with: :exception + # + # before_action :check_authorization, :authenticate_user!, :clear_session_user - before_action :check_authorization, :authenticate_user!, :clear_session_user + protect_from_forgery with: :exception, unless: :api_auth_header? + + before_action :check_authorization, :authenticate_user!, :clear_session_user, unless: :api_auth_header? # Ensures any user is logged out if they don't match the current SAML user def clear_session_user @@ -42,4 +46,8 @@ def request_access flash[:notice] = "#{presenter.depositor_full_name} has been emailed your request to see this resource." redirect_to root_url end + + def api_auth_header? + request.headers["HTTP_AUTHORIZATION"].present? + end end diff --git a/app/controllers/lakeshore/file_sets_controller.rb b/app/controllers/lakeshore/file_sets_controller.rb new file mode 100644 index 00000000..a0cd28c9 --- /dev/null +++ b/app/controllers/lakeshore/file_sets_controller.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true +module Lakeshore + class FileSetsController < APIController + def update + file_sets_controller = CurationConcerns::FileSetsController.new + file_sets_controller.request = request + file_sets_controller.response = response + + # does callbacks https://stackoverflow.com/questions/5767222/rails-call-another-controller-action-from-a-controller/30143216#comment74479993_30143216 + file_sets_controller.process(:update) + head 204 + end + end +end diff --git a/config/routes.rb b/config/routes.rb index 038236f4..f48e1389 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -73,6 +73,7 @@ post "ingest/:asset_type", to: "ingest#create", defaults: { format: :json } post "update/:id", to: "ingest#update", defaults: { format: :json } if Figaro.env.LAKESHORE_ENV != "production" get "derivatives/:id/:file", to: "derivatives#show" + patch "file_sets/:id", to: "file_sets#update", defaults: { format: :json } end get "/login_confirm", to: "dummy#login_confirm" diff --git a/spec/routing/api_routes_spec.rb b/spec/routing/api_routes_spec.rb index 010f9b38..e2d4bf6a 100644 --- a/spec/routing/api_routes_spec.rb +++ b/spec/routing/api_routes_spec.rb @@ -2,7 +2,7 @@ require 'rails_helper' describe Rails.application.routes do - context "when using the API" do + describe "API" do it "routes to the reindexing endpoint" do expect(post: "/api/reindex").to route_to(controller: "lakeshore/reindex", action: "create", format: :json) end @@ -33,6 +33,18 @@ id: "1234", format: :json) end + + describe "file_set" do + describe "PATCH" do + it "is routable" do + expect(patch: "/api/file_sets/1234").to route_to(controller: "lakeshore/file_sets", + action: "update", + format: :json, + id: "1234") + end + end + end + context "when the update api is deleting relationships" do it "Kevin disables the update api in production using the LAKESHORE_ENV env var" do allow(Figaro.env).to receive(:LAKESHORE_ENV).and_return("production") From ce99b08606e0cf0578b03cea6fbb0a84ad426826 Mon Sep 17 00:00:00 2001 From: Kevin Musiorski <1021154+RudyOnRails@users.noreply.github.com> Date: Fri, 1 Jun 2018 18:09:10 -0500 Subject: [PATCH 10/16] 2907: new fileset update api - tweaks (#485) * passes response back to the delegate so genuine status code flows through to client * adds individual shell scripts for common request testing * GUI: fileset depositor should be updated on binary version update https://cits.artic.edu/issues/2772 * error handling for when AICUser is not found, but postgres/devise acct is * overrides file_sets behavior so we can add in duplicate checking * adds duplicate checking to fileset updates, but rendering of error messages hash TODO --- .rubocop.yml | 6 + .../file_sets_controller_behavior.rb | 230 ++++++++++++++++++ .../lakeshore/file_sets_controller.rb | 12 +- app/jobs/ingest_file_job.rb | 41 ++++ .../permissions/with_aic_depositor.rb | 10 +- config/routes.rb | 2 +- script/api/assets/create.sh | 14 ++ script/api/file_sets/update_binary_only.sh | 5 + script/api/file_sets/update_title_only.sh | 5 + spec/routing/api_routes_spec.rb | 8 +- 10 files changed, 323 insertions(+), 10 deletions(-) create mode 100644 app/controllers/concerns/curation_concerns/file_sets_controller_behavior.rb create mode 100644 app/jobs/ingest_file_job.rb create mode 100755 script/api/assets/create.sh create mode 100755 script/api/file_sets/update_binary_only.sh create mode 100755 script/api/file_sets/update_title_only.sh diff --git a/.rubocop.yml b/.rubocop.yml index 2ef26786..2a5fbaec 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -24,6 +24,10 @@ Metrics/AbcSize: Metrics/MethodLength: Enabled: false +Metrics/ModuleLength: + Exclude: + - app/controllers/concerns/curation_concerns/file_sets_controller_behavior.rb + Metrics/ClassLength: Exclude: - 'lib/aic.rb' @@ -36,11 +40,13 @@ Metrics/CyclomaticComplexity: Exclude: - app/services/file_set_replacement_service.rb - app/models/concerns/permissions/lakeshore_visibility.rb + - app/controllers/concerns/curation_concerns/file_sets_controller_behavior.rb Metrics/PerceivedComplexity: Exclude: - app/services/file_set_replacement_service.rb - app/models/concerns/permissions/lakeshore_visibility.rb + - app/controllers/concerns/curation_concerns/file_sets_controller_behavior.rb Style/BlockDelimiters: Exclude: diff --git a/app/controllers/concerns/curation_concerns/file_sets_controller_behavior.rb b/app/controllers/concerns/curation_concerns/file_sets_controller_behavior.rb new file mode 100644 index 00000000..24067ab8 --- /dev/null +++ b/app/controllers/concerns/curation_concerns/file_sets_controller_behavior.rb @@ -0,0 +1,230 @@ +# frozen_string_literal: true +module CurationConcerns + module FileSetsControllerBehavior + extend ActiveSupport::Concern + include Blacklight::Base + include Blacklight::AccessControls::Catalog + + included do + include CurationConcerns::ThemedLayoutController + with_themed_layout '1_column' + load_and_authorize_resource class: ::FileSet, except: :show + helper_method :curation_concern + include CurationConcerns::ParentContainer + copy_blacklight_config_from(::CatalogController) + + class_attribute :show_presenter, :form_class + self.show_presenter = CurationConcerns::FileSetPresenter + self.form_class = CurationConcerns::Forms::FileSetEditForm + + # A little bit of explanation, CanCan(Can) sets the @file_set via the .load_and_authorize_resource + # method. However the interface for various CurationConcern modules leverages the #curation_concern method + # Thus we have file_set and curation_concern that are aliases for each other. + attr_accessor :file_set + alias_method :curation_concern, :file_set + private :file_set= + alias_method :curation_concern=, :file_set= + private :curation_concern= + helper_method :file_set + end + + # routed to /files/new + def new + end + + # routed to /files/:id/edit + def edit + initialize_edit_form + end + + # routed to /files (POST) + def create + create_from_upload(params) + end + + def create_from_upload(params) + # check error condition No files + return render_json_response(response_type: :bad_request, options: { message: 'Error! No file to save' }) unless params.key?(:file_set) && params.fetch(:file_set).key?(:files) + + file = params[:file_set][:files].detect { |f| f.respond_to?(:original_filename) } + if !file + render_json_response(response_type: :bad_request, options: { message: 'Error! No file for upload', description: 'unknown file' }) + elsif empty_file?(file) + render_json_response(response_type: :unprocessable_entity, options: { errors: { files: "#{file.original_filename} has no content! (Zero length file)" }, description: t('curation_concerns.api.unprocessable_entity.empty_file') }) + else + process_file(file) + end + rescue RSolr::Error::Http => error + logger.error "FileSetController::create rescued #{error.class}\n\t#{error}\n #{error.backtrace.join("\n")}\n\n" + render_json_response(response_type: :internal_error, options: { message: 'Error occurred while creating a FileSet.' }) + ensure + # remove the tempfile (only if it is a temp file) + file.tempfile.delete if file.respond_to?(:tempfile) + end + + # routed to /files/:id + def show + respond_to do |wants| + wants.html { presenter } + wants.json { presenter } + additional_response_formats(wants) + end + end + + def destroy + parent = curation_concern.parent + actor.destroy + redirect_to [main_app, parent], notice: 'The file has been deleted.' + end + + # routed to /files/:id (PUT) + def update + success = if wants_to_revert? + actor.revert_content(params[:revision]) + elsif params.key?(:file_set) + if params[:file_set].key?(:files) + file = params[:file_set][:files].first + sufia_uploaded_file = Sufia::UploadedFile.new(file: file, + uploaded_batch_id: UploadedBatch.create.id, + user: current_user) + sufia_uploaded_file.valid? ? actor.update_content(file) : false + else + update_metadata + end + end + if success + after_update_response + else + respond_to do |wants| + wants.html do + initialize_edit_form + flash[:error] = "There was a problem processing your request." + render 'edit', status: :unprocessable_entity + end + wants.json { render_json_response(response_type: :unprocessable_entity, options: { errors: curation_concern.errors }) } + end + end + rescue RSolr::Error::Http => error + flash[:error] = error.message + logger.error "FileSetsController::update rescued #{error.class}\n\t#{error.message}\n #{error.backtrace.join("\n")}\n\n" + render action: 'edit' + end + + def after_update_response + respond_to do |wants| + wants.html do + redirect_to [main_app, curation_concern], notice: "The file #{view_context.link_to(curation_concern, [main_app, curation_concern])} has been updated." + end + wants.json do + @presenter = show_presenter.new(curation_concern, current_ability) + render :show, status: :ok, location: polymorphic_path([main_app, curation_concern]) + end + end + end + + def versions + @version_list = version_list + end + + # this is provided so that implementing application can override this behavior and map params to different attributes + def update_metadata + file_attributes = form_class.model_attributes(attributes) + actor.update_metadata(file_attributes) + end + + protected + + def presenter + @presenter ||= begin + _, document_list = search_results(params) + curation_concern = document_list.first + raise CanCan::AccessDenied unless curation_concern + show_presenter.new(curation_concern, current_ability, request) + end + end + + def search_builder_class + CurationConcerns::FileSetSearchBuilder + end + + def initialize_edit_form + @groups = current_user.groups + end + + # Override this method to add additional response + # formats to your local app + def additional_response_formats(_) + # nop + end + + def file_set_params + params.require(:file_set).permit( + :visibility_during_embargo, :embargo_release_date, :visibility_after_embargo, :visibility_during_lease, :lease_expiration_date, :visibility_after_lease, :visibility, title: []) + end + + def version_list + CurationConcerns::VersionListPresenter.new(curation_concern.original_file.versions.all) + end + + def wants_to_revert? + params.key?(:revision) && params[:revision] != curation_concern.latest_content_version.label + end + + def actor + @actor ||= ::CurationConcerns::Actors::FileSetActor.new(curation_concern, current_user) + end + + def attributes + # params.fetch(:file_set, {}).dup # use a copy of the hash so that original params stays untouched when interpret_visibility modifies things + params.fetch(:file_set, {}).except(:files).permit!.dup # use a copy of the hash so that original params stays untouched when interpret_visibility modifies things + end + + def _prefixes + # This allows us to use the unauthorized and form_permission template in curation_concerns/base + @_prefixes ||= super + ['curation_concerns/base'] + end + + def json_error(error, name = nil, additional_arguments = {}) + args = { error: error } + args[:name] = name if name + render additional_arguments.merge(json: [args]) + end + + def empty_file?(file) + (file.respond_to?(:tempfile) && file.tempfile.empty?) || (file.respond_to?(:size) && file.empty?) + end + + def process_file(file) + update_metadata_from_upload_screen + actor.create_metadata(find_parent_by_id, params[:file_set]) + if actor.create_content(file) + respond_to do |format| + format.html do + if request.xhr? + render 'jq_upload', formats: 'json', content_type: 'text/html' + else + redirect_to [main_app, curation_concern.parent] + end + end + format.json do + render 'jq_upload', status: :created, location: polymorphic_path([main_app, curation_concern]) + end + end + else + msg = curation_concern.errors.full_messages.join(', ') + flash[:error] = msg + json_error "Error creating file #{file.original_filename}: #{msg}" + end + end + + # this is provided so that implementing application can override this behavior and map params to different attributes + def update_metadata_from_upload_screen + # Relative path is set by the jquery uploader when uploading a directory + curation_concern.relative_path = params[:relative_path] if params[:relative_path] + end + + def curation_concern_type + ::FileSet + end + end +end diff --git a/app/controllers/lakeshore/file_sets_controller.rb b/app/controllers/lakeshore/file_sets_controller.rb index a0cd28c9..ca383fd2 100644 --- a/app/controllers/lakeshore/file_sets_controller.rb +++ b/app/controllers/lakeshore/file_sets_controller.rb @@ -5,10 +5,14 @@ def update file_sets_controller = CurationConcerns::FileSetsController.new file_sets_controller.request = request file_sets_controller.response = response - - # does callbacks https://stackoverflow.com/questions/5767222/rails-call-another-controller-action-from-a-controller/30143216#comment74479993_30143216 - file_sets_controller.process(:update) - head 204 + begin + # does callbacks https://stackoverflow.com/questions/5767222/rails-call-another-controller-action-from-a-controller/30143216#comment74479993_30143216 + file_sets_controller.process(:update) + rescue Permissions::WithAICDepositor::AICUserNotFound => e + render plain: e.message, status: 500 + else + head file_sets_controller.response.code + end end end end diff --git a/app/jobs/ingest_file_job.rb b/app/jobs/ingest_file_job.rb new file mode 100644 index 00000000..c9119ab4 --- /dev/null +++ b/app/jobs/ingest_file_job.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true +class IngestFileJob < ActiveJob::Base + queue_as CurationConcerns.config.ingest_queue_name + + # @param [FileSet] file_set + # @param [String] filepath the cached file within the CurationConcerns.config.working_path + # @param [User] user + # @option opts [String] mime_type + # @option opts [String] filename + # @option opts [String] relation, ex. :original_file + def perform(file_set, filepath, user, opts = {}) + relation = opts.fetch(:relation, :original_file).to_sym + + # Wrap in an IO decorator to attach passed-in options + local_file = Hydra::Derivatives::IoDecorator.new(File.open(filepath, "rb")) + local_file.mime_type = opts.fetch(:mime_type, nil) + local_file.original_name = opts.fetch(:filename, File.basename(filepath)) + + # Tell AddFileToFileSet service to skip versioning because versions will be minted by + # VersionCommitter when necessary during save_characterize_and_record_committer. + Hydra::Works::AddFileToFileSet.call(file_set, + local_file, + relation, + versioning: false) + + # Should update the file_set's depositor to the current user + file_set.depositor = user.email + + # Persist changes to the file_set + file_set.save! + + repository_file = file_set.send(relation) + + # Do post file ingest actions + CurationConcerns::VersioningService.create(repository_file, user) + + # TODO: this is a problem, the file may not be available at this path on another machine. + # It may be local, or it may be in s3 + CharacterizeJob.perform_later(file_set, repository_file.id, filepath) + end +end diff --git a/app/models/concerns/permissions/with_aic_depositor.rb b/app/models/concerns/permissions/with_aic_depositor.rb index 0f6360e9..a2092c78 100644 --- a/app/models/concerns/permissions/with_aic_depositor.rb +++ b/app/models/concerns/permissions/with_aic_depositor.rb @@ -2,6 +2,8 @@ # Intercepts the existing #depositor methods and replaces them with an # #aic_depositor method that uses the same predicate but with an AICUser resource. module Permissions::WithAICDepositor + class AICUserNotFound < StandardError; end + extend ActiveSupport::Concern included do @@ -9,7 +11,13 @@ module Permissions::WithAICDepositor property :dept_created, predicate: AIC.deptCreated, multiple: false, class_name: "Department" def depositor=(depositor) - self.aic_depositor = AICUser.find_by_nick(depositor).uri + result = AICUser.find_by_nick(depositor) + + if result + self.aic_depositor = result.uri + else + raise AICUserNotFound, "AICUser resource #{depositor} not found, contact LAKE_support@artic.edu\n" + end end # To retain Sufia's expectations, #depositor will return the nick of the AICUser diff --git a/config/routes.rb b/config/routes.rb index f48e1389..36e24c04 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -73,7 +73,7 @@ post "ingest/:asset_type", to: "ingest#create", defaults: { format: :json } post "update/:id", to: "ingest#update", defaults: { format: :json } if Figaro.env.LAKESHORE_ENV != "production" get "derivatives/:id/:file", to: "derivatives#show" - patch "file_sets/:id", to: "file_sets#update", defaults: { format: :json } + put "file_sets/:id", to: "file_sets#update", defaults: { format: :json } end get "/login_confirm", to: "dummy#login_confirm" diff --git a/script/api/assets/create.sh b/script/api/assets/create.sh new file mode 100755 index 00000000..608266e7 --- /dev/null +++ b/script/api/assets/create.sh @@ -0,0 +1,14 @@ +#!/bin/bash + +curl -u citi:password -X POST\ + -F 'metadata[pref_label]=The Tardis'\ + -F 'metadata[visibility]=department'\ + -F 'metadata[uid]=SI-101010'\ + -F 'metadata[document_type_uri]=http://definitions.artic.edu/doctypes/ConservationStillImage'\ + -F 'metadata[depositor]=laketest'\ + -F 'metadata[dept_created]=112'\ + -F 'metadata[imaging_uid][]=uid-1'\ + -F 'metadata[imaging_uid][]=uid-2'\ + -F 'content[intermediate]=@spec/fixtures/tardis.png'\ + -F 'sharing=[{ "type" : "group", "name" : "6", "access" : "read" }, {"type" : "person", "name" : "scossu", "access" : "edit"}]'\ + http://localhost:3000/api/ingest/StillImage diff --git a/script/api/file_sets/update_binary_only.sh b/script/api/file_sets/update_binary_only.sh new file mode 100755 index 00000000..e56e7ef0 --- /dev/null +++ b/script/api/file_sets/update_binary_only.sh @@ -0,0 +1,5 @@ +#!/bin/bash + +curl -i -u citi:password -X PUT\ + -F 'file_set[files][]=@spec/fixtures/sun.png'\ + http://localhost:3000/api/file_sets/4f622807-720c-4f88-a047-eb0eb55ee375 diff --git a/script/api/file_sets/update_title_only.sh b/script/api/file_sets/update_title_only.sh new file mode 100755 index 00000000..25b5fdeb --- /dev/null +++ b/script/api/file_sets/update_title_only.sh @@ -0,0 +1,5 @@ +#!/bin/bash + +curl -i -u citi:password -X PUT\ + -F 'file_set[title][]=new title with spaces'\ + http://localhost:3000/api/file_sets/4f622807-720c-4f88-a047-eb0eb55ee375 diff --git a/spec/routing/api_routes_spec.rb b/spec/routing/api_routes_spec.rb index e2d4bf6a..5146fd88 100644 --- a/spec/routing/api_routes_spec.rb +++ b/spec/routing/api_routes_spec.rb @@ -37,10 +37,10 @@ describe "file_set" do describe "PATCH" do it "is routable" do - expect(patch: "/api/file_sets/1234").to route_to(controller: "lakeshore/file_sets", - action: "update", - format: :json, - id: "1234") + expect(put: "/api/file_sets/1234").to route_to(controller: "lakeshore/file_sets", + action: "update", + format: :json, + id: "1234") end end end From 1b29311c3666c8be8a89783f4d9e268a00da8c07 Mon Sep 17 00:00:00 2001 From: Kevin Musiorski <1021154+RudyOnRails@users.noreply.github.com> Date: Fri, 8 Jun 2018 12:22:50 -0500 Subject: [PATCH 11/16] 1686 ChecksumValidator refactor and removal of DUVS service from Sufia Uploads Controller (#486) removes old validate_duplicate_upload calls from sufia::uploadedfile controller b/c it's already implemented in the model. just forgot :/. refactors ChecksumValidator class. also leaves in CreateAssetJob and its check for duplicates since there may be other ways to get files in besides the batch upload. plus it is too much at once and I want to focus on just the checksum validator removes stubbing of #duplicate_upload since it no longer exists stubs #true and error messages on the @upload to force the error --- app/controllers/sufia/uploads_controller.rb | 19 +----- app/validators/checksum_validator.rb | 62 ++++++++++++------- .../sufia/uploads_controller_spec.rb | 9 +-- 3 files changed, 44 insertions(+), 46 deletions(-) diff --git a/app/controllers/sufia/uploads_controller.rb b/app/controllers/sufia/uploads_controller.rb index df91520c..73a4287b 100644 --- a/app/controllers/sufia/uploads_controller.rb +++ b/app/controllers/sufia/uploads_controller.rb @@ -2,7 +2,7 @@ module Sufia class UploadsController < ApplicationController load_and_authorize_resource class: UploadedFile - before_action :validate_asset_type, :validate_duplicate_upload, only: [:create] + before_action :validate_asset_type, only: [:create] def create @upload.attributes = { file: uploaded_file, user: current_user, use_uri: use_uri, uploaded_batch_id: uploaded_batch_id } @@ -36,11 +36,6 @@ def validate_asset_type render json: { files: [error_message] } end - def validate_duplicate_upload - return if duplicate_upload.empty? - render json: { files: [duplicate_error_message] } - end - def uploaded_file params[:files].first end @@ -68,22 +63,10 @@ def error_message } end - def duplicate_error_message - { - error: t('lakeshore.upload.errors.duplicate', name: uploaded_file.original_filename), - duplicate_path: polymorphic_path(duplicate_upload.first), - duplicate_name: duplicate_upload.first.to_s - } - end - # We use this controller with both the single upload and batch upload form # so it needs to work with both kinds of parameter hashes. def asset_attributes params.fetch(:generic_work, nil) || params.fetch(:batch_upload_item, nil) end - - def duplicate_upload - @duplicate_upload ||= DuplicateUploadVerificationService.new(uploaded_file).duplicates - end end end diff --git a/app/validators/checksum_validator.rb b/app/validators/checksum_validator.rb index b6c527d9..058f2b71 100644 --- a/app/validators/checksum_validator.rb +++ b/app/validators/checksum_validator.rb @@ -6,38 +6,56 @@ class ChecksumValidator < ActiveModel::Validator def validate(record) @record = record - if !verification_service.duplicates.empty? - record.errors.add(:checksum, in_solr_error_message) - elsif Sufia::UploadedFile.begun_ingestion.map(&:checksum).include?(record.checksum) - record.errors.add(:checksum, begun_ingestion_error_message) - elsif Sufia::UploadedFile.where(uploaded_batch_id: record.uploaded_batch_id).map(&:checksum).include?(record.checksum) - record.errors.add(:checksum, already_in_batch_error_message) + if duplicates_in_solr? + add_errors(:duplicate) + elsif duplicates_in_process? + add_errors(:begun_ingestion) + elsif duplicates_in_batch? + add_errors(:already_in_batch) end end private - def in_solr_error_message - { - error: I18n.t('lakeshore.upload.errors.duplicate', name: @record.file.filename), - duplicate_path: polymorphic_path(verification_service.duplicates.first), - duplicate_name: verification_service.duplicates.first.to_s - } + def add_errors(type_of_duplicate) + record.errors.add(:checksum, build_message_hash(type_of_duplicate)) end - def begun_ingestion_error_message - { - error: I18n.t('lakeshore.upload.errors.begun_ingestion', name: @record.file.filename) - } + def duplicates_in_solr? + duplicate_file_sets.present? end - def already_in_batch_error_message - { - error: I18n.t('lakeshore.upload.errors.already_in_batch', name: @record.file.filename) - } + def duplicates_in_process? + Sufia::UploadedFile.begun_ingestion.map(&:checksum).include?(record.checksum) end - def verification_service - @verification_service ||= DuplicateUploadVerificationService.new(record.file) + def duplicates_in_batch? + Sufia::UploadedFile.where(uploaded_batch_id: record.uploaded_batch_id).map(&:checksum).include?(record.checksum) + end + + def build_message_hash(type_of_duplicate) + message_hash = {} + + message_hash[:error] = I18n.t("lakeshore.upload.errors.#{type_of_duplicate}", name: record.file.filename) + + if type_of_duplicate == :duplicate + message_hash[:duplicate_path] = polymorphic_path(duplicate_file_sets.first.parent) + message_hash[:duplicate_name] = duplicate_file_sets.first.parent.to_s + end + + message_hash + end + + # @return [Array] + def solr_duplicates? + duplicate_file_sets.map(&:parent) + end + + # @return [Array] + def duplicate_file_sets + @duplicate_file_sets ||= begin + return [] if record.file.nil? + FileSet.where(digest_ssim: record.checksum) + end end end diff --git a/spec/controllers/sufia/uploads_controller_spec.rb b/spec/controllers/sufia/uploads_controller_spec.rb index 4a47bb25..1af88ec0 100644 --- a/spec/controllers/sufia/uploads_controller_spec.rb +++ b/spec/controllers/sufia/uploads_controller_spec.rb @@ -8,7 +8,6 @@ let(:duplicates) { [] } before do LakeshoreTesting.restore - allow(controller).to receive(:duplicate_upload).and_return(duplicates) end describe "uploading a single asset" do @@ -24,12 +23,10 @@ end context "when the file is a duplicate" do - let(:parent) { create(:asset) } - let(:duplicate_file) { create(:file_set) } - let(:duplicates) { [duplicate_file] } - before do - parent.members << duplicate_file + allow_any_instance_of(Sufia::UploadedFile).to receive(:save).and_return(false) + allow_any_instance_of(Sufia::UploadedFile).to receive_message_chain(:errors, :messages).and_return(checksum: ["sun.png is a duplicate"]) + post :create, files: [file], generic_work: work_attributes, format: 'json' end it "reports the error" do From 45a81469e2ee307357e822e2f6303786cea06d02 Mon Sep 17 00:00:00 2001 From: Adam Wead Date: Tue, 12 Jun 2018 23:45:54 -0400 Subject: [PATCH 12/16] 2773: Check every file for duplicates in the ingest API #478 https://cits.artic.edu/issues/2773 --- .../lakeshore/ingest_controller.rb | 26 +------ app/models/lakeshore/ingest.rb | 62 +++------------ app/models/lakeshore/ingest_file.rb | 67 ++++++++++++++++ app/models/lakeshore/ingest_registry.rb | 76 ++++++++++++++++++ .../asset_type_verification_service.rb | 2 +- .../duplicate_upload_verification_service.rb | 2 +- .../create_duplicates_spec.rb | 41 ++++++++++ .../ingest_controller/create_spec.rb | 43 +++-------- .../ingest_controller/update_spec.rb | 6 +- spec/fixtures/api_409.json | 16 ++-- spec/models/lakeshore/ingest_file_spec.rb | 77 +++++++++++++++++++ spec/models/lakeshore/ingest_registry_spec.rb | 59 ++++++++++++++ spec/models/lakeshore/ingest_spec.rb | 45 ++--------- 13 files changed, 361 insertions(+), 161 deletions(-) create mode 100644 app/models/lakeshore/ingest_file.rb create mode 100644 app/models/lakeshore/ingest_registry.rb create mode 100644 spec/controllers/lakeshore/ingest_controller/create_duplicates_spec.rb create mode 100644 spec/models/lakeshore/ingest_file_spec.rb create mode 100644 spec/models/lakeshore/ingest_registry_spec.rb diff --git a/app/controllers/lakeshore/ingest_controller.rb b/app/controllers/lakeshore/ingest_controller.rb index 4525168e..529f04b3 100644 --- a/app/controllers/lakeshore/ingest_controller.rb +++ b/app/controllers/lakeshore/ingest_controller.rb @@ -6,7 +6,7 @@ class IngestController < APIController # load_and_authorize_resource :curation_concern, class: 'GenericWork' delegate :intermediate_file, :asset_type, :ingestor, :attributes_for_actor, - :check_duplicates_turned_off?, :represented_resources, :force_preferred_representation?, to: :ingest + :represented_resources, :force_preferred_representation?, to: :ingest before_action :validate_ingest, :validate_asset_type, only: [:create] before_action :validate_duplicate_upload, :validate_preferred_representations, only: [:create, :update] @@ -41,10 +41,8 @@ def validate_asset_type end def validate_duplicate_upload - return if check_duplicates_turned_off? - return if duplicate_upload.empty? - ingest.errors.add(:intermediate_file, "is a duplicate of #{duplicate_upload.first}") - render json: duplicate_error, status: :conflict + return if ingest.unique? + render json: ingest.duplicate_error, status: :conflict end def validate_preferred_representations @@ -70,23 +68,5 @@ def curation_concern GenericWork.new end end - - def duplicate_upload - @duplicate_upload ||= DuplicateUploadVerificationService.new(intermediate_file).duplicate_file_sets - end - - def duplicate_error - { - message: "Duplicate detected.", - uploaded_resource: { - id: ingest.params["metadata"].fetch("uid", nil), - file_name: intermediate_file.original_filename - }, - stored_resource: { - fileset_id: duplicate_upload.first.id, - pref_label: duplicate_upload.first.parent.pref_label - } - } - end end end diff --git a/app/models/lakeshore/ingest.rb b/app/models/lakeshore/ingest.rb index ba94e349..7ce51faa 100644 --- a/app/models/lakeshore/ingest.rb +++ b/app/models/lakeshore/ingest.rb @@ -3,18 +3,19 @@ module Lakeshore class Ingest include ActiveModel::Validations - attr_reader :ingestor, :submitted_asset_type, :document_type_uri, :original_file, - :intermediate_file, :presevation_master_file, :legacy_file, :additional_files, :params, - :preferred_representation_for, :force_preferred_representation + attr_reader :ingestor, :submitted_asset_type, :document_type_uri, :params, + :preferred_representation_for, :force_preferred_representation, :registry validates :ingestor, :asset_type, :document_type_uri, :intermediate_file, presence: true + delegate :intermediate_file, :duplicate_error, to: :registry + # @param [ActionController::Parameters] params from the controller def initialize(params) @params = params @submitted_asset_type = params.fetch(:asset_type, nil) - register_files(params.fetch(:content, {})) register_terms(params.fetch(:metadata, {})) + @registry = IngestRegistry.new(params.fetch(:content, {}), ingestor) end def asset_type @@ -28,7 +29,7 @@ def asset_type # want the intermediate file to be the representative. def files return [] unless valid? || valid_update? - [intermediate_upload, original_upload, presevation_master_upload, legacy_upload].compact + additional_uploads + registry.upload_ids end def attributes_for_actor @@ -49,6 +50,12 @@ def check_duplicates_turned_off? params.fetch(:duplicate_check, nil) == "false" end + # @return [true, false] + # If the ingest is not unique, then errors should be reported + def unique? + (check_duplicates_turned_off? || intermediate_file.nil? || registry.unique?) + end + # @return [Array] # Returns an array of ids from preferred_representation_for that already have preferred representations defined. def represented_resources @@ -66,7 +73,6 @@ def force_preferred_representation? private def register_terms(metadata) - return unless metadata.present? @document_type_uri = metadata.fetch(:document_type_uri, nil) @preferred_representation_for = metadata.fetch(:preferred_representation_for, []) @ingestor = find_or_create_user(metadata.fetch(:depositor, nil)) @@ -78,15 +84,6 @@ def find_or_create_user(key) User.find_by_user_key(key) || User.create!(email: key) end - def register_files(content) - return unless content.present? - @original_file = content.delete(:original) - @intermediate_file = content.delete(:intermediate) - @presevation_master_file = content.delete(:pres_master) - @legacy_file = content.delete(:legacy) - @additional_files = content - end - # @return [Array] # Removes any additional permissions having to do with the depositor. # The depositor's permissions are fixed and are not allowed to be altered. @@ -95,40 +92,5 @@ def permitted_permissions permission.fetch("type", nil) == "person" && permission.fetch("name", nil) == ingestor.email end end - - def original_upload - return unless original_file - Sufia::UploadedFile.create(file: original_file, - user: ingestor, - use_uri: AICType.OriginalFileSet).id.to_s - end - - def intermediate_upload - return unless intermediate_file - Sufia::UploadedFile.create(file: intermediate_file, - user: ingestor, - use_uri: AICType.IntermediateFileSet).id.to_s - end - - def presevation_master_upload - return unless presevation_master_file - Sufia::UploadedFile.create(file: presevation_master_file, - user: ingestor, - use_uri: AICType.PreservationMasterFileSet).id.to_s - end - - def legacy_upload - return unless legacy_file - Sufia::UploadedFile.create(file: legacy_file, - user: ingestor, - use_uri: AICType.LegacyFileSet).id.to_s - end - - def additional_uploads - return [] unless additional_files - additional_files.values.map do |file| - Sufia::UploadedFile.create(file: file, user: ingestor) - end - end end end diff --git a/app/models/lakeshore/ingest_file.rb b/app/models/lakeshore/ingest_file.rb new file mode 100644 index 00000000..ace7c2bc --- /dev/null +++ b/app/models/lakeshore/ingest_file.rb @@ -0,0 +1,67 @@ +# frozen_string_literal: true +module Lakeshore + class IngestFile + attr_reader :file, :type, :user + + delegate :original_filename, to: :file + delegate :errors, to: :uploaded_file + + def self.uri_map + { + original: AICType.OriginalFileSet, + intermediate: AICType.IntermediateFileSet, + pres_master: AICType.PreservationMasterFileSet, + legacy: AICType.LegacyFileSet + } + end + + def self.types + uri_map.keys + end + + # @param [User] user depositor performing the ingest from the API + # @param [ActionDispatch::Http::UploadedFile] file uploaded from the API + # @param [Symbol] type for the FileSet that has a registered URI + def initialize(user:, file:, type:) + @type = type + @file = file + @user = user + end + + def uri + @uri ||= register_uri + end + + def duplicate? + errors.key?(:checksum) + end + + def unique? + !duplicate? + end + + def uploaded_file + @uploaded_file ||= create_file(file, user) + end + + # @return [String] id of uploaded file for API + def uploaded_file_id + return if uploaded_file.nil? + uploaded_file.id.to_s + end + + private + + def register_uri + return if type.nil? + self.class.uri_map[type] || raise(UnsupportedFileSetTypeError, "'#{type}' is not a supported file set type") + end + + def create_file(file, user) + return if file.nil? + Sufia::UploadedFile.create(file: file, user: user, use_uri: uri) + end + + class UnsupportedFileSetTypeError < StandardError; end + end +end diff --git a/app/models/lakeshore/ingest_registry.rb b/app/models/lakeshore/ingest_registry.rb new file mode 100644 index 00000000..9473cce8 --- /dev/null +++ b/app/models/lakeshore/ingest_registry.rb @@ -0,0 +1,76 @@ +# frozen_string_literal: true +module Lakeshore + class IngestRegistry + attr_reader :intermediate_file, :user, :files + + # @param [Hash] content from api request + # @param [User] user + def initialize(content, user) + @intermediate_file = register_intermediate_file(content.delete(:intermediate), user) + @user = user + @files = [@intermediate_file].compact + register_files(content) + end + + # @param [true, false] if there are no duplicates in the set of files + def unique? + duplicates.empty? + end + + # @return [Array] + # Array of numeric ids for each Sufia::UploadedFile in the array of {files} + def upload_ids + return [] if duplicates.present? + files.map(&:uploaded_file_id).compact + end + + # @return [Hash] + # @note creates a JSON report for duplicate typed and non-typed files + def duplicate_error + error = { message: "Duplicate files detected." } + IngestFile.types.each do |type| + error_files = duplicates.select { |file| (file.type == type) } + next if error_files.empty? + error[type] = error_files.map { |file| error_details(file) } + end + + untyped_duplicates = duplicates.select { |file| file.type.nil? } + if untyped_duplicates.present? + error[:other] = untyped_duplicates.map { |file| error_details(file) } + end + + error + end + + private + + def register_intermediate_file(file, user) + file = IngestFile.new(user: user, file: file, type: :intermediate) + return if file.file.nil? + file + end + + # @note creates ingest files for both typed and non-typed files + def register_files(content) + (content.keys.map(&:to_sym) & IngestFile.types).each do |type| + files << IngestFile.new(user: user, file: content.delete(type), type: type) + end + + content.values.map do |file| + files << IngestFile.new(user: user, file: file, type: nil) + end + end + + def duplicates + @duplicates ||= files.select(&:duplicate?) + end + + def error_details(file) + { + error: "#{file.errors.messages[:checksum].first[:error]} "\ + "#{file.errors.messages[:checksum].first[:duplicate_name]}", + path: file.errors.messages[:checksum].first[:duplicate_path] + } + end + end +end diff --git a/app/services/asset_type_verification_service.rb b/app/services/asset_type_verification_service.rb index 2f7d5094..9f4fd65b 100644 --- a/app/services/asset_type_verification_service.rb +++ b/app/services/asset_type_verification_service.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class AssetTypeVerificationService class << self - # @param [Rack::Test::UploadedFile] file uploaded via Rack + # @param [ActionDispatch::Http::UploadedFile, Lakeshore::IngestFile] file uploaded via Rack # @param [AICType, String] asset_type # @return [Boolean] def call(file, asset_type) diff --git a/app/services/duplicate_upload_verification_service.rb b/app/services/duplicate_upload_verification_service.rb index 9fb3f666..9beb4629 100644 --- a/app/services/duplicate_upload_verification_service.rb +++ b/app/services/duplicate_upload_verification_service.rb @@ -6,7 +6,7 @@ def self.unique?(file) new(file).duplicate_file_sets.empty? end - # @param [Sufia::UploadedFile] file uploaded via Rack + # @param [ActionDispatch::Http::UploadedFile, Sufia::UploadedFile] file uploaded via Rack def initialize(file) @file = file end diff --git a/spec/controllers/lakeshore/ingest_controller/create_duplicates_spec.rb b/spec/controllers/lakeshore/ingest_controller/create_duplicates_spec.rb new file mode 100644 index 00000000..19517809 --- /dev/null +++ b/spec/controllers/lakeshore/ingest_controller/create_duplicates_spec.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: false +require 'rails_helper' + +describe Lakeshore::IngestController, custom_description: "Lakeshore::IngestController#create" do + let(:apiuser) { create(:apiuser) } + let(:user) { create(:user1) } + let(:file_set) { build(:file_set, id: "existing-file-set-id") } + let(:parent) { build(:work, pref_label: "work pref_label") } + + let(:image_asset) do + ActionDispatch::Http::UploadedFile.new(filename: "sun.png", + content_type: "image/png", + tempfile: File.new(File.join(fixture_path, "sun.png"))) + end + + context "when uploading a duplicate file" do + let(:json_response) { JSON.parse(File.open(File.join(fixture_path, "api_409.json")).read).to_json } + + let(:metadata) do + { + "visibility" => "department", + "depositor" => user.email, + "document_type_uri" => AICDocType.ConservationStillImage, + "uid" => "upload-id" + } + end + + before do + sign_in_basic(apiuser) + allow(AssetTypeVerificationService).to receive(:call).and_return(true) + allow(file_set).to receive(:parent).and_return(parent) + allow_any_instance_of(ChecksumValidator).to receive(:duplicate_file_sets).and_return([file_set]) + post :create, asset_type: "StillImage", content: { intermediate: image_asset }, metadata: metadata + end + + it "returns an error with the correct status" do + expect(response.status).to eq(409) + expect(response.body).to eq(json_response) + end + end +end diff --git a/spec/controllers/lakeshore/ingest_controller/create_spec.rb b/spec/controllers/lakeshore/ingest_controller/create_spec.rb index 0f62e96a..e42914a0 100644 --- a/spec/controllers/lakeshore/ingest_controller/create_spec.rb +++ b/spec/controllers/lakeshore/ingest_controller/create_spec.rb @@ -13,10 +13,7 @@ tempfile: File.new(File.join(fixture_path, "sun.png"))) end - before do - LakeshoreTesting.reset_uploads - sign_in_basic(apiuser) - end + before { sign_in_basic(apiuser) } let(:metadata) do { "visibility" => "department", "depositor" => user.email, "document_type_uri" => AICDocType.ConservationStillImage } @@ -69,8 +66,10 @@ context "when uploading a text asset to StillImage" do before do - allow(AssetTypeVerificationService).to receive(:call).with("asset", AICType.StillImage).and_return(false) - post :create, asset_type: "StillImage", content: { intermediate: "asset" }, metadata: metadata + allow(AssetTypeVerificationService).to receive(:call) + .with(kind_of(Lakeshore::IngestFile), AICType.StillImage) + .and_return(false) + post :create, asset_type: "StillImage", content: { intermediate: image_asset }, metadata: metadata end it { is_expected.to be_bad_request } its(:body) { is_expected.to eq("[\"Intermediate file is not the correct asset type\"]") } @@ -78,40 +77,16 @@ context "when uploading an image asset to Text" do before do - allow(AssetTypeVerificationService).to receive(:call).with("asset", AICType.Text).and_return(false) - post :create, asset_type: "Text", content: { intermediate: "asset" }, metadata: metadata + allow(AssetTypeVerificationService).to receive(:call) + .with(kind_of(Lakeshore::IngestFile), AICType.Text) + .and_return(false) + post :create, asset_type: "Text", content: { intermediate: image_asset }, metadata: metadata end it { is_expected.to be_bad_request } its(:body) { is_expected.to eq("[\"Intermediate file is not the correct asset type\"]") } end end - context "when uploading a duplicate file" do - let(:file_set) { build(:file_set, id: "existing-file-set-id") } - let(:parent) { build(:work, pref_label: "work pref_label") } - let(:json_response) { JSON.parse(File.open(File.join(fixture_path, "api_409.json")).read).to_json } - - let(:metadata) do - { - "visibility" => "department", - "depositor" => user.email, - "document_type_uri" => AICDocType.ConservationStillImage, - "uid" => "upload-id" - } - end - - subject { response } - - before do - allow(AssetTypeVerificationService).to receive(:call).and_return(true) - allow(controller).to receive(:duplicate_upload).and_return([file_set]) - allow(file_set).to receive(:parent).and_return(parent) - post :create, asset_type: "StillImage", content: { intermediate: image_asset }, metadata: metadata - end - its(:status) { is_expected.to eq(409) } - its(:body) { is_expected.to eq(json_response) } - end - context "when the ingestor does not exist in Fedora" do let(:metadata) do { "visibility" => "department", "depositor" => "bogus", "document_type_uri" => AICDocType.ConservationStillImage } diff --git a/spec/controllers/lakeshore/ingest_controller/update_spec.rb b/spec/controllers/lakeshore/ingest_controller/update_spec.rb index c4a71141..80c599b2 100644 --- a/spec/controllers/lakeshore/ingest_controller/update_spec.rb +++ b/spec/controllers/lakeshore/ingest_controller/update_spec.rb @@ -32,7 +32,6 @@ before do LakeshoreTesting.restore allow(Lakeshore::CreateAllDerivatives).to receive(:perform_later) - allow(controller).to receive(:duplicate_upload).and_return([]) end context "with an intermediate file" do @@ -153,10 +152,11 @@ let(:parent) { build(:work, pref_label: "work pref_label") } before do + allow_any_instance_of(ChecksumValidator).to receive(:duplicate_file_sets).and_return([file_set]) allow(Lakeshore::CreateAllDerivatives).to receive(:perform_later) - allow(controller).to receive(:duplicate_upload).and_return([file_set]) allow(file_set).to receive(:parent).and_return(parent) - post :update, id: asset.id, content: { intermediate: replacement_asset }, metadata: metadata, duplicate_check: duplicate_check_param + post :update, id: asset.id, content: { intermediate: replacement_asset }, + metadata: metadata, duplicate_check: duplicate_check_param asset.reload end diff --git a/spec/fixtures/api_409.json b/spec/fixtures/api_409.json index 1d8343a9..80239128 100644 --- a/spec/fixtures/api_409.json +++ b/spec/fixtures/api_409.json @@ -1,11 +1,9 @@ { - "message" : "Duplicate detected.", - "uploaded_resource" : { - "id": "upload-id", - "file_name" : "sun.png" - }, - "stored_resource" : { - "fileset_id" : "existing-file-set-id", - "pref_label" : "work pref_label" - } + "message" : "Duplicate files detected.", + "intermediate" : [ + { + "error" : "sun.png is a duplicate of: work pref_label", + "path" : "/concern/works" + } + ] } diff --git a/spec/models/lakeshore/ingest_file_spec.rb b/spec/models/lakeshore/ingest_file_spec.rb new file mode 100644 index 00000000..f0daac51 --- /dev/null +++ b/spec/models/lakeshore/ingest_file_spec.rb @@ -0,0 +1,77 @@ +# frozen_string_literal: true +require 'rails_helper' + +RSpec.describe Lakeshore::IngestFile do + let(:user) { create(:user) } + let(:type) { :original } + + let(:file) do + ActionDispatch::Http::UploadedFile.new(filename: "sun.png", + content_type: "image/png", + tempfile: File.new(File.join(fixture_path, "sun.png"))) + end + + subject { described_class.new(file: file, user: user, type: type) } + + it { is_expected.to delegate_method(:original_filename).to(:file) } + it { is_expected.to delegate_method(:errors).to(:uploaded_file) } + + describe "::types" do + subject { described_class } + its(:types) { is_expected.to contain_exactly(:original, :intermediate, :pres_master, :legacy) } + end + + describe "#uploaded_file" do + its(:uploaded_file) { is_expected.to be_a(Sufia::UploadedFile) } + end + + describe "#uri" do + context "with an original file" do + its(:uri) { is_expected.to eq(AICType.OriginalFileSet) } + end + + context "with an intermediate file" do + let(:type) { :intermediate } + its(:uri) { is_expected.to eq(AICType.IntermediateFileSet) } + end + + context "with an original file" do + let(:type) { :pres_master } + its(:uri) { is_expected.to eq(AICType.PreservationMasterFileSet) } + end + + context "with an original file" do + let(:type) { :legacy } + its(:uri) { is_expected.to eq(AICType.LegacyFileSet) } + end + + context "without a file type" do + let(:type) { nil } + its(:uri) { is_expected.to be_nil } + end + + context "with an unregistered file" do + let(:type) { :unregistered } + it "raises an error" do + expect { + described_class.new(file: file, user: user, type: :unregistered).uri + }.to raise_error(Lakeshore::IngestFile::UnsupportedFileSetTypeError, + "'unregistered' is not a supported file set type") + end + end + end + + describe "#duplicate? and #unique?" do + context "with a unique file" do + it { is_expected.not_to be_duplicate } + it { is_expected.to be_unique } + end + + context "with a duplicate file" do + before { allow(subject).to receive(:errors).and_return(checksum: "error") } + + it { is_expected.to be_duplicate } + it { is_expected.not_to be_unique } + end + end +end diff --git a/spec/models/lakeshore/ingest_registry_spec.rb b/spec/models/lakeshore/ingest_registry_spec.rb new file mode 100644 index 00000000..6926383f --- /dev/null +++ b/spec/models/lakeshore/ingest_registry_spec.rb @@ -0,0 +1,59 @@ +# frozen_string_literal: true +require 'rails_helper' + +describe Lakeshore::IngestRegistry do + let(:user) { create(:user) } + + subject(:registry) { described_class.new(content, user) } + + describe "unique?" do + context "without any content in the api request" do + let(:content) { {} } + it { is_expected.to be_unique } + end + + context "with non-duplicate files" do + let(:content) { { intermediate: "intermediate file", pres_master: "pres master file" } } + before { allow(registry).to receive(:duplicates).and_return([]) } + it { is_expected.to be_unique } + end + + context "with one or more duplicate files in the registry" do + let(:content) { { intermediate: "intermediate file", pres_master: "duplicate" } } + before { allow(registry).to receive(:duplicates).and_return(["duplicate"]) } + it { is_expected.not_to be_unique } + end + end + + describe "#intermediate_file" do + context "without any content from the api request" do + let(:content) { {} } + its(:intermediate_file) { is_expected.to be_nil } + end + end + + describe "#files" do + context "without any content in the api request" do + let(:content) { {} } + subject { registry } + + its(:files) { is_expected.to be_empty } + end + + context "with both named and non-named file types in the api request" do + let(:content) { { intermediate: "intermediate file", pres_master: "pres master file", other: "other file" } } + + it "is an array of ingest files" do + expect(registry.files.count).to eq(3) + end + end + + context "with only named file types in the api request" do + let(:content) { { intermediate: "intermediate file", pres_master: "pres master file" } } + + it "is an array of ingest files" do + expect(registry.files.count).to eq(2) + end + end + end +end diff --git a/spec/models/lakeshore/ingest_spec.rb b/spec/models/lakeshore/ingest_spec.rb index d8bcd49f..f9181a2e 100644 --- a/spec/models/lakeshore/ingest_spec.rb +++ b/spec/models/lakeshore/ingest_spec.rb @@ -19,7 +19,7 @@ let(:params) do { asset_type: "StillImage", - content: { intermediate: "file_set" }, + content: { intermediate: file }, metadata: { document_type_uri: "doc_type", depositor: user.email } } end @@ -72,48 +72,13 @@ end end - describe "#files" do - let(:params) do - { - asset_type: "StillImage", - content: content, - metadata: { document_type_uri: "doc_type", depositor: user.email } - } - end - - subject do - described_class.new(controller_params).files.map do |id| - Sufia::UploadedFile.find(id).use_uri - end - end - - context "with only an intermediate file" do - let(:content) { { intermediate: file } } - it { is_expected.to contain_exactly(AICType.IntermediateFileSet) } - end - - context "with original, intermediate, legacy, and preservation files" do - let(:content) do - { original: file, pres_master: file0, intermediate: file1, legacy: file2 } - end - it { is_expected.to contain_exactly(AICType.OriginalFileSet, - AICType.IntermediateFileSet, - AICType.PreservationMasterFileSet, - AICType.LegacyFileSet) } - end - - context "with assorted other files" do - let(:content) do - { "intermediate" => file, "odd0" => file0, "odd1" => file1, "odd2" => file2 } - end - - it { is_expected.to contain_exactly(AICType.IntermediateFileSet, nil, nil, nil) } - end - end - describe "#attributes_for_actor" do subject { ingest.attributes_for_actor } + let(:mock_service) { double("MockService", duplicates: [], duplicate_file_sets: []) } + + before { allow(DuplicateUploadVerificationService).to receive(:new).and_return(mock_service) } + context "without additional sharing permissions" do let(:params) do { From 03b9fa2fa4361a0263846852ae88a79c12f99e90 Mon Sep 17 00:00:00 2001 From: Kevin Musiorski Date: Fri, 15 Jun 2018 11:33:40 -0500 Subject: [PATCH 13/16] update email address --- app/models/concerns/permissions/with_aic_depositor.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/concerns/permissions/with_aic_depositor.rb b/app/models/concerns/permissions/with_aic_depositor.rb index a2092c78..dbf2c9ed 100644 --- a/app/models/concerns/permissions/with_aic_depositor.rb +++ b/app/models/concerns/permissions/with_aic_depositor.rb @@ -16,7 +16,7 @@ def depositor=(depositor) if result self.aic_depositor = result.uri else - raise AICUserNotFound, "AICUser resource #{depositor} not found, contact LAKE_support@artic.edu\n" + raise AICUserNotFound, "AICUser '#{depositor}' not found, contact collections_support@artic.edu\n" end end From f73034314523738bc1b4d77d0c5474b75efcd0e3 Mon Sep 17 00:00:00 2001 From: Adam Wead Date: Wed, 20 Jun 2018 14:45:47 -0400 Subject: [PATCH 14/16] Don't memoize calls to duplicate_file_sets in ChecksumValidator (#489) After pairing up on some duplicate problems, we discovered that Solr was getting called multiple times during the validation process. We should not memoize the result of the Solr query because the first time it queries it, it may not have the value we're looking for. --- app/validators/checksum_validator.rb | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/app/validators/checksum_validator.rb b/app/validators/checksum_validator.rb index 058f2b71..45d15951 100644 --- a/app/validators/checksum_validator.rb +++ b/app/validators/checksum_validator.rb @@ -53,9 +53,7 @@ def solr_duplicates? # @return [Array] def duplicate_file_sets - @duplicate_file_sets ||= begin - return [] if record.file.nil? - FileSet.where(digest_ssim: record.checksum) - end + return [] if record.file.nil? + FileSet.where(digest_ssim: record.checksum) end end From 34ef683257fef41b00e70f2d6ae232d4cbefe9ff Mon Sep 17 00:00:00 2001 From: Adam Wead Date: Wed, 20 Jun 2018 14:48:50 -0400 Subject: [PATCH 15/16] Adding batch upload to IngestFile (#488) REDMINE URL: https://cits.artic.edu/issues/2773 Adds batch upload id to the IngestFile so we can catch duplicate uploads in the same batch. --- app/models/lakeshore/ingest_file.rb | 8 +++++--- app/models/lakeshore/ingest_registry.rb | 10 +++++++--- spec/models/lakeshore/ingest_file_spec.rb | 4 ++-- 3 files changed, 14 insertions(+), 8 deletions(-) diff --git a/app/models/lakeshore/ingest_file.rb b/app/models/lakeshore/ingest_file.rb index ace7c2bc..45b6686e 100644 --- a/app/models/lakeshore/ingest_file.rb +++ b/app/models/lakeshore/ingest_file.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module Lakeshore class IngestFile - attr_reader :file, :type, :user + attr_reader :file, :type, :user, :batch_id delegate :original_filename, to: :file delegate :errors, to: :uploaded_file @@ -22,10 +22,12 @@ def self.types # @param [User] user depositor performing the ingest from the API # @param [ActionDispatch::Http::UploadedFile] file uploaded from the API # @param [Symbol] type for the FileSet that has a registered URI - def initialize(user:, file:, type:) + # @param [UploadedBatch] batch id for the Sufia::UploadedFile + def initialize(user:, file:, type:, batch_id:) @type = type @file = file @user = user + @batch_id = batch_id end def uri @@ -59,7 +61,7 @@ def register_uri def create_file(file, user) return if file.nil? - Sufia::UploadedFile.create(file: file, user: user, use_uri: uri) + Sufia::UploadedFile.create(file: file, user: user, use_uri: uri, uploaded_batch_id: batch_id) end class UnsupportedFileSetTypeError < StandardError; end diff --git a/app/models/lakeshore/ingest_registry.rb b/app/models/lakeshore/ingest_registry.rb index 9473cce8..f2e2ae6c 100644 --- a/app/models/lakeshore/ingest_registry.rb +++ b/app/models/lakeshore/ingest_registry.rb @@ -45,7 +45,7 @@ def duplicate_error private def register_intermediate_file(file, user) - file = IngestFile.new(user: user, file: file, type: :intermediate) + file = IngestFile.new(user: user, file: file, type: :intermediate, batch_id: uploaded_batch.id) return if file.file.nil? file end @@ -53,11 +53,11 @@ def register_intermediate_file(file, user) # @note creates ingest files for both typed and non-typed files def register_files(content) (content.keys.map(&:to_sym) & IngestFile.types).each do |type| - files << IngestFile.new(user: user, file: content.delete(type), type: type) + files << IngestFile.new(user: user, file: content.delete(type), type: type, batch_id: uploaded_batch.id) end content.values.map do |file| - files << IngestFile.new(user: user, file: file, type: nil) + files << IngestFile.new(user: user, file: file, type: nil, batch_id: uploaded_batch.id) end end @@ -72,5 +72,9 @@ def error_details(file) path: file.errors.messages[:checksum].first[:duplicate_path] } end + + def uploaded_batch + @uploaed_batch ||= UploadedBatch.create + end end end diff --git a/spec/models/lakeshore/ingest_file_spec.rb b/spec/models/lakeshore/ingest_file_spec.rb index f0daac51..5443cc7b 100644 --- a/spec/models/lakeshore/ingest_file_spec.rb +++ b/spec/models/lakeshore/ingest_file_spec.rb @@ -11,7 +11,7 @@ tempfile: File.new(File.join(fixture_path, "sun.png"))) end - subject { described_class.new(file: file, user: user, type: type) } + subject { described_class.new(file: file, user: user, type: type, batch_id: UploadedBatch.create.id) } it { is_expected.to delegate_method(:original_filename).to(:file) } it { is_expected.to delegate_method(:errors).to(:uploaded_file) } @@ -54,7 +54,7 @@ let(:type) { :unregistered } it "raises an error" do expect { - described_class.new(file: file, user: user, type: :unregistered).uri + described_class.new(file: file, user: user, type: :unregistered, batch_id: UploadedBatch.create.id).uri }.to raise_error(Lakeshore::IngestFile::UnsupportedFileSetTypeError, "'unregistered' is not a supported file set type") end From 1676cf1321cd4537352deb3ed7a04b0af086a3dd Mon Sep 17 00:00:00 2001 From: Kevin Musiorski <1021154+RudyOnRails@users.noreply.github.com> Date: Wed, 20 Jun 2018 19:00:09 -0500 Subject: [PATCH 16/16] 2907: allow depositor to be passed in to api params (#490) * if depositor key is in API side params, sign in the depositor (but raise exception if not found) after successful API user authentication, otherwise just sign in API user. --- app/controllers/lakeshore/api_controller.rb | 6 +++- config/environments/development.rb | 4 +-- .../lakeshore/file_sets_controller_spec.rb | 29 +++++++++++++++++++ 3 files changed, 36 insertions(+), 3 deletions(-) create mode 100644 spec/controllers/lakeshore/file_sets_controller_spec.rb diff --git a/app/controllers/lakeshore/api_controller.rb b/app/controllers/lakeshore/api_controller.rb index 8bc11cbf..2a844ab3 100644 --- a/app/controllers/lakeshore/api_controller.rb +++ b/app/controllers/lakeshore/api_controller.rb @@ -8,7 +8,11 @@ def authenticate_api_user resource = User.find_by_email(username) return head :unauthorized unless resource if resource.valid_password?(password) && resource.api? - sign_in :user, resource + if params[:depositor] + sign_in :user, User.find_by_email!(params[:depositor]) + else + sign_in :user, resource + end end end end diff --git a/config/environments/development.rb b/config/environments/development.rb index ddb6e0d2..bfc0b0c3 100644 --- a/config/environments/development.rb +++ b/config/environments/development.rb @@ -49,6 +49,6 @@ # config.action_view.raise_on_missing_translations = true # default the queue adapter to :inline or override to :resque, if needed - config.active_job.queue_adapter = :inline - # config.active_job.queue_adapter = :resque + # config.active_job.queue_adapter = :inline + config.active_job.queue_adapter = :resque end diff --git a/spec/controllers/lakeshore/file_sets_controller_spec.rb b/spec/controllers/lakeshore/file_sets_controller_spec.rb new file mode 100644 index 00000000..b1925724 --- /dev/null +++ b/spec/controllers/lakeshore/file_sets_controller_spec.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true +require 'rails_helper' + +describe Lakeshore::FileSetsController do + let(:apiuser) { create(:apiuser) } + before do + sign_in_basic(apiuser) + LakeshoreTesting.restore + end + + let(:asset) { create(:asset, :with_intermediate_file_set) } + let(:file_set1) { asset.intermediate_file_set.first } + let(:file) { create(:image_file) } + let(:aic_user1) { create(:aic_user1) } + + describe "#update" do + before { put :update, params } + context "when API user exists" do + context "but is not an AICUser and no depositor param exists" do + let(:file_set_params) { { files: [file] } } + let(:params) { { file_set: file_set_params, id: file_set1.id } } + it "returns a 500 respone code with an error message" do + expect(response).to have_http_status(500) + expect(response.body).to include("AICUser 'apiuser' not found, contact collections_support@artic.edu\n") + end + end + end + end +end