From 0f0048e8e33df01a58e192ad95ee5d5d89f33aa8 Mon Sep 17 00:00:00 2001 From: Alan Guo Xiang Tan Date: Thu, 1 Sep 2022 02:15:01 +0800 Subject: [PATCH] DEV: Enable new user menu when experimental sidebar hamburger is enabled (#18133) When `enable_experimental_sidebar_hamburger` site setting is enabled, we will switch to rendering the new user menu. --- app/jobs/regular/notify_reviewable.rb | 52 +++-- app/models/user.rb | 18 +- app/serializers/current_user_serializer.rb | 5 +- spec/jobs/notify_reviewable_spec.rb | 258 ++++++++++++--------- spec/models/user_spec.rb | 10 +- 5 files changed, 188 insertions(+), 155 deletions(-) diff --git a/app/jobs/regular/notify_reviewable.rb b/app/jobs/regular/notify_reviewable.rb index 8d0c4f7c273..7709ca8ca3e 100644 --- a/app/jobs/regular/notify_reviewable.rb +++ b/app/jobs/regular/notify_reviewable.rb @@ -31,35 +31,37 @@ class Jobs::NotifyReviewable < ::Jobs::Base counts[r.reviewable_by_group_id] += 1 if r.reviewable_by_group_id end - redesigned_menu_enabled_user_ids = User.redesigned_user_menu_enabled_user_ids - - new_menu_admins = User.real.admins.where(id: redesigned_menu_enabled_user_ids) - notify_users(new_menu_admins, all_updates[:admins]) - - legacy_menu_admins = User.real.admins.where("id NOT IN (?)", @contacted).pluck(:id) - notify_legacy( - legacy_menu_admins, - count: counts[:admins], - updates: all_updates[:admins], - ) + if SiteSetting.enable_experimental_sidebar_hamburger + notify_users( + User.real.admins, + all_updates[:admins] + ) + else + notify_legacy( + User.real.admins.pluck(:id), + count: counts[:admins], + updates: all_updates[:admins], + ) + end if reviewable.reviewable_by_moderator? - new_menu_mods = User - .real - .moderators - .where("id IN (?)", redesigned_menu_enabled_user_ids - @contacted.to_a) - notify_users(new_menu_mods, all_updates[:moderators]) - - legacy_menu_mods = User.real.moderators.where("id NOT IN (?)", @contacted).pluck(:id) - notify_legacy( - legacy_menu_mods, - count: counts[:moderators], - updates: all_updates[:moderators], - ) + if SiteSetting.enable_experimental_sidebar_hamburger + notify_users( + User.real.moderators.where("id NOT IN (?)", @contacted), + all_updates[:moderators] + ) + else + notify_legacy( + User.real.moderators.where("id NOT IN (?)", @contacted).pluck(:id), + count: counts[:moderators], + updates: all_updates[:moderators], + ) + end end if SiteSetting.enable_category_group_moderation? && (group = reviewable.reviewable_by_group) users = group.users.includes(:group_users).where("users.id NOT IN (?)", @contacted) + users.find_each do |user| count = 0 updates = {} @@ -67,12 +69,14 @@ class Jobs::NotifyReviewable < ::Jobs::Base updates.merge!(all_updates[gu.group_id]) count += counts[gu.group_id] end - if redesigned_menu_enabled_user_ids.include?(user.id) + + if SiteSetting.enable_experimental_sidebar_hamburger notify_user(user, updates) else notify_legacy([user.id], count: count, updates: updates) end end + @contacted += users.pluck(:id) end end diff --git a/app/models/user.rb b/app/models/user.rb index 94f691cb3ba..62815394345 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1639,24 +1639,8 @@ class User < ActiveRecord::Base user_status && !user_status.expired? end - REDESIGN_USER_MENU_REDIS_KEY_PREFIX = "redesigned_user_menu_for_user_" - - def self.redesigned_user_menu_enabled_user_ids - Discourse.redis.scan_each(match: "#{REDESIGN_USER_MENU_REDIS_KEY_PREFIX}*").map do |key| - key.sub(REDESIGN_USER_MENU_REDIS_KEY_PREFIX, "").to_i - end - end - def redesigned_user_menu_enabled? - Discourse.redis.get("#{REDESIGN_USER_MENU_REDIS_KEY_PREFIX}#{self.id}") == "1" - end - - def enable_redesigned_user_menu - Discourse.redis.setex("#{REDESIGN_USER_MENU_REDIS_KEY_PREFIX}#{self.id}", 6.months, "1") - end - - def disable_redesigned_user_menu - Discourse.redis.del("#{REDESIGN_USER_MENU_REDIS_KEY_PREFIX}#{self.id}") + SiteSetting.enable_experimental_sidebar_hamburger end def sidebar_categories_ids diff --git a/app/serializers/current_user_serializer.rb b/app/serializers/current_user_serializer.rb index 5c891273d19..64597982ae5 100644 --- a/app/serializers/current_user_serializer.rb +++ b/app/serializers/current_user_serializer.rb @@ -324,10 +324,7 @@ class CurrentUserSerializer < BasicUserSerializer end def redesigned_user_menu_enabled - if defined?(@redesigned_user_menu_enabled) - return @redesigned_user_menu_enabled - end - @redesigned_user_menu_enabled = object.redesigned_user_menu_enabled? + object.redesigned_user_menu_enabled? end def likes_notifications_disabled diff --git a/spec/jobs/notify_reviewable_spec.rb b/spec/jobs/notify_reviewable_spec.rb index 3ade8a22826..dd88982f406 100644 --- a/spec/jobs/notify_reviewable_spec.rb +++ b/spec/jobs/notify_reviewable_spec.rb @@ -4,133 +4,167 @@ RSpec.describe Jobs::NotifyReviewable do # remove all the legacy stuff here when redesigned_user_menu_enabled is # removed describe '#execute' do - fab!(:legacy_menu_admin) { Fabricate(:admin, moderator: true) } - fab!(:legacy_menu_mod) { Fabricate(:moderator) } + fab!(:admin) { Fabricate(:admin, moderator: true) } + fab!(:moderator) { Fabricate(:moderator) } fab!(:group_user) { Fabricate(:group_user) } - fab!(:legacy_menu_user) { group_user.user } - fab!(:group) { group_user.group } + fab!(:user) { group_user.user } - fab!(:new_menu_admin) { Fabricate(:admin, moderator: true) } - fab!(:new_menu_mod) { Fabricate(:moderator) } - fab!(:new_menu_user) { Fabricate(:user, groups: [group]) } - - before do - [new_menu_admin, new_menu_mod, new_menu_user].each(&:enable_redesigned_user_menu) - end - - after do - [new_menu_admin, new_menu_mod, new_menu_user].each(&:disable_redesigned_user_menu) - end - - it "will notify users of new reviewable content and respects backward compatibility for the legacy user menu" do + it "will notify users of new reviewable content for the new user menu" do + SiteSetting.enable_experimental_sidebar_hamburger = true SiteSetting.enable_category_group_moderation = true - GroupUser.create!(group_id: group.id, user_id: legacy_menu_mod.id) + GroupUser.create!(group_id: group.id, user_id: moderator.id) # Content for admins only admin_reviewable = Fabricate(:reviewable, reviewable_by_moderator: false) - new_menu_admin.update!(last_seen_reviewable_id: admin_reviewable.id) + admin.update!(last_seen_reviewable_id: admin_reviewable.id) + messages = MessageBus.track_publish do described_class.new.execute(reviewable_id: admin_reviewable.id) end - expect(messages.size).to eq(2) - legacy_menu_admin_msg = messages.find { |m| m.user_ids.include?(legacy_menu_admin.id) } - expect(legacy_menu_admin_msg.data[:reviewable_count]).to eq(1) - expect(legacy_menu_admin_msg.channel).to eq("/reviewable_counts") - expect(legacy_menu_admin_msg.data.key?(:unseen_reviewable_count)).to eq(false) - new_menu_admin_msg = messages.find { |m| m.user_ids == [new_menu_admin.id] } - expect(new_menu_admin_msg.data[:reviewable_count]).to eq(1) - expect(new_menu_admin_msg.channel).to eq("/reviewable_counts/#{new_menu_admin.id}") - expect(new_menu_admin_msg.data[:unseen_reviewable_count]).to eq(0) + expect(messages.size).to eq(1) - expect(messages.any? { |m| m.user_ids.include?(legacy_menu_mod.id) }).to eq(false) - expect(messages.any? { |m| m.user_ids.include?(legacy_menu_user.id) }).to eq(false) - expect(messages.any? { |m| m.user_ids.include?(new_menu_mod.id) }).to eq(false) - expect(messages.any? { |m| m.user_ids.include?(new_menu_user.id) }).to eq(false) + admin_message = messages.first + + expect(admin_message.channel).to eq("/reviewable_counts/#{admin.id}") + expect(admin_message.user_ids).to eq([admin.id]) + expect(admin_message.data[:reviewable_count]).to eq(1) + expect(admin_message.data[:unseen_reviewable_count]).to eq(0) # Content for moderators - mod_reviewable = Fabricate(:reviewable, reviewable_by_moderator: true) + moderator_reviewable = Fabricate(:reviewable, reviewable_by_moderator: true) + messages = MessageBus.track_publish do - described_class.new.execute(reviewable_id: mod_reviewable.id) + described_class.new.execute(reviewable_id: moderator_reviewable.id) end - expect(messages.size).to eq(4) - legacy_menu_admin_msg = messages.find { |m| m.user_ids == [legacy_menu_admin.id] } - expect(legacy_menu_admin_msg.data[:reviewable_count]).to eq(2) - expect(legacy_menu_admin_msg.channel).to eq("/reviewable_counts") - expect(legacy_menu_admin_msg.data.key?(:unseen_reviewable_count)).to eq(false) + expect(messages.size).to eq(2) - new_menu_admin_msg = messages.find { |m| m.user_ids == [new_menu_admin.id] } - expect(new_menu_admin_msg.data[:reviewable_count]).to eq(2) - expect(new_menu_admin_msg.channel).to eq("/reviewable_counts/#{new_menu_admin.id}") - expect(new_menu_admin_msg.data[:unseen_reviewable_count]).to eq(1) + admin_message = messages.find { |m| m.user_ids == [admin.id] } - legacy_menu_mod_msg = messages.find { |m| m.user_ids == [legacy_menu_mod.id] } - expect(legacy_menu_mod_msg.data[:reviewable_count]).to eq(1) - expect(legacy_menu_mod_msg.channel).to eq("/reviewable_counts") - expect(legacy_menu_mod_msg.data.key?(:unseen_reviewable_count)).to eq(false) + expect(admin_message.channel).to eq("/reviewable_counts/#{admin.id}") + expect(admin_message.data[:reviewable_count]).to eq(2) + expect(admin_message.data[:unseen_reviewable_count]).to eq(1) - new_menu_mod_msg = messages.find { |m| m.user_ids == [new_menu_mod.id] } - expect(new_menu_mod_msg.data[:reviewable_count]).to eq(1) - expect(new_menu_mod_msg.channel).to eq("/reviewable_counts/#{new_menu_mod.id}") - expect(new_menu_mod_msg.data[:unseen_reviewable_count]).to eq(1) + moderator_message = messages.find { |m| m.user_ids == [moderator.id] } - expect(messages.any? { |m| m.user_ids.include?(legacy_menu_user.id) }).to eq(false) - expect(messages.any? { |m| m.user_ids.include?(new_menu_user.id) }).to eq(false) + expect(moderator_message.channel).to eq("/reviewable_counts/#{moderator.id}") + expect(moderator_message.data[:reviewable_count]).to eq(1) + expect(moderator_message.data[:unseen_reviewable_count]).to eq(1) - new_menu_mod.update!(last_seen_reviewable_id: mod_reviewable.id) + moderator.update!(last_seen_reviewable_id: moderator_reviewable.id) # Content for a group group_reviewable = Fabricate(:reviewable, reviewable_by_moderator: true, reviewable_by_group: group) + messages = MessageBus.track_publish do described_class.new.execute(reviewable_id: group_reviewable.id) end - expect(messages.size).to eq(6) - legacy_menu_admin_msg = messages.find { |m| m.user_ids == [legacy_menu_admin.id] } - expect(legacy_menu_admin_msg.data[:reviewable_count]).to eq(3) - expect(legacy_menu_admin_msg.channel).to eq("/reviewable_counts") - expect(legacy_menu_admin_msg.data.key?(:unseen_reviewable_count)).to eq(false) - new_menu_admin_msg = messages.find { |m| m.user_ids == [new_menu_admin.id] } - expect(new_menu_admin_msg.data[:reviewable_count]).to eq(3) - expect(new_menu_admin_msg.channel).to eq("/reviewable_counts/#{new_menu_admin.id}") - expect(new_menu_admin_msg.data[:unseen_reviewable_count]).to eq(2) + expect(messages.size).to eq(3) - legacy_menu_mod_msg = messages.find { |m| m.user_ids == [legacy_menu_mod.id] } - expect(legacy_menu_mod_msg.data[:reviewable_count]).to eq(2) - expect(legacy_menu_mod_msg.channel).to eq("/reviewable_counts") - expect(legacy_menu_mod_msg.data.key?(:unseen_reviewable_count)).to eq(false) + admin_message = messages.find { |m| m.user_ids == [admin.id] } - new_menu_mod_msg = messages.find { |m| m.user_ids == [new_menu_mod.id] } - expect(new_menu_mod_msg.data[:reviewable_count]).to eq(2) - expect(new_menu_mod_msg.channel).to eq("/reviewable_counts/#{new_menu_mod.id}") - expect(new_menu_mod_msg.data[:unseen_reviewable_count]).to eq(1) + expect(admin_message.channel).to eq("/reviewable_counts/#{admin.id}") + expect(admin_message.data[:reviewable_count]).to eq(3) + expect(admin_message.data[:unseen_reviewable_count]).to eq(2) - legacy_menu_user_msg = messages.find { |m| m.user_ids == [legacy_menu_user.id] } - expect(legacy_menu_user_msg.data[:reviewable_count]).to eq(1) - expect(legacy_menu_user_msg.channel).to eq("/reviewable_counts") - expect(legacy_menu_user_msg.data.key?(:unseen_reviewable_count)).to eq(false) + moderator_message = messages.find { |m| m.user_ids == [moderator.id] } - new_menu_user_msg = messages.find { |m| m.user_ids == [new_menu_user.id] } - expect(new_menu_user_msg.data[:reviewable_count]).to eq(1) - expect(new_menu_user_msg.channel).to eq("/reviewable_counts/#{new_menu_user.id}") - expect(new_menu_user_msg.data[:unseen_reviewable_count]).to eq(1) + expect(moderator_message.channel).to eq("/reviewable_counts/#{moderator.id}") + expect(moderator_message.data[:reviewable_count]).to eq(2) + expect(moderator_message.data[:unseen_reviewable_count]).to eq(1) + + group_user_message = messages.find { |m| m.user_ids == [user.id] } + + expect(group_user_message.channel).to eq("/reviewable_counts/#{user.id}") + expect(group_user_message.data[:reviewable_count]).to eq(1) + expect(group_user_message.data[:unseen_reviewable_count]).to eq(1) + end + + it "will notify users of new reviewable content for the old user menu" do + SiteSetting.enable_experimental_sidebar_hamburger = false + SiteSetting.enable_category_group_moderation = true + + GroupUser.create!(group_id: group.id, user_id: moderator.id) + + # Content for admins only + admin_reviewable = Fabricate(:reviewable, reviewable_by_moderator: false) + admin.update!(last_seen_reviewable_id: admin_reviewable.id) + + messages = MessageBus.track_publish do + described_class.new.execute(reviewable_id: admin_reviewable.id) + end + + expect(messages.size).to eq(1) + + admin_message = messages.first + + expect(admin_message.channel).to eq("/reviewable_counts") + expect(admin_message.user_ids).to eq([admin.id]) + expect(admin_message.data[:reviewable_count]).to eq(1) + expect(admin_message.data.has_key?(:unseen_reviewable_count)).to eq(false) + + # Content for moderators + moderator_reviewable = Fabricate(:reviewable, reviewable_by_moderator: true) + + messages = MessageBus.track_publish do + described_class.new.execute(reviewable_id: moderator_reviewable.id) + end + + expect(messages.size).to eq(2) + + admin_message = messages.find { |m| m.user_ids == [admin.id] } + expect(admin_message.channel).to eq("/reviewable_counts") + expect(admin_message.data[:reviewable_count]).to eq(2) + expect(admin_message.data.has_key?(:unseen_reviewable_count)).to eq(false) + + moderator_message = messages.find { |m| m.user_ids == [moderator.id] } + expect(moderator_message.channel).to eq("/reviewable_counts") + expect(moderator_message.data[:reviewable_count]).to eq(1) + expect(moderator_message.data.key?(:unseen_reviewable_count)).to eq(false) + + moderator.update!(last_seen_reviewable_id: moderator_reviewable.id) + + # Content for a group + group_reviewable = Fabricate(:reviewable, reviewable_by_moderator: true, reviewable_by_group: group) + + messages = MessageBus.track_publish do + described_class.new.execute(reviewable_id: group_reviewable.id) + end + + expect(messages.size).to eq(3) + + admin_message = messages.find { |m| m.user_ids == [admin.id] } + expect(admin_message.data[:reviewable_count]).to eq(3) + expect(admin_message.channel).to eq("/reviewable_counts") + expect(admin_message.data.key?(:unseen_reviewable_count)).to eq(false) + + moderator_message = messages.find { |m| m.user_ids == [moderator.id] } + expect(moderator_message.data[:reviewable_count]).to eq(2) + expect(moderator_message.channel).to eq("/reviewable_counts") + expect(moderator_message.data.key?(:unseen_reviewable_count)).to eq(false) + + group_user_message = messages.find { |m| m.user_ids == [user.id] } + expect(group_user_message.data[:reviewable_count]).to eq(1) + expect(group_user_message.channel).to eq("/reviewable_counts") + expect(group_user_message.data.key?(:unseen_reviewable_count)).to eq(false) end it "won't notify a group when disabled" do SiteSetting.enable_category_group_moderation = false - GroupUser.create!(group_id: group.id, user_id: legacy_menu_mod.id) - GroupUser.create!(group_id: group.id, user_id: new_menu_mod.id) - r3 = Fabricate(:reviewable, reviewable_by_moderator: true, reviewable_by_group: group) + GroupUser.create!(group_id: group.id, user_id: moderator.id) + reviewable = Fabricate(:reviewable, reviewable_by_moderator: true, reviewable_by_group: group) + messages = MessageBus.track_publish("/reviewable_counts") do - described_class.new.execute(reviewable_id: r3.id) + described_class.new.execute(reviewable_id: reviewable.id) end - group_msg = messages.find { |m| m.user_ids.include?(legacy_menu_user.id) } - expect(group_msg).to be_blank - group_msg = messages.find { |m| m.user_ids.include?(new_menu_user.id) } - expect(group_msg).to be_blank + + group_user_message = messages.find { |m| m.user_ids.include?(user.id) } + + expect(group_user_message).to be_blank end it "respects priority" do @@ -138,38 +172,48 @@ RSpec.describe Jobs::NotifyReviewable do Reviewable.set_priorities(medium: 2.0) SiteSetting.reviewable_default_visibility = 'medium' - GroupUser.create!(group_id: group.id, user_id: legacy_menu_mod.id) + GroupUser.create!(group_id: group.id, user_id: moderator.id) # Content for admins only - r1 = Fabricate(:reviewable, reviewable_by_moderator: false) + admin_reviewable = Fabricate(:reviewable, reviewable_by_moderator: false) + messages = MessageBus.track_publish("/reviewable_counts") do - described_class.new.execute(reviewable_id: r1.id) + described_class.new.execute(reviewable_id: admin_reviewable.id) end - legacy_menu_admin_msg = messages.find { |m| m.user_ids.include?(legacy_menu_admin.id) } - expect(legacy_menu_admin_msg.data[:reviewable_count]).to eq(0) + + admin_message = messages.find { |m| m.user_ids.include?(admin.id) } + expect(admin_message.data[:reviewable_count]).to eq(0) # Content for moderators - r2 = Fabricate(:reviewable, reviewable_by_moderator: true) + moderator_reviewable = Fabricate(:reviewable, reviewable_by_moderator: true) + messages = MessageBus.track_publish("/reviewable_counts") do - described_class.new.execute(reviewable_id: r2.id) + described_class.new.execute(reviewable_id: moderator_reviewable.id) end - legacy_menu_admin_msg = messages.find { |m| m.user_ids.include?(legacy_menu_admin.id) } - expect(legacy_menu_admin_msg.data[:reviewable_count]).to eq(0) - legacy_menu_mod_msg = messages.find { |m| m.user_ids.include?(legacy_menu_mod.id) } - expect(legacy_menu_mod_msg.data[:reviewable_count]).to eq(0) + + admin_message = messages.find { |m| m.user_ids.include?(admin.id) } + + expect(admin_message.data[:reviewable_count]).to eq(0) + + moderator_message = messages.find { |m| m.user_ids.include?(moderator.id) } + expect(moderator_message.data[:reviewable_count]).to eq(0) # Content for a group - r3 = Fabricate(:reviewable, reviewable_by_moderator: true, reviewable_by_group: group) + group_reviewable = Fabricate(:reviewable, reviewable_by_moderator: true, reviewable_by_group: group) + messages = MessageBus.track_publish("/reviewable_counts") do - described_class.new.execute(reviewable_id: r3.id) + described_class.new.execute(reviewable_id: group_reviewable.id) end - legacy_menu_admin_msg = messages.find { |m| m.user_ids.include?(legacy_menu_admin.id) } - expect(legacy_menu_admin_msg.data[:reviewable_count]).to eq(0) - mod_messages = messages.select { |m| m.user_ids.include?(legacy_menu_mod.id) } - expect(mod_messages.size).to eq(1) - expect(mod_messages[0].data[:reviewable_count]).to eq(0) - group_msg = messages.find { |m| m.user_ids.include?(legacy_menu_user.id) } - expect(group_msg.data[:reviewable_count]).to eq(0) + + admin_message = messages.find { |m| m.user_ids.include?(admin.id) } + expect(admin_message.data[:reviewable_count]).to eq(0) + + moderator_messages = messages.select { |m| m.user_ids.include?(moderator.id) } + expect(moderator_messages.size).to eq(1) + expect(moderator_messages[0].data[:reviewable_count]).to eq(0) + + group_user_message = messages.find { |m| m.user_ids.include?(user.id) } + expect(group_user_message.data[:reviewable_count]).to eq(0) end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 27d587595cd..9cf035f0654 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -2067,21 +2067,25 @@ RSpec.describe User do end context "with redesigned_user_menu_enabled on" do + before do + SiteSetting.enable_experimental_sidebar_hamburger = true + end + it "adds all_unread_notifications and grouped_unread_notifications to the payload" do user.update!(admin: true) - user.enable_redesigned_user_menu Fabricate(:notification, user: user, notification_type: 1) Fabricate(:notification, notification_type: 15, high_priority: true, read: false, user: user) + messages = MessageBus.track_publish("/notification/#{user.id}") do user.publish_notifications_state end + expect(messages.size).to eq(1) message = messages.first + expect(message.data[:all_unread_notifications_count]).to eq(2) expect(message.data[:grouped_unread_notifications]).to eq({ 1 => 1, 15 => 1 }) - ensure - user.disable_redesigned_user_menu end end end