Skip to content

Commit

Permalink
Prevent Facebook count updates to non-canonical locations
Browse files Browse the repository at this point in the history
  • Loading branch information
chrismaddocks committed Jun 2, 2017
1 parent 8c6e430 commit c8943f6
Show file tree
Hide file tree
Showing 3 changed files with 28 additions and 7 deletions.
15 changes: 14 additions & 1 deletion app/models/concerns/wakes/metrics/facebook.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ module Facebook
included do
store_accessor :document, :facebook_count, :facebook_count_updated_at

before_validation :prevent_decreasing_update, on: :update
before_validation :prevent_update_of_legacy_location, :prevent_decreasing_update, on: :update

def enqueue_facebook_count_update
Wakes::UpdateFacebookMetricsJob.perform_later(self)
Expand All @@ -24,6 +24,14 @@ def update_facebook_count(new_facebook_count)

protected

def prevent_update_of_legacy_location
if facebook_count_will_change? && !canonical
Rails.logger.error "Received a request to update facebook metrics for non-canonical location #{path}, " \
"from #{old_count} to #{new_count}. Ignoring it!"
throw :abort
end
end

def prevent_decreasing_update
if new_count < old_count
Rails.logger.error "Received a request to update facebook metrics for location #{path}, " \
Expand All @@ -39,6 +47,11 @@ def old_count
def new_count
changes[:document]&.second.try(:[], :facebook_count).to_i
end

# from https://github.com/rails/rails/issues/16808#issuecomment-112264052
def facebook_count_will_change?
changes[:document] && changes[:document].map { |v| v.try(:[], :facebook_count) }.uniq.length > 1
end
end
end
end
Expand Down
2 changes: 1 addition & 1 deletion app/services/wakes/facebook_count_updater_service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ class Wakes::FacebookCountUpdaterService
attr_reader :locations, :wrapper

def initialize(*locations)
@locations = locations.flatten
@locations = locations.flatten.select(&:canonical)
@wrapper = Wakes::FacebookMetricsWrapper
end

Expand Down
18 changes: 13 additions & 5 deletions spec/services/wakes/facebook_count_updater_service_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,8 @@
let!(:location_1) { create(:location, :facebook_count => 4) }
let!(:location_2) { create(:location, :facebook_count => 10) }
let!(:location_3) { create(:location, :facebook_count => 12) }
let!(:legacy_location_1) { create(:location, :non_canonical, :facebook_count => 3) }
let!(:legacy_location_2) { create(:location, :non_canonical, :facebook_count => 6) }

subject { described_class.new([location_1, location_2, location_3]) }

Expand All @@ -78,28 +80,34 @@
.to receive(:new).and_return(facebook_wrapper)
allow(facebook_wrapper).to receive(:share_counts).and_return(location_1.url => 20,
location_2.url => 30,
location_3.url => 40)
location_3.url => 40,
legacy_location_1.url => 50,
legacy_location_2.url => 60)
end

before do
location_1.resource.locations << create(:location, :non_canonical, :facebook_count => 3)
location_2.resource.locations << create(:location, :non_canonical, :facebook_count => 6)
location_1.resource.locations << legacy_location_1
location_2.resource.locations << legacy_location_2
end

it 'updates the facebook count of all the locations' do
it 'updates the facebook count of all canonical locations' do
subject.update_facebook_count
expect(location_1.facebook_count).to eq(20)
expect(location_2.facebook_count).to eq(30)
expect(location_3.facebook_count).to eq(40)
expect(legacy_location_1.facebook_count).to eq(3) # original value
expect(legacy_location_2.facebook_count).to eq(6) # original value
end

it 'updates the facebook_count_updated_at for all locations' do
it 'updates the facebook_count_updated_at for all canonical locations' do
time = Time.zone.now
Timecop.freeze(time)
subject.update_facebook_count
expect(location_1.facebook_count_updated_at).to be_within(0.1).of(time)
expect(location_2.facebook_count_updated_at).to be_within(0.1).of(time)
expect(location_3.facebook_count_updated_at).to be_within(0.1).of(time)
expect(legacy_location_1.facebook_count_updated_at).not_to be_within(0.1).of(time)
expect(legacy_location_2.facebook_count_updated_at).not_to be_within(0.1).of(time)
end

it 'aggregates the facebook counts of associated resource' do
Expand Down

0 comments on commit c8943f6

Please sign in to comment.