diff --git a/app/assets/javascripts/discourse/app/components/user-menu/likes-notifications-list.js b/app/assets/javascripts/discourse/app/components/user-menu/likes-notifications-list.js new file mode 100644 index 00000000000..9deb8a7dd3c --- /dev/null +++ b/app/assets/javascripts/discourse/app/components/user-menu/likes-notifications-list.js @@ -0,0 +1,7 @@ +import UserMenuNotificationsList from "discourse/components/user-menu/notifications-list"; + +export default class UserMenuLikesNotificationsList extends UserMenuNotificationsList { + get filterByTypes() { + return ["liked", "liked_consolidated"]; + } +} diff --git a/app/assets/javascripts/discourse/app/components/user-menu/mentions-notifications-list.js b/app/assets/javascripts/discourse/app/components/user-menu/mentions-notifications-list.js new file mode 100644 index 00000000000..a6974ab267d --- /dev/null +++ b/app/assets/javascripts/discourse/app/components/user-menu/mentions-notifications-list.js @@ -0,0 +1,7 @@ +import UserMenuNotificationsList from "discourse/components/user-menu/notifications-list"; + +export default class UserMenuMentionsNotificationsList extends UserMenuNotificationsList { + get filterByTypes() { + return ["mentioned"]; + } +} diff --git a/app/assets/javascripts/discourse/app/components/user-menu/menu.js b/app/assets/javascripts/discourse/app/components/user-menu/menu.js index 1d50097c087..a28bf8ba8af 100644 --- a/app/assets/javascripts/discourse/app/components/user-menu/menu.js +++ b/app/assets/javascripts/discourse/app/components/user-menu/menu.js @@ -1,23 +1,98 @@ import GlimmerComponent from "discourse/components/glimmer"; import { tracked } from "@glimmer/tracking"; import { action } from "@ember/object"; +import UserMenuTab from "discourse/lib/user-menu/tab"; const DEFAULT_TAB_ID = "all-notifications"; const DEFAULT_PANEL_COMPONENT = "user-menu/notifications-list"; +const CORE_TOP_TABS = [ + class extends UserMenuTab { + get id() { + return DEFAULT_TAB_ID; + } + + get icon() { + return "bell"; + } + + get panelComponent() { + return DEFAULT_PANEL_COMPONENT; + } + }, + + class extends UserMenuTab { + get id() { + return "replies"; + } + + get icon() { + return "reply"; + } + + get panelComponent() { + return "user-menu/replies-notifications-list"; + } + }, + + class extends UserMenuTab { + get id() { + return "mentions"; + } + + get icon() { + return "at"; + } + + get panelComponent() { + return "user-menu/mentions-notifications-list"; + } + }, + + class extends UserMenuTab { + get id() { + return "likes"; + } + + get icon() { + return "heart"; + } + + get panelComponent() { + return "user-menu/likes-notifications-list"; + } + + get shouldDisplay() { + return !this.currentUser.likes_notifications_disabled; + } + }, +]; + export default class UserMenu extends GlimmerComponent { @tracked currentTabId = DEFAULT_TAB_ID; @tracked currentPanelComponent = DEFAULT_PANEL_COMPONENT; - get topTabs() { - const tabs = this._coreTopTabs; + constructor() { + super(...arguments); + this.topTabs = this._topTabs; + this.bottomTabs = this._bottomTabs; + } + + get _topTabs() { + const tabs = []; + CORE_TOP_TABS.forEach((tabClass) => { + const tab = new tabClass(this.currentUser, this.siteSettings, this.site); + if (tab.shouldDisplay) { + tabs.push(tab); + } + }); return tabs.map((tab, index) => { tab.position = index; return tab; }); } - get bottomTabs() { + get _bottomTabs() { const topTabsLength = this.topTabs.length; return this._coreBottomTabs.map((tab, index) => { tab.position = index + topTabsLength; @@ -25,16 +100,6 @@ export default class UserMenu extends GlimmerComponent { }); } - get _coreTopTabs() { - return [ - { - id: DEFAULT_TAB_ID, - icon: "bell", - panelComponent: DEFAULT_PANEL_COMPONENT, - }, - ]; - } - get _coreBottomTabs() { return [ { diff --git a/app/assets/javascripts/discourse/app/components/user-menu/notifications-list.js b/app/assets/javascripts/discourse/app/components/user-menu/notifications-list.js index f9c659d6a31..a60cc0ebf73 100644 --- a/app/assets/javascripts/discourse/app/components/user-menu/notifications-list.js +++ b/app/assets/javascripts/discourse/app/components/user-menu/notifications-list.js @@ -29,9 +29,9 @@ export default class UserMenuNotificationsList extends UserMenuItemsList { get itemsCacheKey() { let key = "recent-notifications"; - const types = this.filterByTypes?.toString(); - if (types) { - key += `-type-${types}`; + const types = this.filterByTypes; + if (types?.length > 0) { + key += `-type-${types.join(",")}`; } return key; } @@ -52,9 +52,10 @@ export default class UserMenuNotificationsList extends UserMenuItemsList { silent: this.currentUser.enforcedSecondFactor, }; - const types = this.filterByTypes?.toString(); - if (types) { - params.filter_by_types = types; + const types = this.filterByTypes; + if (types?.length > 0) { + params.filter_by_types = types.join(","); + params.silent = true; } return this.store .findStale("notification", params) @@ -64,6 +65,7 @@ export default class UserMenuNotificationsList extends UserMenuItemsList { dismissWarningModal() { // TODO: add warning modal when there are unread high pri notifications + // TODO: review child components and override if necessary return null; } diff --git a/app/assets/javascripts/discourse/app/components/user-menu/replies-notifications-list.js b/app/assets/javascripts/discourse/app/components/user-menu/replies-notifications-list.js new file mode 100644 index 00000000000..3403dac0d64 --- /dev/null +++ b/app/assets/javascripts/discourse/app/components/user-menu/replies-notifications-list.js @@ -0,0 +1,7 @@ +import UserMenuNotificationsList from "discourse/components/user-menu/notifications-list"; + +export default class UserMenuRepliesNotificationsList extends UserMenuNotificationsList { + get filterByTypes() { + return ["replied"]; + } +} diff --git a/app/assets/javascripts/discourse/app/lib/user-menu/tab.js b/app/assets/javascripts/discourse/app/lib/user-menu/tab.js new file mode 100644 index 00000000000..cc6999a7e4a --- /dev/null +++ b/app/assets/javascripts/discourse/app/lib/user-menu/tab.js @@ -0,0 +1,27 @@ +export default class UserMenuTab { + constructor(currentUser, siteSettings, site) { + this.currentUser = currentUser; + this.siteSettings = siteSettings; + this.site = site; + } + + get shouldDisplay() { + return true; + } + + get count() { + return 0; + } + + get panelComponent() { + throw new Error("not implemented"); + } + + get id() { + throw new Error("not implemented"); + } + + get icon() { + throw new Error("not implemented"); + } +} diff --git a/app/assets/javascripts/discourse/tests/integration/components/user-menu/menu-test.js b/app/assets/javascripts/discourse/tests/integration/components/user-menu/menu-test.js index 45cf7c6c201..54b0cc7c23b 100644 --- a/app/assets/javascripts/discourse/tests/integration/components/user-menu/menu-test.js +++ b/app/assets/javascripts/discourse/tests/integration/components/user-menu/menu-test.js @@ -1,14 +1,26 @@ import { module, test } from "qunit"; import { setupRenderingTest } from "discourse/tests/helpers/component-test"; -import { query, queryAll } from "discourse/tests/helpers/qunit-helpers"; -import { render } from "@ember/test-helpers"; +import { exists, query, queryAll } from "discourse/tests/helpers/qunit-helpers"; +import { click, render } from "@ember/test-helpers"; +import { NOTIFICATION_TYPES } from "discourse/tests/fixtures/concerns/notification-types"; import { hbs } from "ember-cli-htmlbars"; +import pretender from "discourse/tests/helpers/create-pretender"; module("Integration | Component | user-menu", function (hooks) { setupRenderingTest(hooks); const template = hbs``; + test("default tab is all notifications", async function (assert) { + await render(template); + const activeTab = query(".top-tabs.tabs-list .btn.active"); + assert.strictEqual(activeTab.id, "user-menu-button-all-notifications"); + const notifications = queryAll("#quick-access-all-notifications ul li"); + assert.strictEqual(notifications[0].className, "edited"); + assert.strictEqual(notifications[1].className, "replied"); + assert.strictEqual(notifications[2].className, "liked-consolidated"); + }); + test("notifications panel has a11y attributes", async function (assert) { await render(template); const panel = query("#quick-access-all-notifications"); @@ -26,21 +38,27 @@ module("Integration | Component | user-menu", function (hooks) { assert.strictEqual(activeTab.getAttribute("aria-selected"), "true"); }); + test("inactive tab has a11y attributes that indicate it's inactive", async function (assert) { + await render(template); + const inactiveTab = query(".top-tabs.tabs-list .btn:not(.active)"); + assert.strictEqual(inactiveTab.getAttribute("tabindex"), "-1"); + assert.strictEqual(inactiveTab.getAttribute("aria-selected"), "false"); + }); + test("the menu has a group of tabs at the top", async function (assert) { await render(template); const tabs = queryAll(".top-tabs.tabs-list .btn"); - assert.strictEqual(tabs.length, 1); - ["all-notifications"].forEach((tab, index) => { - assert.strictEqual(tabs[index].id, `user-menu-button-${tab}`); - assert.strictEqual( - tabs[index].getAttribute("data-tab-number"), - index.toString() - ); - assert.strictEqual( - tabs[index].getAttribute("aria-controls"), - `quick-access-${tab}` - ); - }); + assert.strictEqual(tabs.length, 4); + ["all-notifications", "replies", "mentions", "likes"].forEach( + (tab, index) => { + assert.strictEqual(tabs[index].id, `user-menu-button-${tab}`); + assert.strictEqual(tabs[index].dataset.tabNumber, index.toString()); + assert.strictEqual( + tabs[index].getAttribute("aria-controls"), + `quick-access-${tab}` + ); + } + ); }); test("the menu has a group of tabs at the bottom", async function (assert) { @@ -49,7 +67,162 @@ module("Integration | Component | user-menu", function (hooks) { assert.strictEqual(tabs.length, 1); const preferencesTab = tabs[0]; assert.ok(preferencesTab.href.endsWith("/u/eviltrout/preferences")); - assert.strictEqual(preferencesTab.getAttribute("data-tab-number"), "1"); + assert.strictEqual(preferencesTab.dataset.tabNumber, "4"); assert.strictEqual(preferencesTab.getAttribute("tabindex"), "-1"); }); + + test("likes tab is hidden if current user's like notifications frequency is 'never'", async function (assert) { + this.currentUser.set("likes_notifications_disabled", true); + await render(template); + assert.ok(!exists("#user-menu-button-likes")); + + const tabs = Array.from(queryAll(".tabs-list .btn")); // top and bottom tabs + assert.strictEqual(tabs.length, 4); + + assert.deepEqual( + tabs.map((t) => t.dataset.tabNumber), + [...Array(4).keys()].map((n) => n.toString()), + "data-tab-number of the tabs has no gaps when the likes tab is hidden" + ); + }); + + test("changing tabs", async function (assert) { + await render(template); + let queryParams; + pretender.get("/notifications", (request) => { + queryParams = request.queryParams; + let data; + if (queryParams.filter_by_types === "mentioned") { + data = [ + { + id: 6, + user_id: 1, + notification_type: NOTIFICATION_TYPES.mentioned, + read: true, + high_priority: false, + created_at: "2021-11-25T19:31:13.241Z", + post_number: 6, + topic_id: 10, + fancy_title: "Greetings!", + slug: "greetings", + data: { + topic_title: "Greetings!", + original_post_id: 20, + original_post_type: 1, + original_username: "discobot", + revision_number: null, + display_username: "discobot", + }, + }, + ]; + } else if (queryParams.filter_by_types === "liked,liked_consolidated") { + data = [ + { + id: 60, + user_id: 1, + notification_type: NOTIFICATION_TYPES.liked, + read: true, + high_priority: false, + created_at: "2021-11-25T19:31:13.241Z", + post_number: 6, + topic_id: 10, + fancy_title: "Greetings!", + slug: "greetings", + data: { + topic_title: "Greetings!", + original_post_id: 20, + original_post_type: 1, + original_username: "discobot", + revision_number: null, + display_username: "discobot", + }, + }, + { + id: 63, + user_id: 1, + notification_type: NOTIFICATION_TYPES.liked, + read: true, + high_priority: false, + created_at: "2021-11-25T19:31:13.241Z", + post_number: 6, + topic_id: 10, + fancy_title: "Greetings!", + slug: "greetings", + data: { + topic_title: "Greetings!", + original_post_id: 20, + original_post_type: 1, + original_username: "discobot", + revision_number: null, + display_username: "discobot", + }, + }, + { + id: 20, + user_id: 1, + notification_type: NOTIFICATION_TYPES.liked_consolidated, + read: true, + high_priority: false, + created_at: "2021-11-25T19:31:13.241Z", + post_number: 6, + topic_id: 10, + fancy_title: "Greetings 123!", + slug: "greetings 123", + data: { + topic_title: "Greetings 123!", + original_post_id: 20, + original_post_type: 1, + original_username: "discobot", + revision_number: null, + display_username: "discobot", + }, + }, + ]; + } else { + throw new Error( + `unexpected notification type ${queryParams.filter_by_types}` + ); + } + + return [ + 200, + { "Content-Type": "application/json" }, + { notifications: data }, + ]; + }); + + await click("#user-menu-button-mentions"); + assert.ok(exists("#quick-access-mentions.quick-access-panel")); + assert.strictEqual( + queryParams.filter_by_types, + "mentioned", + "request params has filter_by_types set to `mentioned`" + ); + assert.strictEqual(queryParams.silent, "true"); + let activeTabs = queryAll(".top-tabs .btn.active"); + assert.strictEqual(activeTabs.length, 1); + assert.strictEqual( + activeTabs[0].id, + "user-menu-button-mentions", + "active tab is now the mentions tab" + ); + assert.strictEqual(queryAll("#quick-access-mentions ul li").length, 1); + + await click("#user-menu-button-likes"); + assert.ok(exists("#quick-access-likes.quick-access-panel")); + assert.strictEqual( + queryParams.filter_by_types, + "liked,liked_consolidated", + "request params has filter_by_types set to `liked` and `liked_consolidated" + ); + assert.strictEqual(queryParams.silent, "true"); + activeTabs = queryAll(".top-tabs .btn.active"); + assert.strictEqual(activeTabs.length, 1); + assert.strictEqual( + activeTabs[0].id, + "user-menu-button-likes", + "active tab is now the likes tab" + ); + assert.strictEqual(queryAll("#quick-access-likes ul li").length, 3); + }); }); diff --git a/app/assets/javascripts/discourse/tests/integration/components/user-menu/notifications-list-test.js b/app/assets/javascripts/discourse/tests/integration/components/user-menu/notifications-list-test.js index 0d39fe23b0b..082201aec06 100644 --- a/app/assets/javascripts/discourse/tests/integration/components/user-menu/notifications-list-test.js +++ b/app/assets/javascripts/discourse/tests/integration/components/user-menu/notifications-list-test.js @@ -53,6 +53,11 @@ module( ); }); + test("doesn't request the full notifications list in silent mode", async function (assert) { + await render(template); + assert.strictEqual(queryParams.silent, undefined); + }); + test("displays a show all button that takes to the notifications page of the current user", async function (assert) { await render(template); const showAllBtn = query(".panel-body-bottom .btn.show-all"); diff --git a/app/controllers/notifications_controller.rb b/app/controllers/notifications_controller.rb index d89917b4f18..eabc490a31c 100644 --- a/app/controllers/notifications_controller.rb +++ b/app/controllers/notifications_controller.rb @@ -18,11 +18,19 @@ class NotificationsController < ApplicationController guardian.ensure_can_see_notifications!(user) + if notification_types = params[:filter_by_types]&.split(",").presence + notification_types.map! do |type| + Notification.types[type.to_sym] || ( + raise Discourse::InvalidParameters.new("invalid notification type: #{type}") + ) + end + end + if params[:recent].present? limit = (params[:limit] || 15).to_i limit = 50 if limit > 50 - notifications = Notification.recent_report(current_user, limit) + notifications = Notification.recent_report(current_user, limit, notification_types) changed = false if notifications.present? && !(params.has_key?(:silent) || @readonly_mode) @@ -31,11 +39,15 @@ class NotificationsController < ApplicationController changed = current_user.saw_notification_id(max_id) end - user.reload - user.publish_notifications_state if changed + if changed + current_user.reload + current_user.publish_notifications_state + end - render_json_dump(notifications: serialize_data(notifications, NotificationSerializer), - seen_notification_id: current_user.seen_notification_id) + render_json_dump( + notifications: serialize_data(notifications, NotificationSerializer), + seen_notification_id: current_user.seen_notification_id + ) else offset = params[:offset].to_i diff --git a/app/models/notification.rb b/app/models/notification.rb index 168f6e40c73..022745b4472 100644 --- a/app/models/notification.rb +++ b/app/models/notification.rb @@ -205,7 +205,7 @@ class Notification < ActiveRecord::Base Post.find_by(topic_id: topic_id, post_number: post_number) end - def self.recent_report(user, count = nil) + def self.recent_report(user, count = nil, types = []) return unless user && user.user_option count ||= 10 @@ -214,6 +214,7 @@ class Notification < ActiveRecord::Base .recent(count) .includes(:topic) + notifications = notifications.where(notification_type: types) if types.present? if user.user_option.like_notification_frequency == UserOption.like_notification_frequency_type[:never] [ Notification.types[:liked], @@ -228,17 +229,23 @@ class Notification < ActiveRecord::Base notifications = notifications.to_a if notifications.present? - - ids = DB.query_single(<<~SQL, limit: count.to_i) + builder = DB.build(<<~SQL) SELECT n.id FROM notifications n - WHERE - n.high_priority = TRUE AND - n.user_id = #{user.id.to_i} AND - NOT read + /*where*/ ORDER BY n.id ASC - LIMIT :limit + /*limit*/ SQL + builder.where(<<~SQL, user_id: user.id) + n.high_priority = TRUE AND + n.user_id = :user_id AND + NOT read + SQL + builder.where("notification_type IN (:types)", types: types) if types.present? + builder.limit(count.to_i) + + ids = builder.query_single + if ids.length > 0 notifications += user .notifications diff --git a/app/models/user_option.rb b/app/models/user_option.rb index 0db31719a95..1752ae4fbc3 100644 --- a/app/models/user_option.rb +++ b/app/models/user_option.rb @@ -200,6 +200,10 @@ class UserOption < ActiveRecord::Base email_messages_level == UserOption.email_level_types[:never] end + def likes_notifications_disabled? + like_notification_frequency == UserOption.like_notification_frequency_type[:never] + end + def self.user_tzinfo(user_id) timezone = UserOption.where(user_id: user_id).pluck(:timezone).first || 'UTC' diff --git a/app/serializers/current_user_serializer.rb b/app/serializers/current_user_serializer.rb index 0625e336cf2..d20ca3c3374 100644 --- a/app/serializers/current_user_serializer.rb +++ b/app/serializers/current_user_serializer.rb @@ -75,6 +75,7 @@ class CurrentUserSerializer < BasicUserSerializer :status, :sidebar_category_ids, :sidebar_tag_names, + :likes_notifications_disabled, :redesigned_user_menu_enabled delegate :user_stat, to: :object, private: true @@ -346,4 +347,8 @@ class CurrentUserSerializer < BasicUserSerializer end @redesigned_user_menu_enabled = object.redesigned_user_menu_enabled? end + + def likes_notifications_disabled + object.user_option&.likes_notifications_disabled? + end end diff --git a/spec/requests/notifications_controller_spec.rb b/spec/requests/notifications_controller_spec.rb index a305e4eb928..8009153e1be 100644 --- a/spec/requests/notifications_controller_spec.rb +++ b/spec/requests/notifications_controller_spec.rb @@ -111,6 +111,75 @@ describe NotificationsController do expect(JSON.parse(response.body)['notifications'][0]['read']).to eq(false) end + context "when filter_by_types param is present" do + fab!(:liked1) do + Fabricate( + :notification, + user: user, + notification_type: Notification.types[:liked], + created_at: 2.minutes.ago + ) + end + fab!(:liked2) do + Fabricate( + :notification, + user: user, + notification_type: Notification.types[:liked], + created_at: 10.minutes.ago + ) + end + fab!(:replied) do + Fabricate( + :notification, + user: user, + notification_type: Notification.types[:replied], + created_at: 7.minutes.ago + ) + end + fab!(:mentioned) do + Fabricate( + :notification, + user: user, + notification_type: Notification.types[:mentioned] + ) + end + + it "correctly filters notifications to the type(s) given" do + get "/notifications.json", params: { recent: true, filter_by_types: "liked,replied" } + expect(response.status).to eq(200) + expect( + response.parsed_body["notifications"].map { |n| n["id"] } + ).to eq([liked1.id, replied.id, liked2.id]) + + get "/notifications.json", params: { recent: true, filter_by_types: "replied" } + expect(response.status).to eq(200) + expect( + response.parsed_body["notifications"].map { |n| n["id"] } + ).to eq([replied.id]) + end + + it "doesn't include notifications from other users" do + Fabricate( + :notification, + user: Fabricate(:user), + notification_type: Notification.types[:liked] + ) + get "/notifications.json", params: { recent: true, filter_by_types: "liked" } + expect(response.status).to eq(200) + expect( + response.parsed_body["notifications"].map { |n| n["id"] } + ).to eq([liked1.id, liked2.id]) + end + + it "limits the number of returned notifications according to the limit param" do + get "/notifications.json", params: { recent: true, filter_by_types: "liked", limit: 1 } + expect(response.status).to eq(200) + expect( + response.parsed_body["notifications"].map { |n| n["id"] } + ).to eq([liked1.id]) + end + end + context 'when username params is not valid' do it 'should raise the right error' do get "/notifications.json", params: { username: 'somedude' } diff --git a/spec/serializers/current_user_serializer_spec.rb b/spec/serializers/current_user_serializer_spec.rb index 3e2395d24c7..2f30e338433 100644 --- a/spec/serializers/current_user_serializer_spec.rb +++ b/spec/serializers/current_user_serializer_spec.rb @@ -298,4 +298,20 @@ RSpec.describe CurrentUserSerializer do ) end end + + describe "#likes_notifications_disabled" do + it "is true if the user disables likes notifications" do + user.user_option.update!(like_notification_frequency: UserOption.like_notification_frequency_type[:never]) + expect(serializer.as_json[:likes_notifications_disabled]).to eq(true) + end + + it "is false if the user doesn't disable likes notifications" do + user.user_option.update!(like_notification_frequency: UserOption.like_notification_frequency_type[:always]) + expect(serializer.as_json[:likes_notifications_disabled]).to eq(false) + user.user_option.update!(like_notification_frequency: UserOption.like_notification_frequency_type[:first_time_and_daily]) + expect(serializer.as_json[:likes_notifications_disabled]).to eq(false) + user.user_option.update!(like_notification_frequency: UserOption.like_notification_frequency_type[:first_time]) + expect(serializer.as_json[:likes_notifications_disabled]).to eq(false) + end + end end