Skip to content

models: Validate Bibliographic::HostBibTask.email #11

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.

Sign up for GitHub

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

Open
awilfox wants to merge 1 commit into
base: main
Choose a base branch
from
Open

models: Validate Bibliographic::HostBibTask.email #11

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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 .idea/altmedia.iml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion app/models/bibliographic/host_bib_task.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ class HostBibTask < ActiveRecord::Base
succeeded: 6,
failed: 9
}, default: :starting
validates :email, presence: true

class << self
def create_from!(file, email)
Expand Down Expand Up @@ -38,7 +39,7 @@ def upload_errors(file, mms_ids)

def create_bib_task!(host_bib_task, mms_ids)
ActiveRecord::Base.transaction do
host_bib_task.save
host_bib_task.save!
Copy link
Member Author

@awilfox awilfox Feb 5, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

To clarify: this change is needed to ensure the validation runs; otherwise, on validation failure, we just end up with a nil id and the below bulk_insert fails with a NULL constraint error.

Copy link
Contributor

@yzhoubk yzhoubk Feb 6, 2026

Choose a reason for hiding this comment

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

@awilfox Would you mind holding off on merging this PR until we've had a discussion it tomorrow?

attributes = mms_ids.map do |mms_id|
# The default value 'pending' won't be included in bulk inserts.
{ mms_id:, host_bib_task_id: host_bib_task.id, marc_status: 'pending' }
Expand Down
1 change: 1 addition & 0 deletions spec/data/calnet/013191303.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,4 @@ extra:
uid: "013191303"
berkeleyEduIsMemberOf:
- cn=edu:berkeley:org:libr:framework:alma-admins,ou=campus groups,dc=berkeley,dc=edu
berkeleyEduAlternateID: ethomas@berkeley.edu
1 change: 1 addition & 0 deletions spec/data/calnet/013191305.yml
View file
Open in desktop
Copy link
Contributor

@davezuckerman davezuckerman Feb 4, 2026

Choose a reason for hiding this comment

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

This is using an email of a former employee in LIT. I think they probably put it in there but not sure if we should still be using it. Looks like it's used in several places

Copy link
Member Author

@awilfox awilfox Feb 5, 2026

Choose a reason for hiding this comment

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

I copied that from the same identifier in spec/data/alma_patrons - I agree it's not ideal, but it was for testing purposes.

Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,4 @@ extra:
uid: "013191305"
berkeleyEduIsMemberOf:
- cn=edu:berkeley:org:libr:framework:LIBR-framework-admins,ou=campus groups,dc=berkeley,dc=edu
berkeleyEduAlternateID: ethomas@berkeley.edu
2 changes: 1 addition & 1 deletion spec/lib/bibliographic/report_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

module Bibliographic
RSpec.describe Report do
let(:host_bib_task) { Bibliographic::HostBibTask.create(filename: 'fake.txt') }
let(:host_bib_task) { Bibliographic::HostBibTask.create(filename: 'fake.txt', email: 'test@test.example') }
let(:report) { described_class.new(host_bib_task, 6) }

# succeed mmsids for csv content
Expand Down
18 changes: 17 additions & 1 deletion spec/models/host_bib_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
RSpec.describe Bibliographic::HostBib, type: :model do
let(:record_id) { double('record_id') }
let(:mms_id) { '991083840969706532' }
let(:host_bib_task) { Bibliographic::HostBibTask.create(filename: 'fake.txt') }
let(:host_bib_task) { Bibliographic::HostBibTask.create(filename: 'fake.txt', email: 'test@test.example') }
let(:marc_stub) { instance_double(MARC::Record) }
let(:expected_subfields) { { 'w' => '991083840969706532', 't' => 'Seconde partie du discours aux Welches ' } }

Expand Down Expand Up @@ -42,6 +42,22 @@ def marc_stub
end
end

describe 'task creation' do
context 'without an email address' do
it 'is invalid' do
task = Bibliographic::HostBibTask.new(filename: 'fake.txt', email: nil)
expect(task).not_to be_valid
end
end

context 'with an email address' do
it 'is valid' do
task = Bibliographic::HostBibTask.new(filename: 'fake.txt', email: 'test@test.example')
expect(task).to be_valid
end
end
end

context 'host_bib not nil' do
it_behaves_like 'host_bib has alma marc', 'pending'
it_behaves_like 'host_bib has alma marc', 'retrieving'
Expand Down
2 changes: 1 addition & 1 deletion spec/models/linked_bib_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

RSpec.describe Bibliographic::LinkedBib, type: :model do
let(:mms_id) { '991083840969706532' }
let(:host_bib_task) { Bibliographic::HostBibTask.create(filename: 'fake.txt') }
let(:host_bib_task) { Bibliographic::HostBibTask.create(filename: 'fake.txt', email: 'test@test.example') }
let(:host_bib) { host_bib_task.host_bibs.create(mms_id:) }
let(:subfields_from_774) { { 'w' => '991083840969706532', 't' => 'Seconde partie du discours aux Welches ' } }

Expand Down