Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Date issued is no longer a required field #6047

Merged
merged 1 commit into from
Sep 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/controllers/bookmarks_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ def status_action documents
else
case params['action']
when 'publish'
if media_object.title.nil? || media_object.date_issued.nil?
if media_object.title.nil?
errors += ["#{id}, Unable to Publish Item. Missing required fields."]
else
success_ids << id
Expand Down
4 changes: 2 additions & 2 deletions app/controllers/media_objects_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ def update_media_object
error_messages = []
unless @media_object.valid?
invalid_fields = @media_object.errors.attribute_names
required_fields = [:title, :date_issued]
required_fields = [:title]
unless required_fields.any? { |f| invalid_fields.include? f }
invalid_fields.each do |field|
#NOTE this will erase all values for fields with multiple values
Expand Down Expand Up @@ -413,7 +413,7 @@ def update_status
begin
case status
when 'publish'
unless media_object.title.present? && media_object.date_issued.present?
unless media_object.title.present?
errors += ["#{media_object&.title} (#{id}) (missing required fields)"]
next
end
Expand Down
4 changes: 2 additions & 2 deletions app/models/batch_entries.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ class BatchEntries < ActiveRecord::Base
def ensure_mininum_viable_metadata
return nil if minimal_viable_metadata?
self.error = true
self.error_message = 'To successfully ingest, either title and date issued must be set or a bibliographic id must be provided'
self.error_message = 'To successfully ingest, either title must be set or a bibliographic id must be provided'
end

def queue
Expand All @@ -54,7 +54,7 @@ def minimal_viable_metadata?
return false if payload.nil? # nil guard
fields = JSON.parse(payload)['fields']
return false if fields.blank?
return false if (fields['date_issued'].blank? || fields['title'].blank?) && fields['bibliographic_id'].blank?
return false if fields['title'].blank? && fields['bibliographic_id'].blank?
true
end

Expand Down
1 change: 1 addition & 0 deletions app/models/iiif_manifest_presenter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ def metadata_field(label, value, default = nil)
end

def combined_display_date(media_object)
#FIXME Does this need to change now that date_issued is not required and thus could be nil
result = media_object.date_issued
result += " (Creation date: #{media_object.date_created})" if media_object.date_created.present?
Comment on lines 104 to 105
Copy link
Contributor

@masaball masaball Sep 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this does need to be changed. If someone provides a creation date but not an issued date then this method will raise an exception as written. That may be edge case enough (especially since it is new behavior) to not block this PR, but we should at least create a follow-up ticket to rework this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joncameron What do you think about un-combining the display date so it is two separate metadata values in the IIIF manifest?

"Publication date: #{media_object.date_issued}" if media_object.date_issued.present?
"Creation date: #{media_object.date_created}" if media_object.date_created.present?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would love it—that makes a lot more sense and especially with this change. I created #6050 for changing the manifest generation to separate the date fields.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since a new issue has been created, I'll go ahead and approve this PR 👍

result
Expand Down
1 change: 0 additions & 1 deletion app/models/media_object.rb
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,6 @@ class MediaObject < ActiveFedora::Base
# validates :governing_policies, presence: true if Proc.new { |mo| mo.changes["governing_policy_ids"].empty? }

validates :title, presence: true, if: :resource_description_active?
validates :date_issued, presence: true, if: :resource_description_active?
validate :validate_language, if: :resource_description_active?
validate :validate_related_items, if: :resource_description_active?
validate :validate_dates, if: :resource_description_active?
Expand Down
2 changes: 1 addition & 1 deletion app/views/media_objects/_resource_description.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ Unless required by applicable law or agreed to in writing, software distributed

<%= render partial: 'text_field',
locals: {form: form, field: :date_issued,
options: {display_label: 'Publication date', required: true}} %>
options: {display_label: 'Publication date'}} %>

<%= render partial: 'text_field',
locals: {form: form, field: :creator,
Expand Down
1 change: 0 additions & 1 deletion spec/controllers/bookmarks_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,6 @@
it "should show a warning for incomplete items" do
invalid_mo = media_objects.first
invalid_mo.title = nil
invalid_mo.date_issued = nil
invalid_mo.save

post 'publish'
Expand Down
1 change: 0 additions & 1 deletion spec/controllers/master_files_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@
context "must provide a valid media object" do
before do
media_object.title = nil
media_object.date_issued = nil
media_object.workflow.last_completed_step = 'file-upload'
media_object.save(validate: false)
end
Expand Down
9 changes: 2 additions & 7 deletions spec/controllers/media_objects_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -359,7 +359,7 @@
expect(new_media_object.bibliographic_id).to eq({source: "local", id: bib_id})
expect(new_media_object.title).to eq ex_media_object.title
expect(new_media_object.creator).to eq [] #creator no longer required, so supplied value won't be used
expect(new_media_object.date_issued).to eq ex_media_object.date_issued
expect(new_media_object.date_issued).to eq nil #date_issued no longer required, so supplied value won't be used
end
it "should create a new media_object, removing invalid data for non-required fields" do
media_object = FactoryBot.create(:media_object)
Expand Down Expand Up @@ -618,13 +618,12 @@
media_object.save
login_user media_object.collection.managers.first

put :update, params: { id: media_object.id, step: 'resource-description', media_object: {title: '', date_issued: ''} }
put :update, params: { id: media_object.id, step: 'resource-description', media_object: {title: ''} }
expect(response.response_code).to eq(200)
expect(flash[:error]).not_to be_empty
media_object.reload
expect(media_object.valid?).to be_truthy
expect(media_object.title).not_to be_blank
expect(media_object.date_issued).not_to be_blank
end
end

Expand Down Expand Up @@ -1470,7 +1469,6 @@
it "item is invalid" do
media_object = FactoryBot.create(:media_object, collection: collection)
media_object.title = nil
media_object.date_issued = nil
media_object.workflow.last_completed_step = 'file-upload'
media_object.save!(validate: false)
get 'update_status', params: { id: media_object.id, status: 'publish' }
Expand All @@ -1482,7 +1480,6 @@
it "item is invalid and no last_completed_step" do
media_object = FactoryBot.create(:media_object, collection: collection)
media_object.title = nil
media_object.date_issued = nil
media_object.workflow.last_completed_step = ''
media_object.save!(validate: false)
get 'update_status', params: { id: media_object.id, status: 'publish' }
Expand All @@ -1504,7 +1501,6 @@
it 'unpublishes invalid items' do
media_object = FactoryBot.create(:published_media_object, collection: collection)
media_object.title = nil
media_object.date_issued = nil
media_object.save!(validate: false)
get 'update_status', params: { id: media_object.id, status: 'unpublish' }
media_object.reload
Expand All @@ -1514,7 +1510,6 @@
it 'unpublishes invalid items and no last completed step' do
media_object = FactoryBot.create(:published_media_object, collection: collection)
media_object.title = nil
media_object.date_issued = nil
media_object.workflow.last_completed_step = ''
media_object.save!(validate: false)
get 'update_status', params: { id: media_object.id, status: 'unpublish' }
Expand Down
2 changes: 1 addition & 1 deletion spec/lib/avalon/batch/entry_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
let(:testdir) {'spec/fixtures/'}
let(:filename) {'videoshort.mp4'}
let(:collection) {FactoryBot.build(:collection)}
let(:entry_fields) {{ title: Faker::Lorem.sentence, date_issued: "#{DateTime.now.strftime('%F')}" }}
let(:entry_fields) {{ title: Faker::Lorem.sentence }}
let(:entry_files) { [{ file: File.join(testdir, filename), skip_transcoding: false }] }
let(:entry_opts) { {user_key: '[email protected]', collection: collection} }
let(:entry) { Avalon::Batch::Entry.new(entry_fields, entry_files, entry_opts, nil, nil) }
Expand Down
8 changes: 4 additions & 4 deletions spec/models/batch_entries_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,8 @@

describe 'validating payload' do
describe 'with sufficient metadata' do
it 'does not record an error when title and date_issued are present' do
payload = { fields: { title: 'foo', date_issued: Time.now.utc } }
it 'does not record an error when title is present' do
payload = { fields: { title: 'foo' } }
be = BatchEntries.new(payload: payload.to_json, batch_registries: batch_registry)
be.save
be.reload
Expand All @@ -58,12 +58,12 @@
be.reload
expect(be.error).to be_truthy
end
it 'records an error when only title is present' do
it 'does not record an error when only title is present' do
payload = { fields: { title: 'foo' } }
be = BatchEntries.new(payload: payload.to_json, batch_registries: batch_registry)
be.save
be.reload
expect(be.error).to be_truthy
expect(be.error).to be_falsey
end
it 'records an error when only date issued is present' do
payload = { fields: { date_issued: Time.now.utc } }
Expand Down
1 change: 0 additions & 1 deletion spec/models/media_object_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,6 @@
# Force the validations to run by being on the resource-description workflow step
subject(:media_object) { FactoryBot.build(:media_object).tap {|mo| mo.workflow.last_completed_step = "resource-description"} }

it {is_expected.to validate_presence_of(:date_issued)}
it {is_expected.to validate_presence_of(:title)}
end

Expand Down