From 2add7954dd4286acebc6c49beba87b5e8f4a4266 Mon Sep 17 00:00:00 2001 From: fatkodima Date: Mon, 22 Jan 2018 00:43:04 +0200 Subject: [PATCH 1/4] Reduce number of transactions for #mark_collection_as_read --- lib/unread/read_mark.rb | 2 +- lib/unread/readable.rb | 37 ++++++++++++++++++++++++------------ spec/unread/readable_spec.rb | 10 +++++++--- 3 files changed, 33 insertions(+), 16 deletions(-) diff --git a/lib/unread/read_mark.rb b/lib/unread/read_mark.rb index 4b66f3c..da3fbfe 100644 --- a/lib/unread/read_mark.rb +++ b/lib/unread/read_mark.rb @@ -1,5 +1,5 @@ class ReadMark < ActiveRecord::Base - belongs_to :readable, polymorphic: true + belongs_to :readable, polymorphic: true, inverse_of: :read_marks validates_presence_of :reader_id, :reader_type, :readable_type diff --git a/lib/unread/readable.rb b/lib/unread/readable.rb index 3e58b1d..2279e37 100644 --- a/lib/unread/readable.rb +++ b/lib/unread/readable.rb @@ -27,23 +27,36 @@ def mark_collection_as_read(collection, reader) if global_timestamp && global_timestamp >= timestamp # The object is implicitly marked as read, so there is nothing to do else - # This transaction is needed, so that parent transaction won't rollback even there's an error. - ReadMark.transaction(requires_new: true) do - begin - rm = obj.read_marks.where(reader_id: reader.id, reader_type: reader.class.base_class.name).first || obj.read_marks.build - rm.reader_id = reader.id - rm.reader_type = reader.class.base_class.name - rm.timestamp = timestamp - rm.save! - rescue ActiveRecord::RecordNotUnique - raise ActiveRecord::Rollback - end - end + mark_collection_item_as_read(obj, reader) end end end end + def mark_collection_item_as_read(obj, reader) + marking_proc = proc { |obj| + rm = obj.read_marks.find_or_initialize_by(reader: reader) + rm.timestamp = obj.send(readable_options[:on]) + rm.save! + } + + if using_postgresql? + ReadMark.transaction(requires_new: true) do + begin + marking_proc.call(obj) + rescue ActiveRecord::RecordNotUnique + raise ActiveRecord::Rollback + end + end + else + begin + marking_proc.call(obj) + rescue ActiveRecord::RecordNotUnique + # The object is explicitly marked as read, so there is nothing to do + end + end + end + # A scope with all items accessable for the given reader # It's used in cleanup_read_marks! to support a filtered cleanup # Should be overriden if a reader doesn't have access to all items diff --git a/spec/unread/readable_spec.rb b/spec/unread/readable_spec.rb index 01cbf2b..b742b74 100644 --- a/spec/unread/readable_spec.rb +++ b/spec/unread/readable_spec.rb @@ -269,8 +269,8 @@ it "should mark the rest as read when the first record is not unique" do Email.mark_as_read! [ @email1 ], for: @reader - allow(@email1).to receive_message_chain("read_marks.build").and_return(@email1.read_marks.build) - allow(@email1).to receive_message_chain("read_marks.where").and_return([]) + allow(@email1).to receive_message_chain("read_marks.find_or_initialize_by") + .and_return(@email1.read_marks.build(reader: @reader)) expect do Email.mark_as_read! [ @email1, @email2 ], for: @reader @@ -295,7 +295,6 @@ expect(@email2.unread?(@reader)).to be_falsey end - it "should mark all objects as read" do Email.mark_as_read! :all, for: @reader @@ -330,6 +329,11 @@ Email.mark_as_read! :foo, :bar }.to raise_error(ArgumentError) end + + it "should work with STI readers" do + Email.mark_as_read! [ @email1 ], for: Customer.find(@sti_reader.id) + expect(@email1.unread?(@sti_reader)).to be_falsey + end end describe :cleanup_read_marks! do From 4d9105561cce1335fd0b00d0600d71ebdbefbc8d Mon Sep 17 00:00:00 2001 From: Georg Ledermann Date: Sun, 28 Jan 2018 07:57:32 +0100 Subject: [PATCH 2/4] Fix warning, obj is already given --- lib/unread/readable.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/unread/readable.rb b/lib/unread/readable.rb index 2279e37..d120be3 100644 --- a/lib/unread/readable.rb +++ b/lib/unread/readable.rb @@ -34,7 +34,7 @@ def mark_collection_as_read(collection, reader) end def mark_collection_item_as_read(obj, reader) - marking_proc = proc { |obj| + marking_proc = proc { rm = obj.read_marks.find_or_initialize_by(reader: reader) rm.timestamp = obj.send(readable_options[:on]) rm.save! @@ -43,14 +43,14 @@ def mark_collection_item_as_read(obj, reader) if using_postgresql? ReadMark.transaction(requires_new: true) do begin - marking_proc.call(obj) + marking_proc.call rescue ActiveRecord::RecordNotUnique raise ActiveRecord::Rollback end end else begin - marking_proc.call(obj) + marking_proc.call rescue ActiveRecord::RecordNotUnique # The object is explicitly marked as read, so there is nothing to do end From 2d750ba3a94de740c91716f78debb9979e53ce7d Mon Sep 17 00:00:00 2001 From: Georg Ledermann Date: Sun, 28 Jan 2018 07:57:52 +0100 Subject: [PATCH 3/4] Use timestamp --- lib/unread/readable.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/unread/readable.rb b/lib/unread/readable.rb index d120be3..5e0f252 100644 --- a/lib/unread/readable.rb +++ b/lib/unread/readable.rb @@ -27,16 +27,16 @@ def mark_collection_as_read(collection, reader) if global_timestamp && global_timestamp >= timestamp # The object is implicitly marked as read, so there is nothing to do else - mark_collection_item_as_read(obj, reader) + mark_collection_item_as_read(obj, reader, timestamp) end end end end - def mark_collection_item_as_read(obj, reader) + def mark_collection_item_as_read(obj, reader, timestamp) marking_proc = proc { rm = obj.read_marks.find_or_initialize_by(reader: reader) - rm.timestamp = obj.send(readable_options[:on]) + rm.timestamp = timestamp rm.save! } From bc716f0bae44413efaedfd1f8e123861e4fdc621 Mon Sep 17 00:00:00 2001 From: Georg Ledermann Date: Sun, 28 Jan 2018 07:57:58 +0100 Subject: [PATCH 4/4] Add some comments --- lib/unread/readable.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/unread/readable.rb b/lib/unread/readable.rb index 5e0f252..8a66907 100644 --- a/lib/unread/readable.rb +++ b/lib/unread/readable.rb @@ -41,10 +41,14 @@ def mark_collection_item_as_read(obj, reader, timestamp) } if using_postgresql? + # With PostgreSQL, a transaction is unusable after a unique constraint vialoation. + # To avoid this, nested transactions are required. + # http://api.rubyonrails.org/classes/ActiveRecord/Transactions/ClassMethods.html#module-ActiveRecord::Transactions::ClassMethods-label-Exception+handling+and+rolling+back ReadMark.transaction(requires_new: true) do begin marking_proc.call rescue ActiveRecord::RecordNotUnique + # The object is explicitly marked as read, so rollback the inner transaction raise ActiveRecord::Rollback end end