DEV: Resolve TODO comments for martin-brennan

I am changing many of these to notes or resolving them as is,
most of these I have not actively worked on in years so someone
else can work on them when we get to these areas again.

app/assets/javascripts/discourse/app/instance-initializers/register-media-optimization-upload-processor.js

@@ -16,8 +16,6 @@ export default {
       //
       // Until Safari makes some progress with OffscreenCanvas or other
       // alternatives we cannot support this workflow.
-      //
-      // TODO (martin): Revisit around 2022-06-01 to see the state of iOS Safari.
       if (
         capabilities.isIOS &&
         !siteSettings.composer_ios_media_optimisation_image_enabled

app/controllers/admin/emojis_controller.rb

@@ -5,9 +5,11 @@ class Admin::EmojisController < Admin::AdminController
     render_serialized(Emoji.custom, EmojiSerializer, root: false)
   end
 
-  # TODO (martin) Figure out a way that this kind of custom logic can
+  # NOTE: This kind of custom logic also needs to be implemented to
   # be run in the ExternalUploadManager when a direct S3 upload is completed,
   # related to preventDirectS3Uploads in the UppyUploadMixin.
+  #
+  # Until then, preventDirectS3Uploads is set to true in the UppyUploadMixin.
   def create
     file = params[:file] || params[:files].first
     name = params[:name] || File.basename(file.original_filename, ".*")

app/jobs/regular/sync_acls_for_uploads.rb

@@ -9,10 +9,8 @@ module Jobs
       return if !Discourse.store.external?
       return if !args.key?(:upload_ids)
 
-      # TODO (martin) Change the logging here to debug after acl_stale implemented.
+      # NOTE: These log messages are set to warn to ensure this is working
-      #
+      # as intended in initial production trials, this will need to be set to debug
-      # Note...these log messages are set to warn to ensure this is working
-      # as intended in initial production trials, this will be set to debug
       # after an acl_stale column is added to uploads.
       time =
         Benchmark.measure do

config/locales/server.en.yml

@@ -1983,7 +1983,7 @@ en:
     enable_direct_s3_uploads: "Allows for direct multipart uploads to Amazon S3, see https://meta.discourse.org/t/a-new-era-for-file-uploads-in-discourse/210469 for more details."
     backup_time_of_day: "Time of day UTC when the backup should occur."
     backup_with_uploads: "Include uploads in scheduled backups. Disabling this will only backup the database."
-    backup_location: "Location where backups are stored. IMPORTANT: S3 requires valid S3 credentials entered in Files settings."
+    backup_location: "Location where backups are stored. IMPORTANT: S3 requires valid S3 credentials entered in Files settings. When changing this to S3 from Local, you must run the `s3:ensure_cors_rules` rake task."
     backup_gzip_compression_level_for_uploads: "Gzip compression level used for compressing uploads."
     include_thumbnails_in_backups: "Include generated thumbnails in backups. Disabling this will make backups smaller, but requires a rebake of all posts after a restore."
 

lib/backup_restore/s3_backup_store.rb

@@ -53,9 +53,6 @@ module BackupRestore
       obj = s3_helper.object(filename)
       raise BackupFileExists.new if obj.exists?
 
-      # TODO (martin) We can remove this at a later date when we move this
-      # ensure CORS for backups and direct uploads to a post-site-setting
-      # change event, so the rake task doesn't have to be run manually.
       @s3_helper.ensure_cors!([S3CorsRulesets::BACKUP_DIRECT_UPLOAD])
 
       presigned_url(obj, :put, UPLOAD_URL_EXPIRES_AFTER_SECONDS)

lib/tasks/uploads.rake

@@ -545,10 +545,12 @@ task "uploads:sync_s3_acls" => :environment do
   end
 end
 
-#
+# NOTE: This needs to be updated to use the _first_ UploadReference
-# TODO (martin) Update this rake task to use the _first_ UploadReference
 # record for each upload to determine security, and do not mark things
 # as secure if the first record is something public e.g. a site setting.
+#
+# Alternatively, we need to overhaul this rake task to work with whatever
+# other strategy we come up with for secure uploads.
 task "uploads:disable_secure_uploads" => :environment do
   RailsMultisite::ConnectionManagement.each_connection do |db|
     unless Discourse.store.external?
@@ -597,9 +599,12 @@ end
 # have their secure status changed will have all associated posts
 # rebaked.
 #
-# TODO (martin) Update this rake task to use the _first_ UploadReference
+# NOTE: This needs to be updated to use the _first_ UploadReference
 # record for each upload to determine security, and do not mark things
 # as secure if the first record is something public e.g. a site setting.
+#
+# Alternatively, we need to overhaul this rake task to work with whatever
+# other strategy we come up with for secure uploads.
 task "uploads:secure_upload_analyse_and_update" => :environment do
   RailsMultisite::ConnectionManagement.each_connection do |db|
     unless Discourse.store.external?

plugins/chat/app/controllers/chat/api/direct_messages_controller.rb

@@ -1,7 +1,5 @@
 # frozen_string_literal: true
 
-# TODO (martin) Remove this endpoint when we move to do the channel creation
-# when a message is first sent to avoid double-request round trips for DMs.
 class Chat::Api::DirectMessagesController < Chat::ApiController
   def create
     with_service(Chat::CreateDirectMessageChannel) do

plugins/chat/assets/javascripts/discourse/services/chat.js

@@ -129,7 +129,7 @@ export default class Chat extends Service {
               if (!channel) {
                 return;
               }
-              // TODO (martin) We need to do something here for thread tracking
+              // NOTE: We need to do something here for thread tracking
               // state as well on presence change, otherwise we will be back in
               // the same place as the channels were.
               //

plugins/chat/config/routes.rb

@@ -51,9 +51,6 @@ Chat::Engine.routes.draw do
           "channel_threads_current_user_notifications_settings#update"
     post "/channels/:channel_id/threads/:thread_id/mark-thread-title-prompt-seen/me" =>
            "channel_threads_current_user_title_prompt_seen#update"
-
-    # TODO (martin) Remove this when we refactor the DM channel creation to happen
-    # via message creation in a different API controller.
     post "/direct-message-channels" => "direct_messages#create"
 
     put "/channels/:channel_id/messages/:message_id/restore" => "channel_messages#restore"

spec/system/bookmarks_spec.rb

@@ -65,7 +65,7 @@ describe "Bookmarking posts and topics", type: :system do
     bookmark_menu.click_menu_option("custom")
     expect(bookmark_modal).to be_open
 
-    # TODO (martin) Not sure why, but I need to click this twice for the panel to open :/
+    # NOTE: (martin) Not sure why, but I need to click this twice for the panel to open :/
     bookmark_modal.open_options_panel
     bookmark_modal.open_options_panel
 

spec/system/composer_uploads_spec.rb

@@ -43,11 +43,7 @@ describe "Uploading files in the composer", type: :system do
       SiteSetting.authorized_extensions += "|mp4"
     end
 
-    # TODO (martin): Video streaming is not yet available in Chrome for Testing,
+    it "generates a topic preview thumbnail from the video" do
-    # we need to come back to this in a few months and try again.
-    #
-    # c.f. https://groups.google.com/g/chromedriver-users/c/1SMbByMfO2U
-    xit "generates a topic preview thumbnail from the video" do
       visit "/new-topic"
       expect(composer).to be_opened
       topic.fill_in_composer_title("Video upload test")
@@ -61,8 +57,7 @@ describe "Uploading files in the composer", type: :system do
       composer.submit
 
       expect(find("#topic-title")).to have_content("Video upload test")
-      # I think topic list previews need to be enabled for this?
+      expect(Topic.last.image_upload_id).to eq(Upload.last.id)
-      #expect(topic.image_upload_id).to eq(Upload.last.id)
     end
 
     it "generates a thumbnail from the video" do

spec/system/page_objects/pages/admin_site_settings.rb

@@ -85,10 +85,5 @@ module PageObjects
         assert_selector(".admin-detail .row.setting", minimum: count)
       end
     end
-
-    # TODO (martin) Remove this after discourse-topic-voting no longer
-    # relies on this, it was renamed to AdminSiteSettings.
-    class AdminSettings < PageObjects::Pages::AdminSiteSettings
-    end
   end
 end