From 380910aeddf668f7718630fbf7c91442b678d325 Mon Sep 17 00:00:00 2001
From: Keegan George <kgeorge13@gmail.com>
Date: Fri, 20 Dec 2024 11:22:33 +0900
Subject: [PATCH] DEV: Cleanup todos from codebase (#30394)

This PR involves cleaning up the codebase from my (@keegangeorge's) todos.

In particular:
- Remove Form Template related todos (these are no longer in the roadmap)
- Remove old left-over AI summarization related code after moving to AI (https://github.com/discourse/discourse-ai/pull/658)
- Update one form template related spec
---
 .../form-template-field/dropdown.hbs          |   2 -
 .../form-template-field/multi-select.gjs      |   2 -
 .../components/form-template-field.scss       |   1 -
 app/models/summarization_strategy.rb          |  18 ----
 config/locales/server.en.yml                  |   2 -
 config/site_settings.yml                      |  12 ---
 lib/discourse_plugin_registry.rb              |   2 -
 lib/summarization/base.rb                     | 102 ------------------
 lib/validators/summarization_validator.rb     |  21 ----
 .../admin_customize_form_templates_spec.rb    |   2 +-
 .../page_objects/components/ace_editor.rb     |   5 +
 11 files changed, 6 insertions(+), 163 deletions(-)
 delete mode 100644 app/models/summarization_strategy.rb
 delete mode 100644 lib/summarization/base.rb
 delete mode 100644 lib/validators/summarization_validator.rb

diff --git a/app/assets/javascripts/discourse/app/components/form-template-field/dropdown.hbs b/app/assets/javascripts/discourse/app/components/form-template-field/dropdown.hbs
index 479c732e0c9..a9f2e303177 100644
--- a/app/assets/javascripts/discourse/app/components/form-template-field/dropdown.hbs
+++ b/app/assets/javascripts/discourse/app/components/form-template-field/dropdown.hbs
@@ -14,8 +14,6 @@
     </span>
   {{/if}}
 
-  {{! TODO(@keegan): Update implementation to use <ComboBox/> instead }}
-  {{! Current using <select> as it integrates easily with FormData (will update in v2) }}
   <select
     name={{@id}}
     class="form-template-field__dropdown"
diff --git a/app/assets/javascripts/discourse/app/components/form-template-field/multi-select.gjs b/app/assets/javascripts/discourse/app/components/form-template-field/multi-select.gjs
index 8a1130ddd2d..97d72629d68 100644
--- a/app/assets/javascripts/discourse/app/components/form-template-field/multi-select.gjs
+++ b/app/assets/javascripts/discourse/app/components/form-template-field/multi-select.gjs
@@ -29,8 +29,6 @@ export default class FormTemplateFieldMultiSelect extends Component {
         </span>
       {{/if}}
 
-      {{! TODO(@keegan): Update implementation to use <MultiSelect/> instead }}
-      {{! Current using <select multiple> as it integrates easily with FormData (will update in v2) }}
       <select
         name={{@id}}
         required={{if @validations.required "required" ""}}
diff --git a/app/assets/stylesheets/common/components/form-template-field.scss b/app/assets/stylesheets/common/components/form-template-field.scss
index ce49fd27c0f..f1a37ef3046 100644
--- a/app/assets/stylesheets/common/components/form-template-field.scss
+++ b/app/assets/stylesheets/common/components/form-template-field.scss
@@ -7,7 +7,6 @@
     width: 100%;
   }
 
-  // TODO(@keegan): Remove after updating dropdown/multi-select to use select-kit
   &__dropdown {
     border: 1px solid var(--primary-medium);
     border-radius: var(--d-input-border-radius);
diff --git a/app/models/summarization_strategy.rb b/app/models/summarization_strategy.rb
deleted file mode 100644
index dfc5c377b6b..00000000000
--- a/app/models/summarization_strategy.rb
+++ /dev/null
@@ -1,18 +0,0 @@
-# frozen_string_literal: true
-
-# TODO(@keegan): Remove after removing SiteSetting.summarization_strategy
-
-require "enum_site_setting"
-
-class SummarizationStrategy < EnumSiteSetting
-  def self.valid_value?(val)
-    true
-  end
-
-  def self.values
-    @values ||=
-      Summarization::Base.available_strategies.map do |strategy|
-        { name: strategy.display_name, value: strategy.model }
-      end
-  end
-end
diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml
index d33a79b497a..4d958dbb698 100644
--- a/config/locales/server.en.yml
+++ b/config/locales/server.en.yml
@@ -1837,8 +1837,6 @@ en:
     summary_percent_filter: "When a user clicks 'Summarize This Topic', show the top % of posts"
     summary_max_results: "Maximum posts returned by 'Summarize This Topic'"
     summary_timeline_button: "Show a 'Summarize' button in the timeline"
-    summarization_strategy: "Additional ways to summarize content registered by plugins"
-    custom_summarization_allowed_groups: "Groups allowed to summarize contents using the `summarization_strategy`."
 
     enable_personal_messages: "DEPRECATED, use the 'personal message enabled groups' setting instead. Allow trust level 1 (configurable via min trust to send messages) users to create messages and reply to messages. Note that staff can always send messages no matter what."
     personal_message_enabled_groups: "Allow users in these groups to CREATE personal messages. IMPORTANT: 1) all users can REPLY to messages. 2) Admins and mods can CREATE messages to any user. 3) Trust level groups include higher levels; choose trust_level_1 to allow TL1, TL2, TL3, TL4 but not allow TL0. 4) Group interaction settings override this setting for messaging specific groups."
diff --git a/config/site_settings.yml b/config/site_settings.yml
index 01eec47f63a..6e1bcb61e31 100644
--- a/config/site_settings.yml
+++ b/config/site_settings.yml
@@ -2720,18 +2720,6 @@ uncategorized:
     client: true
     default: false
 
-  summarization_strategy:
-    client: true
-    hidden: true
-    default: ""
-    enum: "SummarizationStrategy"
-    validator: "SummarizationValidator"
-  custom_summarization_allowed_groups:
-    hidden: true
-    type: group_list
-    list_type: compact
-    default: "3|13" # 3: @staff, 13: @trust_level_3
-
   automatic_topic_heat_values: true
 
   # View heat thresholds
diff --git a/lib/discourse_plugin_registry.rb b/lib/discourse_plugin_registry.rb
index 7a5e560a686..a2a764ba32f 100644
--- a/lib/discourse_plugin_registry.rb
+++ b/lib/discourse_plugin_registry.rb
@@ -118,8 +118,6 @@ class DiscoursePluginRegistry
 
   define_filtered_register :list_suggested_for_providers
 
-  define_filtered_register :summarization_strategies
-
   define_filtered_register :post_action_notify_user_handlers
 
   define_filtered_register :post_strippers
diff --git a/lib/summarization/base.rb b/lib/summarization/base.rb
deleted file mode 100644
index 24b91dc05c8..00000000000
--- a/lib/summarization/base.rb
+++ /dev/null
@@ -1,102 +0,0 @@
-# frozen_string_literal: true
-
-# TODO(@keegan): Remove after removing SiteSetting.summarization_strategy
-# Keeping because its still needed for SiteSetting to function.
-# Remove after settings are migrated to AI
-
-# Base class that defines the interface that every summarization
-# strategy must implement.
-# Above each method, you'll find an explanation of what
-# it does and what it should return.
-
-module Summarization
-  class Base
-    class << self
-      def available_strategies
-        DiscoursePluginRegistry.summarization_strategies
-      end
-
-      def find_strategy(strategy_model)
-        available_strategies.detect { |s| s.model == strategy_model }
-      end
-
-      def selected_strategy
-        return if SiteSetting.summarization_strategy.blank?
-
-        find_strategy(SiteSetting.summarization_strategy)
-      end
-
-      def can_see_summary?(target, user)
-        return false if SiteSetting.summarization_strategy.blank?
-        return false if target.class == Topic && target.private_message?
-
-        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
-
-    # Some strategies could require other conditions to work correctly,
-    # like site settings.
-    # This method gets called when admins attempt to select it,
-    # checking if we met those conditions.
-    def correctly_configured?
-      raise NotImplemented
-    end
-
-    # Strategy name to display to admins in the available strategies dropdown.
-    def display_name
-      raise NotImplemented
-    end
-
-    # If we don't meet the conditions to enable this strategy,
-    # we'll display this hint as an error to admins.
-    def configuration_hint
-      raise NotImplemented
-    end
-
-    # The idea behind this method is "give me a collection of texts,
-    # and I'll handle the summarization to the best of my capabilities.".
-    # It's important to emphasize the "collection of texts" part, which implies
-    # it's not tied to any model and expects the "content" to be a hash instead.
-    #
-    # @param content { Hash } - Includes the content to summarize, plus additional
-    # context to help the strategy produce a better result. Keys present in the content hash:
-    #  - resource_path (optional): Helps the strategy build links to the content in the summary (e.g. "/t/-/:topic_id/POST_NUMBER")
-    #  - content_title (optional): Provides guidance about what the content is about.
-    #  - contents (required): Array of hashes with content to summarize (e.g. [{ poster: "asd", id: 1, text: "This is a text" }])
-    #    All keys are required.
-    # @param &on_partial_blk { Block - Optional } - If the strategy supports it, the passed block
-    # will get called with partial summarized text as its generated.
-    #
-    # @param current_user { User } - User requesting the summary.
-    #
-    # @returns { Hash } - The summarized content, plus chunks if the content couldn't be summarized in one pass. Example:
-    #   {
-    #     summary: "This is the final summary",
-    #     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" },
-    #     ],
-    #   }
-    def summarize(content, current_user)
-      raise NotImplemented
-    end
-
-    # Returns the string we'll store in the selected strategy site setting.
-    def model
-      raise NotImplemented
-    end
-  end
-end
diff --git a/lib/validators/summarization_validator.rb b/lib/validators/summarization_validator.rb
deleted file mode 100644
index e6f87e7eac0..00000000000
--- a/lib/validators/summarization_validator.rb
+++ /dev/null
@@ -1,21 +0,0 @@
-# frozen_string_literal: true
-
-# TODO(@keegan): Remove after removing SiteSetting.summarization_strategy
-
-class SummarizationValidator
-  def initialize(opts = {})
-    @opts = opts
-  end
-
-  def valid_value?(val)
-    strategy = Summarization::Base.find_strategy(val)
-
-    return true unless strategy
-
-    strategy.correctly_configured?.tap { |is_valid| @strategy = strategy unless is_valid }
-  end
-
-  def error_message
-    @strategy.configuration_hint
-  end
-end
diff --git a/spec/system/admin_customize_form_templates_spec.rb b/spec/system/admin_customize_form_templates_spec.rb
index 49176a0d7bb..83cc844f7d2 100644
--- a/spec/system/admin_customize_form_templates_spec.rb
+++ b/spec/system/admin_customize_form_templates_spec.rb
@@ -69,7 +69,7 @@ describe "Admin Customize Form Templates", type: :system do
     it "should prefill form data" do
       visit("/admin/customize/form-templates/#{form_template.id}")
       expect(form_template_page).to have_name_value(form_template.name)
-      # TODO(@keegan) difficult to test the ace editor content, todo later
+      expect(ace_editor).to have_content(form_template.template)
     end
   end
 
diff --git a/spec/system/page_objects/components/ace_editor.rb b/spec/system/page_objects/components/ace_editor.rb
index cbb29db863b..7cb0043d430 100644
--- a/spec/system/page_objects/components/ace_editor.rb
+++ b/spec/system/page_objects/components/ace_editor.rb
@@ -26,6 +26,11 @@ module PageObjects
           visible: false,
         )
       end
+
+      def has_content?(content)
+        editor_content = all(".ace_line").map(&:text).join("\n")
+        editor_content == content
+      end
     end
   end
 end