From f0e942f6474ffa41be835e2deda9ebdac2f6c93c Mon Sep 17 00:00:00 2001 From: Sam Date: Thu, 18 Feb 2016 16:57:22 +1100 Subject: [PATCH] PERF: move 3 more option columns out of the user table --- .../javascripts/discourse/models/user.js.es6 | 6 +- .../discourse/templates/user/preferences.hbs | 4 +- app/jobs/regular/update_top_redirection.rb | 2 +- app/models/topic_tracking_state.rb | 7 +- app/models/topic_user.rb | 55 +++++------ app/models/user.rb | 85 ----------------- app/models/user_option.rb | 71 ++++++++++++++ app/serializers/current_user_serializer.rb | 10 +- app/serializers/listable_topic_serializer.rb | 2 +- app/serializers/user_option_serializer.rb | 13 ++- app/serializers/user_serializer.rb | 10 -- app/services/user_updater.rb | 12 +-- db/fixtures/009_users.rb | 9 +- ...9_move_tracking_options_to_user_options.rb | 16 ++++ lib/topic_query.rb | 2 +- spec/components/topic_query_spec.rb | 12 +-- spec/fabricators/user_option_fabricator.rb | 2 + spec/models/topic_user_spec.rb | 13 ++- spec/models/user_option_spec.rb | 92 ++++++++++++++++++ spec/models/user_spec.rb | 94 +------------------ spec/serializers/user_serializer_spec.rb | 4 + spec/services/user_updater_spec.rb | 7 +- 22 files changed, 278 insertions(+), 250 deletions(-) create mode 100644 db/migrate/20160225050319_move_tracking_options_to_user_options.rb create mode 100644 spec/fabricators/user_option_fabricator.rb create mode 100644 spec/models/user_option_spec.rb diff --git a/app/assets/javascripts/discourse/models/user.js.es6 b/app/assets/javascripts/discourse/models/user.js.es6 index 516f7b1f7a1..c57574431e7 100644 --- a/app/assets/javascripts/discourse/models/user.js.es6 +++ b/app/assets/javascripts/discourse/models/user.js.es6 @@ -141,13 +141,11 @@ const User = RestModel.extend({ save() { const data = this.getProperties( - 'auto_track_topics_after_msecs', 'bio_raw', 'website', 'location', 'name', 'locale', - 'new_topic_duration_minutes', 'custom_fields', 'user_fields', 'muted_usernames', @@ -165,7 +163,9 @@ const User = RestModel.extend({ 'enable_quoting', 'disable_jump_reply', 'automatically_unpin_topics', - 'digest_after_days' + 'digest_after_days', + 'new_topic_duration_minutes', + 'auto_track_topics_after_msecs' ].forEach(s => { data[s] = this.get(`user_option.${s}`); }); diff --git a/app/assets/javascripts/discourse/templates/user/preferences.hbs b/app/assets/javascripts/discourse/templates/user/preferences.hbs index 4c65524e82b..06bab9cbf45 100644 --- a/app/assets/javascripts/discourse/templates/user/preferences.hbs +++ b/app/assets/javascripts/discourse/templates/user/preferences.hbs @@ -201,12 +201,12 @@
- {{combo-box valueAttribute="value" content=considerNewTopicOptions value=model.new_topic_duration_minutes}} + {{combo-box valueAttribute="value" content=considerNewTopicOptions value=model.user_option.new_topic_duration_minutes}}
- {{combo-box valueAttribute="value" content=autoTrackDurations value=model.auto_track_topics_after_msecs}} + {{combo-box valueAttribute="value" content=autoTrackDurations value=model.user_option.auto_track_topics_after_msecs}}
{{preference-checkbox labelKey="user.external_links_in_new_tab" checked=model.user_option.external_links_in_new_tab}} diff --git a/app/jobs/regular/update_top_redirection.rb b/app/jobs/regular/update_top_redirection.rb index 496f86f5891..776cdfca7de 100644 --- a/app/jobs/regular/update_top_redirection.rb +++ b/app/jobs/regular/update_top_redirection.rb @@ -4,7 +4,7 @@ module Jobs def execute(args) if user = User.find_by(id: args[:user_id]) - user.update_column(:last_redirected_to_top_at, args[:redirected_at]) + user.user_option.update_column(:last_redirected_to_top_at, args[:redirected_at]) end end end diff --git a/app/models/topic_tracking_state.rb b/app/models/topic_tracking_state.rb index 5f96c2e2622..7ffc6ad1e1a 100644 --- a/app/models/topic_tracking_state.rb +++ b/app/models/topic_tracking_state.rb @@ -104,9 +104,9 @@ class TopicTrackingState def self.treat_as_new_topic_clause User.where("GREATEST(CASE - WHEN COALESCE(u.new_topic_duration_minutes, :default_duration) = :always THEN u.created_at - WHEN COALESCE(u.new_topic_duration_minutes, :default_duration) = :last_visit THEN COALESCE(u.previous_visit_at,u.created_at) - ELSE (:now::timestamp - INTERVAL '1 MINUTE' * COALESCE(u.new_topic_duration_minutes, :default_duration)) + WHEN COALESCE(uo.new_topic_duration_minutes, :default_duration) = :always THEN u.created_at + WHEN COALESCE(uo.new_topic_duration_minutes, :default_duration) = :last_visit THEN COALESCE(u.previous_visit_at,u.created_at) + ELSE (:now::timestamp - INTERVAL '1 MINUTE' * COALESCE(uo.new_topic_duration_minutes, :default_duration)) END, us.new_since, :min_date)", now: DateTime.now, last_visit: User::NewTopicDuration::LAST_VISIT, @@ -169,6 +169,7 @@ class TopicTrackingState FROM topics JOIN users u on u.id = :user_id JOIN user_stats AS us ON us.user_id = u.id + JOIN user_options AS uo ON uo.user_id = u.id JOIN categories c ON c.id = topics.category_id LEFT JOIN topic_users tu ON tu.topic_id = topics.id AND tu.user_id = u.id WHERE u.id = :user_id AND diff --git a/app/models/topic_user.rb b/app/models/topic_user.rb index 25298af1f47..1a502378247 100644 --- a/app/models/topic_user.rb +++ b/app/models/topic_user.rb @@ -104,7 +104,7 @@ class TopicUser < ActiveRecord::Base if rows == 0 now = DateTime.now - auto_track_after = User.select(:auto_track_topics_after_msecs).find_by(id: user_id).try(:auto_track_topics_after_msecs) + auto_track_after = UserOption.where(user_id: user_id).pluck(:auto_track_topics_after_msecs).first auto_track_after ||= SiteSetting.default_other_auto_track_topics_after_msecs if auto_track_after >= 0 && auto_track_after <= (attrs[:total_msecs_viewed].to_i || 0) @@ -140,6 +140,31 @@ class TopicUser < ActiveRecord::Base # Update the last read and the last seen post count, but only if it doesn't exist. # This would be a lot easier if psql supported some kind of upsert + UPDATE_TOPIC_USER_SQL = "UPDATE topic_users + SET + last_read_post_number = GREATEST(:post_number, tu.last_read_post_number), + highest_seen_post_number = t.highest_post_number, + total_msecs_viewed = LEAST(tu.total_msecs_viewed + :msecs,86400000), + notification_level = + case when tu.notifications_reason_id is null and (tu.total_msecs_viewed + :msecs) > + coalesce(uo.auto_track_topics_after_msecs,:threshold) and + coalesce(uo.auto_track_topics_after_msecs, :threshold) >= 0 then + :tracking + else + tu.notification_level + end + FROM topic_users tu + join topics t on t.id = tu.topic_id + join users u on u.id = :user_id + join user_options uo on uo.user_id = :user_id + WHERE + tu.topic_id = topic_users.topic_id AND + tu.user_id = topic_users.user_id AND + tu.topic_id = :topic_id AND + tu.user_id = :user_id + RETURNING + topic_users.notification_level, tu.notification_level old_level, tu.last_read_post_number + " def update_last_read(user, topic_id, post_number, msecs, opts={}) return if post_number.blank? msecs = 0 if msecs.to_i < 0 @@ -160,31 +185,7 @@ class TopicUser < ActiveRecord::Base # ... user visited the topic but did not read the posts # # 86400000 = 1 day - rows = exec_sql("UPDATE topic_users - SET - last_read_post_number = GREATEST(:post_number, tu.last_read_post_number), - highest_seen_post_number = t.highest_post_number, - total_msecs_viewed = LEAST(tu.total_msecs_viewed + :msecs,86400000), - notification_level = - case when tu.notifications_reason_id is null and (tu.total_msecs_viewed + :msecs) > - coalesce(u.auto_track_topics_after_msecs,:threshold) and - coalesce(u.auto_track_topics_after_msecs, :threshold) >= 0 then - :tracking - else - tu.notification_level - end - FROM topic_users tu - join topics t on t.id = tu.topic_id - join users u on u.id = :user_id - WHERE - tu.topic_id = topic_users.topic_id AND - tu.user_id = topic_users.user_id AND - tu.topic_id = :topic_id AND - tu.user_id = :user_id - RETURNING - topic_users.notification_level, tu.notification_level old_level, tu.last_read_post_number - ", - args).values + rows = exec_sql(UPDATE_TOPIC_USER_SQL,args).values if rows.length == 1 before = rows[0][1].to_i @@ -206,7 +207,7 @@ class TopicUser < ActiveRecord::Base if rows.length == 0 # The user read at least one post in a topic that they haven't viewed before. args[:new_status] = notification_levels[:regular] - if (user.auto_track_topics_after_msecs || SiteSetting.default_other_auto_track_topics_after_msecs) == 0 + if (user.user_option.auto_track_topics_after_msecs || SiteSetting.default_other_auto_track_topics_after_msecs) == 0 args[:new_status] = notification_levels[:tracking] end TopicTrackingState.publish_read(topic_id, post_number, user.id, args[:new_status]) diff --git a/app/models/user.rb b/app/models/user.rb index 5634cbeaf38..71ee83831a2 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -77,8 +77,6 @@ class User < ActiveRecord::Base after_initialize :add_trust_level - before_create :set_default_user_preferences - after_create :create_email_token after_create :create_user_stat after_create :create_user_option @@ -90,7 +88,6 @@ class User < ActiveRecord::Base before_save :update_username_lower before_save :ensure_password_is_hashed - after_save :update_tracked_topics after_save :clear_global_notice_if_needed after_save :refresh_avatar after_save :badge_grant @@ -626,20 +623,6 @@ class User < ActiveRecord::Base Promotion.new(self).change_trust_level!(level, opts) end - def treat_as_new_topic_start_date - duration = new_topic_duration_minutes || SiteSetting.default_other_new_topic_duration_minutes.to_i - times = [case duration - when User::NewTopicDuration::ALWAYS - created_at - when User::NewTopicDuration::LAST_VISIT - previous_visit_at || user_stat.new_since - else - duration.minutes.ago - end, user_stat.new_since, Time.at(SiteSetting.min_new_topics_time).to_datetime] - - times.max - end - def readable_name return "#{name} (#{username})" if name.present? && name != username username @@ -730,51 +713,6 @@ class User < ActiveRecord::Base .exists? end - def should_be_redirected_to_top - redirected_to_top.present? - end - - def redirected_to_top - # redirect is enabled - return unless SiteSetting.redirect_users_to_top_page - # top must be in the top_menu - return unless SiteSetting.top_menu =~ /(^|\|)top(\||$)/i - # not enough topics - return unless period = SiteSetting.min_redirected_to_top_period - - if !seen_before? || (trust_level == 0 && !redirected_to_top_yet?) - update_last_redirected_to_top! - return { - reason: I18n.t('redirected_to_top_reasons.new_user'), - period: period - } - elsif last_seen_at < 1.month.ago - update_last_redirected_to_top! - return { - reason: I18n.t('redirected_to_top_reasons.not_seen_in_a_month'), - period: period - } - end - - # don't redirect to top - nil - end - - def redirected_to_top_yet? - last_redirected_to_top_at.present? - end - - def update_last_redirected_to_top! - key = "user:#{id}:update_last_redirected_to_top" - delay = SiteSetting.active_user_rate_limit_secs - - # only update last_redirected_to_top_at once every minute - return unless $redis.setnx(key, "1") - $redis.expire(key, delay) - - # delay the update - Jobs.enqueue_in(delay / 2, :update_top_redirection, user_id: self.id, redirected_at: Time.zone.now) - end def refresh_avatar return if @import_mode @@ -880,11 +818,6 @@ class User < ActiveRecord::Base end end - def update_tracked_topics - return unless auto_track_topics_after_msecs_changed? - TrackedTopicsUpdater.new(id, auto_track_topics_after_msecs).call - end - def clear_global_notice_if_needed if admin && SiteSetting.has_login_hint SiteSetting.has_login_hint = false @@ -970,14 +903,6 @@ class User < ActiveRecord::Base end end - def set_default_user_preferences - set_default_other_new_topic_duration_minutes - set_default_other_auto_track_topics_after_msecs - - # needed, otherwise the callback chain is broken... - true - end - def set_default_categories_preferences values = [] @@ -1025,12 +950,6 @@ class User < ActiveRecord::Base end - %w{new_topic_duration_minutes auto_track_topics_after_msecs}.each do |s| - define_method("set_default_other_#{s}") do - self.send("#{s}=", SiteSetting.send("default_other_#{s}").to_i) if has_attribute?(s) - end - end - end # == Schema Information @@ -1054,7 +973,6 @@ end # admin :boolean default(FALSE), not null # last_emailed_at :datetime # trust_level :integer not null -# email_private_messages :boolean default(TRUE) # approved :boolean default(FALSE), not null # approved_by_id :integer # approved_at :datetime @@ -1062,11 +980,9 @@ end # suspended_at :datetime # suspended_till :datetime # date_of_birth :date -# auto_track_topics_after_msecs :integer # views :integer default(0), not null # flag_level :integer default(0), not null # ip_address :inet -# new_topic_duration_minutes :integer # moderator :boolean default(FALSE) # blocked :boolean default(FALSE) # title :string(255) @@ -1074,7 +990,6 @@ end # primary_group_id :integer # locale :string(10) # registration_ip_address :inet -# last_redirected_to_top_at :datetime # trust_level_locked :boolean default(FALSE), not null # staged :boolean default(FALSE), not null # diff --git a/app/models/user_option.rb b/app/models/user_option.rb index 3dac2ded9e0..46d7e673079 100644 --- a/app/models/user_option.rb +++ b/app/models/user_option.rb @@ -3,6 +3,8 @@ class UserOption < ActiveRecord::Base belongs_to :user before_create :set_defaults + after_save :update_tracked_topics + def set_defaults self.email_always = SiteSetting.default_email_always self.mailing_list_mode = SiteSetting.default_email_mailing_list_mode @@ -16,6 +18,9 @@ class UserOption < ActiveRecord::Base self.disable_jump_reply = SiteSetting.default_other_disable_jump_reply self.edit_history_public = SiteSetting.default_other_edit_history_public + self.new_topic_duration_minutes = SiteSetting.default_other_new_topic_duration_minutes + self.auto_track_topics_after_msecs = SiteSetting.default_other_auto_track_topics_after_msecs + if SiteSetting.default_email_digest_frequency.to_i <= 0 self.email_digests = false @@ -26,4 +31,70 @@ class UserOption < ActiveRecord::Base true end + + def update_tracked_topics + return unless auto_track_topics_after_msecs_changed? + TrackedTopicsUpdater.new(id, auto_track_topics_after_msecs).call + end + + def redirected_to_top_yet? + last_redirected_to_top_at.present? + end + + def update_last_redirected_to_top! + key = "user:#{id}:update_last_redirected_to_top" + delay = SiteSetting.active_user_rate_limit_secs + + # only update last_redirected_to_top_at once every minute + return unless $redis.setnx(key, "1") + $redis.expire(key, delay) + + # delay the update + Jobs.enqueue_in(delay / 2, :update_top_redirection, user_id: self.id, redirected_at: Time.zone.now) + end + + def should_be_redirected_to_top + redirected_to_top.present? + end + + def redirected_to_top + # redirect is enabled + return unless SiteSetting.redirect_users_to_top_page + # top must be in the top_menu + return unless SiteSetting.top_menu =~ /(^|\|)top(\||$)/i + # not enough topics + return unless period = SiteSetting.min_redirected_to_top_period + + if !user.seen_before? || (user.trust_level == 0 && !redirected_to_top_yet?) + update_last_redirected_to_top! + return { + reason: I18n.t('redirected_to_top_reasons.new_user'), + period: period + } + elsif user.last_seen_at < 1.month.ago + update_last_redirected_to_top! + return { + reason: I18n.t('redirected_to_top_reasons.not_seen_in_a_month'), + period: period + } + end + + # don't redirect to top + nil + end + + def treat_as_new_topic_start_date + duration = new_topic_duration_minutes || SiteSetting.default_other_new_topic_duration_minutes.to_i + times = [case duration + when User::NewTopicDuration::ALWAYS + user.created_at + when User::NewTopicDuration::LAST_VISIT + user.previous_visit_at || user.user_stat.new_since + else + duration.minutes.ago + end, user.user_stat.new_since, Time.at(SiteSetting.min_new_topics_time).to_datetime] + + times.max + end + end diff --git a/app/serializers/current_user_serializer.rb b/app/serializers/current_user_serializer.rb index 5cf912d4734..2c916e47563 100644 --- a/app/serializers/current_user_serializer.rb +++ b/app/serializers/current_user_serializer.rb @@ -70,6 +70,14 @@ class CurrentUserSerializer < BasicUserSerializer object.user_option.automatically_unpin_topics end + def should_be_redirected_to_top + object.user_option.should_be_redirected_to_top + end + + def redirected_to_top + object.user_option.redirected_to_top + end + def site_flagged_posts_count PostAction.flagged_posts_count end @@ -103,7 +111,7 @@ class CurrentUserSerializer < BasicUserSerializer end def include_redirected_to_top? - object.redirected_to_top.present? + object.user_option.redirected_to_top.present? end def custom_fields diff --git a/app/serializers/listable_topic_serializer.rb b/app/serializers/listable_topic_serializer.rb index 609bf1d72a8..3589d7a2312 100644 --- a/app/serializers/listable_topic_serializer.rb +++ b/app/serializers/listable_topic_serializer.rb @@ -45,7 +45,7 @@ class ListableTopicSerializer < BasicTopicSerializer def seen return true if !scope || !scope.user return true if object.user_data && !object.user_data.last_read_post_number.nil? - return true if object.created_at < scope.user.treat_as_new_topic_start_date + return true if object.created_at < scope.user.user_option.treat_as_new_topic_start_date false end diff --git a/app/serializers/user_option_serializer.rb b/app/serializers/user_option_serializer.rb index 77c9d68540a..440587a4d42 100644 --- a/app/serializers/user_option_serializer.rb +++ b/app/serializers/user_option_serializer.rb @@ -11,10 +11,21 @@ class UserOptionSerializer < ApplicationSerializer :disable_jump_reply, :digest_after_days, :automatically_unpin_topics, - :edit_history_public + :edit_history_public, + :auto_track_topics_after_msecs, + :new_topic_duration_minutes def include_edit_history_public? !SiteSetting.edit_history_visible_to_public end + + def auto_track_topics_after_msecs + object.auto_track_topics_after_msecs || SiteSetting.default_other_auto_track_topics_after_msecs + end + + def new_topic_duration_minutes + object.new_topic_duration_minutes || SiteSetting.default_other_new_topic_duration_minutes + end + end diff --git a/app/serializers/user_serializer.rb b/app/serializers/user_serializer.rb index 1ad96ce2405..730a23da741 100644 --- a/app/serializers/user_serializer.rb +++ b/app/serializers/user_serializer.rb @@ -82,8 +82,6 @@ class UserSerializer < BasicUserSerializer :can_delete_all_posts private_attributes :locale, - :auto_track_topics_after_msecs, - :new_topic_duration_minutes, :muted_category_ids, :tracked_category_ids, :watched_category_ids, @@ -253,14 +251,6 @@ class UserSerializer < BasicUserSerializer ### PRIVATE ATTRIBUTES ### - def auto_track_topics_after_msecs - object.auto_track_topics_after_msecs || SiteSetting.default_other_auto_track_topics_after_msecs - end - - def new_topic_duration_minutes - object.new_topic_duration_minutes || SiteSetting.default_other_new_topic_duration_minutes - end - def muted_category_ids CategoryUser.lookup(object, :muted).pluck(:category_id) end diff --git a/app/services/user_updater.rb b/app/services/user_updater.rb index e028a0d14b0..b3b8e9031c7 100644 --- a/app/services/user_updater.rb +++ b/app/services/user_updater.rb @@ -18,7 +18,9 @@ class UserUpdater :disable_jump_reply, :edit_history_public, :automatically_unpin_topics, - :digest_after_days + :digest_after_days, + :new_topic_duration_minutes, + :auto_track_topics_after_msecs ] def initialize(actor, user) @@ -38,14 +40,6 @@ class UserUpdater user.name = attributes.fetch(:name) { user.name } user.locale = attributes.fetch(:locale) { user.locale } - if attributes[:auto_track_topics_after_msecs] - user.auto_track_topics_after_msecs = attributes[:auto_track_topics_after_msecs].to_i - end - - if attributes[:new_topic_duration_minutes] - user.new_topic_duration_minutes = attributes[:new_topic_duration_minutes].to_i - end - if guardian.can_grant_title?(user) user.title = attributes.fetch(:title) { user.title } end diff --git a/db/fixtures/009_users.rb b/db/fixtures/009_users.rb index ccdcaceace4..d65c8f2ef73 100644 --- a/db/fixtures/009_users.rb +++ b/db/fixtures/009_users.rb @@ -32,9 +32,9 @@ duration = Rails.env.production? ? 60 : 0 if User.exec_sql("SELECT 1 FROM schema_migration_details WHERE EXISTS( SELECT 1 FROM INFORMATION_SCHEMA.COLUMNS - WHERE table_name = 'users' AND column_name = 'enable_quoting' + WHERE table_name = 'users' AND column_name = 'last_redirected_to_top_at' ) AND - name = 'AllowDefaultsOnUsersTable' AND + name = 'MoveTrackingOptionsToUserOptions' AND created_at < (current_timestamp at time zone 'UTC' - interval '#{duration} minutes') ").to_a.length > 0 @@ -54,7 +54,10 @@ if User.exec_sql("SELECT 1 FROM schema_migration_details edit_history_public automatically_unpin_topics digest_after_days - ].each do |column| + auto_track_topics_after_msecs + new_topic_duration_minutes + last_redirected_to_top_at +].each do |column| User.exec_sql("ALTER TABLE users DROP column IF EXISTS #{column}") end diff --git a/db/migrate/20160225050319_move_tracking_options_to_user_options.rb b/db/migrate/20160225050319_move_tracking_options_to_user_options.rb new file mode 100644 index 00000000000..87dabf39959 --- /dev/null +++ b/db/migrate/20160225050319_move_tracking_options_to_user_options.rb @@ -0,0 +1,16 @@ +class MoveTrackingOptionsToUserOptions < ActiveRecord::Migration + def change + add_column :user_options, :auto_track_topics_after_msecs, :integer + add_column :user_options, :new_topic_duration_minutes, :integer + add_column :user_options, :last_redirected_to_top_at, :datetime + + execute < true)), @user.treat_as_new_topic_start_date) + result = TopicQuery.new_filter(default_results(options.reverse_merge(:unordered => true)), @user.user_option.treat_as_new_topic_start_date) result = remove_muted_topics(result, @user) result = remove_muted_categories(result, @user, exclude: options[:category]) diff --git a/spec/components/topic_query_spec.rb b/spec/components/topic_query_spec.rb index 44f219d4700..fad4faaea0d 100644 --- a/spec/components/topic_query_spec.rb +++ b/spec/components/topic_query_spec.rb @@ -294,8 +294,8 @@ describe TopicQuery do context 'user with auto_track_topics list_unread' do before do - user.auto_track_topics_after_msecs = 0 - user.save + user.user_option.auto_track_topics_after_msecs = 0 + user.user_option.save end it 'only contains the partially read topic' do @@ -360,8 +360,8 @@ describe TopicQuery do expect(topic_query.list_new.topics).to eq([new_topic]) - user.new_topic_duration_minutes = 5 - user.save + user.user_option.new_topic_duration_minutes = 5 + user.user_option.save new_topic.created_at = 10.minutes.ago new_topic.save expect(topic_query.list_new.topics).to eq([]) @@ -561,8 +561,8 @@ describe TopicQuery do let!(:fully_read_archived) { Fabricate(:post, user: creator).topic } before do - user.auto_track_topics_after_msecs = 0 - user.save + user.user_option.auto_track_topics_after_msecs = 0 + user.user_option.save TopicUser.update_last_read(user, partially_read.id, 0, 0) TopicUser.update_last_read(user, fully_read.id, 1, 0) TopicUser.update_last_read(user, fully_read_closed.id, 1, 0) diff --git a/spec/fabricators/user_option_fabricator.rb b/spec/fabricators/user_option_fabricator.rb new file mode 100644 index 00000000000..f42ddaec9c0 --- /dev/null +++ b/spec/fabricators/user_option_fabricator.rb @@ -0,0 +1,2 @@ +Fabricator(:user_option) do +end diff --git a/spec/models/topic_user_spec.rb b/spec/models/topic_user_spec.rb index 9f9f5651a84..f51b90b3352 100644 --- a/spec/models/topic_user_spec.rb +++ b/spec/models/topic_user_spec.rb @@ -48,7 +48,12 @@ describe TopicUser do let(:topic_creator_user) { TopicUser.get(topic, topic.user) } let(:post) { Fabricate(:post, topic: topic, user: user) } - let(:new_user) { Fabricate(:user, auto_track_topics_after_msecs: 1000) } + let(:new_user) { + u = Fabricate(:user) + u.user_option.update_columns(auto_track_topics_after_msecs: 1000) + u + } + let(:topic_new_user) { TopicUser.get(topic, new_user)} let(:yesterday) { DateTime.now.yesterday } @@ -68,15 +73,15 @@ describe TopicUser do describe 'notifications' do it 'should be set to tracking if auto_track_topics is enabled' do - user.update_column(:auto_track_topics_after_msecs, 0) + user.user_option.update_column(:auto_track_topics_after_msecs, 0) ensure_topic_user expect(TopicUser.get(topic, user).notification_level).to eq(TopicUser.notification_levels[:tracking]) end it 'should reset regular topics to tracking topics if auto track is changed' do ensure_topic_user - user.auto_track_topics_after_msecs = 0 - user.save + user.user_option.auto_track_topics_after_msecs = 0 + user.user_option.save expect(topic_user.notification_level).to eq(TopicUser.notification_levels[:tracking]) end diff --git a/spec/models/user_option_spec.rb b/spec/models/user_option_spec.rb new file mode 100644 index 00000000000..e3144989add --- /dev/null +++ b/spec/models/user_option_spec.rb @@ -0,0 +1,92 @@ +require 'rails_helper' +require_dependency 'user_option' + +describe UserOption do + + describe "should_be_redirected_to_top" do + let!(:user) { Fabricate(:user) } + + it "should be redirected to top when there is a reason to" do + user.user_option.expects(:redirected_to_top).returns({ reason: "42" }) + expect(user.user_option.should_be_redirected_to_top).to eq(true) + end + + it "should not be redirected to top when there is no reason to" do + user.user_option.expects(:redirected_to_top).returns(nil) + expect(user.user_option.should_be_redirected_to_top).to eq(false) + end + + end + + describe ".redirected_to_top" do + let!(:user) { Fabricate(:user) } + + it "should have no reason when `SiteSetting.redirect_users_to_top_page` is disabled" do + SiteSetting.expects(:redirect_users_to_top_page).returns(false) + expect(user.user_option.redirected_to_top).to eq(nil) + end + + context "when `SiteSetting.redirect_users_to_top_page` is enabled" do + before { SiteSetting.expects(:redirect_users_to_top_page).returns(true) } + + it "should have no reason when top is not in the `SiteSetting.top_menu`" do + SiteSetting.expects(:top_menu).returns("latest") + expect(user.user_option.redirected_to_top).to eq(nil) + end + + context "and when top is in the `SiteSetting.top_menu`" do + before { SiteSetting.expects(:top_menu).returns("latest|top") } + + it "should have no reason when there are not enough topics" do + SiteSetting.expects(:min_redirected_to_top_period).returns(nil) + expect(user.user_option.redirected_to_top).to eq(nil) + end + + context "and there are enough topics" do + + before { SiteSetting.expects(:min_redirected_to_top_period).returns(:monthly) } + + describe "a new user" do + before do + user.stubs(:trust_level).returns(0) + user.stubs(:last_seen_at).returns(5.minutes.ago) + end + + it "should have a reason for the first visit" do + expect(user.user_option.redirected_to_top).to eq({ + reason: I18n.t('redirected_to_top_reasons.new_user'), + period: :monthly + }) + end + + it "should not have a reason for next visits" do + user.user_option.expects(:last_redirected_to_top_at).returns(10.minutes.ago) + user.user_option.expects(:update_last_redirected_to_top!).never + + expect(user.user_option.redirected_to_top).to eq(nil) + end + end + + describe "an older user" do + before { user.stubs(:trust_level).returns(1) } + + it "should have a reason when the user hasn't been seen in a month" do + user.last_seen_at = 2.months.ago + user.user_option.expects(:update_last_redirected_to_top!).once + + expect(user.user_option.redirected_to_top).to eq({ + reason: I18n.t('redirected_to_top_reasons.not_seen_in_a_month'), + period: :monthly + }) + end + + end + + end + + end + + end + + end +end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 5acf8bfb6d0..dfbabbe0399 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -986,95 +986,6 @@ describe User do end end - describe "should_be_redirected_to_top" do - let!(:user) { Fabricate(:user) } - - it "should be redirected to top when there is a reason to" do - user.expects(:redirected_to_top).returns({ reason: "42" }) - expect(user.should_be_redirected_to_top).to eq(true) - end - - it "should not be redirected to top when there is no reason to" do - user.expects(:redirected_to_top).returns(nil) - expect(user.should_be_redirected_to_top).to eq(false) - end - - end - - describe ".redirected_to_top" do - let!(:user) { Fabricate(:user) } - - it "should have no reason when `SiteSetting.redirect_users_to_top_page` is disabled" do - SiteSetting.expects(:redirect_users_to_top_page).returns(false) - expect(user.redirected_to_top).to eq(nil) - end - - context "when `SiteSetting.redirect_users_to_top_page` is enabled" do - before { SiteSetting.expects(:redirect_users_to_top_page).returns(true) } - - it "should have no reason when top is not in the `SiteSetting.top_menu`" do - SiteSetting.expects(:top_menu).returns("latest") - expect(user.redirected_to_top).to eq(nil) - end - - context "and when top is in the `SiteSetting.top_menu`" do - before { SiteSetting.expects(:top_menu).returns("latest|top") } - - it "should have no reason when there are not enough topics" do - SiteSetting.expects(:min_redirected_to_top_period).returns(nil) - expect(user.redirected_to_top).to eq(nil) - end - - context "and there are enough topics" do - - before { SiteSetting.expects(:min_redirected_to_top_period).returns(:monthly) } - - describe "a new user" do - before do - user.stubs(:trust_level).returns(0) - user.stubs(:last_seen_at).returns(5.minutes.ago) - end - - it "should have a reason for the first visit" do - user.expects(:last_redirected_to_top_at).returns(nil) - user.expects(:update_last_redirected_to_top!).once - - expect(user.redirected_to_top).to eq({ - reason: I18n.t('redirected_to_top_reasons.new_user'), - period: :monthly - }) - end - - it "should not have a reason for next visits" do - user.expects(:last_redirected_to_top_at).returns(10.minutes.ago) - user.expects(:update_last_redirected_to_top!).never - - expect(user.redirected_to_top).to eq(nil) - end - end - - describe "an older user" do - before { user.stubs(:trust_level).returns(1) } - - it "should have a reason when the user hasn't been seen in a month" do - user.last_seen_at = 2.months.ago - user.expects(:update_last_redirected_to_top!).once - - expect(user.redirected_to_top).to eq({ - reason: I18n.t('redirected_to_top_reasons.not_seen_in_a_month'), - period: :monthly - }) - end - - end - - end - - end - - end - - end describe "automatic avatar creation" do it "sets a system avatar for new users" do @@ -1281,9 +1192,8 @@ describe User do expect(options.edit_history_public).to eq(true) expect(options.automatically_unpin_topics).to eq(false) expect(options.email_direct).to eq(false) - - expect(user.new_topic_duration_minutes).to eq(-1) - expect(user.auto_track_topics_after_msecs).to eq(0) + expect(options.new_topic_duration_minutes).to eq(-1) + expect(options.auto_track_topics_after_msecs).to eq(0) expect(CategoryUser.lookup(user, :watching).pluck(:category_id)).to eq([1]) expect(CategoryUser.lookup(user, :tracking).pluck(:category_id)).to eq([2]) diff --git a/spec/serializers/user_serializer_spec.rb b/spec/serializers/user_serializer_spec.rb index 3a9db1a231b..16320fdfb84 100644 --- a/spec/serializers/user_serializer_spec.rb +++ b/spec/serializers/user_serializer_spec.rb @@ -19,6 +19,8 @@ describe UserSerializer do it "serializes options correctly" do # so we serialize more stuff SiteSetting.edit_history_visible_to_public = false + SiteSetting.default_other_auto_track_topics_after_msecs = 0 + SiteSetting.default_other_new_topic_duration_minutes = 60*24 user = Fabricate.build(:user, user_profile: Fabricate.build(:user_profile), @@ -29,6 +31,8 @@ describe UserSerializer do json = UserSerializer.new(user, scope: Guardian.new(user), root: false).as_json expect(json[:user_option][:edit_history_public]).to eq(true) + expect(json[:user_option][:new_topic_duration_minutes]).to eq(60*24) + expect(json[:user_option][:auto_track_topics_after_msecs]).to eq(0) end end diff --git a/spec/services/user_updater_spec.rb b/spec/services/user_updater_spec.rb index f94d2bb9e75..7866206bfee 100644 --- a/spec/services/user_updater_spec.rb +++ b/spec/services/user_updater_spec.rb @@ -46,13 +46,18 @@ describe UserUpdater do updater.update(bio_raw: 'my new bio', email_always: 'true', mailing_list_mode: true, - digest_after_days: "8") + digest_after_days: "8", + new_topic_duration_minutes: 100, + auto_track_topics_after_msecs: 101 + ) user.reload expect(user.user_profile.bio_raw).to eq 'my new bio' expect(user.user_option.email_always).to eq true expect(user.user_option.mailing_list_mode).to eq true expect(user.user_option.digest_after_days).to eq 8 + expect(user.user_option.new_topic_duration_minutes).to eq 100 + expect(user.user_option.auto_track_topics_after_msecs).to eq 101 end context 'when update succeeds' do