From 238d71bcad4ebe4994c52ecaa1bfdf90b441f2b6 Mon Sep 17 00:00:00 2001 From: Roman Rizzi Date: Thu, 20 Jul 2023 15:25:46 -0300 Subject: [PATCH] FEATURE: Regenerate outdated summaries. (#22718) Users unable to generate new summaries won't be able to regenerate them. They'll only see the warning saying it's outdated. --- .../discourse/app/widgets/summary-box.js | 96 ++++++++++++------- .../app/widgets/toggle-topic-summary.js | 79 ++++++++++----- .../common/base/topic-summary.scss | 6 +- app/controllers/topics_controller.rb | 8 +- app/models/summary_section.rb | 8 ++ app/services/topic_summarization.rb | 45 ++++++--- config/locales/client.en.yml | 11 ++- spec/services/topic_summarization_spec.rb | 45 +++++---- 8 files changed, 208 insertions(+), 90 deletions(-) diff --git a/app/assets/javascripts/discourse/app/widgets/summary-box.js b/app/assets/javascripts/discourse/app/widgets/summary-box.js index 89e79d2670a..c59e17c52c6 100644 --- a/app/assets/javascripts/discourse/app/widgets/summary-box.js +++ b/app/assets/javascripts/discourse/app/widgets/summary-box.js @@ -50,54 +50,86 @@ export default createWidget("summary-box", { html(attrs) { const html = []; - if (attrs.summary) { + const summary = attrs.summary; + + if (summary && !attrs.skipAgeCheck) { html.push( new RawHtml({ - html: `
${attrs.summary}
`, + html: `
${summary.summarized_text}
`, }) ); - html.push( - h("div.summarized-on", {}, [ - new RenderGlimmer( - this, - "div", - hbs`{{@data.summarizedOn}} - {{d-icon "info-circle"}} - - {{i18n "summary.model_used" model=@data.attrs.summarizedBy}} - `, - { - attrs, - summarizedOn: I18n.t("summary.summarized_on", { - date: attrs.summarizedOn, - }), - } - ), - ]) - ); + + const summarizationInfo = [ + h("p", {}, [ + I18n.t("summary.summarized_on", { date: summary.summarized_on }), + this.buildTooltip(attrs), + ]), + ]; + + if (summary.outdated) { + summarizationInfo.push(this.outdatedSummaryWarning(attrs)); + } + + html.push(h("div.summarized-on", {}, summarizationInfo)); } else { html.push(this.attach("summary-skeleton")); - this.fetchSummary(attrs.topicId); + this.fetchSummary(attrs.topicId, attrs.skipAgeCheck); } return html; }, - showFullSummarizedOn() { - this.state.expandSummarizedOn = true; - this.scheduleRerender(); + buildTooltip(attrs) { + return new RenderGlimmer( + this, + "span", + hbs`{{d-icon "info-circle"}} + {{i18n "summary.model_used" model=@data.summarizedBy}} + `, + { + summarizedBy: attrs.summary.summarized_by, + } + ); }, - fetchSummary(topicId) { - ajax(`/t/${topicId}/strategy-summary`) + outdatedSummaryWarning(attrs) { + let outdatedText = I18n.t("summary.outdated"); + + if ( + !attrs.hasTopRepliesSummary && + attrs.summary.new_posts_since_summary > 0 + ) { + outdatedText += " "; + outdatedText += I18n.t("summary.outdated_posts", { + count: attrs.summary.new_posts_since_summary, + }); + } + + return h("p.outdated-summary", {}, [ + outdatedText, + iconNode("exclamation-triangle", { class: "info-icon" }), + ]); + }, + + fetchSummary(topicId, skipAgeCheck) { + let fetchURL = `/t/${topicId}/strategy-summary`; + + if (skipAgeCheck) { + fetchURL += "?skip_age_check=true"; + } + + ajax(fetchURL) .then((data) => { cookAsync(data.summary).then((cooked) => { // We store the summary in the parent so we can re-render it without doing a new request. - this.sendWidgetEvent("summaryUpdated", { - summary: cooked.string, - summarizedOn: shortDateNoYear(data.summarized_on), - summarizedBy: data.summarized_by, - }); + data.summarized_text = cooked.string; + data.summarized_on = shortDateNoYear(data.summarized_on); + + if (skipAgeCheck) { + data.regenerated = true; + } + + this.sendWidgetEvent("summaryUpdated", data); }); }) .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 aec44c8dfae..fef771cc617 100644 --- a/app/assets/javascripts/discourse/app/widgets/toggle-topic-summary.js +++ b/app/assets/javascripts/discourse/app/widgets/toggle-topic-summary.js @@ -51,22 +51,17 @@ export default createWidget("toggle-topic-summary", { const summarizationButtons = []; if (attrs.summarizable) { - const expandTitle = I18n.t("summary.strategy.button_title"); - const collapseTitle = I18n.t("summary.strategy.hide_button_title"); - const canCollapse = !this.loadingSummary() && this.summaryBoxVisble(); + const canRegenerate = + !state.regenerate && + state.summary.outdated && + state.summary.can_regenerate; + const canCollapse = + !canRegenerate && !this.loadingSummary() && this.summaryBoxVisble(); + const summarizeButton = canCollapse + ? this.hideSummaryButton() + : this.generateSummaryButton(canRegenerate); - summarizationButtons.push( - this.attach("button", { - className: "btn btn-primary topic-strategy-summarization", - icon: canCollapse ? "chevron-up" : "magic", - translatedTitle: canCollapse ? collapseTitle : expandTitle, - translatedLabel: canCollapse ? collapseTitle : expandTitle, - action: state.expandSummaryBox - ? "toggleSummaryBox" - : "expandSummaryBox", - disabled: this.loadingSummary(), - }) - ); + summarizationButtons.push(summarizeButton); } if (attrs.hasTopRepliesSummary) { @@ -89,23 +84,55 @@ export default createWidget("toggle-topic-summary", { } if (this.summaryBoxVisble()) { - attrs.summary = this.state.summary; - attrs.summarizedOn = this.state.summarizedOn; - attrs.summarizedBy = this.state.summarizedBy; + attrs.summary = state.summary; + attrs.skipAgeCheck = state.regenerate; + html.push(this.attach("summary-box", attrs)); } return html; }, - loadingSummary() { - return this.summaryBoxVisble() && !this.state.summary; + generateSummaryButton(canRegenerate) { + const title = canRegenerate + ? "summary.buttons.regenerate" + : "summary.buttons.generate"; + const icon = canRegenerate ? "sync" : "magic"; + + return this.attach("button", { + className: "btn btn-primary topic-strategy-summarization", + icon, + title: I18n.t(title), + translatedTitle: I18n.t(title), + translatedLabel: I18n.t(title), + action: canRegenerate ? "regenerateSummary" : "expandSummaryBox", + disabled: this.loadingSummary(), + }); }, - summaryUpdatedEvent(update) { - this.state.summary = update.summary; - this.state.summarizedOn = update.summarizedOn; - this.state.summarizedBy = update.summarizedBy; + hideSummaryButton() { + return this.attach("button", { + className: "btn btn-primary topic-strategy-summarization", + icon: "chevron-up", + title: "summary.buttons.hide", + label: "summary.buttons.hide", + action: "toggleSummaryBox", + disabled: this.loadingSummary(), + }); + }, + + loadingSummary() { + return ( + this.summaryBoxVisble() && (!this.state.summary || this.state.regenerate) + ); + }, + + summaryUpdatedEvent(summary) { + this.state.summary = summary; + + if (summary.regenerated) { + this.state.regenerate = false; + } }, summaryBoxVisble() { @@ -117,6 +144,10 @@ export default createWidget("toggle-topic-summary", { this.state.summaryBoxHidden = false; }, + regenerateSummary() { + this.state.regenerate = true; + }, + toggleSummaryBox() { this.state.summaryBoxHidden = !this.state.summaryBoxHidden; }, diff --git a/app/assets/stylesheets/common/base/topic-summary.scss b/app/assets/stylesheets/common/base/topic-summary.scss index 174bcde18ab..5f81922beb4 100644 --- a/app/assets/stylesheets/common/base/topic-summary.scss +++ b/app/assets/stylesheets/common/base/topic-summary.scss @@ -30,9 +30,13 @@ .summarized-on { text-align: right; - .model-used { + .info-icon { margin-left: 3px; } } + + .outdated-summary { + color: var(--primary-medium); + } } } diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index 9bf783a3a38..dd4da3ad621 100644 --- a/app/controllers/topics_controller.rb +++ b/app/controllers/topics_controller.rb @@ -1179,13 +1179,19 @@ class TopicsController < ApplicationController RateLimiter.new(current_user, "summary", 6, 5.minutes).performed! if current_user + opts = params.permit(:skip_age_check) + hijack do - summary = TopicSummarization.new(strategy).summarize(topic, current_user) + summary = TopicSummarization.new(strategy).summarize(topic, current_user, opts) render json: { summary: summary.summarized_text, summarized_on: summary.updated_at, summarized_by: summary.algorithm, + outdated: summary.outdated, + can_regenerate: Summarization::Base.can_request_summary_for?(current_user), + new_posts_since_summary: + topic.highest_post_number.to_i - summary.content_range&.max.to_i, } end end diff --git a/app/models/summary_section.rb b/app/models/summary_section.rb index 613c59b54e6..21c8ab639fd 100644 --- a/app/models/summary_section.rb +++ b/app/models/summary_section.rb @@ -2,6 +2,14 @@ class SummarySection < ActiveRecord::Base belongs_to :target, polymorphic: true + + def mark_as_outdated + @outdated = true + end + + def outdated + @outdated || false + end end # == Schema Information diff --git a/app/services/topic_summarization.rb b/app/services/topic_summarization.rb index 5d26549f78e..6321831f7c3 100644 --- a/app/services/topic_summarization.rb +++ b/app/services/topic_summarization.rb @@ -5,14 +5,25 @@ class TopicSummarization @strategy = strategy end - def summarize(topic, user) + def summarize(topic, user, opts = {}) existing_summary = SummarySection.find_by(target: topic, meta_section_id: nil) - # 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) + return if !user && !existing_summary - return existing_summary if existing_summary && fresh?(existing_summary, topic) + targets_data = summary_targets(topic).pluck(:post_number, :raw, :username) + + current_topic_sha = build_sha(targets_data.map(&:first)) + can_summarize = Summarization::Base.can_request_summary_for?(user) + + if use_cached?(existing_summary, can_summarize, current_topic_sha, !!opts[:skip_age_check]) + # It's important that we signal a cached summary is outdated + if can_summarize && new_targets?(existing_summary, current_topic_sha) + existing_summary.mark_as_outdated + end + + return existing_summary + end delete_cached_summaries_of(topic) if existing_summary @@ -22,8 +33,6 @@ class TopicSummarization contents: [], } - targets_data = summary_targets(topic).pluck(:post_number, :raw, :username) - targets_data.map do |(pn, raw, username)| content[:contents] << { poster: username, id: pn, text: raw } end @@ -34,7 +43,7 @@ class TopicSummarization end def summary_targets(topic) - @targets ||= topic.has_summary? ? best_replies(topic) : pick_selection(topic) + topic.has_summary? ? best_replies(topic) : pick_selection(topic) end private @@ -73,11 +82,17 @@ class TopicSummarization 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 + # For users without permissions to generate a summary or fresh summaries, we return what we have cached. + def use_cached?(existing_summary, can_summarize, current_sha, skip_age_check) + existing_summary && + !( + can_summarize && new_targets?(existing_summary, current_sha) && + (skip_age_check || existing_summary.created_at < 1.hour.ago) + ) + end - latest_post_to_summarize <= summary.content_range.to_a.last + def new_targets?(summary, current_sha) + summary.original_content_sha != current_sha end def cache_summary(result, post_numbers, topic) @@ -87,7 +102,7 @@ class TopicSummarization algorithm: strategy.model, content_range: (post_numbers.first..post_numbers.last), summarized_text: result[:summary], - original_content_sha: Digest::SHA256.hexdigest(post_numbers.join), + original_content_sha: build_sha(post_numbers), ) result[:chunks].each do |chunk| @@ -96,11 +111,15 @@ class TopicSummarization algorithm: strategy.model, content_range: chunk[:ids].min..chunk[:ids].max, summarized_text: chunk[:summary], - original_content_sha: Digest::SHA256.hexdigest(chunk[:ids].join), + original_content_sha: build_sha(chunk[:ids]), meta_section_id: main_summary.id, ) end main_summary end + + def build_sha(ids) + Digest::SHA256.hexdigest(ids.join) + end end diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 5a5d1aab2cb..fffdee4ec82 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -2043,10 +2043,18 @@ en: in_progress: "Summarizing topic using AI..." summarized_on: "Summarized with AI on %{date}" model_used: "AI used: %{model}" + outdated: "Summary is outdated" + outdated_posts: + one: "(%{count} post missing)" + other: "(%{count} posts missing)" 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." + buttons: + hide: "Hide summary" + generate: "Summarize with AI" + regenerate: "Regenerate summary" # This string uses the ICU Message Format. See https://meta.discourse.org/t/7035 for translation guidelines. description_time_MF: | @@ -2062,9 +2070,6 @@ en: disable: "Show All Posts" short_label: "Top replies" short_title: "Show this topic top replies: the most interesting posts as determined by the community" - strategy: - hide_button_title: "Hide summary" - button_title: "Summarize with AI" deleted_filter: enabled_description: "This topic contains deleted posts, which have been hidden." diff --git a/spec/services/topic_summarization_spec.rb b/spec/services/topic_summarization_spec.rb index aff3b8cc817..00ff67c183e 100644 --- a/spec/services/topic_summarization_spec.rb +++ b/spec/services/topic_summarization_spec.rb @@ -147,29 +147,42 @@ describe TopicSummarization do 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)) } + context "when the user can requests new summaries" do + context "when there are no new posts" do + it "returns the cached summary" do + section = summarization.summarize(topic, user) - it "deletes existing summaries and create a new one" do - section = summarization.summarize(topic, user) - - expect(section.summarized_text).to eq(summarized_text) + expect(section.summarized_text).to eq(cached_text) + end end - it "does nothing if the last summary is less than 12 hours old" do - cached_summary.update!(created_at: 6.hours.ago) + context "when there are new posts" do + before { cached_summary.update!(original_content_sha: "outdated_sha") } - section = summarization.summarize(topic, user) + it "returns a new summary" do + section = summarization.summarize(topic, user) - expect(section.summarized_text).to eq(cached_text) - end - end + expect(section.summarized_text).to eq(summarized_text) + end - context "when the summary targets are still the same" do - it "doesn't create a new summary" do - section = summarization.summarize(topic, user) + context "when the cached summary is less than one hour old" do + before { cached_summary.update!(created_at: 30.minutes.ago) } - expect(section.summarized_text).to eq(cached_text) + it "returns the cached summary" do + cached_summary.update!(created_at: 30.minutes.ago) + + section = summarization.summarize(topic, user) + + expect(section.summarized_text).to eq(cached_text) + expect(section.outdated).to eq(true) + end + + it "returns a new summary if the skip_age_check flag is passed" do + section = summarization.summarize(topic, user, skip_age_check: true) + + expect(section.summarized_text).to eq(summarized_text) + end + end end end end