From 1fba835d4f47990d31b49e6f037e3cf249e687bb Mon Sep 17 00:00:00 2001 From: Robin Ward Date: Thu, 17 Mar 2016 14:41:00 -0400 Subject: [PATCH] FIX: Use a logging table for daily likes given. Use it for badges. --- app/controllers/post_actions_controller.rb | 9 --- app/models/badge.rb | 10 +-- app/models/given_daily_like.rb | 34 ++++++++++ app/models/post_action.rb | 4 +- app/models/user_history.rb | 3 +- ...20160317174357_create_given_daily_likes.rb | 34 ++++++++++ .../post_actions_controller_spec.rb | 14 ---- spec/models/given_daily_like_spec.rb | 50 ++++++++++++++ spec/models/post_action_spec.rb | 67 +++++++++++-------- 9 files changed, 165 insertions(+), 60 deletions(-) create mode 100644 app/models/given_daily_like.rb create mode 100644 db/migrate/20160317174357_create_given_daily_likes.rb create mode 100644 spec/models/given_daily_like_spec.rb diff --git a/app/controllers/post_actions_controller.rb b/app/controllers/post_actions_controller.rb index f707c57e676..4a7585890e0 100644 --- a/app/controllers/post_actions_controller.rb +++ b/app/controllers/post_actions_controller.rb @@ -23,15 +23,6 @@ class PostActionsController < ApplicationController @post.reload render_post_json(@post, _add_raw = false) end - rescue RateLimiter::LimitExceeded => e - # Special case: if we hit the create like rate limit, record it in user history - # so we can award a badge - if e.type == "create_like" - UserHistory.create!(action: UserHistory.actions[:rate_limited_like], - target_user_id: current_user.id, - post_id: @post.id) - end - render_rate_limit_error(e) end def destroy diff --git a/app/models/badge.rb b/app/models/badge.rb index 7e6b237f946..8baf6180ed4 100644 --- a/app/models/badge.rb +++ b/app/models/badge.rb @@ -286,11 +286,11 @@ SQL def self.like_rate_limit(count) <<-SQL - SELECT uh.target_user_id AS user_id, MAX(uh.created_at) AS granted_at - FROM user_histories AS uh - WHERE uh.action = #{UserHistory.actions[:rate_limited_like]} - AND (:backfill OR uh.target_user_id IN (:user_ids)) - GROUP BY uh.target_user_id + SELECT gdl.user_id, current_timestamp AS granted_at + FROM given_daily_likes AS gdl + WHERE gdl.limit_reached + AND (:backfill OR gdl.user_id IN (:user_ids)) + GROUP BY gdl.user_id HAVING COUNT(*) >= #{count} SQL end diff --git a/app/models/given_daily_like.rb b/app/models/given_daily_like.rb new file mode 100644 index 00000000000..38161e0eb5d --- /dev/null +++ b/app/models/given_daily_like.rb @@ -0,0 +1,34 @@ +class GivenDailyLike < ActiveRecord::Base + belongs_to :user + + def self.find_for(user_id, date) + where(user_id: user_id, given_date: date) + end + + def self.increment_for(user_id) + return unless user_id + given_date = Date.today + + # upsert would be nice here + rows = find_for(user_id, given_date).update_all('likes_given = likes_given + 1') + + if rows == 0 + create(user_id: user_id, given_date: given_date, likes_given: 1) + else + find_for(user_id, given_date) + .where('limit_reached = false AND likes_given >= :limit', limit: SiteSetting.max_likes_per_day) + .update_all(limit_reached: true) + end + end + + def self.decrement_for(user_id) + return unless user_id + + given_date = Date.today + + find_for(user_id, given_date).update_all('likes_given = likes_given - 1') + find_for(user_id, given_date) + .where('limit_reached = true AND likes_given < :limit', limit: SiteSetting.max_likes_per_day) + .update_all(limit_reached: false) + end +end diff --git a/app/models/post_action.rb b/app/models/post_action.rb index 31f221cc9f2..08aaae4a0ca 100644 --- a/app/models/post_action.rb +++ b/app/models/post_action.rb @@ -286,12 +286,11 @@ SQL BadgeGranter.queue_badge_grant(Badge::Trigger::PostAction, post_action: post_action) end end + GivenDailyLike.increment_for(user.id) # agree with other flags if staff_took_action PostAction.agree_flags!(post, user) - - # update counters post_action.try(:update_counters) end @@ -311,6 +310,7 @@ SQL if action = finder.first action.remove_act!(user) action.post.unhide! if action.staff_took_action + GivenDailyLike.decrement_for(user.id) end end diff --git a/app/models/user_history.rb b/app/models/user_history.rb index ccabc8ce697..d2afa1c526e 100644 --- a/app/models/user_history.rb +++ b/app/models/user_history.rb @@ -52,7 +52,8 @@ class UserHistory < ActiveRecord::Base grant_moderation: 34, revoke_moderation: 35, backup_operation: 36, - rate_limited_like: 37) + rate_limited_like: 37 # not used anymore + ) end # Staff actions is a subset of all actions, used to audit actions taken by staff users. diff --git a/db/migrate/20160317174357_create_given_daily_likes.rb b/db/migrate/20160317174357_create_given_daily_likes.rb new file mode 100644 index 00000000000..42cf767b951 --- /dev/null +++ b/db/migrate/20160317174357_create_given_daily_likes.rb @@ -0,0 +1,34 @@ +class CreateGivenDailyLikes < ActiveRecord::Migration + def up + create_table :given_daily_likes, id: false, force: true do |t| + t.integer :user_id, null: false + t.integer :likes_given, null: false + t.date :given_date, null: false + t.boolean :limit_reached, null: false, default: false + end + add_index :given_daily_likes, [:user_id, :given_date], unique: true + add_index :given_daily_likes, [:limit_reached, :user_id] + + max_likes_rows = execute("SELECT value FROM site_settings WHERE name = 'max_likes_per_day'") + if max_likes_rows && max_likes_rows.cmd_tuples > 0 + max_likes = max_likes_rows[0]['value'].to_i + end + max_likes ||= 50 + + execute "INSERT INTO given_daily_likes (user_id, likes_given, limit_reached, given_date) + SELECT pa.user_id, + COUNT(*), + CASE WHEN COUNT(*) >= #{max_likes} THEN true + ELSE false + END, + pa.created_at::date + FROM post_actions AS pa + WHERE pa.post_action_type_id = 2 + AND pa.deleted_at IS NULL + GROUP BY pa.user_id, pa.created_at::date" + end + + def down + drop_table :given_daily_likes + end +end diff --git a/spec/controllers/post_actions_controller_spec.rb b/spec/controllers/post_actions_controller_spec.rb index 0a441a0af0e..ca822b832e4 100644 --- a/spec/controllers/post_actions_controller_spec.rb +++ b/spec/controllers/post_actions_controller_spec.rb @@ -7,20 +7,6 @@ describe PostActionsController do expect { xhr :post, :create }.to raise_error(Discourse::NotLoggedIn) end - describe 'logged in as regular user' do - before do - @user = log_in(:user) - @post = Fabricate(:post, user: Fabricate(:coding_horror)) - end - - it 'creates user history if the rate limit for a like is hit' do - PostAction.expects(:act).once.raises(RateLimiter::LimitExceeded.new(60, 'create_like')) - expect(-> { - xhr :post, :create, id: @post.id, post_action_type_id: PostActionType.types[:like] - }).to change(UserHistory, :count).by(1) - end - end - describe 'logged in as moderator' do before do @user = log_in(:moderator) diff --git a/spec/models/given_daily_like_spec.rb b/spec/models/given_daily_like_spec.rb new file mode 100644 index 00000000000..b3a86367769 --- /dev/null +++ b/spec/models/given_daily_like_spec.rb @@ -0,0 +1,50 @@ +require 'rails_helper' + +describe GivenDailyLike do + + it 'no errors without a user' do + expect(-> { GivenDailyLike.increment_for(nil) }).not_to raise_error + expect(-> { GivenDailyLike.decrement_for(nil) }).not_to raise_error + end + + context 'with a user' do + let(:user) { Fabricate(:user) } + + def value_for(user_id, date) + GivenDailyLike.find_for(user_id, date).pluck(:likes_given)[0] || 0 + end + + def limit_reached_for(user_id, date) + GivenDailyLike.find_for(user_id, date).pluck(:limit_reached)[0] || false + end + + it 'can be incremented and decremented' do + SiteSetting.max_likes_per_day = 2 + + Timecop.freeze(Date.today) do + dt = Date.today + + expect(value_for(user.id, dt)).to eq(0) + expect(limit_reached_for(user.id, dt)).to eq(false) + + GivenDailyLike.increment_for(user.id) + expect(value_for(user.id, dt)).to eq(1) + expect(limit_reached_for(user.id, dt)).to eq(false) + + GivenDailyLike.increment_for(user.id) + expect(value_for(user.id, dt)).to eq(2) + expect(limit_reached_for(user.id, dt)).to eq(true) + + GivenDailyLike.decrement_for(user.id) + expect(value_for(user.id, dt)).to eq(1) + expect(limit_reached_for(user.id, dt)).to eq(false) + + GivenDailyLike.decrement_for(user.id) + expect(value_for(user.id, dt)).to eq(0) + expect(limit_reached_for(user.id, dt)).to eq(false) + end + end + + end + +end diff --git a/spec/models/post_action_spec.rb b/spec/models/post_action_spec.rb index e7a906bc601..d3b2a9ad4f7 100644 --- a/spec/models/post_action_spec.rb +++ b/spec/models/post_action_spec.rb @@ -168,19 +168,24 @@ describe PostAction do describe "update_counters" do it "properly updates topic counters" do - # we need this to test it - TopicUser.change(codinghorror, post.topic, posted: true) + Timecop.freeze(Date.today) do + # we need this to test it + TopicUser.change(codinghorror, post.topic, posted: true) - PostAction.act(moderator, post, PostActionType.types[:like]) - PostAction.act(codinghorror, second_post, PostActionType.types[:like]) + expect(GivenDailyLike.value_for(moderator.id, Date.today)).to eq(0) - post.topic.reload - expect(post.topic.like_count).to eq(2) + PostAction.act(moderator, post, PostActionType.types[:like]) + PostAction.act(codinghorror, second_post, PostActionType.types[:like]) - tu = TopicUser.get(post.topic, codinghorror) - expect(tu.liked).to be true - expect(tu.bookmarked).to be false + post.topic.reload + expect(post.topic.like_count).to eq(2) + expect(GivenDailyLike.value_for(moderator.id, Date.today)).to eq(1) + + tu = TopicUser.get(post.topic, codinghorror) + expect(tu.liked).to be true + expect(tu.bookmarked).to be false + end end end @@ -239,29 +244,33 @@ describe PostAction do end it 'should increase the `like_count` and `like_score` when a user likes something' do - PostAction.act(codinghorror, post, PostActionType.types[:like]) - post.reload - expect(post.like_count).to eq(1) - expect(post.like_score).to eq(1) - post.topic.reload - expect(post.topic.like_count).to eq(1) + Timecop.freeze(Date.today) do + PostAction.act(codinghorror, post, PostActionType.types[:like]) + post.reload + expect(post.like_count).to eq(1) + expect(post.like_score).to eq(1) + post.topic.reload + expect(post.topic.like_count).to eq(1) + expect(GivenDailyLike.value_for(codinghorror.id, Date.today)).to eq(1) - # When a staff member likes it - PostAction.act(moderator, post, PostActionType.types[:like]) - post.reload - expect(post.like_count).to eq(2) - expect(post.like_score).to eq(4) + # When a staff member likes it + PostAction.act(moderator, post, PostActionType.types[:like]) + post.reload + expect(post.like_count).to eq(2) + expect(post.like_score).to eq(4) - # Removing likes - PostAction.remove_act(codinghorror, post, PostActionType.types[:like]) - post.reload - expect(post.like_count).to eq(1) - expect(post.like_score).to eq(3) + # Removing likes + PostAction.remove_act(codinghorror, post, PostActionType.types[:like]) + post.reload + expect(post.like_count).to eq(1) + expect(post.like_score).to eq(3) + expect(GivenDailyLike.value_for(codinghorror.id, Date.today)).to eq(0) - PostAction.remove_act(moderator, post, PostActionType.types[:like]) - post.reload - expect(post.like_count).to eq(0) - expect(post.like_score).to eq(0) + PostAction.remove_act(moderator, post, PostActionType.types[:like]) + post.reload + expect(post.like_count).to eq(0) + expect(post.like_score).to eq(0) + end end end