Discource-C
/
discourse
mirror of https://github.com/discourse/discourse.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

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.
This commit is contained in:
Régis Hanol 2020-08-07 18:13:02 +02:00 committed by GitHub
parent 3593e582a3
commit bc63232d2e
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
9 changed files with 176 additions and 130 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

140
app/assets/javascripts/discourse/app/widgets/hamburger-menu.js
View File

@ -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

4
app/controllers/reviewables_controller.rb
View File

@ -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

62
app/jobs/regular/notify_reviewable.rb
View File

@ -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

1
config/routes.rb
View File

@ -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"

30
lib/topic_view.rb
View File

@ -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

2
spec/jobs/notify_reviewable_spec.rb
View File

@ -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

27
spec/requests/reviewables_controller_spec.rb
View File

@ -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

17
test/javascripts/acceptance/hamburger-menu-test.js Normal file
View File

@ -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");
});

23
test/javascripts/widgets/hamburger-menu-test.js
View File

@ -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"}}',