diff --git a/.github/workflows/feature-screenshots.yml b/.github/workflows/feature-screenshots.yml index 1f2f591..9758036 100644 --- a/.github/workflows/feature-screenshots.yml +++ b/.github/workflows/feature-screenshots.yml @@ -25,7 +25,14 @@ jobs: RAILS_ENV: test PGUSER: discourse PGPASSWORD: discourse - PLUGIN_NAME: ${{ github.event.repository.name }} + # MUST be lowercase. Discourse derives the compiled stylesheet bundle + # slug from the on-disk plugin directory name, and the route that + # serves `/stylesheets/_.css` is constrained to + # `[-a-z0-9_]+`. Checking out into `plugins/JtechTools/` (the + # repo's casing) made every plugin CSS request 404 → text/html → + # "Refused to apply style" in the browser. Commit b284c8d fixed + # this for local dev; the CI workflow was missed. + PLUGIN_NAME: jtech-tools CHEAP_SOURCE_MAPS: "1" MINIO_RUNNER_LOG_LEVEL: DEBUG MINIO_RUNNER_INSTALL_DIR: /home/discourse/.minio_runner diff --git a/app/controllers/discourse_mod_categories/messages_controller.rb b/app/controllers/discourse_mod_categories/messages_controller.rb index 0c51787..d4046c6 100644 --- a/app/controllers/discourse_mod_categories/messages_controller.rb +++ b/app/controllers/discourse_mod_categories/messages_controller.rb @@ -130,16 +130,17 @@ def add_note_reply raise Discourse::InvalidParameters.new(:raw) if raw.empty? replies = note_replies(topic) - replies << { + reply = { "id" => SecureRandom.hex(8), "user_id" => current_user.id, "raw" => raw, "created_at" => Time.zone.now.iso8601, } + replies << reply topic.custom_fields[TOPIC_PRIVATE_NOTE_REPLIES_FIELD] = replies topic.custom_fields[TOPIC_PRIVATE_NOTE_ACTIVITY_FIELD] = Time.zone.now.iso8601 topic.save_custom_fields(true) - notify_staff_of_note(topic) + notify_staff_of_reply(topic, reply) render json: { replies: serialized_note_replies(topic) } end @@ -231,7 +232,7 @@ def notes_feed { topic_id: topic.id, topic_title: topic.title, - url: "#{topic.relative_url}/#{topic.highest_post_number}", + url: "#{topic.relative_url}/#{topic.highest_post_number}#mod-private-note", note: note, reply_count: replies.is_a?(Array) ? replies.size : 0, activity_at: activity_at, @@ -368,14 +369,16 @@ def normalize_max_tl(value) # the frontend notification-type renderer can render it with the shield # icon, accurate text, and a link straight to the moderator note. The # note lives on the topic, so the link resolves to the topic at its - # highest post number — the same target the notes feed uses. + # highest post number with a `#mod-private-note` anchor so the browser + # (and the component's own scroll-into-view) lands on the note section + # instead of the topic's first post when the topic is short. # # The live pop-up alert is published on the same `/notification-alert/` # MessageBus channel core uses for flags/replies. Creating a # `Notification` row alone only fills the bell list — it never pops up. def notify_staff_of_note(topic) note = topic.custom_fields[TOPIC_PRIVATE_NOTE_FIELD].to_s - note_url = "#{topic.relative_url}/#{topic.highest_post_number}" + note_url = "#{topic.relative_url}/#{topic.highest_post_number}#mod-private-note" User .where(admin: true) @@ -388,6 +391,8 @@ def notify_staff_of_note(topic) # Stable marker the frontend renderer keys off to recognize THIS # custom notification as a moderator note. mod_note: true, + mod_note_kind: "note", + excerpt: note.truncate(300), url: note_url, message: "discourse_mod_categories.note_notification", title: "discourse_mod_categories.note_notification_title", @@ -410,6 +415,48 @@ def notify_staff_of_note(topic) end end + # Sends a notification for a single reply in the moderator-note thread. + # Each reply gets its own bell row and live pop-up — carrying the reply + # author, the reply excerpt, and a URL anchored at the specific reply — + # so multiple replies in the same topic stack as distinct entries instead + # of looking like duplicate "note added" rows. + def notify_staff_of_reply(topic, reply) + reply_id = reply["id"].to_s + reply_raw = reply["raw"].to_s + reply_url = + "#{topic.relative_url}/#{topic.highest_post_number}#mod-private-note-reply-#{reply_id}" + + User + .where(admin: true) + .or(User.where(moderator: true)) + .where.not(id: current_user.id) + .find_each do |staff_user| + data = { + topic_title: topic.title, + display_username: current_user.username, + mod_note: true, + mod_note_kind: "reply", + reply_id: reply_id, + excerpt: reply_raw.truncate(300), + url: reply_url, + message: "discourse_mod_categories.note_reply_notification", + title: "discourse_mod_categories.note_reply_notification_title", + } + + Notification.create!( + notification_type: Notification.types[:custom], + user_id: staff_user.id, + topic_id: topic.id, + post_number: topic.highest_post_number, + high_priority: true, + data: data.to_json, + ) + + publish_reply_alert(staff_user, topic, reply_raw, reply_url) + staff_user.publish_notifications_state + end + end + # Fires the small live notification pop-up for one staff member. The # payload mirrors `PostAlerter.create_notification_alert`, but carries an # explicit `translated_title` so the pop-up text clearly names a @@ -437,6 +484,32 @@ def publish_note_alert(staff_user, topic, note, note_url) MessageBus.publish("/notification-alert/#{staff_user.id}", payload, user_ids: [staff_user.id]) end + # Per-reply variant of publish_note_alert — the excerpt is the reply body + # so a stack of replies pops up as distinct toasts, and the title says + # "replied to" so the recipient can tell a reply from the original note. + def publish_reply_alert(staff_user, topic, reply_raw, reply_url) + return if staff_user.suspended? + return unless staff_user.allow_live_notifications? + + payload = { + notification_type: Notification.types[:custom], + topic_title: topic.title, + topic_id: topic.id, + post_number: topic.highest_post_number, + excerpt: reply_raw.truncate(300), + username: current_user.username, + post_url: reply_url, + translated_title: + I18n.t( + "discourse_mod_categories.note_reply_notification_alert", + username: current_user.username, + topic: topic.title, + ), + } + + MessageBus.publish("/notification-alert/#{staff_user.id}", payload, user_ids: [staff_user.id]) + end + def private_note_author(topic) user_id = topic.custom_fields[TOPIC_PRIVATE_NOTE_USER_FIELD] user = user_id && User.find_by(id: user_id) diff --git a/assets/javascripts/discourse/components/mod-private-note.gjs b/assets/javascripts/discourse/components/mod-private-note.gjs index 68524f5..d0cb0a9 100644 --- a/assets/javascripts/discourse/components/mod-private-note.gjs +++ b/assets/javascripts/discourse/components/mod-private-note.gjs @@ -98,6 +98,36 @@ export default class ModPrivateNote extends Component { this.readTopicState(topic); } + // Notifications and the user-menu notes feed link to the topic with a + // `#mod-private-note` or `#mod-private-note-reply-` hash. Without an + // explicit scroll, Discourse's post-stream scrolls the linked post into + // view AFTER the browser's native hash jump, leaving the target + // off-screen — especially when the topic only has one post, which + // silently lands at the top of the thread. Each reply article also + // carries its own id so a reply notification anchors to that reply. + @action + scrollToNoteIfAnchored() { + if (typeof window === "undefined") { + return; + } + const hash = window.location.hash || ""; + if ( + hash !== "#mod-private-note" && + !hash.startsWith("#mod-private-note-reply-") + ) { + return; + } + // Defer past Discourse's own scroll-to-post on initial topic load, + // and resolve the element after the replies finish rendering — a + // per-reply hash may point at an article that isn't in the DOM yet + // when the outer note container inserts. + setTimeout(() => { + const id = hash.slice(1); + const target = document.getElementById(id); + target?.scrollIntoView({ behavior: "smooth", block: "start" }); + }, 250); + } + // Re-read all per-topic state from the current topic. Called on initial // insert and whenever the connector is reused for a different topic. @action @@ -347,7 +377,11 @@ export default class ModPrivateNote extends Component { {{didUpdate this.refreshOnNavigation @topic.id}} > {{#if this.visible}} -
+
{{icon "lock"}} {{i18n @@ -395,7 +429,10 @@ export default class ModPrivateNote extends Component { {{#each this.decoratedReplies as |reply|}} -
+
{{#if reply.avatarUrl}} ]` hash that + // the note component scrolls into view on insert. get linkHref() { if (this.isModNote && this.notification.data?.url) { return this.notification.data.url; @@ -26,7 +35,9 @@ export default function modNoteNotificationRenderer(NotificationTypeBase) { get linkTitle() { if (this.isModNote) { - return i18n("discourse_mod_categories.note_notification_title"); + return this.modNoteKind === "reply" + ? i18n("discourse_mod_categories.note_reply_notification_title") + : i18n("discourse_mod_categories.note_notification_title"); } // Core `custom.js` behavior. if (this.notification.data?.title) { @@ -45,20 +56,28 @@ export default function modNoteNotificationRenderer(NotificationTypeBase) { return `notification.${this.notification.data?.message}`; } - // Accurate, self-describing label naming the acting moderator. + // Accurate, self-describing label naming the acting moderator — + // "added a moderator note" vs "replied to a moderator note". get label() { if (this.isModNote) { - return i18n("discourse_mod_categories.note_notification", { - username: this.username, - }); + return this.modNoteKind === "reply" + ? i18n("discourse_mod_categories.note_reply_notification", { + username: this.username, + }) + : i18n("discourse_mod_categories.note_notification", { + username: this.username, + }); } return super.label; } - // Second line: the topic the note is on. + // Second line: the reply excerpt (so stacked reply notifications are + // self-describing) when available, falling back to the topic title. get description() { if (this.isModNote) { - return this.notification.data?.topic_title; + return ( + this.notification.data?.excerpt || this.notification.data?.topic_title + ); } return super.description; } diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index b2687b0..b448a4b 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -81,6 +81,8 @@ en: empty: No moderator notes yet. note_notification: '%{username} added a moderator note' note_notification_title: Moderator note + note_reply_notification: '%{username} replied to a moderator note' + note_reply_notification_title: Moderator note reply audience: label: Show this prompt to all: Everyone diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 696afb8..74f2704 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -205,3 +205,4 @@ en: discourse_mod_categories: note_notification_alert: '%{username} added a moderator note on "%{topic}"' + note_reply_notification_alert: '%{username} replied to a moderator note on "%{topic}"' diff --git a/spec/requests/mod_messages_spec.rb b/spec/requests/mod_messages_spec.rb index 063151f..b01e8fe 100644 --- a/spec/requests/mod_messages_spec.rb +++ b/spec/requests/mod_messages_spec.rb @@ -703,7 +703,7 @@ def seed_thread expect(response.status).to eq(403) end - it "links each note to the topic's last post" do + it "links each note to the topic's last post anchored at the mod-private-note section" do topic.custom_fields["mod_topic_private_note"] = "Review me." topic.save_custom_fields(true) sign_in(moderator) @@ -711,7 +711,7 @@ def seed_thread get "/discourse-mod-categories/notes-feed.json" url = response.parsed_body["notes"].first["url"] - expect(url).to match(%r{/#{topic.id}/\d+\z}) + expect(url).to match(%r{/#{topic.id}/\d+#mod-private-note\z}) end end diff --git a/spec/requests/mod_note_notifications_spec.rb b/spec/requests/mod_note_notifications_spec.rb index 42af6fd..aadd743 100644 --- a/spec/requests/mod_note_notifications_spec.rb +++ b/spec/requests/mod_note_notifications_spec.rb @@ -90,7 +90,9 @@ def set_note(raw = "Heads up, staff.") data = JSON.parse(custom_notifications(admin).first.data) expect(data["mod_note"]).to eq(true) expect(data["topic_title"]).to eq(topic.title) - expect(data["url"]).to eq("#{topic.relative_url}/#{topic.reload.highest_post_number}") + expect(data["url"]).to eq( + "#{topic.relative_url}/#{topic.reload.highest_post_number}#mod-private-note", + ) end it "publishes a live pop-up alert to every other staff member" do @@ -107,7 +109,9 @@ def set_note(raw = "Heads up, staff.") expect(alerted).not_to include("/notification-alert/#{moderator.id}") payload = messages.first.data - expect(payload[:post_url]).to eq("#{topic.relative_url}/#{topic.reload.highest_post_number}") + expect(payload[:post_url]).to eq( + "#{topic.relative_url}/#{topic.reload.highest_post_number}#mod-private-note", + ) expect(payload[:translated_title]).to include(moderator.username) expect(payload[:translated_title]).to include(topic.title) end @@ -171,6 +175,50 @@ def set_note(raw = "Heads up, staff.") expect(alerted).to include("/notification-alert/#{admin.id}") end + it "marks each reply notification as a reply with the excerpt and anchored URL" do + sign_in(moderator) + + post "/discourse-mod-categories/topic/#{topic.id}/note-reply.json", + params: { + raw: "Following up on this thread.", + } + + data = JSON.parse(custom_notifications(admin).first.data) + expect(data["mod_note"]).to eq(true) + expect(data["mod_note_kind"]).to eq("reply") + expect(data["excerpt"]).to eq("Following up on this thread.") + expect(data["reply_id"]).to be_present + expect(data["message"]).to eq("discourse_mod_categories.note_reply_notification") + expect(data["url"]).to eq( + "#{topic.relative_url}/#{topic.reload.highest_post_number}#mod-private-note-reply-#{data["reply_id"]}", + ) + end + + it "creates a distinct notification per reply so they stack in the bell" do + sign_in(moderator) + + post "/discourse-mod-categories/topic/#{topic.id}/note-reply.json", + params: { + raw: "First reply.", + } + post "/discourse-mod-categories/topic/#{topic.id}/note-reply.json", + params: { + raw: "Second reply.", + } + post "/discourse-mod-categories/topic/#{topic.id}/note-reply.json", + params: { + raw: "Third reply.", + } + + rows = custom_notifications(admin).order(:id) + expect(rows.count).to eq(3) + + excerpts = rows.map { |n| JSON.parse(n.data)["excerpt"] } + reply_ids = rows.map { |n| JSON.parse(n.data)["reply_id"] } + expect(excerpts).to eq(["First reply.", "Second reply.", "Third reply."]) + expect(reply_ids.uniq.size).to eq(3) + end + it "does not notify the moderator who wrote the reply" do sign_in(other_moderator) diff --git a/spec/requests/whisper_unread_badge_spec.rb b/spec/requests/whisper_unread_badge_spec.rb index 9d289ec..ff48b7b 100644 --- a/spec/requests/whisper_unread_badge_spec.rb +++ b/spec/requests/whisper_unread_badge_spec.rb @@ -133,5 +133,94 @@ topic.reload expect(topic.highest_post_number).to eq(regular_reply.post_number) end + + it "stamps non_whisper_bumped_at into a topic custom field on whisper creation" do + # Backdate BOTH non-whisper posts so the max(:created_at) is + # deterministically regular_reply (15 min ago) — op was fabricated + # at ~now, so without the older backdate it would win the max() and + # the stamp wouldn't match what the assertion expects. + op.update_columns(created_at: 30.minutes.ago) + regular_reply.update_columns(created_at: 15.minutes.ago) + + sign_in(moderator) + post "/posts.json", + params: { + :topic_id => topic.id, + :raw => "Whisper body long enough to satisfy min_post_length.", + DiscourseModCategories::POST_WHISPER_ARMED_PARAM => true, + DiscourseModCategories::POST_WHISPER_TARGETS_FIELD => [target.id], + } + expect(response.status).to eq(200) + + stamped = + topic.reload.custom_fields[DiscourseModCategories::TOPIC_NON_WHISPER_BUMPED_AT_FIELD].to_s + expect(stamped).not_to be_empty + expect(Time.zone.parse(stamped)).to be_within(1.second).of(regular_reply.reload.created_at) + end + end + + describe "audience-aware /latest ordering" do + fab!(:public_topic, :topic) + fab!(:public_topic_op) { Fabricate(:post, topic: public_topic, user: author) } + + before do + # Pin a clear ordering: the whispered topic was bumped at the whisper + # time (30 min ago via fabrication), the public topic is more recent. + # An audience member should see the whispered topic at the top — the + # whisper IS the latest activity for them. A non-audience viewer + # should see the public topic first because, for them, the whispered + # topic's effective bump is the older regular_reply. + regular_reply.update_columns(created_at: 1.hour.ago) + ::Topic.where(id: topic.id).update_all( + bumped_at: 5.minutes.ago, + last_posted_at: 5.minutes.ago, + ) + ::Topic.where(id: public_topic.id).update_all( + bumped_at: 30.minutes.ago, + last_posted_at: 30.minutes.ago, + ) + # Simulate the on(:post_created) stamp. + topic.custom_fields[ + DiscourseModCategories::TOPIC_NON_WHISPER_BUMPED_AT_FIELD + ] = regular_reply.created_at.iso8601 + topic.save_custom_fields(true) + end + + def latest_topic_ids(as_user) + sign_in(as_user) + get "/latest.json" + expect(response.status).to eq(200) + response.parsed_body["topic_list"]["topics"].map { |t| t["id"] } + end + + it "keeps the whispered topic at the top of /latest for staff" do + ids = latest_topic_ids(admin) + expect(ids.index(topic.id)).to be < ids.index(public_topic.id) + end + + it "keeps the whispered topic at the top of /latest for a whisper participant" do + # Participant is in TOPIC_WHISPER_PARTICIPANTS_FIELD per the outer before. + ids = latest_topic_ids(participant) + expect(ids.index(topic.id)).to be < ids.index(public_topic.id) + end + + it "demotes the whispered topic below the public topic for a non-audience viewer" do + ids = latest_topic_ids(stranger) + expect(ids.index(public_topic.id)).to be < ids.index(topic.id) + end + + it "skips the timestamp cast when the non_whisper_bumped_at value is malformed" do + # The custom field is normally written by on(:post_created) as an + # iso8601 string, but a corrupted, hand-edited, or legacy value + # shouldn't blow up /latest. The modifier's regex guard + # `~ '^[0-9]{4}-[0-9]{2}-[0-9]{2}'` makes the CASE branch fall + # through to topics.bumped_at instead of attempting the cast. + topic.custom_fields[DiscourseModCategories::TOPIC_NON_WHISPER_BUMPED_AT_FIELD] = "not-a-time" + topic.save_custom_fields(true) + + sign_in(stranger) + get "/latest.json" + expect(response.status).to eq(200) + end end end diff --git a/spec/system/feature_screenshots_spec.rb b/spec/system/feature_screenshots_spec.rb index 6a34ab1..dcfcc8b 100644 --- a/spec/system/feature_screenshots_spec.rb +++ b/spec/system/feature_screenshots_spec.rb @@ -2,25 +2,28 @@ require "rails_helper" -# Visual captures of each behavior added by the -# "Audience-aware whisper unread + merge mod-note bell + badge targeting" -# change set, so a reviewer can eyeball them from CI without spinning up a -# local Discourse. PNGs are written into `tmp/capybara/feature_screenshots/` -# and are picked up by the `feature-screenshots.yml` workflow's -# `actions/upload-artifact@v6` step (`if: always()` — uploaded regardless -# of pass/fail). +# Visual captures of the mod-note + whisper-bump behaviors so a reviewer +# can eyeball each from CI without spinning up a local Discourse. PNGs +# are written into `tmp/capybara/feature_screenshots/` and picked up by +# the `feature-screenshots.yml` workflow's `actions/upload-artifact@v6` +# step (`if: always()` — uploaded regardless of pass/fail). +# +# Scope: this spec is intentionally focused on the features actively +# under development (mod-note anchor + per-reply fan-out, audience-aware +# whisper bumping). Earlier broad-coverage scenarios were trimmed so the +# CI artifact stays small and every shot has a clear reviewer purpose. RSpec.describe "Feature screenshots" do fab!(:admin) { Fabricate(:admin, username: "screen_admin") } fab!(:moderator) { Fabricate(:moderator, username: "screen_mod") } + fab!(:other_moderator, :moderator) { Fabricate(:moderator, username: "screen_other_mod") } fab!(:author, :user) { Fabricate(:user, username: "screen_author") } fab!(:audience_user, :user) { Fabricate(:user, username: "screen_audience") } fab!(:stranger, :user) { Fabricate(:user, username: "screen_stranger") } - fab!(:badge_holder, :user) { Fabricate(:user, username: "screen_badge_holder") } - fab!(:badge) { Fabricate(:badge, name: "ScreenshotBadge") } fab!(:category) let(:targets_field) { DiscourseModCategories::POST_WHISPER_TARGETS_FIELD } let(:participants_field) { DiscourseModCategories::TOPIC_WHISPER_PARTICIPANTS_FIELD } + let(:nwba_field) { DiscourseModCategories::TOPIC_NON_WHISPER_BUMPED_AT_FIELD } before do SiteSetting.mod_categories_enabled = true @@ -31,8 +34,6 @@ Group.refresh_automatic_groups! SiteSetting.approve_unless_allowed_groups = Group::AUTO_GROUPS[:trust_level_0].to_s - BadgeGranter.grant(badge, badge_holder) - FileUtils.mkdir_p(File.join(Rails.root, "tmp/capybara/feature_screenshots")) end @@ -44,113 +45,355 @@ def shot(name) rescue Timeout::Error # Capture anyway rather than failing on a slow image. end - # Absolute path so the file lands where the workflow expects, regardless - # of Capybara.save_path. Relative paths are interpreted relative to - # Capybara.save_path (`tmp/capybara/`), which would produce - # `tmp/capybara/tmp/capybara/feature_screenshots/...`. path = File.join(Rails.root, "tmp/capybara/feature_screenshots/#{name}.png") page.save_screenshot(path) end - def topic_with_whisper(audience_ids: [audience_user.id]) - topic = Fabricate(:topic, category: category, title: "Audience-aware whisper demo") - Fabricate(:post, topic: topic, user: author, raw: "OP body for the visual capture.") - Fabricate(:post, topic: topic, user: author, raw: "Public reply visible to everyone.") - whisper = Fabricate(:post, topic: topic, user: moderator, raw: "Mod-only whisper body.") - whisper.custom_fields[targets_field] = audience_ids - whisper.save_custom_fields(true) - topic.custom_fields[participants_field] = audience_ids + # Seeds a topic with a moderator note (default bottom placement) and + # optional staff replies. Returns the saved topic. + def seed_topic_with_note(title:, note:, position: "bottom", replies: [], filler_posts: 0) + topic = Fabricate(:topic, category: category, title: title) + Fabricate(:post, topic: topic, user: author, raw: "OP body for #{title}.") + filler_posts.times do |i| + Fabricate( + :post, + topic: topic, + user: author, + raw: "Filler reply ##{i + 1} keeping the thread long.", + ) + end + topic.custom_fields["mod_topic_private_note"] = note + topic.custom_fields["mod_topic_private_note_user_id"] = moderator.id + topic.custom_fields["mod_topic_private_note_position"] = position + topic.custom_fields["mod_topic_private_note_created_at"] = 30.minutes.ago.iso8601 + topic.custom_fields["mod_topic_private_note_activity_at"] = Time.zone.now.iso8601 + topic.custom_fields["mod_topic_private_note_replies"] = replies if replies.any? topic.save_custom_fields(true) - # Mirror the on(:post_created) rollback so the visual matches what - # production sees after a whisper is posted. - non_whisper_max = - Post - .where(topic_id: topic.id, deleted_at: nil) - .where.not(id: PostCustomField.where(name: targets_field).select(:post_id)) - .maximum(:post_number) - Topic.where(id: topic.id).update_all(highest_post_number: non_whisper_max) if non_whisper_max - topic.reload + topic end - it "1. captures the topic list with no unread bump for a non-audience viewer" do - topic_with_whisper - sign_in(stranger) - visit("/latest") - expect(page).to have_css(".topic-list", wait: 15) - shot("01_non_audience_no_badge") - end - - it "2. captures the topic list WITH the unread bump for an audience viewer" do - topic_with_whisper(audience_ids: [audience_user.id]) - sign_in(audience_user) - visit("/latest") - expect(page).to have_css(".topic-list", wait: 15) - shot("02_audience_sees_badge") - end - - it "3. captures the standard bell with a mod-note notification (no separate header pip)" do + # Builds a single mod-note bell notification of either kind ("note" or + # "reply"), pointing at the topic's note section or a specific reply. + def fab_mod_note_notification(user:, topic:, kind: "note", reply_id: nil, excerpt: nil) + anchor = kind == "reply" ? "#mod-private-note-reply-#{reply_id}" : "#mod-private-note" Notification.create!( notification_type: Notification.types[:custom], - user_id: moderator.id, + user_id: user.id, + topic_id: topic.id, + post_number: topic.reload.highest_post_number, high_priority: true, data: { - topic_title: "Heads up, staff", - display_username: admin.username, + topic_title: topic.title, + display_username: moderator.username, mod_note: true, - url: "/", - message: "discourse_mod_categories.note_notification", - title: "discourse_mod_categories.note_notification_title", + mod_note_kind: kind, + reply_id: reply_id, + excerpt: excerpt || topic.custom_fields["mod_topic_private_note"].to_s, + url: "#{topic.relative_url}/#{topic.highest_post_number}#{anchor}", + message: + ( + if kind == "reply" + "discourse_mod_categories.note_reply_notification" + else + "discourse_mod_categories.note_notification" + end + ), + title: + ( + if kind == "reply" + "discourse_mod_categories.note_reply_notification_title" + else + "discourse_mod_categories.note_notification_title" + end + ), }.to_json, ) + end - sign_in(moderator) - visit("/") - expect(page).to have_css(".d-header", wait: 15) - shot("03_bell_header_no_separate_pip") + # ────────────────────────────────────────────────────────────────────── + # Mod-note rendering on a topic page (renumbered "7-10" so the file + # filenames line up with reviewer expectations). + # ────────────────────────────────────────────────────────────────────── - begin - find(".header-dropdown-toggle.current-user button", match: :first).click - rescue StandardError - nil - end - sleep 0.5 - shot("04_user_menu_with_mod_note_in_bell") + it "7. captures the mod-private-note rendered ABOVE the post stream (top placement)" do + topic = + seed_topic_with_note( + title: "Mod note top placement demo", + note: "Pinned at the top so staff see it before posts.", + position: "top", + ) + + sign_in(moderator) + visit("/t/#{topic.slug}/#{topic.id}") + expect(page).to have_css(".mod-private-note", wait: 15) + shot("07_mod_note_top_placement") end - it "4. captures the whisper composer toolbar modal with the badge picker" do - topic = Fabricate(:topic, category: category, title: "Whisper composer demo") - Fabricate(:post, topic: topic, user: author, raw: "OP for whisper composer demo.") + it "8. captures a mod-note thread with multiple staff replies" do + topic = + seed_topic_with_note( + title: "Mod note reply thread demo", + note: "Triage starts here.", + replies: [ + { + "id" => "demo-rep-001", + "user_id" => moderator.id, + "raw" => "I'll DM the user and ask for context.", + "created_at" => 90.minutes.ago.iso8601, + }, + { + "id" => "demo-rep-002", + "user_id" => other_moderator.id, + "raw" => "Sounds good — watching the next reply.", + "created_at" => 60.minutes.ago.iso8601, + }, + { + "id" => "demo-rep-003", + "user_id" => admin.id, + "raw" => "Resolved on my end, closing the loop.", + "created_at" => 30.minutes.ago.iso8601, + }, + ], + ) sign_in(moderator) visit("/t/#{topic.slug}/#{topic.id}") - expect(page).to have_css(".topic-post", wait: 15) - - find("#topic-footer-buttons .create", match: :first).click - expect(page).to have_css(".d-editor-input", wait: 15) - - # The whisper toolbar button — clicking it as staff opens the target modal. - find( - ".d-editor-button-bar button.mod-whisper-target, " \ - ".d-editor-button-bar button[title='" \ - "#{I18n.t("js.discourse_mod_categories.whisper.toolbar_title")}']", - match: :first, - ).click - - expect(page).to have_css(".mod-whisper-target-modal", wait: 15) - # The badge picker appears when at least one enabled badge exists; the - # fab!(:badge) at the top of the spec ensures that. - shot("05_whisper_modal_with_badge_picker") + expect(page).to have_css(".mod-private-note-reply", count: 3, wait: 15) + shot("08_mod_note_thread_with_replies") end - it "5. captures the PM composer with the 'Add badge group' button" do - sign_in(moderator) + it "9. captures the user-menu shield tab listing notes from multiple topics" do + # Titles must be >= min_topic_title_length (15 by default) or + # Fabricate(:topic, ...) raises ActiveRecord::RecordInvalid before + # the test ever hits the browser — the blank-page failure shot in + # the previous CI run was Capybara capturing about:blank because no + # `visit` had happened yet. + 3.times do |i| + seed_topic_with_note( + title: "Triage topic #{i + 1} needs follow-up", + note: "Triage note #{i + 1} — needs follow-up.", + ) + end + + sign_in(admin) visit("/") expect(page).to have_css(".d-header", wait: 15) + find(".header-dropdown-toggle.current-user button", match: :first).click + # Discourse core renders `id="user-menu-button-"` on every + # registered user-menu tab button — matches the proven pattern from + # moderator_messages_spec.rb and gallery_expansion_spec.rb. + expect(page).to have_css("#user-menu-button-discourse-mod-notes", wait: 15) + find("#user-menu-button-discourse-mod-notes").click + expect(page).to have_css(".mod-notes-panel .mod-notes-item", minimum: 3, wait: 15) + sleep 0.3 + shot("09_shield_tab_with_multiple_notes") + end + + it "10. captures a bell reply notification rendering the reply excerpt as description" do + topic = Fabricate(:topic, category: category, title: "Bell reply excerpt demo") + Fabricate(:post, topic: topic, user: author, raw: "OP for bell reply excerpt demo.") + fab_mod_note_notification( + user: admin, + topic: topic, + kind: "reply", + reply_id: "bell-excerpt-001", + excerpt: + "Following up on the abuse report — please look at the new screenshot the user uploaded.", + ) - # Open a new PM via the URL fragment that opens the composer. - visit("/new-message?username=#{audience_user.username}") - expect(page).to have_css(".composer-fields", wait: 15) + sign_in(admin) + visit("/") + expect(page).to have_css(".d-header", wait: 15) + find(".header-dropdown-toggle.current-user button", match: :first).click + expect(page).to have_css(".notification.custom", wait: 10) + sleep 0.3 + shot("10_bell_reply_notification_shows_excerpt") + end + + # ────────────────────────────────────────────────────────────────────── + # Bell-notification click-through (renumbered "11-12"). + # ────────────────────────────────────────────────────────────────────── + + it "11. captures stacked per-reply mod-note notifications in the bell" do + topic = seed_topic_with_note(title: "Stacked replies demo", note: "Please review this thread.") + + %w[r-aaaa r-bbbb r-cccc].each_with_index do |reply_id, index| + fab_mod_note_notification( + user: admin, + topic: topic, + kind: "reply", + reply_id: reply_id, + excerpt: ["First reply body.", "Second reply body.", "Third reply body."][index], + ) + end + + sign_in(admin) + visit("/") + expect(page).to have_css(".d-header", wait: 15) + find(".header-dropdown-toggle.current-user button", match: :first).click + expect(page).to have_css(".notification.custom", wait: 10) sleep 0.5 - shot("06_pm_composer_add_badge_group_button") + shot("11_bell_stacked_reply_notifications") + end + + it "12. captures a reply notification scrolling into a 15-post thread with bottom mod note" do + # 15 real posts so the mod-note panel sits well below the initial + # viewport — clicking the reply notification has to actually scroll, + # not just land on a single-post topic. + reply_id = "long-thread-reply-001" + topic = + seed_topic_with_note( + title: "Long thread reply anchor demo", + note: "Top-level moderator note pinned to the bottom of the long thread.", + filler_posts: 14, + replies: [ + { + "id" => reply_id, + "user_id" => moderator.id, + "raw" => "The reply this notification points to — should be the focus on click.", + "created_at" => 5.minutes.ago.iso8601, + }, + ], + ) + fab_mod_note_notification( + user: admin, + topic: topic, + kind: "reply", + reply_id: reply_id, + excerpt: "The reply this notification points to.", + ) + + sign_in(admin) + visit("/") + expect(page).to have_css(".d-header", wait: 15) + find(".header-dropdown-toggle.current-user button", match: :first).click + expect(page).to have_css(".notification.custom", wait: 10) + find(".notification.custom a", match: :first).click + + expect(page).to have_css("#mod-private-note-reply-#{reply_id}", wait: 15) + # Give the deferred scrollIntoView (~250ms) plus rendering settle time. + sleep 1.0 + shot("12_reply_notification_scroll_in_long_thread") + end + + # ────────────────────────────────────────────────────────────────────── + # Audience-aware whisper bumping on /latest. Two paired scenarios that + # prove the same topic appears in different positions depending on + # whether the viewer is in the whisper's audience. + # ────────────────────────────────────────────────────────────────────── + + def seed_audience_aware_bump_scenario + # Two topics seeded with a clear baseline ordering: + # public_topic bumped 30 min ago (older) + # whisper_topic bumped 5 min ago (newer) — by a whisper visible to audience_user only + # The whisper-bump fix should: + # * Keep whisper_topic at top for audience_user (and staff). + # * Demote whisper_topic below public_topic for stranger. + public_topic = Fabricate(:topic, category: category, title: "Public conversation") + Fabricate(:post, topic: public_topic, user: author, raw: "Newest *public* post in the list.") + ::Topic.where(id: public_topic.id).update_all( + bumped_at: 30.minutes.ago, + last_posted_at: 30.minutes.ago, + ) + + whisper_topic = Fabricate(:topic, category: category, title: "Topic with whisper at bottom") + Fabricate(:post, topic: whisper_topic, user: author, raw: "Public OP for whisper topic.") + Fabricate(:post, topic: whisper_topic, user: author, raw: "Public reply on whisper topic.") + whisper = + Fabricate( + :post, + topic: whisper_topic, + user: moderator, + raw: "Staff-only whisper most recent.", + ) + whisper.custom_fields[targets_field] = [audience_user.id] + whisper.save_custom_fields(true) + whisper_topic.custom_fields[participants_field] = [audience_user.id] + + # Backdate the public posts BEFORE reading their max(created_at) for the + # non-whisper-bumped-at stamp. Without this, the public posts have + # created_at ≈ now, the NWBA stamp becomes "now", and the modifier's + # demotion still puts whisper_topic above public_topic (whose bumped_at + # is 30 min ago) — defeating the test premise. Mirrors the request + # spec's update_columns(created_at: 1.hour.ago) pattern. + whisper_topic.posts.where.not(id: whisper.id).update_all(created_at: 1.hour.ago) + last_public_post_time = whisper_topic.posts.where.not(id: whisper.id).maximum(:created_at) + whisper_topic.custom_fields[nwba_field] = last_public_post_time.iso8601 + whisper_topic.save_custom_fields(true) + + # Roll back highest_post_number (mirrors on(:post_created)) so the + # unread-badge math is also audience-aware for this scenario. + non_whisper_max = + whisper_topic.posts.where.not(id: whisper.id).where(deleted_at: nil).maximum(:post_number) + ::Topic.where(id: whisper_topic.id).update_all( + bumped_at: 5.minutes.ago, + last_posted_at: 5.minutes.ago, + highest_post_number: non_whisper_max, + ) + + [whisper_topic, public_topic] + end + + it "13. captures /latest for an AUDIENCE member — whispered topic at the top" do + whisper_topic, _public_topic = seed_audience_aware_bump_scenario + + sign_in(audience_user) + visit("/latest") + expect(page).to have_css(".topic-list-item", minimum: 2, wait: 15) + # The whispered topic should be the first item — proves the audience + # member still sees the whisper-bump. + expect(page).to have_css( + ".topic-list-item:first-of-type a.title[href*='#{whisper_topic.slug}']", + wait: 5, + ) + shot("13_latest_audience_user_sees_whisper_at_top") + end + + it "14. captures /latest for a NON-AUDIENCE viewer — whispered topic demoted" do + whisper_topic, public_topic = seed_audience_aware_bump_scenario + + sign_in(stranger) + visit("/latest") + expect(page).to have_css(".topic-list-item", minimum: 2, wait: 15) + # The public_topic should now appear above the whisper_topic — proves + # the non-audience viewer doesn't see ghost activity from the whisper. + expect(page).to have_css( + ".topic-list-item:first-of-type a.title[href*='#{public_topic.slug}']", + wait: 5, + ) + shot("14_latest_non_audience_user_sees_public_topic_first") + end + + # ────────────────────────────────────────────────────────────────────── + # CSS sanity check: confirm whisper.scss is still loading and styling + # the whisper banner on a posted whisper. If this shot ever lands + # unstyled, the same SCSS-pipeline regression that bit us last round + # is back and any new styles added to whisper.scss are at risk. + # ────────────────────────────────────────────────────────────────────── + + it "15. captures the whisper banner styling on a posted whisper (CSS sanity)" do + topic = Fabricate(:topic, category: category, title: "Whisper banner CSS check") + Fabricate(:post, topic: topic, user: author, raw: "OP body for the visual capture.") + Fabricate(:post, topic: topic, user: author, raw: "Public reply visible to everyone.") + whisper = Fabricate(:post, topic: topic, user: moderator, raw: "Mod-only whisper body.") + whisper.custom_fields[targets_field] = [audience_user.id] + whisper.save_custom_fields(true) + topic.custom_fields[participants_field] = [audience_user.id] + topic.save_custom_fields(true) + + non_whisper_max = + Post + .where(topic_id: topic.id, deleted_at: nil) + .where.not(id: PostCustomField.where(name: targets_field).select(:post_id)) + .maximum(:post_number) + Topic.where(id: topic.id).update_all(highest_post_number: non_whisper_max) if non_whisper_max + + sign_in(audience_user) + visit("/t/#{topic.slug}/#{topic.id}") + expect(page).to have_css(".mod-whisper-banner", wait: 15) + # If the banner exists but is invisible / unstyled, the screenshot + # will surface it; the visual sanity check is the whole point. + sleep 0.3 + shot("15_whisper_banner_css_sanity") end end diff --git a/spec/system/moderator_messages_spec.rb b/spec/system/moderator_messages_spec.rb index 6754b81..706967d 100644 --- a/spec/system/moderator_messages_spec.rb +++ b/spec/system/moderator_messages_spec.rb @@ -607,7 +607,7 @@ def navigate_to_topic(title) topic.custom_fields["mod_topic_private_note_activity_at"] = Time.zone.now.iso8601 topic.save_custom_fields(true) - note_url = "#{topic.relative_url}/#{topic.reload.highest_post_number}" + note_url = "#{topic.relative_url}/#{topic.reload.highest_post_number}#mod-private-note" Notification.create!( notification_type: Notification.types[:custom], user_id: other_moderator.id, diff --git a/sub_plugins/mod_categories.rb b/sub_plugins/mod_categories.rb index 39eea89..11b40cc 100644 --- a/sub_plugins/mod_categories.rb +++ b/sub_plugins/mod_categories.rb @@ -241,6 +241,11 @@ def self.targeted_checklists # as group targets. POST_WHISPER_TARGET_BADGES_FIELD = "mod_whisper_target_badge_ids" TOPIC_WHISPER_PARTICIPANTS_FIELD = "mod_whisper_participant_ids" + # ISO8601 timestamp of the latest NON-whisper post in the topic. Written + # alongside the highest_post_number rollback so the topic-list query + # modifier can sort non-audience users by this value instead of the live + # Topic#bumped_at, while audience members keep the actual bump time. + TOPIC_NON_WHISPER_BUMPED_AT_FIELD = "mod_non_whisper_bumped_at" MAX_WHISPER_TARGETS = 10 # Explicit boolean armed flag sent by the composer. A boolean survives # form-encoding even when the target id array is empty, so it — not the @@ -311,6 +316,10 @@ class Engine < ::Rails::Engine DiscourseModCategories::TOPIC_PRIVATE_NOTE_ACTIVITY_FIELD, :string, ) + register_topic_custom_field_type( + DiscourseModCategories::TOPIC_NON_WHISPER_BUMPED_AT_FIELD, + :string, + ) register_user_custom_field_type(DiscourseModCategories::USER_NOTES_SEEN_FIELD, :string) register_user_custom_field_type(DiscourseModCategories::USER_CHECKLIST_VERSION_FIELD, :integer) register_user_custom_field_type(DiscourseModCategories::USER_TARGETED_CHECKLIST_FIELD, :json) @@ -630,8 +639,15 @@ class Engine < ::Rails::Engine # :listable_topic serializer override adds the bump back for audience # members on serialization, so they still see the badge. Runs for EVERY # whisper (including staff-only whisper-backs with no recipients). + # + # Also stamp the latest non-whisper post's created_at into a topic custom + # field, which the :topic_query_create_list_topics modifier below uses + # to sort the /latest list audience-aware: audience members see the + # topic bumped to the whisper time (Topic#bumped_at), non-audience + # users see it at the non-whisper time. Topic#bumped_at itself is left + # alone so the DB column keeps reflecting the actual latest activity. if topic - non_whisper_max = + non_whisper_scope = ::Post .where(topic_id: topic.id, deleted_at: nil) .where.not( @@ -640,11 +656,19 @@ class Engine < ::Rails::Engine name: DiscourseModCategories::POST_WHISPER_TARGETS_FIELD, ).select(:post_id), ) - .maximum(:post_number) || 0 + non_whisper_max = non_whisper_scope.maximum(:post_number) || 0 + non_whisper_created_at = non_whisper_scope.maximum(:created_at) if non_whisper_max > 0 && non_whisper_max < topic.highest_post_number ::Topic.where(id: topic.id).update_all(highest_post_number: non_whisper_max) end + + if non_whisper_created_at + topic.custom_fields[ + DiscourseModCategories::TOPIC_NON_WHISPER_BUMPED_AT_FIELD + ] = non_whisper_created_at.iso8601 + topic.save_custom_fields(true) + end end recipient_ids = @@ -679,6 +703,98 @@ class Engine < ::Rails::Engine end end + # Audience-aware ordering on the topic list. The DB column Topic#bumped_at + # is left at the actual latest-activity time (including whispers), and the + # `on(:post_created)` hook above writes the latest non-whisper post's + # created_at into a topic custom field. This modifier patches the + # `/latest` (and friends) topic-list query to use that custom field as + # the effective sort key for users who are NOT in the topic's whisper + # audience, while audience members keep the live `bumped_at`. + # + # Audience criterion in this modifier: staff OR the user_id appears in + # the topic's `mod_whisper_participant_ids` custom field (the cumulative + # whisper-conversation participants of the topic). Explicit per-whisper + # user/group/badge targets are folded into the participants list by the + # composer flow (see TOPIC_WHISPER_PARTICIPANTS_FIELD writes elsewhere + # in this file), so this single check covers all four audience kinds. + # + # The modifier is wrapped in `rescue StandardError` so any breakage from + # a future Discourse upgrade (renamed hook, query shape change, schema + # change) falls back to the unmodified scope instead of breaking + # /latest entirely. The fallback is Option B — whispers bump for + # everyone — which is annoying but recoverable. CI exercises the path + # via specs in whisper_unread_badge_spec.rb so we'd see breakage early. + register_modifier(:topic_query_create_list_topics) do |scope, _options, topic_query| + begin + user = topic_query.user + + # Staff are in the audience for every whisper — sort by live bumped_at. + next scope if user&.staff? + + user_id = user&.id + nwba_field = DiscourseModCategories::TOPIC_NON_WHISPER_BUMPED_AT_FIELD + participants_field = DiscourseModCategories::TOPIC_WHISPER_PARTICIPANTS_FIELD + + connection = ::ActiveRecord::Base.connection + nwba_field_quoted = connection.quote(nwba_field) + participants_field_quoted = connection.quote(participants_field) + + scope = + scope.joins( + "LEFT OUTER JOIN topic_custom_fields nwba " \ + "ON nwba.topic_id = topics.id AND nwba.name = #{nwba_field_quoted}", + ).joins( + "LEFT OUTER JOIN topic_custom_fields part " \ + "ON part.topic_id = topics.id AND part.name = #{participants_field_quoted}", + ) + + is_audience_sql = + if user_id + # The participants field is registered as :json, so its `value` + # column holds a JSON-serialized array of integer user_ids. + # `value::jsonb @> ''::jsonb` is the safe containment check: + # `[5,7]::jsonb @> 5::jsonb` is true, no false positives from + # substring overlap (e.g., 15 doesn't match 5). The LIKE guard + # skips obviously-malformed legacy rows so the ::jsonb cast + # cannot raise mid-query. + "(part.value IS NOT NULL AND part.value <> '' " \ + "AND part.value LIKE '[%]' " \ + "AND part.value::jsonb @> '#{user_id.to_i}'::jsonb)" + else + "FALSE" + end + + # The regex guard `~ '^[0-9]{4}-[0-9]{2}-[0-9]{2}'` ensures the + # ::timestamp cast only runs on values that LOOK like ISO8601 dates, + # so a corrupted or human-edited custom-field value (e.g. legacy + # data, a typo, the literal string "not-a-time") falls through to + # topics.bumped_at instead of blowing up the entire /latest query. + # The outer `rescue StandardError` below is a last-resort net for + # Ruby-level errors raised while BUILDING the modifier scope (e.g. + # a future Discourse refactor changing AR method signatures); it + # cannot catch SQL execution errors because the reorder is lazy + # and runs after the modifier returns. The regex guard is the + # primary defense against bad data. + effective_bumped_at = <<~SQL.squish + CASE + WHEN #{is_audience_sql} THEN topics.bumped_at + WHEN nwba.value IS NOT NULL + AND nwba.value <> '' + AND nwba.value ~ '^[0-9]{4}-[0-9]{2}-[0-9]{2}' + THEN nwba.value::timestamp + ELSE topics.bumped_at + END + SQL + + scope.reorder(::Arel.sql("(#{effective_bumped_at}) DESC, topics.id DESC")) + rescue StandardError => e + ::Rails.logger.warn( + "[jtech-tools] topic_query audience-aware sort fell back: #{e.class}: #{e.message}", + ) + scope + end + end + add_to_serializer(:post, :mod_is_whisper) do object.custom_fields.key?(DiscourseModCategories::POST_WHISPER_TARGETS_FIELD) end