From bc63232d2ef0d083815f364ad63b37e290ec1960 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Hanol?= Date: Fri, 7 Aug 2020 18:13:02 +0200 Subject: [PATCH] FIX: sync reviewable count when opening the hamburger menu (#10368) When a tab is open but left unattended for a while, the red, green, and blue pills tend to go out of sync. So whevener we open the notifications menu, we sync up the notification count (eg. blue and green pills) with the server. However, the reviewable count (eg. the red pill) is not a notification and is located in the hamburger menu. This commit adds a new route on the server side to retrieve the reviewable count for the current user and a ping (refreshReviewableCount) from the client side to sync the reviewable count whenever they open the hamburger menu. REFACTOR: I also refactored the hamburger-menu widget code to prevent repetitive uses of "this.". PERF: I improved the performance of the 'notify_reviewable' job by doing only 1 query to the database to retrieve all the pending reviewables and then tallying based on the various rights. --- .../discourse/app/widgets/hamburger-menu.js | 140 +++++++++++------- app/controllers/reviewables_controller.rb | 4 + app/jobs/regular/notify_reviewable.rb | 62 +++----- config/routes.rb | 1 + lib/topic_view.rb | 30 ++-- spec/jobs/notify_reviewable_spec.rb | 2 +- spec/requests/reviewables_controller_spec.rb | 27 ++++ .../acceptance/hamburger-menu-test.js | 17 +++ .../widgets/hamburger-menu-test.js | 23 +-- 9 files changed, 176 insertions(+), 130 deletions(-) create mode 100644 test/javascripts/acceptance/hamburger-menu-test.js diff --git a/app/assets/javascripts/discourse/app/widgets/hamburger-menu.js b/app/assets/javascripts/discourse/app/widgets/hamburger-menu.js index 8f24468812..8caaa0fd30 100644 --- a/app/assets/javascripts/discourse/app/widgets/hamburger-menu.js +++ b/app/assets/javascripts/discourse/app/widgets/hamburger-menu.js @@ -27,16 +27,22 @@ createWidget("priority-faq-link", { }, click(e) { - if (this.siteSettings.faq_url === this.attrs.href) { + const { + attrs: { href }, + currentUser, + siteSettings + } = this; + + if (siteSettings.faq_url === href) { ajax(userPath("read-faq"), { type: "POST" }).then(() => { - this.currentUser.set("read_faq", true); + currentUser.set("read_faq", true); if (wantsNewWindow(e)) { return; } e.preventDefault(); - DiscourseURL.routeTo(this.attrs.href); + DiscourseURL.routeTo(href); }); } else { if (wantsNewWindow(e)) { @@ -44,12 +50,14 @@ createWidget("priority-faq-link", { } e.preventDefault(); - DiscourseURL.routeTo(this.attrs.href); + DiscourseURL.routeTo(href); } } }); export default createWidget("hamburger-menu", { + buildKey: () => "hamburger-menu", + tagName: "div.hamburger-panel", settings: { @@ -59,6 +67,10 @@ export default createWidget("hamburger-menu", { showAbout: true }, + defaultState() { + return { loaded: false, loading: false }; + }, + adminLinks() { const { currentUser } = this; @@ -88,15 +100,8 @@ export default createWidget("hamburger-menu", { return tts ? tts.lookupCount(type) : 0; }, - showUserDirectory() { - if (!this.siteSettings.enable_user_directory) return false; - if (this.siteSettings.hide_user_profiles_from_public && !this.currentUser) - return false; - return true; - }, - generalLinks() { - const { siteSettings } = this; + const { attrs, currentUser, siteSettings, state } = this; const links = []; links.push({ @@ -106,7 +111,7 @@ export default createWidget("hamburger-menu", { title: "filters.latest.help" }); - if (this.currentUser) { + if (currentUser) { links.push({ route: "discovery.new", className: "new-topics-link", @@ -124,22 +129,20 @@ export default createWidget("hamburger-menu", { title: "filters.unread.help", count: this.lookupCount("unread") }); - } - // Staff always see the review link. Non-staff will see it if there are items to review - if ( - this.currentUser && - (this.currentUser.staff || this.currentUser.reviewable_count) - ) { - links.push({ - route: siteSettings.reviewable_default_topics - ? "review.topics" - : "review", - className: "review", - label: "review.title", - badgeCount: "reviewable_count", - badgeClass: "reviewables" - }); + // Staff always see the review link. + // Non-staff will see it if there are items to review + if (currentUser.staff || currentUser.reviewable_count) { + links.push({ + route: siteSettings.reviewable_default_topics + ? "review.topics" + : "review", + className: "review", + label: "review.title", + badgeCount: "reviewable_count", + badgeClass: "reviewables" + }); + } } links.push({ @@ -157,7 +160,9 @@ export default createWidget("hamburger-menu", { }); } - if (this.showUserDirectory()) { + const canSeeUserProfiles = + currentUser || !siteSettings.hide_user_profiles_from_public; + if (siteSettings.enable_user_directory && canSeeUserProfiles) { links.push({ route: "users", className: "user-directory-link", @@ -165,7 +170,7 @@ export default createWidget("hamburger-menu", { }); } - if (this.siteSettings.enable_group_directory) { + if (siteSettings.enable_group_directory) { links.push({ route: "groups", className: "groups-link", @@ -173,23 +178,25 @@ export default createWidget("hamburger-menu", { }); } - if (this.siteSettings.tagging_enabled) { + if (siteSettings.tagging_enabled) { links.push({ route: "tags", label: "tagging.tags" }); } const extraLinks = flatten( - applyDecorators(this, "generalLinks", this.attrs, this.state) + applyDecorators(this, "generalLinks", attrs, state) ); + return links.concat(extraLinks).map(l => this.attach("link", l)); }, listCategories() { - const maxCategoriesToDisplay = this.siteSettings - .header_dropdown_category_count; + const { currentUser, site, siteSettings } = this; + const maxCategoriesToDisplay = siteSettings.header_dropdown_category_count; + let categories = []; - if (this.currentUser) { - const allCategories = this.site + if (currentUser) { + const allCategories = site .get("categories") .filter(c => c.notification_level !== NotificationLevels.MUTED); @@ -203,7 +210,8 @@ export default createWidget("hamburger-menu", { ); }); - const topCategoryIds = this.currentUser.get("top_category_ids") || []; + const topCategoryIds = currentUser.get("top_category_ids") || []; + topCategoryIds.forEach(id => { const category = allCategories.find(c => c.id === id); if (category && !categories.includes(category)) { @@ -217,14 +225,14 @@ export default createWidget("hamburger-menu", { .sort((a, b) => b.topic_count - a.topic_count) ); } else { - categories = this.site + categories = site .get("categoriesByCount") .filter(c => c.notification_level !== NotificationLevels.MUTED); } - if (!this.siteSettings.allow_uncategorized_topics) { + if (!siteSettings.allow_uncategorized_topics) { categories = categories.filter( - c => c.id !== this.site.uncategorized_category_id + c => c.id !== site.uncategorized_category_id ); } @@ -235,8 +243,10 @@ export default createWidget("hamburger-menu", { }, footerLinks(prioritizeFaq, faqUrl) { + const { attrs, capabilities, settings, site, siteSettings, state } = this; const links = []; - if (this.settings.showAbout) { + + if (settings.showAbout) { links.push({ route: "about", className: "about-link", @@ -244,12 +254,11 @@ export default createWidget("hamburger-menu", { }); } - if (this.settings.showFAQ && !prioritizeFaq) { + if (settings.showFAQ && !prioritizeFaq) { links.push({ href: faqUrl, className: "faq-link", label: "faq" }); } - const { site } = this; - if (!site.mobileView && !this.capabilities.touch) { + if (!site.mobileView && !capabilities.touch) { links.push({ href: "", action: "showKeyboard", @@ -258,29 +267,28 @@ export default createWidget("hamburger-menu", { }); } - if ( - this.site.mobileView || - (this.siteSettings.enable_mobile_theme && this.capabilities.touch) - ) { + const mobileTouch = siteSettings.enable_mobile_theme && capabilities.touch; + if (site.mobileView || mobileTouch) { links.push({ action: "toggleMobileView", className: "mobile-toggle-link", - label: this.site.mobileView ? "desktop_view" : "mobile_view" + label: site.mobileView ? "desktop_view" : "mobile_view" }); } const extraLinks = flatten( - applyDecorators(this, "footerLinks", this.attrs, this.state) + applyDecorators(this, "footerLinks", attrs, state) ); + return links.concat(extraLinks).map(l => this.attach("link", l)); }, panelContents() { - const { currentUser } = this; + const { attrs, currentUser, settings, siteSettings, state } = this; const results = []; - const faqUrl = this.siteSettings.faq_url || getURL("/faq"); + const faqUrl = siteSettings.faq_url || getURL("/faq"); const prioritizeFaq = - this.settings.showFAQ && this.currentUser && !this.currentUser.read_faq; + settings.showFAQ && currentUser && !currentUser.read_faq; if (prioritizeFaq) { results.push( @@ -300,7 +308,7 @@ export default createWidget("hamburger-menu", { name: "admin-links", contents: () => { const extraLinks = flatten( - applyDecorators(this, "admin-links", this.attrs, this.state) + applyDecorators(this, "admin-links", attrs, state) ); return this.adminLinks().concat(extraLinks); } @@ -315,7 +323,7 @@ export default createWidget("hamburger-menu", { }) ); - if (this.settings.showCategories) { + if (settings.showCategories) { results.push(this.listCategories()); results.push(h("hr.categories-separator")); } @@ -331,7 +339,27 @@ export default createWidget("hamburger-menu", { return results; }, - html() { + refreshReviewableCount(state) { + const { currentUser } = this; + + if (state.loading || !currentUser) return; + + state.loading = true; + + return ajax("/review/count.json") + .then(({ count }) => currentUser.set("reviewable_count", count)) + .finally(() => { + state.loaded = true; + state.loading = false; + this.scheduleRerender(); + }); + }, + + html(attrs, state) { + if (!state.loaded) { + this.refreshReviewableCount(state); + } + return this.attach("menu-panel", { contents: () => this.panelContents(), maxWidth: this.settings.maxWidth diff --git a/app/controllers/reviewables_controller.rb b/app/controllers/reviewables_controller.rb index cdfe645ba4..65de25b429 100644 --- a/app/controllers/reviewables_controller.rb +++ b/app/controllers/reviewables_controller.rb @@ -68,6 +68,10 @@ class ReviewablesController < ApplicationController render_json_dump(json, rest_serializer: true) end + def count + render_json_dump(count: Reviewable.pending_count(current_user)) + end + def topics topic_ids = Set.new diff --git a/app/jobs/regular/notify_reviewable.rb b/app/jobs/regular/notify_reviewable.rb index c6b02c114e..88632a66ae 100644 --- a/app/jobs/regular/notify_reviewable.rb +++ b/app/jobs/regular/notify_reviewable.rb @@ -3,53 +3,39 @@ class Jobs::NotifyReviewable < ::Jobs::Base def execute(args) - reviewable = Reviewable.find_by(id: args[:reviewable_id]) - return unless reviewable.present? + return unless reviewable = Reviewable.find_by(id: args[:reviewable_id]) @contacted = Set.new - notify_admins - notify_moderators if reviewable.reviewable_by_moderator? - if SiteSetting.enable_category_group_moderation? && reviewable.reviewable_by_group.present? - notify_group(reviewable.reviewable_by_group) + counts = Hash.new(0) + + Reviewable.default_visible.pending.each do |r| + counts[:admins] += 1 + counts[:moderators] += 1 if r.reviewable_by_moderator? + counts[r.reviewable_by_group_id] += 1 if r.reviewable_by_group_id + end + + # admins + notify(counts[:admins], User.real.admins.pluck(:id)) + + # moderators + if reviewable.reviewable_by_moderator? + notify(counts[:moderators], User.real.moderators.where("id NOT IN (?)", @contacted).pluck(:id)) + end + + # category moderators + if SiteSetting.enable_category_group_moderation? && (group = reviewable.reviewable_by_group) + group.users.includes(:group_users).where("users.id NOT IN (?)", @contacted).find_each do |user| + count = user.group_users.map { |gu| counts[gu.group_id] }.sum + notify(count, [user.id]) + end end end -protected - - def users - return User if @contacted.blank? - User.where("id NOT IN (?)", @contacted) - end - - def pending - Reviewable.default_visible.pending - end - - def notify_admins - notify(pending.count, users.admins.pluck(:id)) - end - - def notify_moderators - user_ids = users.moderators.pluck(:id) - notify(pending.where(reviewable_by_moderator: true).count, user_ids) - end - - def notify_group(group) - @group_counts = {} - group.users.includes(:group_users).where("users.id NOT IN (?)", @contacted).each do |u| - reviewable_count = u.group_users.map { |gu| count_for_group(gu.group_id) }.sum - MessageBus.publish("/reviewable_counts", { reviewable_count: reviewable_count }, user_ids: [u.id]) - end - end - - def count_for_group(group_id) - @group_counts[group_id] ||= pending.where(reviewable_by_group_id: group_id).count - end + protected def notify(count, user_ids) return if user_ids.blank? - data = { reviewable_count: count } MessageBus.publish("/reviewable_counts", data, user_ids: user_ids) @contacted += user_ids diff --git a/config/routes.rb b/config/routes.rb index 906cb2b206..c124e0b7b3 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -338,6 +338,7 @@ Discourse::Application.routes.draw do get "review" => "reviewables#index" # For ember app get "review/:reviewable_id" => "reviewables#show", constraints: { reviewable_id: /\d+/ } get "review/:reviewable_id/explain" => "reviewables#explain", constraints: { reviewable_id: /\d+/ } + get "review/count" => "reviewables#count" get "review/topics" => "reviewables#topics" get "review/settings" => "reviewables#settings" put "review/settings" => "reviewables#settings" diff --git a/lib/topic_view.rb b/lib/topic_view.rb index 537a53fccc..3752ac3092 100644 --- a/lib/topic_view.rb +++ b/lib/topic_view.rb @@ -451,38 +451,40 @@ class TopicView end def reviewable_counts - if @reviewable_counts.nil? - - post_ids = @posts.map(&:id) - + @reviewable_counts ||= begin sql = <<~SQL - SELECT target_id, + SELECT + target_id, MAX(r.id) reviewable_id, COUNT(*) total, SUM(CASE WHEN s.status = :pending THEN 1 ELSE 0 END) pending - FROM reviewables r - JOIN reviewable_scores s ON reviewable_id = r.id - WHERE r.target_id IN (:post_ids) AND + FROM + reviewables r + JOIN + reviewable_scores s ON reviewable_id = r.id + WHERE + r.target_id IN (:post_ids) AND r.target_type = 'Post' - GROUP BY target_id + GROUP BY + target_id SQL - @reviewable_counts = {} + counts = {} DB.query( sql, pending: ReviewableScore.statuses[:pending], - post_ids: post_ids + post_ids: @posts.map(&:id) ).each do |row| - @reviewable_counts[row.target_id] = { + counts[row.target_id] = { total: row.total, pending: row.pending, reviewable_id: row.reviewable_id } end - end - @reviewable_counts + counts + end end def pending_posts diff --git a/spec/jobs/notify_reviewable_spec.rb b/spec/jobs/notify_reviewable_spec.rb index c321cc5478..783b78e2c9 100644 --- a/spec/jobs/notify_reviewable_spec.rb +++ b/spec/jobs/notify_reviewable_spec.rb @@ -111,6 +111,6 @@ describe Jobs::NotifyReviewable do described_class.new.execute(reviewable_id: reviewable.id) end - expect(messages.size).to eq(1) + expect(messages.size).to eq(0) end end diff --git a/spec/requests/reviewables_controller_spec.rb b/spec/requests/reviewables_controller_spec.rb index 28a919b309..521a5cdefc 100644 --- a/spec/requests/reviewables_controller_spec.rb +++ b/spec/requests/reviewables_controller_spec.rb @@ -24,6 +24,11 @@ describe ReviewablesController do delete "/review/123" expect(response.code).to eq("403") end + + it "denies count" do + get "/review/count.json" + expect(response.code).to eq("403") + end end context "regular user" do @@ -615,6 +620,28 @@ describe ReviewablesController do end end + context "#count" do + fab!(:admin) { Fabricate(:admin) } + + before do + sign_in(admin) + end + + it "returns the number of reviewables" do + get "/review/count.json" + expect(response.code).to eq("200") + json = response.parsed_body + expect(json["count"]).to eq(0) + + Fabricate(:reviewable_queued_post) + + get "/review/count.json" + expect(response.code).to eq("200") + json = response.parsed_body + expect(json["count"]).to eq(1) + end + end + end end diff --git a/test/javascripts/acceptance/hamburger-menu-test.js b/test/javascripts/acceptance/hamburger-menu-test.js new file mode 100644 index 0000000000..629a903990 --- /dev/null +++ b/test/javascripts/acceptance/hamburger-menu-test.js @@ -0,0 +1,17 @@ +import { acceptance, updateCurrentUser } from "helpers/qunit-helpers"; + +acceptance("Opening the hamburger menu with some reviewables", { + loggedIn: true, + pretend: (server, helper) => { + server.get("/review/count.json", () => helper.response({ count: 3 })); + } +}); + +QUnit.test("As a staff member", async assert => { + updateCurrentUser({ moderator: true, admin: false }); + + await visit("/"); + await click(".hamburger-dropdown"); + + assert.equal(find(".review .badge-notification.reviewables").text(), "3"); +}); diff --git a/test/javascripts/widgets/hamburger-menu-test.js b/test/javascripts/widgets/hamburger-menu-test.js index 675e85352e..68b0075d11 100644 --- a/test/javascripts/widgets/hamburger-menu-test.js +++ b/test/javascripts/widgets/hamburger-menu-test.js @@ -48,20 +48,16 @@ widgetTest("staff menu - not staff", { } }); -widgetTest("staff menu", { +widgetTest("staff menu - moderator", { template: '{{mount-widget widget="hamburger-menu"}}', beforeEach() { - this.currentUser.setProperties({ - moderator: true, - reviewable_count: 3 - }); + this.currentUser.set("moderator", true); }, test(assert) { assert.ok(find(".admin-link").length); assert.ok(find(".review").length); - assert.equal(find(".reviewables").text(), "3"); assert.ok(!find(".settings-link").length); } }); @@ -78,21 +74,6 @@ widgetTest("staff menu - admin", { } }); -widgetTest("reviewable content", { - template: '{{mount-widget widget="hamburger-menu"}}', - - beforeEach() { - this.currentUser.setProperties({ - moderator: true, - reviewable_count: 5 - }); - }, - - test(assert) { - assert.equal(find(".reviewables").text(), "5"); - } -}); - widgetTest("logged in links", { template: '{{mount-widget widget="hamburger-menu"}}',