From c04d4171ff9c8ad95f494af51ca7a35928c86011 Mon Sep 17 00:00:00 2001 From: Sam Date: Fri, 2 Dec 2016 17:03:31 +1100 Subject: [PATCH 1/4] FIX: whisper no longer experimental - Regular users are not notified of whispers - Regular users no longer have "stuck" topics in unread - Additional tracking for staff highest post number - Remove a bunch of unused columns in topics table --- app/controllers/application_controller.rb | 2 +- app/controllers/users_controller.rb | 2 +- app/models/post.rb | 2 +- app/models/post_action.rb | 6 +- app/models/topic.rb | 101 +++++++++++++++--- app/models/topic_tracking_state.rb | 30 ++++-- app/models/topic_user.rb | 16 ++- app/serializers/listable_topic_serializer.rb | 2 +- config/locales/server.en.yml | 2 +- ...202011139_add_whisper_support_to_topics.rb | 16 +++ lib/post_creator.rb | 7 +- lib/post_jobs_enqueuer.rb | 2 +- lib/topic_query.rb | 13 ++- lib/unread.rb | 10 +- spec/components/post_creator_spec.rb | 33 +++++- spec/components/topic_query_spec.rb | 32 ++++-- spec/components/unread_spec.rb | 82 +++++++++----- spec/fabricators/topic_user_fabricator.rb | 4 + spec/models/post_action_spec.rb | 24 ++--- spec/models/topic_spec.rb | 17 +++ spec/models/topic_tracking_state_spec.rb | 24 ++--- 21 files changed, 324 insertions(+), 103 deletions(-) create mode 100644 db/migrate/20161202011139_add_whisper_support_to_topics.rb create mode 100644 spec/fabricators/topic_user_fabricator.rb diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index a6a1300b514..795a46dfa4b 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -382,7 +382,7 @@ class ApplicationController < ActionController::Base def preload_current_user_data store_preloaded("currentUser", MultiJson.dump(CurrentUserSerializer.new(current_user, scope: guardian, root: false))) - report = TopicTrackingState.report(current_user.id) + report = TopicTrackingState.report(current_user) serializer = ActiveModel::ArraySerializer.new(report, each_serializer: TopicTrackingStateSerializer) store_preloaded("topicTrackingStates", MultiJson.dump(serializer)) end diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 0c226d93b29..d67c12455e1 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -152,7 +152,7 @@ class UsersController < ApplicationController user = fetch_user_from_params guardian.ensure_can_edit!(user) - report = TopicTrackingState.report(user.id) + report = TopicTrackingState.report(user) serializer = ActiveModel::ArraySerializer.new(report, each_serializer: TopicTrackingStateSerializer) render json: MultiJson.dump(serializer) diff --git a/app/models/post.rb b/app/models/post.rb index 84566d5a8a2..e262f259ba7 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -24,7 +24,7 @@ class Post < ActiveRecord::Base rate_limit :limit_posts_per_day belongs_to :user - belongs_to :topic, counter_cache: :posts_count + belongs_to :topic belongs_to :reply_to_user, class_name: "User" diff --git a/app/models/post_action.rb b/app/models/post_action.rb index 5cee56bdde6..5bc031ccae5 100644 --- a/app/models/post_action.rb +++ b/app/models/post_action.rb @@ -435,8 +435,10 @@ SQL post_action_type: post_action_type_key) end - topic_count = Post.where(topic_id: topic_id).sum(column) - Topic.where(id: topic_id).update_all ["#{column} = ?", topic_count] + if column == "like_count" + topic_count = Post.where(topic_id: topic_id).sum(column) + Topic.where(id: topic_id).update_all ["#{column} = ?", topic_count] + end if PostActionType.notify_flag_type_ids.include?(post_action_type_id) PostAction.update_flagged_posts_count diff --git a/app/models/topic.rb b/app/models/topic.rb index a8786ac781f..567f0d4e6b5 100644 --- a/app/models/topic.rb +++ b/app/models/topic.rb @@ -466,23 +466,103 @@ class Topic < ActiveRecord::Base end # Atomically creates the next post number - def self.next_post_number(topic_id, reply = false) + def self.next_post_number(topic_id, reply = false, whisper = false) highest = exec_sql("select coalesce(max(post_number),0) as max from posts where topic_id = ?", topic_id).first['max'].to_i - reply_sql = reply ? ", reply_count = reply_count + 1" : "" - result = exec_sql("UPDATE topics SET highest_post_number = ? + 1#{reply_sql} - WHERE id = ? RETURNING highest_post_number", highest, topic_id) - result.first['highest_post_number'].to_i + if whisper + + result = exec_sql("UPDATE topics + SET highest_staff_post_number = ? + 1 + WHERE id = ? + RETURNING highest_staff_post_number", highest, topic_id) + + result.first['highest_staff_post_number'].to_i + + else + + reply_sql = reply ? ", reply_count = reply_count + 1" : "" + + result = exec_sql("UPDATE topics + SET highest_staff_post_number = :highest + 1, + highest_post_number = :highest + 1#{reply_sql}, + posts_count = posts_count + 1 + WHERE id = :topic_id + RETURNING highest_post_number", highest: highest, topic_id: topic_id) + + result.first['highest_post_number'].to_i + end end + + def self.reset_all_highest! + exec_sql < 4 + GROUP BY topic_id +) +UPDATE topics +SET + highest_staff_post_number = X.highest_post_number, + highest_post_number = Y.highest_post_number, + last_posted_at = Y.last_posted_at, + posts_count = Y.posts_count +FROM X, Y +WHERE + X.topic_id = topics.id AND + Y.topic_id = topics.id AND ( + topics.highest_staff_post_number <> X.highest_post_number OR + topics.highest_post_number <> Y.highest_post_number OR + topics.last_posted_at <> Y.last_posted_at OR + topics.posts_count <> Y.posts_count + ) +SQL + end + + # If a post is deleted we have to update our highest post counters def self.reset_highest(topic_id) result = exec_sql "UPDATE topics - SET highest_post_number = (SELECT COALESCE(MAX(post_number), 0) FROM posts WHERE topic_id = :topic_id AND deleted_at IS NULL), - posts_count = (SELECT count(*) FROM posts WHERE deleted_at IS NULL AND topic_id = :topic_id), - last_posted_at = (SELECT MAX(created_at) FROM POSTS WHERE topic_id = :topic_id AND deleted_at IS NULL) + SET + highest_staff_post_number = ( + SELECT COALESCE(MAX(post_number), 0) FROM posts + WHERE topic_id = :topic_id AND + deleted_at IS NULL + ), + highest_post_number = ( + SELECT COALESCE(MAX(post_number), 0) FROM posts + WHERE topic_id = :topic_id AND + deleted_at IS NULL AND + post_type <> 4 + ), + posts_count = ( + SELECT count(*) FROM posts + WHERE deleted_at IS NULL AND + topic_id = :topic_id AND + post_type <> 4 + ), + + last_posted_at = ( + SELECT MAX(created_at) FROM posts + WHERE topic_id = :topic_id AND + deleted_at IS NULL AND + post_type <> 4 + ) WHERE id = :topic_id RETURNING highest_post_number", topic_id: topic_id + highest_post_number = result.first['highest_post_number'].to_i # Update the forum topic user records @@ -724,10 +804,7 @@ class Topic < ActiveRecord::Base end def update_action_counts - PostActionType.types.each_key do |type| - count_field = "#{type}_count" - update_column(count_field, Post.where(topic_id: id).sum(count_field)) - end + update_column(:like_count, Post.where(topic_id: id).sum(:like_count)) end def posters_summary(options = {}) diff --git a/app/models/topic_tracking_state.rb b/app/models/topic_tracking_state.rb index 97bd347aee7..b8a30b830d7 100644 --- a/app/models/topic_tracking_state.rb +++ b/app/models/topic_tracking_state.rb @@ -38,7 +38,7 @@ class TopicTrackingState publish_read(topic.id, 1, topic.user_id) end - def self.publish_latest(topic) + def self.publish_latest(topic, staff_only=false) return unless topic.archetype == "regular" message = { @@ -52,15 +52,25 @@ class TopicTrackingState } } - group_ids = topic.category && topic.category.secure_group_ids + group_ids = + if staff_only + [Group::AUTO_GROUPS[:staff]] + else + topic.category && topic.category.secure_group_ids + end MessageBus.publish("/latest", message.as_json, group_ids: group_ids) end def self.publish_unread(post) # TODO at high scale we are going to have to defer this, # perhaps cut down to users that are around in the last 7 days as well - # - group_ids = post.topic.category && post.topic.category.secure_group_ids + + group_ids = + if post.post_type == Post.types[:whisper] + [Group::AUTO_GROUPS[:staff]] + else + post.topic.category && post.topic.category.secure_group_ids + end TopicUser .tracking(post.topic_id) @@ -148,7 +158,7 @@ class TopicTrackingState ).where_values[0] end - def self.report(user_id, topic_id = nil) + def self.report(user, topic_id = nil) # Sam: this is a hairy report, in particular I need custom joins and fancy conditions # Dropping to sql_builder so I can make sense of it. @@ -160,12 +170,12 @@ class TopicTrackingState # cycles from usual requests # # - sql = report_raw_sql(topic_id: topic_id, skip_unread: true, skip_order: true) + sql = report_raw_sql(topic_id: topic_id, skip_unread: true, skip_order: true, staff: user.staff?) sql << "\nUNION ALL\n\n" - sql << report_raw_sql(topic_id: topic_id, skip_new: true, skip_order: true) + sql << report_raw_sql(topic_id: topic_id, skip_new: true, skip_order: true, staff: user.staff?) SqlBuilder.new(sql) - .map_exec(TopicTrackingState, user_id: user_id, topic_id: topic_id) + .map_exec(TopicTrackingState, user_id: user.id, topic_id: topic_id) end @@ -176,7 +186,7 @@ class TopicTrackingState if opts && opts[:skip_unread] "1=0" else - TopicQuery.unread_filter(Topic).where_values.join(" AND ") + TopicQuery.unread_filter(Topic, staff: opts && opts[:staff]).where_values.join(" AND ") end new = @@ -190,7 +200,7 @@ class TopicTrackingState u.id AS user_id, topics.id AS topic_id, topics.created_at, - highest_post_number, + #{opts && opts[:staff] ? "highest_staff_post_number highest_post_number" : "highest_post_number"}, last_read_post_number, c.id AS category_id, tu.notification_level" diff --git a/app/models/topic_user.rb b/app/models/topic_user.rb index 9738083ba7a..f4369f56988 100644 --- a/app/models/topic_user.rb +++ b/app/models/topic_user.rb @@ -236,6 +236,8 @@ SQL topic_users.notification_level, tu.notification_level old_level, tu.last_read_post_number " + UPDATE_TOPIC_USER_SQL_STAFF = UPDATE_TOPIC_USER_SQL.gsub("highest_post_number", "highest_staff_post_number") + INSERT_TOPIC_USER_SQL = "INSERT INTO topic_users (user_id, topic_id, last_read_post_number, highest_seen_post_number, last_visited_at, first_visited_at, notification_level) SELECT :user_id, :topic_id, :post_number, ft.highest_post_number, :now, :now, :new_status FROM topics AS ft @@ -245,6 +247,8 @@ SQL FROM topic_users AS ftu WHERE ftu.user_id = :user_id and ftu.topic_id = :topic_id)" + INSERT_TOPIC_USER_SQL_STAFF = INSERT_TOPIC_USER_SQL.gsub("highest_post_number", "highest_staff_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 @@ -265,7 +269,11 @@ SQL # ... user visited the topic but did not read the posts # # 86400000 = 1 day - rows = exec_sql(UPDATE_TOPIC_USER_SQL,args).values + rows = if user.staff? + exec_sql(UPDATE_TOPIC_USER_SQL_STAFF,args).values + else + exec_sql(UPDATE_TOPIC_USER_SQL,args).values + end if rows.length == 1 before = rows[0][1].to_i @@ -295,7 +303,11 @@ SQL user.update_posts_read!(post_number, mobile: opts[:mobile]) begin - exec_sql(INSERT_TOPIC_USER_SQL, args) + if user.staff? + exec_sql(INSERT_TOPIC_USER_SQL_STAFF, args) + else + exec_sql(INSERT_TOPIC_USER_SQL, args) + end rescue PG::UniqueViolation # if record is inserted between two statements this can happen # we retry once to avoid failing the req diff --git a/app/serializers/listable_topic_serializer.rb b/app/serializers/listable_topic_serializer.rb index 3589d7a2312..e8cd1a7f151 100644 --- a/app/serializers/listable_topic_serializer.rb +++ b/app/serializers/listable_topic_serializer.rb @@ -109,7 +109,7 @@ class ListableTopicSerializer < BasicTopicSerializer protected def unread_helper - @unread_helper ||= Unread.new(object, object.user_data) + @unread_helper ||= Unread.new(object, object.user_data, scope) end end diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index bf472741990..896e76aa2a9 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -967,7 +967,7 @@ en: email_token_grace_period_hours: "Forgot password / activate account tokens are still valid for a grace period of (n) hours after being redeemed." enable_badges: "Enable the badge system" - enable_whispers: "Allow staff private communication within topic. (experimental)" + enable_whispers: "Allow staff private communication within topics." allow_index_in_robots_txt: "Specify in robots.txt that this site is allowed to be indexed by web search engines." email_domains_blacklist: "A pipe-delimited list of email domains that users are not allowed to register accounts with. Example: mailinator.com|trashmail.net" diff --git a/db/migrate/20161202011139_add_whisper_support_to_topics.rb b/db/migrate/20161202011139_add_whisper_support_to_topics.rb new file mode 100644 index 00000000000..0e065889e8a --- /dev/null +++ b/db/migrate/20161202011139_add_whisper_support_to_topics.rb @@ -0,0 +1,16 @@ +class AddWhisperSupportToTopics < ActiveRecord::Migration + def up + remove_column :topics, :bookmark_count + remove_column :topics, :off_topic_count + remove_column :topics, :illegal_count + remove_column :topics, :inappropriate_count + remove_column :topics, :notify_user_count + + add_column :topics, :highest_staff_post_number, :int, default: 0, null: false + execute "UPDATE topics SET highest_staff_post_number = highest_post_number" + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/lib/post_creator.rb b/lib/post_creator.rb index 0cd26aa33d9..9b4fb345dd8 100644 --- a/lib/post_creator.rb +++ b/lib/post_creator.rb @@ -146,6 +146,9 @@ class PostCreator end if @post && errors.blank? + # update counters etc. + @post.topic.reload + publish track_latest_on_category @@ -199,7 +202,9 @@ class PostCreator set_reply_info(post) post.word_count = post.raw.scan(/[[:word:]]+/).size - post.post_number ||= Topic.next_post_number(post.topic_id, post.reply_to_post_number.present?) + + whisper = post.post_type == Post.types[:whisper] + post.post_number ||= Topic.next_post_number(post.topic_id, post.reply_to_post_number.present?, whisper) cooking_options = post.cooking_options || {} cooking_options[:topic_id] = post.topic_id diff --git a/lib/post_jobs_enqueuer.rb b/lib/post_jobs_enqueuer.rb index c5f1f6a9d15..0e05c565f9f 100644 --- a/lib/post_jobs_enqueuer.rb +++ b/lib/post_jobs_enqueuer.rb @@ -35,7 +35,7 @@ class PostJobsEnqueuer def after_post_create TopicTrackingState.publish_unread(@post) if @post.post_number > 1 - TopicTrackingState.publish_latest(@topic) + TopicTrackingState.publish_latest(@topic, @post.post_type == Post.types[:whisper]) Jobs.enqueue_in( SiteSetting.email_time_window_mins.minutes, diff --git a/lib/topic_query.rb b/lib/topic_query.rb index f13cbbd8bcb..3da879c6a32 100644 --- a/lib/topic_query.rb +++ b/lib/topic_query.rb @@ -242,9 +242,12 @@ class TopicQuery .where("COALESCE(tu.notification_level, :tracking) >= :tracking", tracking: TopicUser.notification_levels[:tracking]) end - def self.unread_filter(list) - list.where("tu.last_read_post_number < topics.highest_post_number") - .where("COALESCE(tu.notification_level, :regular) >= :tracking", regular: TopicUser.notification_levels[:regular], tracking: TopicUser.notification_levels[:tracking]) + def self.unread_filter(list, opts) + col_name = opts[:staff] ? "highest_staff_post_number" : "highest_post_number" + + list.where("tu.last_read_post_number < topics.#{col_name}") + .where("COALESCE(tu.notification_level, :regular) >= :tracking", + regular: TopicUser.notification_levels[:regular], tracking: TopicUser.notification_levels[:tracking]) end def prioritize_pinned_topics(topics, options) @@ -320,7 +323,7 @@ class TopicQuery end def unread_results(options={}) - result = TopicQuery.unread_filter(default_results(options.reverse_merge(:unordered => true))) + result = TopicQuery.unread_filter(default_results(options.reverse_merge(:unordered => true)), staff: @user.try(:staff?)) .order('CASE WHEN topics.user_id = tu.user_id THEN 1 ELSE 2 END') self.class.results_filter_callbacks.each do |filter_callback| @@ -656,7 +659,7 @@ class TopicQuery end def unread_messages(params) - TopicQuery.unread_filter(messages_for_groups_or_user(params[:my_group_ids])) + TopicQuery.unread_filter(messages_for_groups_or_user(params[:my_group_ids]), staff: @user.try(:staff?)) .limit(params[:count]) end diff --git a/lib/unread.rb b/lib/unread.rb index a5a062d319a..f04ffcd2289 100644 --- a/lib/unread.rb +++ b/lib/unread.rb @@ -2,7 +2,8 @@ class Unread # This module helps us calculate unread and new post counts - def initialize(topic, topic_user) + def initialize(topic, topic_user, guardian) + @guardian = guardian @topic = topic @topic_user = topic_user end @@ -18,9 +19,12 @@ class Unread def new_posts return 0 if @topic_user.highest_seen_post_number.blank? return 0 if do_not_notify?(@topic_user.notification_level) - return 0 if (@topic_user.last_read_post_number||0) > @topic.highest_post_number - new_posts = (@topic.highest_post_number - @topic_user.highest_seen_post_number) + highest_post_number = @guardian.is_staff? ? @topic.highest_staff_post_number : @topic.highest_post_number + + return 0 if (@topic_user.last_read_post_number||0) > highest_post_number + + new_posts = (highest_post_number - @topic_user.highest_seen_post_number) new_posts = 0 if new_posts < 0 return new_posts end diff --git a/spec/components/post_creator_spec.rb b/spec/components/post_creator_spec.rb index ab1bd548663..a428bb03b81 100644 --- a/spec/components/post_creator_spec.rb +++ b/spec/components/post_creator_spec.rb @@ -334,7 +334,12 @@ describe PostCreator do context 'whisper' do let!(:topic) { Fabricate(:topic, user: user) } - it 'forces replies to whispers to be whispers' do + it 'whispers do not mess up the public view' do + + first = PostCreator.new(user, + topic_id: topic.id, + raw: 'this is the first post').create + whisper = PostCreator.new(user, topic_id: topic.id, reply_to_post_number: 1, @@ -344,6 +349,7 @@ describe PostCreator do expect(whisper).to be_present expect(whisper.post_type).to eq(Post.types[:whisper]) + whisper_reply = PostCreator.new(user, topic_id: topic.id, reply_to_post_number: whisper.post_number, @@ -352,6 +358,29 @@ describe PostCreator do expect(whisper_reply).to be_present expect(whisper_reply.post_type).to eq(Post.types[:whisper]) + + + first.reload + # does not leak into the OP + expect(first.reply_count).to eq(0) + + topic.reload + + # cause whispers should not muck up that number + expect(topic.highest_post_number).to eq(1) + expect(topic.reply_count).to eq(0) + expect(topic.posts_count).to eq(1) + expect(topic.highest_staff_post_number).to eq(3) + + topic.update_columns(highest_staff_post_number:0, highest_post_number:0, posts_count: 0, last_posted_at: 1.year.ago) + + Topic.reset_highest(topic.id) + + topic.reload + expect(topic.highest_post_number).to eq(1) + expect(topic.posts_count).to eq(1) + expect(topic.last_posted_at).to eq(first.created_at) + expect(topic.highest_staff_post_number).to eq(3) end end @@ -624,6 +653,8 @@ describe PostCreator do _post2 = create_post(user: post1.user, topic_id: post1.topic_id) post1.topic.reload + + expect(post1.topic.posts_count).to eq(3) expect(post1.topic.closed).to eq(true) end end diff --git a/spec/components/topic_query_spec.rb b/spec/components/topic_query_spec.rb index c50319ff27d..619a080aaec 100644 --- a/spec/components/topic_query_spec.rb +++ b/spec/components/topic_query_spec.rb @@ -409,6 +409,29 @@ describe TopicQuery do end end + context 'with whispers' do + + it 'correctly shows up in unread for staff' do + + first = create_post(raw: 'this is the first post', title: 'super amazing title') + + _whisper = create_post(topic_id: first.topic.id, + post_type: Post.types[:whisper], + raw: 'this is a whispered reply') + + topic_id = first.topic.id + + TopicUser.update_last_read(user, topic_id, first.post_number, 1) + TopicUser.update_last_read(admin, topic_id, first.post_number, 1) + + TopicUser.change(user.id, topic_id, notification_level: TopicUser.notification_levels[:tracking]) + TopicUser.change(admin.id, topic_id, notification_level: TopicUser.notification_levels[:tracking]) + + expect(TopicQuery.new(user).list_unread.topics).to eq([]) + expect(TopicQuery.new(admin).list_unread.topics).to eq([first.topic]) + end + end + context 'with read data' do let!(:partially_read) { Fabricate(:post, user: creator).topic } let!(:fully_read) { Fabricate(:post, user: creator).topic } @@ -419,8 +442,9 @@ describe TopicQuery do end context 'list_unread' do - it 'contains no topics' do + it 'lists topics correctly' do expect(topic_query.list_unread.topics).to eq([]) + expect(topic_query.list_read.topics).to match_array([fully_read, partially_read]) end end @@ -435,11 +459,6 @@ describe TopicQuery do end end - context 'list_read' do - it 'contain both topics ' do - expect(topic_query.list_read.topics).to match_array([fully_read, partially_read]) - end - end end end @@ -630,7 +649,6 @@ describe TopicQuery do related_by_group_pm = create_pm(sender, target_group_names: [group_with_user.name]) read(user, related_by_group_pm, 1) - expect(TopicQuery.new(user).list_suggested_for(pm_to_group).topics.map(&:id)).to( eq([related_by_group_pm.id, related_by_user_pm.id, pm_to_user.id]) ) diff --git a/spec/components/unread_spec.rb b/spec/components/unread_spec.rb index cfbf4570537..21e4b28b597 100644 --- a/spec/components/unread_spec.rb +++ b/spec/components/unread_spec.rb @@ -3,62 +3,86 @@ require 'unread' describe Unread do - - before do - @topic = Fabricate(:topic, posts_count: 13, highest_post_number: 13) - @topic.notifier.watch_topic!(@topic.user_id) - @topic_user = TopicUser.get(@topic, @topic.user) - @topic_user.stubs(:notification_level).returns(TopicUser.notification_levels[:tracking]) - @topic_user.notification_level = TopicUser.notification_levels[:tracking] - @unread = Unread.new(@topic, @topic_user) + let (:user) { Fabricate.build(:user, id: 1) } + let (:topic) do + Fabricate.build(:topic, + posts_count: 13, + highest_staff_post_number: 15, + highest_post_number: 13, + id: 1) end + let (:topic_user) do + Fabricate.build(:topic_user, + notification_level: TopicUser.notification_levels[:tracking], + topic_id: topic.id, + user_id: user.id) + end + + def unread + Unread.new(topic, topic_user, Guardian.new(user)) + end + + describe 'staff counts' do + it 'shoule correctly return based on staff post number' do + + user.admin = true + + topic_user.last_read_post_number = 13 + topic_user.highest_seen_post_number = 13 + + expect(unread.unread_posts).to eq(0) + expect(unread.new_posts).to eq(2) + end + end + + describe 'unread_posts' do it 'should have 0 unread posts if the user has seen all posts' do - @topic_user.stubs(:last_read_post_number).returns(13) - @topic_user.stubs(:highest_seen_post_number).returns(13) - expect(@unread.unread_posts).to eq(0) + topic_user.last_read_post_number = 13 + topic_user.highest_seen_post_number = 13 + expect(unread.unread_posts).to eq(0) end it 'should have 6 unread posts if the user has seen all but 6 posts' do - @topic_user.stubs(:last_read_post_number).returns(5) - @topic_user.stubs(:highest_seen_post_number).returns(11) - expect(@unread.unread_posts).to eq(6) + topic_user.last_read_post_number = 5 + topic_user.highest_seen_post_number = 11 + expect(unread.unread_posts).to eq(6) end it 'should have 0 unread posts if the user has seen more posts than exist (deleted)' do - @topic_user.stubs(:last_read_post_number).returns(100) - @topic_user.stubs(:highest_seen_post_number).returns(13) - expect(@unread.unread_posts).to eq(0) + topic_user.last_read_post_number = 100 + topic_user.highest_seen_post_number = 13 + expect(unread.unread_posts).to eq(0) end end describe 'new_posts' do it 'should have 0 new posts if the user has read all posts' do - @topic_user.stubs(:last_read_post_number).returns(13) - expect(@unread.new_posts).to eq(0) + topic_user.last_read_post_number = 13 + expect(unread.new_posts).to eq(0) end it 'returns 0 when the topic is the same length as when you last saw it' do - @topic_user.stubs(:highest_seen_post_number).returns(13) - expect(@unread.new_posts).to eq(0) + topic_user.highest_seen_post_number = 13 + expect(unread.new_posts).to eq(0) end it 'has 3 new posts if the user has read 10 posts' do - @topic_user.stubs(:highest_seen_post_number).returns(10) - expect(@unread.new_posts).to eq(3) + topic_user.highest_seen_post_number = 10 + expect(unread.new_posts).to eq(3) end it 'has 0 new posts if the user has read 10 posts but is not tracking' do - @topic_user.stubs(:highest_seen_post_number).returns(10) - @topic_user.stubs(:notification_level).returns(TopicUser.notification_levels[:regular]) - expect(@unread.new_posts).to eq(0) + topic_user.highest_seen_post_number = 10 + topic_user.notification_level = TopicUser.notification_levels[:regular] + expect(unread.new_posts).to eq(0) end it 'has 0 new posts if the user read more posts than exist (deleted)' do - @topic_user.stubs(:highest_seen_post_number).returns(16) - expect(@unread.new_posts).to eq(0) + topic_user.highest_seen_post_number = 16 + expect(unread.new_posts).to eq(0) end - end + end diff --git a/spec/fabricators/topic_user_fabricator.rb b/spec/fabricators/topic_user_fabricator.rb new file mode 100644 index 00000000000..b299806f707 --- /dev/null +++ b/spec/fabricators/topic_user_fabricator.rb @@ -0,0 +1,4 @@ +Fabricator(:topic_user) do + user + topic +end diff --git a/spec/models/post_action_spec.rb b/spec/models/post_action_spec.rb index 9321dedd81e..fb621269dfb 100644 --- a/spec/models/post_action_spec.rb +++ b/spec/models/post_action_spec.rb @@ -199,10 +199,6 @@ describe PostAction do expect { bookmark.save; post.reload }.to change(post, :bookmark_count).by(1) end - it "increases the forum topic's bookmark count when saved" do - expect { bookmark.save; post.topic.reload }.to change(post.topic, :bookmark_count).by(1) - end - describe 'when deleted' do before do @@ -218,9 +214,6 @@ describe PostAction do expect { post.reload }.to change(post, :bookmark_count).by(-1) end - it 'reduces the bookmark count of the forum topic' do - expect { @topic.reload }.to change(post.topic, :bookmark_count).by(-1) - end end end @@ -291,19 +284,24 @@ describe PostAction do end end - describe 'when a user votes for something' do - it 'should increase the vote counts when a user votes' do + describe 'when a user likes something' do + it 'should increase the like counts when a user votes' do expect { - PostAction.act(codinghorror, post, PostActionType.types[:vote]) + PostAction.act(codinghorror, post, PostActionType.types[:like]) post.reload - }.to change(post, :vote_count).by(1) + }.to change(post, :like_count).by(1) end it 'should increase the forum topic vote count when a user votes' do expect { - PostAction.act(codinghorror, post, PostActionType.types[:vote]) + PostAction.act(codinghorror, post, PostActionType.types[:like]) post.topic.reload - }.to change(post.topic, :vote_count).by(1) + }.to change(post.topic, :like_count).by(1) + + expect { + PostAction.remove_act(codinghorror, post, PostActionType.types[:like]) + post.topic.reload + }.to change(post.topic, :like_count).by(-1) end end diff --git a/spec/models/topic_spec.rb b/spec/models/topic_spec.rb index 6b84eff2e65..f2ac723e6ec 100644 --- a/spec/models/topic_spec.rb +++ b/spec/models/topic_spec.rb @@ -1724,4 +1724,21 @@ describe Topic do expect(@topic_status_event_triggered).to eq(true) end + + + it 'allows users to normalize counts' do + + topic = Fabricate(:topic, last_posted_at: 1.year.ago) + post1 = Fabricate(:post, topic: topic, post_number: 1) + post2 = Fabricate(:post, topic: topic, post_type: Post.types[:whisper], post_number: 2) + + Topic.reset_all_highest! + topic.reload + + expect(topic.posts_count).to eq(1) + expect(topic.highest_post_number).to eq(post1.post_number) + expect(topic.highest_staff_post_number).to eq(post2.post_number) + expect(topic.last_posted_at).to be_within(1.second).of (post1.created_at) + end + end diff --git a/spec/models/topic_tracking_state_spec.rb b/spec/models/topic_tracking_state_spec.rb index 5410f69eed4..cd51c5ff765 100644 --- a/spec/models/topic_tracking_state_spec.rb +++ b/spec/models/topic_tracking_state_spec.rb @@ -20,7 +20,7 @@ describe TopicTrackingState do user = Fabricate(:user) post - report = TopicTrackingState.report(user.id) + report = TopicTrackingState.report(user) expect(report.length).to eq(1) CategoryUser.create!(user_id: user.id, @@ -30,12 +30,12 @@ describe TopicTrackingState do create_post(topic_id: post.topic_id) - report = TopicTrackingState.report(user.id) + report = TopicTrackingState.report(user) expect(report.length).to eq(0) TopicUser.create!(user_id: user.id, topic_id: post.topic_id, last_read_post_number: 1, notification_level: 3) - report = TopicTrackingState.report(user.id) + report = TopicTrackingState.report(user) expect(report.length).to eq(1) end @@ -62,18 +62,18 @@ describe TopicTrackingState do TopicUser.change(user.id, post2.topic_id, tracking) TopicUser.change(user.id, post3.topic_id, tracking) - report = TopicTrackingState.report(user.id) + report = TopicTrackingState.report(user) expect(report.length).to eq(3) end it "correctly gets the tracking state" do - report = TopicTrackingState.report(user.id) + report = TopicTrackingState.report(user) expect(report.length).to eq(0) post.topic.notifier.watch_topic!(post.topic.user_id) - report = TopicTrackingState.report(user.id) + report = TopicTrackingState.report(user) expect(report.length).to eq(1) row = report[0] @@ -84,18 +84,18 @@ describe TopicTrackingState do expect(row.user_id).to eq(user.id) # lets not leak out random users - expect(TopicTrackingState.report(post.user_id)).to be_empty + expect(TopicTrackingState.report(post.user)).to be_empty # lets not return anything if we scope on non-existing topic - expect(TopicTrackingState.report(user.id, post.topic_id + 1)).to be_empty + expect(TopicTrackingState.report(user, post.topic_id + 1)).to be_empty # when we reply the poster should have an unread row create_post(user: user, topic: post.topic) - report = TopicTrackingState.report(user.id) + report = TopicTrackingState.report(user) expect(report.length).to eq(0) - report = TopicTrackingState.report(post.user_id) + report = TopicTrackingState.report(post.user) expect(report.length).to eq(1) row = report[0] @@ -111,7 +111,7 @@ describe TopicTrackingState do post.topic.category_id = category.id post.topic.save - expect(TopicTrackingState.report(post.user_id)).to be_empty - expect(TopicTrackingState.report(user.id)).to be_empty + expect(TopicTrackingState.report(post.user)).to be_empty + expect(TopicTrackingState.report(user)).to be_empty end end From 2a19e8f2390d0d67cb5e8352a6e44013916d1062 Mon Sep 17 00:00:00 2001 From: Sam Date: Fri, 2 Dec 2016 17:42:27 +1100 Subject: [PATCH 2/4] UX: improve topic composition on mobile - tighten up space used for composer body - stop collapsing and expanding so much --- .../discourse/components/composer-body.js.es6 | 7 +++ .../discourse/lib/safari-hacks.js.es6 | 56 ++++++++++++++----- 2 files changed, 48 insertions(+), 15 deletions(-) diff --git a/app/assets/javascripts/discourse/components/composer-body.js.es6 b/app/assets/javascripts/discourse/components/composer-body.js.es6 index ec3bc602cd6..faee0ef401e 100644 --- a/app/assets/javascripts/discourse/components/composer-body.js.es6 +++ b/app/assets/javascripts/discourse/components/composer-body.js.es6 @@ -76,6 +76,13 @@ export default Ember.Component.extend({ } }, + @observes('composeState') + disableFullscreen() { + if (this.get('composeState') !== Composer.OPEN) { + positioningWorkaround.blur(); + } + }, + didInsertElement() { this._super(); const $replyControl = $('#reply-control'); diff --git a/app/assets/javascripts/discourse/lib/safari-hacks.js.es6 b/app/assets/javascripts/discourse/lib/safari-hacks.js.es6 index 1aa215f534f..03b6dfff3a1 100644 --- a/app/assets/javascripts/discourse/lib/safari-hacks.js.es6 +++ b/app/assets/javascripts/discourse/lib/safari-hacks.js.es6 @@ -7,6 +7,8 @@ function applicable() { } let workaroundActive = false; +let composingTopic = false; + export function isWorkaroundActive() { return workaroundActive; } @@ -22,27 +24,38 @@ function positioningWorkaround($fixedElement) { var done = false; var originalScrollTop = 0; + positioningWorkaround.blur = function(evt) { + if (workaroundActive) { + done = true; + + $('#main-outlet').show(); + $('header').show(); + + fixedElement.style.position = ''; + fixedElement.style.top = ''; + fixedElement.style.height = ''; + + $(window).scrollTop(originalScrollTop); + + if (evt) { + evt.target.removeEventListener('blur', blurred); + } + workaroundActive = false; + } + }; + var blurredNow = function(evt) { if (!done && _.include($(document.activeElement).parents(), fixedElement)) { // something in focus so skip return; } - done = true; - - $('#main-outlet').show(); - $('header').show(); - - fixedElement.style.position = ''; - fixedElement.style.top = ''; - fixedElement.style.height = ''; - - $(window).scrollTop(originalScrollTop); - - if (evt) { - evt.target.removeEventListener('blur', blurred); + if (composingTopic) { + return false; } - workaroundActive = false; + + positioningWorkaround.blur(evt); + }; var blurred = _.debounce(blurredNow, 250); @@ -73,7 +86,20 @@ function positioningWorkaround($fixedElement) { fixedElement.style.top = '0px'; - const height = Math.max(parseInt(window.innerHeight*0.6), 350); + let ratio = 0.6; + let min = 350; + + composingTopic = false; + + if ($('#reply-control select.category-combobox').length > 0) { + composingTopic = true; + // creating a topic, less height + ratio = 0.54; + min = 300; + } + + const height = Math.max(parseInt(window.innerHeight*ratio), min); + fixedElement.style.height = height + "px"; // I used to do this, but it seems like we don't need to with position From 2793e7bebcba541dfee218ba37719804373bc41c Mon Sep 17 00:00:00 2001 From: Sam Date: Fri, 2 Dec 2016 17:56:01 +1100 Subject: [PATCH 3/4] UX: have webkit safari mobile stop with inner shadows http://stackoverflow.com/questions/3062968/remove-textarea-inner-shadow-on-mobile-safari-iphone --- app/assets/stylesheets/common/foundation/base.scss | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app/assets/stylesheets/common/foundation/base.scss b/app/assets/stylesheets/common/foundation/base.scss index b4267e703fe..9ba9ee42950 100644 --- a/app/assets/stylesheets/common/foundation/base.scss +++ b/app/assets/stylesheets/common/foundation/base.scss @@ -98,3 +98,7 @@ pre code { #offscreen-content { display: none; } + +input { + -webkit-appearance: none; +} From f57feb2a4b3607df4f694db6ce1d7f1ea55e5448 Mon Sep 17 00:00:00 2001 From: Sam Date: Fri, 2 Dec 2016 18:19:39 +1100 Subject: [PATCH 4/4] regression composing topics on desktop caught by smoke test --- .../javascripts/discourse/components/composer-body.js.es6 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/assets/javascripts/discourse/components/composer-body.js.es6 b/app/assets/javascripts/discourse/components/composer-body.js.es6 index faee0ef401e..d3c1e96d2eb 100644 --- a/app/assets/javascripts/discourse/components/composer-body.js.es6 +++ b/app/assets/javascripts/discourse/components/composer-body.js.es6 @@ -78,7 +78,7 @@ export default Ember.Component.extend({ @observes('composeState') disableFullscreen() { - if (this.get('composeState') !== Composer.OPEN) { + if (this.get('composeState') !== Composer.OPEN && positioningWorkaround.blur) { positioningWorkaround.blur(); } },