Permalink
Browse files

Fix multiple library systems incorrectly deleting

books not on the final system looked up and not 
correctly destroying associated copies.
  • Loading branch information...
1 parent 4513502 commit 4ea874267862f6c18606c925e6e821987fcb3cbd @thegreatape committed Nov 4, 2012
View
@@ -3,7 +3,7 @@ class Book < ActiveRecord::Base
belongs_to :user
validates_presence_of :user
- has_many :copies
+ has_many :copies, dependent: :destroy
scope :with_copies_at, lambda {|locations|
where("books.id in (select distinct(copies.book_id) from copies where copies.location_id in (?))", Array(locations))
View
@@ -29,8 +29,9 @@ def update!
results = []
library_systems.each do |system|
bot = system.search_bot(goodreads_id)
- results = bot.lookup
- sync_books results, system
+ system_results = bot.lookup
+ results += system_results
+ sync_books system_results, system
end
delete_books_not_on_list results
end
@@ -0,0 +1,25 @@
+class AddForeignKeyToCopies < ActiveRecord::Migration
+ def up
+ execute <<-SQL
+ delete from copies where copies.id in (
+ select copies.id from copies
+ left join books on copies.book_id = books.id
+ where books.id is null )
+ SQL
+
+ execute <<-SQL
+ alter table copies
+ add constraint fk_copies_books
+ foreign key (book_id)
+ references books (id)
+ SQL
+ end
+
+ def down
+
+ execute <<-SQL
+ alter table copies
+ drop constraint fk_copies_books
+ SQL
+ end
+end
View
@@ -11,14 +11,14 @@
#
# It's strongly recommended to check this file into your version control system.
-ActiveRecord::Schema.define(:version => 20120812201045) do
+ActiveRecord::Schema.define(:version => 20121104182855) do
create_table "books", :force => true do |t|
t.string "title"
t.integer "user_id"
t.string "author"
- t.datetime "created_at", :null => false
- t.datetime "updated_at", :null => false
+ t.datetime "created_at"
+ t.datetime "updated_at"
t.datetime "last_synced_at"
t.string "goodreads_link"
t.string "image_url"
@@ -31,17 +31,17 @@
t.string "call_number"
t.integer "book_id"
t.string "status"
- t.datetime "created_at", :null => false
- t.datetime "updated_at", :null => false
+ t.datetime "created_at"
+ t.datetime "updated_at"
t.datetime "last_synced_at"
t.integer "location_id"
end
create_table "library_systems", :force => true do |t|
t.string "search_bot_class"
t.string "name"
- t.datetime "created_at", :null => false
- t.datetime "updated_at", :null => false
+ t.datetime "created_at"
+ t.datetime "updated_at"
end
create_table "library_systems_users", :force => true do |t|
@@ -52,8 +52,8 @@
create_table "locations", :force => true do |t|
t.integer "library_system_id"
t.string "name"
- t.datetime "created_at", :null => false
- t.datetime "updated_at", :null => false
+ t.datetime "created_at"
+ t.datetime "updated_at"
end
create_table "locations_users", :force => true do |t|
@@ -65,8 +65,8 @@
t.string "oauth_access_token"
t.string "oauth_access_secret"
t.string "goodreads_id"
- t.datetime "created_at", :null => false
- t.datetime "updated_at", :null => false
+ t.datetime "created_at"
+ t.datetime "updated_at"
t.datetime "last_synced_at"
t.text "shelves"
t.text "active_shelves"
@@ -2,7 +2,7 @@
class BookTest < ActiveSupport::TestCase
context "book list syncing" do
- setup do
+ setup do
@list = [
{:title => "The Areas of My Expertise",
:status => 'In',
@@ -20,7 +20,7 @@ class BookTest < ActiveSupport::TestCase
should "produce Copies" do
assert_equal 2, @book.reload.copies.length
- @book.copies.each do |copy|
+ @book.copies.each do |copy|
assert_not_nil copy.location
assert_equal @library_system, copy.location.library_system
assert_not_nil copy.call_number
@@ -33,7 +33,9 @@ class BookTest < ActiveSupport::TestCase
end
should "delete copies no longer on the list" do
- @book.sync_copies [@list.first], @library_system
+ assert_difference "Copy.count", -1 do
+ @book.sync_copies [@list.first], @library_system
+ end
assert_equal 1, @book.reload.copies.length
assert_equal @list.first[:title], @book.copies.first.title
end
@@ -30,7 +30,6 @@ class UserTest < ActiveSupport::TestCase
@user.sync_books @list, Factory(:library_system)
assert_equal 2, @user.reload.books.length
end
-
end
test "goodreads_id should be unique" do
@@ -75,4 +74,32 @@ class UserTest < ActiveSupport::TestCase
end
+ context "with multiple library systems" do
+ setup do
+ @system_a = Factory(:library_system, search_bot_class: 'SearchBots::OrangeNCBot')
+ @system_b = Factory(:library_system, search_bot_class: 'SearchBots::MinutemanBot')
+
+ @system_a.search_bot_class.constantize.any_instance.stubs(:lookup).returns( book('Title 1', 'Copley') + book('Title 2', 'Copley') )
+ @system_b.search_bot_class.constantize.any_instance.stubs(:lookup).returns( book('Title 2', 'Alston') )
+ @user = Factory(:user, library_systems: [@system_a, @system_b])
+
+ @user.update!
+ @user.reload
+ end
+
+ should "have copies from both libraries" do
+ assert_equal 2, @user.books.count
+ end
+ end
+
+ def book(title, location)
+ [ {title: title,
+ author: "John Hodgeman",
+ copies: [{title: "The Areas of My Expertise",
+ status: "Out",
+ call_number: 'A325',
+ location: location }]
+ } ]
+ end
+
end

0 comments on commit 4ea8742

Please sign in to comment.