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

DEV: Do not show handled reviewables in the user menu (#18402) 

Currently, the reviewables tab in the user menu shows pending reviewables at the top of the menu and fills the remaining space in the menu with old/handled reviewables. This PR makes the revieables tab show only pending reviewables and hides the tab altogether from the menu if there are no pending reviewables. We're going to follow-up with another change soon that will show pending reviewables in the main tab of the user menu.

Internal topic: t/73220.
This commit is contained in:
Osama Sayegh 2022-09-30 06:10:07 +03:00 committed by GitHub
parent 69c20a3a5e
commit 079450c9e4
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 37 additions and 124 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
app/assets/javascripts/discourse/app/components/user-menu/menu.js
View File

@ -218,7 +218,9 @@ const CORE_TOP_TABS = [
}
get shouldDisplay() {
return this.currentUser.can_review;
return (
this.currentUser.can_review && this.currentUser.get("reviewable_count")
);
}
get count() {

9
app/assets/javascripts/discourse/tests/acceptance/user-menu-test.js
View File

@ -119,6 +119,7 @@ acceptance("User menu", function (needs) {
});
test("clicking on user menu items", async function (assert) {
updateCurrentUser({ reviewable_count: 1 });
await visit("/");
await click(".d-header-icons .current-user");
await click("#user-menu-button-review-queue");
@ -169,6 +170,7 @@ acceptance("User menu", function (needs) {
});
test("tabs have title attributes", async function (assert) {
updateCurrentUser({ reviewable_count: 1 });
withPluginApi("0.1", (api) => {
api.registerUserMenuTab((UserMenuTab) => {
return class extends UserMenuTab {
@ -208,7 +210,10 @@ acceptance("User menu", function (needs) {
"user-menu-button-messages": I18n.t("user_menu.tabs.messages"),
"user-menu-button-bookmarks": I18n.t("user_menu.tabs.bookmarks"),
"user-menu-button-tiny-tab-1": "Custom title: 73",
"user-menu-button-review-queue": I18n.t("user_menu.tabs.review_queue"),
"user-menu-button-review-queue": I18n.t(
"user_menu.tabs.review_queue_with_unread",
{ count: 1 }
),
"user-menu-button-other-notifications": I18n.t(
"user_menu.tabs.other_notifications"
),
@ -235,6 +240,7 @@ acceptance("User menu", function (needs) {
});
test("tabs added via the plugin API", async function (assert) {
updateCurrentUser({ reviewable_count: 1 });
withPluginApi("0.1", (api) => {
api.registerUserMenuTab((UserMenuTab) => {
return class extends UserMenuTab {
@ -674,6 +680,7 @@ acceptance("User menu", function (needs) {
});
test("the active tab can be clicked again to navigate to a page", async function (assert) {
updateCurrentUser({ reviewable_count: 1 });
withPluginApi("0.1", (api) => {
api.registerUserMenuTab((UserMenuTab) => {
return class extends UserMenuTab {

11
app/assets/javascripts/discourse/tests/integration/components/user-menu/menu-test.js
View File

@ -82,8 +82,9 @@ module("Integration | Component | user-menu", function (hooks) {
);
});
test("reviewables tab is shown if current user can review", async function (assert) {
test("reviewables tab is shown if current user can review and there are pending reviewables", async function (assert) {
this.currentUser.set("can_review", true);
this.currentUser.set("reviewable_count", 1);
await render(template);
const tab = query("#user-menu-button-review-queue");
assert.strictEqual(tab.dataset.tabNumber, "7");
@ -98,6 +99,13 @@ module("Integration | Component | user-menu", function (hooks) {
);
});
test("reviewables tab is not shown if current user can review but there are no pending reviewables", async function (assert) {
this.currentUser.set("can_review", true);
this.currentUser.set("reviewable_count", 0);
await render(template);
assert.notOk(exists("#user-menu-button-review-queue"));
});
test("messages tab isn't shown if current user isn't staff and user does not belong to personal_message_enabled_groups", async function (assert) {
this.currentUser.set("moderator", false);
this.currentUser.set("admin", false);
@ -146,6 +154,7 @@ module("Integration | Component | user-menu", function (hooks) {
test("changing tabs", async function (assert) {
this.currentUser.set("can_review", true);
this.currentUser.set("reviewable_count", 1);
await render(template);
let queryParams;
pretender.get("/notifications", (request) => {

2
app/controllers/reviewables_controller.rb
View File

@ -71,7 +71,7 @@ class ReviewablesController < ApplicationController
end
def user_menu_list
reviewables = Reviewable.recent_list_with_pending_first(current_user).to_a
reviewables = Reviewable.list_for(current_user, limit: 30, status: :pending).to_a
json = {
reviewables: reviewables.map! { |r| r.basic_serializer.new(r, scope: guardian, root: nil).as_json }
}

22
app/models/reviewable.rb
View File

@ -534,28 +534,6 @@ class Reviewable < ActiveRecord::Base
results
end
def self.recent_list_with_pending_first(user, limit: 30)
min_score = Reviewable.min_score_for_priority
query = Reviewable
.includes(:created_by, :topic, :target)
.viewable_by(user, preload: false)
.except(:order)
.order(score: :desc, created_at: :desc)
.limit(limit)
if min_score > 0
query = query.where(<<~SQL, min_score: min_score)
reviewables.score >= :min_score OR reviewables.force_review
SQL
end
records = query.where(status: Reviewable.statuses[:pending]).to_a
if records.size < limit
records += query.where.not(status: Reviewable.statuses[:pending]).to_a
end
records
end
def serializer
self.class.serializer_for(self)
end

81
spec/models/reviewable_spec.rb
View File

@ -300,87 +300,6 @@ RSpec.describe Reviewable, type: :model do
end
end
describe ".recent_list_with_pending_first" do
fab!(:pending_reviewable1) do
Fabricate(
:reviewable,
score: 150,
created_at: 7.minutes.ago,
status: Reviewable.statuses[:pending]
)
end
fab!(:pending_reviewable2) do
Fabricate(
:reviewable,
score: 100,
status: Reviewable.statuses[:pending]
)
end
fab!(:approved_reviewable1) do
Fabricate(
:reviewable,
created_at: 1.minutes.ago,
score: 300,
status: Reviewable.statuses[:approved]
)
end
fab!(:approved_reviewable2) do
Fabricate(
:reviewable,
created_at: 5.minutes.ago,
score: 200,
status: Reviewable.statuses[:approved]
)
end
fab!(:admin) { Fabricate(:admin) }
it "returns a list of reviewables with pending items first" do
list = Reviewable.recent_list_with_pending_first(admin)
expect(list.map(&:id)).to eq([
pending_reviewable1.id,
pending_reviewable2.id,
approved_reviewable1.id,
approved_reviewable2.id
])
pending_reviewable1.update!(status: Reviewable.statuses[:rejected])
rejected_reviewable = pending_reviewable1
list = Reviewable.recent_list_with_pending_first(admin)
expect(list.map(&:id)).to eq([
pending_reviewable2.id,
approved_reviewable1.id,
approved_reviewable2.id,
rejected_reviewable.id,
])
end
it "only includes reviewables whose score is above the minimum or are forced for review" do
SiteSetting.reviewable_default_visibility = 'high'
Reviewable.set_priorities({ high: 200 })
list = Reviewable.recent_list_with_pending_first(admin)
expect(list.map(&:id)).to eq([
approved_reviewable1.id,
approved_reviewable2.id,
])
pending_reviewable1.update!(force_review: true)
list = Reviewable.recent_list_with_pending_first(admin)
expect(list.map(&:id)).to eq([
pending_reviewable1.id,
approved_reviewable1.id,
approved_reviewable2.id,
])
end
it "accepts a limit argument to limit the number of returned records" do
expect(Reviewable.recent_list_with_pending_first(admin, limit: 2).size).to eq(2)
end
end
it "valid_types returns the appropriate types" do
expect(Reviewable.valid_type?('ReviewableUser')).to eq(true)
expect(Reviewable.valid_type?('ReviewableQueuedPost')).to eq(true)

32
spec/requests/reviewables_controller_spec.rb
View File

@ -255,7 +255,7 @@ RSpec.describe ReviewablesController do
end
describe "#user_menu_list" do
it "renders each reviewable with its basic serializers" do
it "renders each reviewable using its basic serializers" do
reviewable_user = Fabricate(:reviewable_user, payload: { username: "someb0dy" })
reviewable_flagged_post = Fabricate(:reviewable_flagged_post)
reviewable_queued_post = Fabricate(:reviewable_queued_post)
@ -284,40 +284,38 @@ RSpec.describe ReviewablesController do
end
it "returns JSON containing basic information of reviewables" do
reviewable1 = Fabricate(:reviewable)
reviewable2 = Fabricate(:reviewable, status: Reviewable.statuses[:approved])
reviewable = Fabricate(:reviewable)
get "/review/user-menu-list.json"
expect(response.status).to eq(200)
reviewables = response.parsed_body["reviewables"]
expect(reviewables.size).to eq(2)
expect(reviewables[0]["flagger_username"]).to eq(reviewable1.created_by.username)
expect(reviewables[0]["id"]).to eq(reviewable1.id)
expect(reviewables[0]["type"]).to eq(reviewable1.type)
expect(reviewables.size).to eq(1)
expect(reviewables[0]["flagger_username"]).to eq(reviewable.created_by.username)
expect(reviewables[0]["id"]).to eq(reviewable.id)
expect(reviewables[0]["type"]).to eq(reviewable.type)
expect(reviewables[0]["pending"]).to eq(true)
expect(reviewables[1]["flagger_username"]).to eq(reviewable2.created_by.username)
expect(reviewables[1]["id"]).to eq(reviewable2.id)
expect(reviewables[1]["type"]).to eq(reviewable2.type)
expect(reviewables[1]["pending"]).to eq(false)
end
it "puts pending reviewables on top" do
approved1 = Fabricate(
it "responds with pending reviewables only" do
Fabricate(
:reviewable,
status: Reviewable.statuses[:approved]
)
pending = Fabricate(
pending1 = Fabricate(
:reviewable,
status: Reviewable.statuses[:pending]
)
approved2 = Fabricate(
Fabricate(
:reviewable,
status: Reviewable.statuses[:approved]
)
pending2 = Fabricate(
:reviewable,
status: Reviewable.statuses[:pending]
)
get "/review/user-menu-list.json"
expect(response.status).to eq(200)
reviewables = response.parsed_body["reviewables"]
expect(reviewables.map { |r| r["id"] }).to eq([pending.id, approved2.id, approved1.id])
expect(reviewables.map { |r| r["id"] }).to eq([pending2.id, pending1.id])
end
end