From 61aeb2da90c7488b9148e8f4881f49850e3a0c43 Mon Sep 17 00:00:00 2001 From: Roman Rizzi Date: Wed, 12 Jul 2023 11:21:51 -0300 Subject: [PATCH] FEATURE: Inline topic summary. Cached version accessible to everyone. (#22551) * FEATURE: Inline topic summary. Cached version accessible to everyone. Anons and non-members of the `custom_summarization_allowed_groups_map` groups can see cached summaries for any accessible topic. After the first 12 hours and if the posts to summarize have changed, allowed users clicking on the button will automatically re-generate it. * Ensure chat summaries work and prevent model hallucinations when there are no messages. --- .../app/components/scrolling-post-stream.js | 7 -- .../app/components/topic-summary.hbs | 14 ---- .../discourse/app/components/topic-summary.js | 28 -------- .../discourse/app/lib/formatter.js | 2 +- .../discourse/app/lib/transform-post.js | 1 + .../app/templates/modal/topic-summary.hbs | 4 -- .../discourse/app/widgets/summary-box.js | 65 ++++++++++++++++++ .../app/widgets/toggle-topic-summary.js | 47 ++++++++----- .../discourse/app/widgets/topic-map.js | 18 +---- .../stylesheets/common/base/topic-post.scss | 15 +++++ .../stylesheets/desktop/topic-post.scss | 6 +- app/assets/stylesheets/mobile/topic-post.scss | 2 +- app/controllers/topics_controller.rb | 13 ++-- app/serializers/topic_view_serializer.rb | 5 ++ .../web_hook_topic_view_serializer.rb | 1 + app/services/topic_summarization.rb | 52 ++++++++++----- config/locales/client.en.yml | 10 +-- lib/summarization/base.rb | 21 ++++-- lib/topic_view.rb | 4 ++ .../chat/api/summaries_controller.rb | 29 ++++---- .../components/chat/modal/channel-summary.js | 1 + plugins/chat/config/locales/server.en.yml | 3 + spec/lib/summarization/base_spec.rb | 63 +++++++++++++++--- .../api/schemas/json/topic_show_response.json | 4 ++ spec/requests/topics_controller_spec.rb | 44 +++++++++++-- spec/services/topic_summarization_spec.rb | 66 +++++++++++++++++-- spec/system/topic_summarization_spec.rb | 9 ++- 27 files changed, 378 insertions(+), 156 deletions(-) delete mode 100644 app/assets/javascripts/discourse/app/components/topic-summary.hbs delete mode 100644 app/assets/javascripts/discourse/app/components/topic-summary.js delete mode 100644 app/assets/javascripts/discourse/app/templates/modal/topic-summary.hbs create mode 100644 app/assets/javascripts/discourse/app/widgets/summary-box.js diff --git a/app/assets/javascripts/discourse/app/components/scrolling-post-stream.js b/app/assets/javascripts/discourse/app/components/scrolling-post-stream.js index 856b26359b8..c5d16602034 100644 --- a/app/assets/javascripts/discourse/app/components/scrolling-post-stream.js +++ b/app/assets/javascripts/discourse/app/components/scrolling-post-stream.js @@ -8,7 +8,6 @@ import offsetCalculator from "discourse/lib/offset-calculator"; import { inject as service } from "@ember/service"; import { bind } from "discourse-common/utils/decorators"; import domUtils from "discourse-common/utils/dom-utils"; -import showModal from "discourse/lib/show-modal"; const DEBOUNCE_DELAY = 50; @@ -269,12 +268,6 @@ export default MountWidget.extend({ this.screenTrack.setOnscreen(onscreenPostNumbers, readPostNumbers); }, - showSummary() { - showModal("topic-summary").setProperties({ - topicId: this.posts.objectAt(0).topic_id, - }); - }, - _scrollTriggered() { scheduleOnce("afterRender", this, this.scrolled); }, diff --git a/app/assets/javascripts/discourse/app/components/topic-summary.hbs b/app/assets/javascripts/discourse/app/components/topic-summary.hbs deleted file mode 100644 index e5b491c0604..00000000000 --- a/app/assets/javascripts/discourse/app/components/topic-summary.hbs +++ /dev/null @@ -1,14 +0,0 @@ - -
- - - {{#unless this.loading}} -

{{this.summary}}

- {{/unless}} -
- - - -
- -
\ No newline at end of file diff --git a/app/assets/javascripts/discourse/app/components/topic-summary.js b/app/assets/javascripts/discourse/app/components/topic-summary.js deleted file mode 100644 index c86b03777ec..00000000000 --- a/app/assets/javascripts/discourse/app/components/topic-summary.js +++ /dev/null @@ -1,28 +0,0 @@ -import Component from "@glimmer/component"; -import { tracked } from "@glimmer/tracking"; -import { ajax } from "discourse/lib/ajax"; -import { popupAjaxError } from "discourse/lib/ajax-error"; -import { action } from "@ember/object"; -import { schedule } from "@ember/runloop"; -import { cookAsync } from "discourse/lib/text"; - -export default class TopicSummary extends Component { - @tracked loading = false; - @tracked summary = null; - - @action - summarize() { - schedule("afterRender", () => { - this.loading = true; - - ajax(`/t/${this.args.topicId}/strategy-summary`) - .then((data) => { - cookAsync(data.summary).then((cooked) => { - this.summary = cooked; - }); - }) - .catch(popupAjaxError) - .finally(() => (this.loading = false)); - }); - } -} diff --git a/app/assets/javascripts/discourse/app/lib/formatter.js b/app/assets/javascripts/discourse/app/lib/formatter.js index 6493ba18e8e..33004ab7666 100644 --- a/app/assets/javascripts/discourse/app/lib/formatter.js +++ b/app/assets/javascripts/discourse/app/lib/formatter.js @@ -7,7 +7,7 @@ export function shortDate(date) { return moment(date).format(I18n.t("dates.medium.date_year")); } -function shortDateNoYear(date) { +export function shortDateNoYear(date) { return moment(date).format(I18n.t("dates.tiny.date_month")); } diff --git a/app/assets/javascripts/discourse/app/lib/transform-post.js b/app/assets/javascripts/discourse/app/lib/transform-post.js index f8f586ae67e..740c7c7f58e 100644 --- a/app/assets/javascripts/discourse/app/lib/transform-post.js +++ b/app/assets/javascripts/discourse/app/lib/transform-post.js @@ -219,6 +219,7 @@ export default function transformPost( postAtts.topicSummaryEnabled = postStream.summary; postAtts.topicWordCount = topic.word_count; postAtts.hasTopRepliesSummary = topic.has_summary; + postAtts.summarizable = topic.summarizable; } if (postAtts.isDeleted) { diff --git a/app/assets/javascripts/discourse/app/templates/modal/topic-summary.hbs b/app/assets/javascripts/discourse/app/templates/modal/topic-summary.hbs deleted file mode 100644 index b745e904c03..00000000000 --- a/app/assets/javascripts/discourse/app/templates/modal/topic-summary.hbs +++ /dev/null @@ -1,4 +0,0 @@ - \ No newline at end of file diff --git a/app/assets/javascripts/discourse/app/widgets/summary-box.js b/app/assets/javascripts/discourse/app/widgets/summary-box.js new file mode 100644 index 00000000000..a3f00e4d8b5 --- /dev/null +++ b/app/assets/javascripts/discourse/app/widgets/summary-box.js @@ -0,0 +1,65 @@ +import { createWidget } from "discourse/widgets/widget"; +import hbs from "discourse/widgets/hbs-compiler"; +import { ajax } from "discourse/lib/ajax"; +import { popupAjaxError } from "discourse/lib/ajax-error"; +import { cookAsync } from "discourse/lib/text"; +import RawHtml from "discourse/widgets/raw-html"; +import I18n from "I18n"; +import { shortDateNoYear } from "discourse/lib/formatter"; +import { h } from "virtual-dom"; + +createWidget("summary-skeleton", { + tagName: "section.placeholder-summary", + template: hbs` +
+
+
+
{{transformed.in_progress_label}}
+ `, + + transform() { + return { in_progress_label: I18n.t("summary.in_progress") }; + }, +}); + +export default createWidget("summary-box", { + tagName: "article.summary-box", + buildKey: (attrs) => `summary-box-${attrs.topicId}`, + + defaultState() { + return { summary: "" }; + }, + + html(attrs, state) { + const html = []; + + if (state.summary) { + html.push(new RawHtml({ html: state.summary })); + html.push( + h( + "div.summarized-on", + {}, + I18n.t("summary.summarized_on", { date: state.summarized_on }) + ) + ); + } else { + html.push(this.attach("summary-skeleton")); + this.fetchSummary(attrs.topicId); + } + + return html; + }, + + fetchSummary(topicId) { + ajax(`/t/${topicId}/strategy-summary`) + .then((data) => { + this.state.summarized_on = shortDateNoYear(data.summarized_on); + + cookAsync(data.summary).then((cooked) => { + this.state.summary = cooked.string; + this.scheduleRerender(); + }); + }) + .catch(popupAjaxError); + }, +}); diff --git a/app/assets/javascripts/discourse/app/widgets/toggle-topic-summary.js b/app/assets/javascripts/discourse/app/widgets/toggle-topic-summary.js index bcbbb7de409..fbd84248f8b 100644 --- a/app/assets/javascripts/discourse/app/widgets/toggle-topic-summary.js +++ b/app/assets/javascripts/discourse/app/widgets/toggle-topic-summary.js @@ -34,15 +34,36 @@ createWidget("toggle-summary-description", { export default createWidget("toggle-topic-summary", { tagName: "section.information.toggle-summary", - html(attrs) { + buildKey: (attrs) => `toggle-topic-summary-${attrs.topicId}`, + + defaultState() { + return { expandSummaryBox: false }; + }, + + html(attrs, state) { const html = []; const summarizationButtons = []; + if (attrs.summarizable) { + const title = I18n.t("summary.strategy.button_title"); + + summarizationButtons.push( + this.attach("button", { + className: "btn btn-primary topic-strategy-summarization", + icon: "magic", + translatedTitle: title, + translatedLabel: title, + action: "expandSummaryBox", + disabled: state.expandSummaryBox, + }) + ); + } + if (attrs.hasTopRepliesSummary) { html.push(this.attach("toggle-summary-description", attrs)); summarizationButtons.push( this.attach("button", { - className: "btn btn-primary", + className: "btn top-replies", icon: attrs.topicSummaryEnabled ? null : "layer-group", title: attrs.topicSummaryEnabled ? null : "summary.short_title", label: attrs.topicSummaryEnabled @@ -53,22 +74,18 @@ export default createWidget("toggle-topic-summary", { ); } - if (attrs.includeSummary) { - const title = I18n.t("summary.strategy.button_title"); - - summarizationButtons.push( - this.attach("button", { - className: "btn btn-primary topic-strategy-summarization", - icon: "magic", - translatedTitle: title, - translatedLabel: title, - action: "showSummary", - }) - ); + if (summarizationButtons) { + html.push(h("div.summarization-buttons", summarizationButtons)); } - html.push(h("div.summarization-buttons", summarizationButtons)); + if (state.expandSummaryBox) { + html.push(this.attach("summary-box", attrs)); + } return html; }, + + expandSummaryBox() { + this.state.expandSummaryBox = true; + }, }); diff --git a/app/assets/javascripts/discourse/app/widgets/topic-map.js b/app/assets/javascripts/discourse/app/widgets/topic-map.js index 6b94820fdc8..67d6bdd323d 100644 --- a/app/assets/javascripts/discourse/app/widgets/topic-map.js +++ b/app/assets/javascripts/discourse/app/widgets/topic-map.js @@ -386,8 +386,7 @@ export default createWidget("topic-map", { contents.push(this.attach("topic-map-expanded", attrs)); } - if (attrs.hasTopRepliesSummary || this._includesSummary()) { - attrs.includeSummary = this._includesSummary(); + if (attrs.hasTopRepliesSummary || attrs.summarizable) { contents.push(this.attach("toggle-topic-summary", attrs)); } @@ -400,19 +399,4 @@ export default createWidget("topic-map", { toggleMap() { this.state.collapsed = !this.state.collapsed; }, - - _includesSummary() { - const customSummaryAllowedGroups = - this.siteSettings.custom_summarization_allowed_groups - .split("|") - .map(parseInt); - - return ( - this.siteSettings.summarization_strategy && - this.currentUser && - this.currentUser.groups.some((g) => - customSummaryAllowedGroups.includes(g.id) - ) - ); - }, }); diff --git a/app/assets/stylesheets/common/base/topic-post.scss b/app/assets/stylesheets/common/base/topic-post.scss index a3c7fe5c4d2..40713ae0d41 100644 --- a/app/assets/stylesheets/common/base/topic-post.scss +++ b/app/assets/stylesheets/common/base/topic-post.scss @@ -843,6 +843,21 @@ aside.quote { .summarization-buttons { display: flex; } + + .placeholder-summary { + padding-top: 0.5em; + } + + .placeholder-summary-text { + display: inline-block; + height: 1em; + margin-top: 0.6em; + width: 100%; + } + + .summarized-on { + text-align: right; + } } } diff --git a/app/assets/stylesheets/desktop/topic-post.scss b/app/assets/stylesheets/desktop/topic-post.scss index e8879adbd7b..16ec5f82095 100644 --- a/app/assets/stylesheets/desktop/topic-post.scss +++ b/app/assets/stylesheets/desktop/topic-post.scss @@ -326,9 +326,13 @@ pre.codeblock-buttons:hover { } } - .toggle-summary .summarization-buttons .topic-strategy-summarization { + .toggle-summary .summarization-buttons .top-replies { margin-left: 10px; } + + .toggle-summary .summary-box { + margin-top: 10px; + } } #topic-footer-buttons { diff --git a/app/assets/stylesheets/mobile/topic-post.scss b/app/assets/stylesheets/mobile/topic-post.scss index ff3cc54ac7b..2d6db363904 100644 --- a/app/assets/stylesheets/mobile/topic-post.scss +++ b/app/assets/stylesheets/mobile/topic-post.scss @@ -204,7 +204,7 @@ a.reply-to-tab { .summarization-buttons { flex-direction: column; - .topic-strategy-summarization { + .top-replies { margin-top: 10px; } } diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index 3bd7beb72c6..d4b112679b4 100644 --- a/app/controllers/topics_controller.rb +++ b/app/controllers/topics_controller.rb @@ -30,7 +30,6 @@ class TopicsController < ApplicationController publish reset_bump_date set_slow_mode - summary ] before_action :consider_user_for_promotion, only: :show @@ -1172,13 +1171,17 @@ class TopicsController < ApplicationController topic = Topic.find(params[:topic_id]) guardian.ensure_can_see!(topic) strategy = Summarization::Base.selected_strategy - raise Discourse::NotFound.new unless strategy - raise Discourse::InvalidAccess unless strategy.can_request_summaries?(current_user) + if strategy.nil? || !Summarization::Base.can_see_summary?(topic, current_user) + raise Discourse::NotFound + end - RateLimiter.new(current_user, "summary", 6, 5.minutes).performed! + RateLimiter.new(current_user, "summary", 6, 5.minutes).performed! if current_user - hijack { render json: { summary: TopicSummarization.new(strategy).summarize(topic) } } + hijack do + summary = TopicSummarization.new(strategy).summarize(topic, current_user) + render json: { summary: summary&.summarized_text, summarized_on: summary&.updated_at } + end end private diff --git a/app/serializers/topic_view_serializer.rb b/app/serializers/topic_view_serializer.rb index d1f66fe93e5..e65181b26b5 100644 --- a/app/serializers/topic_view_serializer.rb +++ b/app/serializers/topic_view_serializer.rb @@ -78,6 +78,7 @@ class TopicViewSerializer < ApplicationSerializer :user_last_posted_at, :is_shared_draft, :slow_mode_enabled_until, + :summarizable, ) has_one :details, serializer: TopicViewDetailsSerializer, root: false, embed: :objects @@ -311,4 +312,8 @@ class TopicViewSerializer < ApplicationSerializer def slow_mode_enabled_until object.topic.slow_mode_topic_timer&.execute_at end + + def summarizable + object.summarizable? + end end diff --git a/app/serializers/web_hook_topic_view_serializer.rb b/app/serializers/web_hook_topic_view_serializer.rb index ff11424db15..4ec6eb280e6 100644 --- a/app/serializers/web_hook_topic_view_serializer.rb +++ b/app/serializers/web_hook_topic_view_serializer.rb @@ -22,6 +22,7 @@ class WebHookTopicViewSerializer < TopicViewSerializer slow_mode_seconds slow_mode_enabled_until bookmarks + summarizable ].each { |attr| define_method("include_#{attr}?") { false } } def include_show_read_indicator? diff --git a/app/services/topic_summarization.rb b/app/services/topic_summarization.rb index b13452a739b..5d26549f78e 100644 --- a/app/services/topic_summarization.rb +++ b/app/services/topic_summarization.rb @@ -5,33 +5,36 @@ class TopicSummarization @strategy = strategy end - def summarize(topic) - DistributedMutex.synchronize("toppic_summarization_#{topic.id}") do - existing_summary = SummarySection.find_by(target: topic, meta_section_id: nil) - return existing_summary.summarized_text if existing_summary + def summarize(topic, user) + existing_summary = SummarySection.find_by(target: topic, meta_section_id: nil) - content = { - resource_path: "#{Discourse.base_path}/t/-/#{topic.id}", - content_title: topic.title, - contents: [], - } + # For users without permissions to generate a summary, we return what we have cached. + # Existing summary shouldn't be nil in this scenario because the controller checks its existence. + return existing_summary if !user || !Summarization::Base.can_request_summary_for?(user) - targets_data = summary_targets(topic).pluck(:post_number, :raw, :username) + return existing_summary if existing_summary && fresh?(existing_summary, topic) - targets_data.map do |(pn, raw, username)| - content[:contents] << { poster: username, id: pn, text: raw } - end + delete_cached_summaries_of(topic) if existing_summary - summarization_result = strategy.summarize(content) + content = { + resource_path: "#{Discourse.base_path}/t/-/#{topic.id}", + content_title: topic.title, + contents: [], + } - cached_summary(summarization_result, targets_data.map(&:first), topic) + targets_data = summary_targets(topic).pluck(:post_number, :raw, :username) - summarization_result[:summary] + targets_data.map do |(pn, raw, username)| + content[:contents] << { poster: username, id: pn, text: raw } end + + summarization_result = strategy.summarize(content) + + cache_summary(summarization_result, targets_data.map(&:first), topic) end def summary_targets(topic) - topic.has_summary? ? best_replies(topic) : pick_selection(topic) + @targets ||= topic.has_summary? ? best_replies(topic) : pick_selection(topic) end private @@ -66,7 +69,18 @@ class TopicSummarization .order(:post_number) end - def cached_summary(result, post_numbers, topic) + def delete_cached_summaries_of(topic) + SummarySection.where(target: topic).destroy_all + end + + def fresh?(summary, topic) + return true if summary.created_at > 12.hours.ago + latest_post_to_summarize = summary_targets(topic).last.post_number + + latest_post_to_summarize <= summary.content_range.to_a.last + end + + def cache_summary(result, post_numbers, topic) main_summary = SummarySection.create!( target: topic, @@ -86,5 +100,7 @@ class TopicSummarization meta_section_id: main_summary.id, ) end + + main_summary end end diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index c5736c637f0..a2095c38e0d 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -2040,7 +2040,9 @@ en: refresh_page: "Refresh page" summary: - enabled_description: "You're viewing a summary of this topic: the most interesting posts as determined by the community." + in_progress: "Summary in progress..." + summarized_on: "Summarized on %{date}" + enabled_description: "You're viewing this topic top replies: the most interesting posts as determined by the community." description: one: "There is %{count} reply." other: "There are %{count} replies." @@ -2057,8 +2059,8 @@ en: enable: "Show top replies" disable: "Show All Posts" - short_label: "Summarize" - short_title: "Show a summary of this topic: the most interesting posts as determined by the community" + short_label: "Top replies" + short_title: "Show this topic top replies: the most interesting posts as determined by the community" strategy: button_title: "Summarize this topic" title: "Topic summary" @@ -3633,7 +3635,7 @@ en: filtered_replies: viewing_posts_by: "Viewing %{post_count} posts by" viewing_subset: "Some replies are collapsed" - viewing_summary: "Viewing a summary of this topic" + viewing_summary: "Viewing this topic top replies" post_number: "%{username}, post #%{post_number}" show_all: "Show all" diff --git a/lib/summarization/base.rb b/lib/summarization/base.rb index e70ac1c2a57..53fbba5ec1e 100644 --- a/lib/summarization/base.rb +++ b/lib/summarization/base.rb @@ -21,13 +21,24 @@ module Summarization find_strategy(SiteSetting.summarization_strategy) end - end - def can_request_summaries?(user) - user_group_ids = user.group_ids + def can_see_summary?(target, user) + return false if SiteSetting.summarization_strategy.blank? - SiteSetting.custom_summarization_allowed_groups_map.any? do |group_id| - user_group_ids.include?(group_id) + has_cached_summary = SummarySection.exists?(target: target, meta_section_id: nil) + return has_cached_summary if user.nil? + + has_cached_summary || can_request_summary_for?(user) + end + + def can_request_summary_for?(user) + return false unless user + + user_group_ids = user.group_ids + + SiteSetting.custom_summarization_allowed_groups_map.any? do |group_id| + user_group_ids.include?(group_id) + end end end diff --git a/lib/topic_view.rb b/lib/topic_view.rb index dd6fe4689e8..4a5474f4558 100644 --- a/lib/topic_view.rb +++ b/lib/topic_view.rb @@ -722,6 +722,10 @@ class TopicView end end + def summarizable? + Summarization::Base.can_see_summary?(@topic, @user) + end + protected def read_posts_set diff --git a/plugins/chat/app/controllers/chat/api/summaries_controller.rb b/plugins/chat/app/controllers/chat/api/summaries_controller.rb index 2070b6128bc..208cf98c11b 100644 --- a/plugins/chat/app/controllers/chat/api/summaries_controller.rb +++ b/plugins/chat/app/controllers/chat/api/summaries_controller.rb @@ -12,22 +12,29 @@ class Chat::Api::SummariesController < Chat::ApiController strategy = Summarization::Base.selected_strategy raise Discourse::NotFound.new unless strategy - raise Discourse::InvalidAccess unless strategy.can_request_summaries?(current_user) + raise Discourse::InvalidAccess unless Summarization::Base.can_request_summary_for?(current_user) RateLimiter.new(current_user, "channel_summary", 6, 5.minutes).performed! hijack do - content = - channel - .chat_messages - .where("chat_messages.created_at > ?", since.hours.ago) - .includes(:user) - .order(created_at: :asc) - .pluck(:username_lower, :message) - .map { "#{_1}: #{_2}" } - .join("\n") + content = { content_title: channel.name } - render json: { summary: strategy.summarize(content).dig(:summary) } + content[:contents] = channel + .chat_messages + .where("chat_messages.created_at > ?", since.hours.ago) + .includes(:user) + .order(created_at: :asc) + .pluck(:id, :username_lower, :message) + .map { { id: _1, poster: _2, text: _3 } } + + summarized_text = + if content[:contents].empty? + I18n.t("chat.summaries.no_targets") + else + strategy.summarize(content).dig(:summary) + end + + render json: { summary: summarized_text } end end end diff --git a/plugins/chat/assets/javascripts/discourse/components/chat/modal/channel-summary.js b/plugins/chat/assets/javascripts/discourse/components/chat/modal/channel-summary.js index 199118729d4..affee4658af 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat/modal/channel-summary.js +++ b/plugins/chat/assets/javascripts/discourse/components/chat/modal/channel-summary.js @@ -27,6 +27,7 @@ export default class ChatModalChannelSummary extends Component { @action summarize(since) { + this.sinceHours = since; this.loading = true; if (this.availableSummaries[since]) { diff --git a/plugins/chat/config/locales/server.en.yml b/plugins/chat/config/locales/server.en.yml index cab3325b153..8e4cbabb7c1 100644 --- a/plugins/chat/config/locales/server.en.yml +++ b/plugins/chat/config/locales/server.en.yml @@ -159,6 +159,9 @@ en: one: "and %{count} other" other: "and %{count} others" + summaries: + no_targets: "There were no messages during the selected period." + discourse_push_notifications: popup: chat_mention: diff --git a/spec/lib/summarization/base_spec.rb b/spec/lib/summarization/base_spec.rb index 489dd1a63ec..34021e07445 100644 --- a/spec/lib/summarization/base_spec.rb +++ b/spec/lib/summarization/base_spec.rb @@ -1,24 +1,67 @@ # frozen_string_literal: true describe Summarization::Base do - subject(:summarization) { described_class.new } - fab!(:user) { Fabricate(:user) } fab!(:group) { Fabricate(:group) } + fab!(:topic) { Fabricate(:topic) } - before { group.add(user) } + let(:plugin) { Plugin::Instance.new } - describe "#can_request_summaries?" do - it "returns true if the user group is present in the custom_summarization_allowed_groups_map setting" do - SiteSetting.custom_summarization_allowed_groups = group.id + before do + group.add(user) - expect(summarization.can_request_summaries?(user)).to eq(true) + strategy = DummyCustomSummarization.new("dummy") + plugin.register_summarization_strategy(strategy) + SiteSetting.summarization_strategy = strategy.model + end + + describe "#can_see_summary?" do + context "when the user cannot generate a summary" do + before { SiteSetting.custom_summarization_allowed_groups = "" } + + it "returns false" do + SiteSetting.custom_summarization_allowed_groups = "" + + expect(described_class.can_see_summary?(topic, user)).to eq(false) + end + + it "returns true if there is a cached summary" do + SummarySection.create!( + target: topic, + summarized_text: "test", + original_content_sha: "123", + algorithm: "test", + meta_section_id: nil, + ) + + expect(described_class.can_see_summary?(topic, user)).to eq(true) + end end - it "returns false if the user group is not present in the custom_summarization_allowed_groups_map setting" do - SiteSetting.custom_summarization_allowed_groups = "" + context "when the user can generate a summary" do + before { SiteSetting.custom_summarization_allowed_groups = group.id } - expect(summarization.can_request_summaries?(user)).to eq(false) + it "returns true if the user group is present in the custom_summarization_allowed_groups_map setting" do + expect(described_class.can_see_summary?(topic, user)).to eq(true) + end + end + + context "when there is no user" do + it "returns false for anons" do + expect(described_class.can_see_summary?(topic, nil)).to eq(false) + end + + it "returns true for anons when there is a cached summary" do + SummarySection.create!( + target: topic, + summarized_text: "test", + original_content_sha: "123", + algorithm: "test", + meta_section_id: nil, + ) + + expect(described_class.can_see_summary?(topic, nil)).to eq(true) + end end end end diff --git a/spec/requests/api/schemas/json/topic_show_response.json b/spec/requests/api/schemas/json/topic_show_response.json index 40e5bd5d16c..11923b92fee 100644 --- a/spec/requests/api/schemas/json/topic_show_response.json +++ b/spec/requests/api/schemas/json/topic_show_response.json @@ -715,6 +715,9 @@ "null" ] }, + "summarizable": { + "type": "boolean" + }, "details": { "type": "object", "additionalProperties": false, @@ -985,6 +988,7 @@ "show_read_indicator", "thumbnails", "slow_mode_enabled_until", + "summarizable", "details" ] } diff --git a/spec/requests/topics_controller_spec.rb b/spec/requests/topics_controller_spec.rb index d7f90ba6399..b5ae4f14bdd 100644 --- a/spec/requests/topics_controller_spec.rb +++ b/spec/requests/topics_controller_spec.rb @@ -5469,10 +5469,27 @@ RSpec.describe TopicsController do end context "for anons" do - it "returns a 404" do + it "returns a 404 if there is no cached summary" do get "/t/#{topic.id}/strategy-summary.json" - expect(response.status).to eq(403) + expect(response.status).to eq(404) + end + + it "returns a cached summary" do + section = + SummarySection.create!( + target: topic, + summarized_text: "test", + algorithm: "test", + original_content_sha: "test", + ) + + get "/t/#{topic.id}/strategy-summary.json" + + expect(response.status).to eq(200) + + summary = response.parsed_body + expect(summary["summary"]).to eq(section.summarized_text) end end @@ -5498,15 +5515,32 @@ RSpec.describe TopicsController do end end - context "when the user is not a member of an allowlited group" do + context "when the user is not a member of an allowlisted group" do fab!(:user) { Fabricate(:user) } before { sign_in(user) } - it "return a 404" do + it "return a 404 if there is no cached summary" do get "/t/#{topic.id}/strategy-summary.json" - expect(response.status).to eq(403) + expect(response.status).to eq(404) + end + + it "returns a cached summary" do + section = + SummarySection.create!( + target: topic, + summarized_text: "test", + algorithm: "test", + original_content_sha: "test", + ) + + get "/t/#{topic.id}/strategy-summary.json" + + expect(response.status).to eq(200) + + summary = response.parsed_body + expect(summary["summary"]).to eq(section.summarized_text) end end end diff --git a/spec/services/topic_summarization_spec.rb b/spec/services/topic_summarization_spec.rb index ace381bd4fa..aff3b8cc817 100644 --- a/spec/services/topic_summarization_spec.rb +++ b/spec/services/topic_summarization_spec.rb @@ -2,6 +2,7 @@ describe TopicSummarization do fab!(:topic) { Fabricate(:topic) } + fab!(:user) { Fabricate(:admin) } fab!(:post_1) { Fabricate(:post, topic: topic) } fab!(:post_2) { Fabricate(:post, topic: topic) } @@ -79,24 +80,25 @@ describe TopicSummarization do let(:summary) { { summary: "This is the final summary", chunks: [] } } it "caches the summary" do - summarized_text = summarization.summarize(topic) + section = summarization.summarize(topic, user) - expect(summarized_text).to eq(summary[:summary]) + expect(section.summarized_text).to eq(summary[:summary]) assert_summary_is_cached(topic, summary) end it "returns the cached version in subsequent calls" do - summarization.summarize(topic) + summarization.summarize(topic, user) cached_summary_text = "This is a cached summary" cached_summary = SummarySection.find_by(target: topic, meta_section_id: nil).update!( summarized_text: cached_summary_text, + updated_at: 24.hours.ago, ) - summarized_text = summarization.summarize(topic) - expect(summarized_text).to eq(cached_summary_text) + section = summarization.summarize(topic, user) + expect(section.summarized_text).to eq(cached_summary_text) end end @@ -112,14 +114,64 @@ describe TopicSummarization do end it "caches the summary and each chunk" do - summarized_text = summarization.summarize(topic) + section = summarization.summarize(topic, user) - expect(summarized_text).to eq(summary[:summary]) + expect(section.summarized_text).to eq(summary[:summary]) assert_summary_is_cached(topic, summary) summary[:chunks].each { |c| assert_chunk_is_cached(topic, c) } end end + + describe "invalidating cached summaries" do + let(:cached_text) { "This is a cached summary" } + let(:summarized_text) { "This is the final summary" } + let(:summary) do + { + summary: summarized_text, + chunks: [ + { ids: [topic.first_post.post_number], summary: "this is the first chunk" }, + { ids: [post_1.post_number, post_2.post_number], summary: "this is the second chunk" }, + ], + } + end + + def cached_summary + SummarySection.find_by(target: topic, meta_section_id: nil) + end + + before do + summarization.summarize(topic, user) + + cached_summary.update!(summarized_text: cached_text, created_at: 24.hours.ago) + end + + context "when the summary targets changed" do + before { cached_summary.update!(content_range: (1..1)) } + + it "deletes existing summaries and create a new one" do + section = summarization.summarize(topic, user) + + expect(section.summarized_text).to eq(summarized_text) + end + + it "does nothing if the last summary is less than 12 hours old" do + cached_summary.update!(created_at: 6.hours.ago) + + section = summarization.summarize(topic, user) + + expect(section.summarized_text).to eq(cached_text) + end + end + + context "when the summary targets are still the same" do + it "doesn't create a new summary" do + section = summarization.summarize(topic, user) + + expect(section.summarized_text).to eq(cached_text) + end + end + end end end diff --git a/spec/system/topic_summarization_spec.rb b/spec/system/topic_summarization_spec.rb index ff1ff1992c8..f7407484b4c 100644 --- a/spec/system/topic_summarization_spec.rb +++ b/spec/system/topic_summarization_spec.rb @@ -10,7 +10,8 @@ RSpec.describe "Topic summarization", type: :system, js: true do let(:plugin) { Plugin::Instance.new } - let(:summarization_result) { { summary: "This is a summary", chunks: [] } } + let(:expected_summary) { "This is a summary" } + let(:summarization_result) { { summary: expected_summary, chunks: [] } } before do sign_in(user) @@ -22,8 +23,10 @@ RSpec.describe "Topic summarization", type: :system, js: true do it "returns a summary using the selected timeframe" do visit("/t/-/#{topic.id}") - find(".topic-strategy-summarization", wait: 5).click + find(".topic-strategy-summarization").click - expect(page.has_css?(".topic-summary-modal", wait: 5)).to eq(true) + summary = find(".summary-box p").text + + expect(summary).to eq(expected_summary) end end