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

DEV: Include pending reviewables in the main tab in the user menu (#18471) 

This commit makes pending reviewables show up in the main tab (a.k.a. "all notifications" tab). Pending reviewables along with unread notifications are always shown first and they're sorted based on their creation date (most recent comes first).

The dismiss button currently only shows up if there are unread notifications and it doesn't dismiss pending reviewables. We may follow up with another change soon that allows makes the dismiss button work with reviewables and remove them from the list without taking any action on them. 

Follow-up to 079450c9e4.
This commit is contained in:
Osama Sayegh 2022-10-05 12:30:02 +03:00 committed by GitHub
parent c0037dc0f0
commit 4d05e3edab
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
12 changed files with 394 additions and 125 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

48
app/assets/javascripts/discourse/app/components/user-menu/messages-list.js
View File

@ -6,18 +6,7 @@ import I18n from "I18n";
import UserMenuNotificationItem from "discourse/lib/user-menu/notification-item";
import UserMenuMessageItem from "discourse/lib/user-menu/message-item";
import Topic from "discourse/models/topic";
function parseDateString(date) {
if (date) {
return new Date(date);
}
}
async function initializeNotifications(rawList) {
const notifications = rawList.map((n) => Notification.create(n));
await Notification.applyTransformations(notifications);
return notifications;
}
import { mergeSortedLists } from "discourse/lib/utilities";
export default class UserMenuMessagesList extends UserMenuNotificationsList {
get dismissTypes() {
@ -63,7 +52,7 @@ export default class UserMenuMessagesList extends UserMenuNotificationsList {
);
const content = [];
const unreadNotifications = await initializeNotifications(
const unreadNotifications = await Notification.initializeNotifications(
data.unread_notifications
);
unreadNotifications.forEach((notification) => {
@ -80,38 +69,29 @@ export default class UserMenuMessagesList extends UserMenuNotificationsList {
const topics = data.topics.map((t) => Topic.create(t));
await Topic.applyTransformations(topics);
const readNotifications = await initializeNotifications(
const readNotifications = await Notification.initializeNotifications(
data.read_notifications
);
let latestReadNotificationDate = parseDateString(
readNotifications[0]?.created_at
);
let latestMessageDate = parseDateString(topics[0]?.bumped_at);
while (latestReadNotificationDate || latestMessageDate) {
if (
!latestReadNotificationDate ||
(latestMessageDate && latestReadNotificationDate < latestMessageDate)
) {
content.push(new UserMenuMessageItem({ message: topics[0] }));
topics.shift();
latestMessageDate = parseDateString(topics[0]?.bumped_at);
} else {
mergeSortedLists(readNotifications, topics, (notification, topic) => {
const notificationCreatedAt = new Date(notification.created_at);
const topicBumpedAt = new Date(topic.bumped_at);
return topicBumpedAt > notificationCreatedAt;
}).forEach((item) => {
if (item instanceof Notification) {
content.push(
new UserMenuNotificationItem({
notification: readNotifications[0],
notification: item,
currentUser: this.currentUser,
siteSettings: this.siteSettings,
site: this.site,
})
);
readNotifications.shift();
latestReadNotificationDate = parseDateString(
readNotifications[0]?.created_at
);
} else {
content.push(new UserMenuMessageItem({ message: item }));
}
}
});
return content;
}

80
app/assets/javascripts/discourse/app/components/user-menu/notifications-list.js
View File

@ -2,11 +2,16 @@ import UserMenuItemsList from "discourse/components/user-menu/items-list";
import I18n from "I18n";
import { action } from "@ember/object";
import { ajax } from "discourse/lib/ajax";
import { postRNWebviewMessage } from "discourse/lib/utilities";
import {
mergeSortedLists,
postRNWebviewMessage,
} from "discourse/lib/utilities";
import showModal from "discourse/lib/show-modal";
import { inject as service } from "@ember/service";
import UserMenuNotificationItem from "discourse/lib/user-menu/notification-item";
import Notification from "discourse/models/notification";
import UserMenuReviewable from "discourse/models/user-menu-reviewable";
import UserMenuReviewableItem from "discourse/lib/user-menu/reviewable-item";
export default class UserMenuNotificationsList extends UserMenuItemsList {
@service currentUser;
@ -31,7 +36,11 @@ export default class UserMenuNotificationsList extends UserMenuItemsList {
}
get showDismiss() {
return this.items.some((item) => !item.notification.read);
return Object.keys(
this.currentUser.get("grouped_unread_notifications") || {}
).any((key) => {
return this.currentUser.get(`grouped_unread_notifications.${key}`) > 0;
});
}
get dismissTitle() {
@ -60,27 +69,70 @@ export default class UserMenuNotificationsList extends UserMenuItemsList {
limit: 30,
recent: true,
bump_last_seen_reviewable: true,
silent: this.currentUser.enforcedSecondFactor,
};
if (this.currentUser.enforcedSecondFactor) {
params.silent = true;
}
const types = this.filterByTypes;
if (types?.length > 0) {
params.filter_by_types = types.join(",");
params.silent = true;
}
const collection = await this.store
.findStale("notification", params)
.refresh();
const notifications = collection.content;
await Notification.applyTransformations(notifications);
return notifications.map((notification) => {
return new UserMenuNotificationItem({
notification,
currentUser: this.currentUser,
siteSettings: this.siteSettings,
site: this.site,
const content = [];
const data = await ajax("/notifications", { data: params });
const notifications = await Notification.initializeNotifications(
data.notifications
);
const reviewables = data.pending_reviewables?.map((r) =>
UserMenuReviewable.create(r)
);
if (reviewables?.length) {
const firstReadNotificationIndex = notifications.findIndex((n) => n.read);
const unreadNotifications = notifications.splice(
0,
firstReadNotificationIndex
);
mergeSortedLists(
unreadNotifications,
reviewables,
(notification, reviewable) => {
const notificationCreatedAt = new Date(notification.created_at);
const reviewableCreatedAt = new Date(reviewable.created_at);
return reviewableCreatedAt > notificationCreatedAt;
}
).forEach((item) => {
const props = {
currentUser: this.currentUser,
siteSettings: this.siteSettings,
site: this.site,
};
if (item instanceof Notification) {
props.notification = item;
content.push(new UserMenuNotificationItem(props));
} else {
props.reviewable = item;
content.push(new UserMenuReviewableItem(props));
}
});
}
notifications.forEach((notification) => {
content.push(
new UserMenuNotificationItem({
notification,
currentUser: this.currentUser,
siteSettings: this.siteSettings,
site: this.site,
})
);
});
return content;
}
dismissWarningModal() {

26
app/assets/javascripts/discourse/app/lib/utilities.js
View File

@ -606,5 +606,31 @@ function clipboardCopyFallback(text) {
return success;
}
// this function takes 2 sorted lists and returns another sorted list that
// contains both of the original lists.
// you need to provide a callback as the 3rd argument that will be called with
// an item from the first list (1st callback argument) and another item from
// the second list (2nd callback argument). The callback should return true if
// its 2nd argument should go before its 1st argument and return false
// otherwise.
export function mergeSortedLists(list1, list2, comparator) {
let index1 = 0;
let index2 = 0;
const merged = [];
while (index1 < list1.length || index2 < list2.length) {
if (
index1 === list1.length ||
(index2 < list2.length && comparator(list1[index1], list2[index2]))
) {
merged.push(list2[index2]);
index2++;
} else {
merged.push(list1[index1]);
index1++;
}
}
return merged;
}
// This prevents a mini racer crash
export default {};

6
app/assets/javascripts/discourse/app/models/notification.js
View File

@ -7,5 +7,11 @@ export default class Notification extends RestModel {
await applyModelTransformations("notification", notifications);
}
static async initializeNotifications(rawList) {
const notifications = rawList.map((n) => this.create(n));
await this.applyTransformations(notifications);
return notifications;
}
@tracked read;
}

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

@ -1,10 +1,11 @@
import { module, test } from "qunit";
import { setupRenderingTest } from "discourse/tests/helpers/component-test";
import { exists, query } from "discourse/tests/helpers/qunit-helpers";
import { exists, query, queryAll } from "discourse/tests/helpers/qunit-helpers";
import { click, render } from "@ember/test-helpers";
import { cloneJSON } from "discourse-common/lib/object";
import NotificationFixtures from "discourse/tests/fixtures/notification-fixtures";
import { hbs } from "ember-cli-htmlbars";
import { NOTIFICATION_TYPES } from "discourse/tests/fixtures/concerns/notification-types";
import pretender, { response } from "discourse/tests/helpers/create-pretender";
import I18n from "I18n";
@ -78,11 +79,10 @@ module(
);
});
test("has a dismiss button if some notifications are not read", async function (assert) {
notificationsData.forEach((notification) => {
notification.read = true;
test("has a dismiss button if some notification types have unread notifications", async function (assert) {
this.currentUser.set("grouped_unread_notifications", {
[NOTIFICATION_TYPES.mentioned]: 1,
});
notificationsData[0].read = false;
await render(template);
const dismissButton = query(
".panel-body-bottom .btn.notifications-dismiss"
@ -109,9 +109,8 @@ module(
test("dismiss button makes a request to the server and then refreshes the notifications list", async function (assert) {
await render(template);
notificationsData = getNotificationsData();
notificationsData.forEach((notification) => {
notification.read = true;
this.currentUser.set("grouped_unread_notifications", {
[NOTIFICATION_TYPES.mentioned]: 1,
});
assert.strictEqual(notificationsFetches, 1);
await click(".panel-body-bottom .btn.notifications-dismiss");
@ -126,5 +125,114 @@ module(
"dismiss button is not shown"
);
});
test("all notifications tab shows pending reviewables and sorts them with unread notifications based on their creation date", async function (assert) {
pretender.get("/notifications", () => {
return response({
notifications: [
{
id: 6,
user_id: 1,
notification_type: NOTIFICATION_TYPES.mentioned,
read: false,
high_priority: false,
created_at: "2021-11-25T19:31:13.241Z",
post_number: 6,
topic_id: 10,
fancy_title: "Unread notification #01",
slug: "unread-notification-01",
data: {
topic_title: "Unread notification #01",
original_post_id: 20,
original_post_type: 1,
original_username: "discobot",
revision_number: null,
display_username: "discobot",
},
},
{
id: 13,
user_id: 1,
notification_type: NOTIFICATION_TYPES.replied,
read: false,
high_priority: false,
created_at: "2021-08-25T19:31:13.241Z",
post_number: 6,
topic_id: 10,
fancy_title: "Unread notification #02",
slug: "unread-notification-02",
data: {
topic_title: "Unread notification #02",
original_post_id: 20,
original_post_type: 1,
original_username: "discobot",
revision_number: null,
display_username: "discobot",
},
},
{
id: 81,
user_id: 1,
notification_type: NOTIFICATION_TYPES.mentioned,
read: true,
high_priority: false,
created_at: "2022-10-25T19:31:13.241Z",
post_number: 6,
topic_id: 10,
fancy_title: "Read notification #01",
slug: "read-notification-01",
data: {
topic_title: "Read notification #01",
original_post_id: 20,
original_post_type: 1,
original_username: "discobot",
revision_number: null,
display_username: "discobot",
},
},
],
pending_reviewables: [
{
flagger_username: "sayo2",
id: 83,
pending: true,
topic_fancy_title: "anything hello world 0011",
type: "ReviewableQueuedPost",
created_at: "2022-09-25T19:31:13.241Z",
},
{
flagger_username: "sayo2",
id: 78,
pending: true,
topic_fancy_title: "anything hello world 0033",
type: "ReviewableQueuedPost",
created_at: "2021-06-25T19:31:13.241Z",
},
],
});
});
await render(template);
const items = queryAll("ul li");
assert.ok(
items[0].textContent.includes("hello world 0011"),
"the first pending reviewable is displayed 1st because it's most recent among pending reviewables and unread notifications"
);
assert.ok(
items[1].textContent.includes("Unread notification #01"),
"the first unread notification is displayed 2nd because it's the 2nd most recent among pending reviewables and unread notifications"
);
assert.ok(
items[2].textContent.includes("Unread notification #02"),
"the second unread notification is displayed 3rd because it's the 3rd most recent among pending reviewables and unread notifications"
);
assert.ok(
items[3].textContent.includes("hello world 0033"),
"the second pending reviewable is displayed 4th because it's the 4th most recent among pending reviewables and unread notifications"
);
assert.ok(
items[4].textContent.includes("Read notification #01"),
"read notifications come after the pending reviewables and unread notifications"
);
});
}
);

40
app/assets/javascripts/discourse/tests/unit/lib/utilities-test.js
View File

@ -12,6 +12,7 @@ import {
getRawSize,
inCodeBlock,
initializeDefaultHomepage,
mergeSortedLists,
setCaretPosition,
setDefaultHomepage,
slugify,
@ -288,6 +289,45 @@ discourseModule("Unit | Utilities", function () {
}
});
});
test("mergeSortedLists", function (assert) {
const comparator = (a, b) => b > a;
assert.deepEqual(
mergeSortedLists([], [1, 2, 3], comparator),
[1, 2, 3],
"it doesn't error when the first list is blank"
);
assert.deepEqual(
mergeSortedLists([3, 2, 1], [], comparator),
[3, 2, 1],
"it doesn't error when the second list is blank"
);
assert.deepEqual(
mergeSortedLists([], [], comparator),
[],
"it doesn't error when the both lists are blank"
);
assert.deepEqual(
mergeSortedLists([5, 4, 0, -1], [1], comparator),
[5, 4, 1, 0, -1],
"it correctly merges lists when one list has 1 item only"
);
assert.deepEqual(
mergeSortedLists([2], [1], comparator),
[2, 1],
"it correctly merges lists when both lists has 1 item each"
);
assert.deepEqual(
mergeSortedLists([1], [1], comparator),
[1, 1],
"it correctly merges lists when both lists has 1 item and their items are identical"
);
assert.deepEqual(
mergeSortedLists([5, 4, 3, 2, 1], [6, 2, 1], comparator),
[6, 5, 4, 3, 2, 2, 1, 1],
"it correctly merges lists that share common items"
);
});
});
discourseModule("Unit | Utilities | clipboard", function (hooks) {

29
app/controllers/notifications_controller.rb
View File

@ -30,8 +30,11 @@ class NotificationsController < ApplicationController
limit = (params[:limit] || 15).to_i
limit = 50 if limit > 50
include_reviewables = false
if SiteSetting.enable_experimental_sidebar_hamburger
notifications = Notification.prioritized_list(current_user, count: limit, types: notification_types)
# notification_types is blank for the "all notifications" user menu tab
include_reviewables = notification_types.blank? && guardian.can_see_review_queue?
else
notifications = Notification.recent_report(current_user, limit, notification_types)
end
@ -43,26 +46,28 @@ class NotificationsController < ApplicationController
end
end
if !params.has_key?(:silent) && params[:bump_last_seen_reviewable] && !@readonly_mode
if !params.has_key?(:silent) && params[:bump_last_seen_reviewable] && !@readonly_mode && include_reviewables
current_user_id = current_user.id
Scheduler::Defer.later "bump last seen reviewable for user" do
# we lookup current_user again in the background thread to avoid
# concurrency issues where the objects returned by the current_user
# and/or methods are changed by the time the deferred block is
# executed
user = User.find_by(id: current_user_id)
next if user.blank?
new_guardian = Guardian.new(user)
if new_guardian.can_see_review_queue?
user.bump_last_seen_reviewable!
end
# concurrency issues where the user object returned by the
# current_user controller method is changed by the time the deferred
# block is executed
User.find_by(id: current_user_id)&.bump_last_seen_reviewable!
end
end
render_json_dump(
json = {
notifications: serialize_data(notifications, NotificationSerializer),
seen_notification_id: current_user.seen_notification_id
)
}
if include_reviewables
json[:pending_reviewables] = Reviewable.basic_serializers_for_list(
Reviewable.user_menu_list_for(current_user),
current_user
).as_json
end
render_json_dump(json)
else
offset = params[:offset].to_i

6
app/controllers/reviewables_controller.rb
View File

@ -71,9 +71,11 @@ class ReviewablesController < ApplicationController
end
def user_menu_list
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 }
reviewables: Reviewable.basic_serializers_for_list(
Reviewable.user_menu_list_for(current_user),
current_user
).as_json
}
render_json_dump(json, rest_serializer: true)
end

8
app/models/reviewable.rb
View File

@ -534,6 +534,14 @@ class Reviewable < ActiveRecord::Base
results
end
def self.user_menu_list_for(user, limit: 30)
list_for(user, limit: limit, status: :pending).to_a
end
def self.basic_serializers_for_list(reviewables, user)
reviewables.map { |r| r.basic_serializer.new(r, scope: user.guardian, root: nil) }
end
def serializer
self.class.serializer_for(self)
end

2
app/serializers/basic_reviewable_serializer.rb
View File

@ -1,7 +1,7 @@
# frozen_string_literal: true
class BasicReviewableSerializer < ApplicationSerializer
attributes :flagger_username, :id, :type, :pending
attributes :flagger_username, :id, :type, :pending, :created_at
def flagger_username
object.created_by&.username

142
spec/requests/notifications_controller_spec.rb
View File

@ -87,60 +87,6 @@ RSpec.describe NotificationsController do
Discourse.clear_redis_readonly!
end
it "should not bump last seen reviewable in readonly mode" do
user.update!(admin: true)
Fabricate(:reviewable)
Discourse.received_redis_readonly!
expect {
get "/notifications.json", params: { recent: true }
expect(response.status).to eq(200)
}.not_to change { user.reload.last_seen_reviewable_id }
ensure
Discourse.clear_redis_readonly!
end
it "should not bump last seen reviewable if the user can't seen reviewables" do
Fabricate(:reviewable)
expect {
get "/notifications.json", params: { recent: true, bump_last_seen_reviewable: true }
expect(response.status).to eq(200)
}.not_to change { user.reload.last_seen_reviewable_id }
end
it "should not bump last seen reviewable if the silent param is present" do
user.update!(admin: true)
Fabricate(:reviewable)
expect {
get "/notifications.json", params: {
recent: true,
silent: true,
bump_last_seen_reviewable: true
}
expect(response.status).to eq(200)
}.not_to change { user.reload.last_seen_reviewable_id }
end
it "should not bump last seen reviewable if the bump_last_seen_reviewable param is not present" do
user.update!(admin: true)
Fabricate(:reviewable)
expect {
get "/notifications.json", params: { recent: true, silent: true }
expect(response.status).to eq(200)
}.not_to change { user.reload.last_seen_reviewable_id }
end
it "bumps last_seen_reviewable_id" do
user.update!(admin: true)
expect(user.last_seen_reviewable_id).to eq(nil)
reviewable = Fabricate(:reviewable)
get "/notifications.json", params: { recent: true, bump_last_seen_reviewable: true }
expect(user.reload.last_seen_reviewable_id).to eq(reviewable.id)
reviewable2 = Fabricate(:reviewable)
get "/notifications.json", params: { recent: true, bump_last_seen_reviewable: true }
expect(user.reload.last_seen_reviewable_id).to eq(reviewable2.id)
end
it "get notifications with all filters" do
notification = Fabricate(:notification, user: user)
notification2 = Fabricate(:notification, user: user)
@ -202,6 +148,7 @@ RSpec.describe NotificationsController do
created_at: 4.minutes.ago
)
end
fab!(:pending_reviewable) { Fabricate(:reviewable) }
it "gets notifications list with unread ones at the top when the setting is enabled" do
SiteSetting.enable_experimental_sidebar_hamburger = true
@ -228,6 +175,93 @@ RSpec.describe NotificationsController do
read_high_priority.id
])
end
it "should not bump last seen reviewable in readonly mode" do
SiteSetting.enable_experimental_sidebar_hamburger = true
user.update!(admin: true)
Discourse.received_redis_readonly!
expect {
get "/notifications.json", params: { recent: true, bump_last_seen_reviewable: true }
expect(response.status).to eq(200)
}.not_to change { user.reload.last_seen_reviewable_id }
ensure
Discourse.clear_redis_readonly!
end
it "should not bump last seen reviewable if the user can't see reviewables" do
SiteSetting.enable_experimental_sidebar_hamburger = true
expect {
get "/notifications.json", params: { recent: true, bump_last_seen_reviewable: true }
expect(response.status).to eq(200)
}.not_to change { user.reload.last_seen_reviewable_id }
end
it "should not bump last seen reviewable if the silent param is present" do
SiteSetting.enable_experimental_sidebar_hamburger = true
user.update!(admin: true)
expect {
get "/notifications.json", params: {
recent: true,
silent: true,
bump_last_seen_reviewable: true
}
expect(response.status).to eq(200)
}.not_to change { user.reload.last_seen_reviewable_id }
end
it "should not bump last seen reviewable if the bump_last_seen_reviewable param is not present" do
SiteSetting.enable_experimental_sidebar_hamburger = true
user.update!(admin: true)
expect {
get "/notifications.json", params: { recent: true }
expect(response.status).to eq(200)
}.not_to change { user.reload.last_seen_reviewable_id }
end
it "bumps last_seen_reviewable_id" do
SiteSetting.enable_experimental_sidebar_hamburger = true
user.update!(admin: true)
expect(user.last_seen_reviewable_id).to eq(nil)
get "/notifications.json", params: { recent: true, bump_last_seen_reviewable: true }
expect(user.reload.last_seen_reviewable_id).to eq(pending_reviewable.id)
reviewable2 = Fabricate(:reviewable)
get "/notifications.json", params: { recent: true, bump_last_seen_reviewable: true }
expect(user.reload.last_seen_reviewable_id).to eq(reviewable2.id)
end
it "includes pending reviewables when the setting is enabled" do
SiteSetting.enable_experimental_sidebar_hamburger = true
user.update!(admin: true)
pending_reviewable2 = Fabricate(:reviewable, created_at: 4.minutes.ago)
Fabricate(:reviewable, status: Reviewable.statuses[:approved])
Fabricate(:reviewable, status: Reviewable.statuses[:rejected])
get "/notifications.json", params: { recent: true }
expect(response.status).to eq(200)
expect(response.parsed_body["pending_reviewables"].map { |r| r["id"] }).to eq([
pending_reviewable.id,
pending_reviewable2.id
])
end
it "doesn't include reviewables when the setting is disabled" do
SiteSetting.enable_experimental_sidebar_hamburger = false
user.update!(admin: true)
get "/notifications.json", params: { recent: true }
expect(response.status).to eq(200)
expect(response.parsed_body.key?("pending_reviewables")).to eq(false)
end
it "doesn't include reviewables if the user can't see the review queue" do
SiteSetting.enable_experimental_sidebar_hamburger = true
user.update!(admin: false)
get "/notifications.json", params: { recent: true }
expect(response.status).to eq(200)
expect(response.parsed_body.key?("pending_reviewables")).to eq(false)
end
end
context "when filter_by_types param is present" do

8
spec/support/common_basic_reviewable_serializer.rb
View File

@ -38,4 +38,12 @@ shared_examples "basic reviewable attributes" do
expect(subject[:flagger_username]).to eq("gg.osama")
end
end
describe "#created_at" do
it "serializes the reviewable's created_at field correctly" do
time = 10.minutes.ago
reviewable.update!(created_at: time)
expect(subject[:created_at]).to eq(time)
end
end
end