From 21a95b000eac9c1d239e6db9ff9d682b5dfd61da Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Thu, 12 Jan 2023 09:41:39 +1000 Subject: [PATCH] DEV: Remove defunct TODOs (#19825) * Firefox now finally returns PerformanceMeasure from performance.measure * Some TODOs were really more NOTE or FIXME material or no longer relevant * retain_hours is not needed in ExternalUploadsManager, it doesn't seem like anywhere in the UI sends this as a param for uploads * https://github.com/discourse/discourse/pull/18413 was merged so we can remove JS test workaround for settings --- .../discourse/app/components/composer-editor.js | 2 -- .../discourse/app/mixins/extendable-uploader.js | 2 +- .../javascripts/discourse/app/mixins/upload-debugging.js | 1 - app/models/external_upload_stub.rb | 3 --- app/services/external_upload_manager.rb | 2 -- lib/s3_helper.rb | 9 +++------ lib/upload_creator.rb | 7 ++----- .../unit/lib/chat-emoji-reaction-store-test.js | 5 +---- 8 files changed, 7 insertions(+), 24 deletions(-) diff --git a/app/assets/javascripts/discourse/app/components/composer-editor.js b/app/assets/javascripts/discourse/app/components/composer-editor.js index 2c8cd343389..7388a6fc3da 100644 --- a/app/assets/javascripts/discourse/app/components/composer-editor.js +++ b/app/assets/javascripts/discourse/app/components/composer-editor.js @@ -106,8 +106,6 @@ export default Component.extend(ComposerUploadUppy, { fileUploadElementId: "file-uploader", mobileFileUploaderId: "mobile-file-upload", - // TODO (martin) Remove this once the chat plugin is using the new composerEventPrefix - eventPrefix: "composer", composerEventPrefix: "composer", uploadType: "composer", uppyId: "composer-editor-uppy", diff --git a/app/assets/javascripts/discourse/app/mixins/extendable-uploader.js b/app/assets/javascripts/discourse/app/mixins/extendable-uploader.js index 8a20a5858ba..d5dab6cce9c 100644 --- a/app/assets/javascripts/discourse/app/mixins/extendable-uploader.js +++ b/app/assets/javascripts/discourse/app/mixins/extendable-uploader.js @@ -74,7 +74,7 @@ export default Mixin.create(UploadDebugging, { } }, - // TODO (martin) This and _onPreProcessComplete will need to be tweaked + // NOTE: This and _onPreProcessComplete will need to be tweaked // if we ever add support for "determinate" preprocessors for uppy, which // means the progress will have a value rather than a started/complete // state ("indeterminate"). diff --git a/app/assets/javascripts/discourse/app/mixins/upload-debugging.js b/app/assets/javascripts/discourse/app/mixins/upload-debugging.js index 0c7a935f3e9..07c6d598547 100644 --- a/app/assets/javascripts/discourse/app/mixins/upload-debugging.js +++ b/app/assets/javascripts/discourse/app/mixins/upload-debugging.js @@ -31,7 +31,6 @@ export default Mixin.create({ _instrumentUploadTimings() { if (!this._performanceApiSupport()) { - // TODO (martin) (2021-01-23) Check if FireFox fixed this yet. warn( "Some browsers do not return a PerformanceMeasure when calling performance.mark, disabling instrumentation. See https://developer.mozilla.org/en-US/docs/Web/API/Performance/measure#return_value and https://bugzilla.mozilla.org/show_bug.cgi?id=1724645", { id: "discourse.upload-debugging" } diff --git a/app/models/external_upload_stub.rb b/app/models/external_upload_stub.rb index 99ed046b97e..10004f29069 100644 --- a/app/models/external_upload_stub.rb +++ b/app/models/external_upload_stub.rb @@ -43,9 +43,6 @@ class ExternalUploadStub < ActiveRecord::Base @statuses ||= Enum.new(created: 1, uploaded: 2, failed: 3) end - # TODO (martin): Lifecycle rule would be best to clean stuff up in the external - # systems, I don't think we really want to be calling out to the external systems - # here right? def self.cleanup! expired_created.delete_all expired_uploaded.delete_all diff --git a/app/services/external_upload_manager.rb b/app/services/external_upload_manager.rb index 23523ab4bc0..58823158ff4 100644 --- a/app/services/external_upload_manager.rb +++ b/app/services/external_upload_manager.rb @@ -149,8 +149,6 @@ class ExternalUploadManager raise ChecksumMismatchError if external_sha1 && external_sha1 != actual_sha1 end - # TODO (martin): See if these additional opts will be needed - # - check if retain_hours is needed opts = { type: external_upload_stub.upload_type, existing_external_upload_key: external_upload_stub.key, diff --git a/lib/s3_helper.rb b/lib/s3_helper.rb index f8082f04caf..b8dc762c84c 100644 --- a/lib/s3_helper.rb +++ b/lib/s3_helper.rb @@ -175,12 +175,9 @@ class S3Helper cors_rules: final_rules, }, ) - rescue Aws::S3::Errors::AccessDenied => err - # TODO (martin) Remove this warning log level once we are sure this new - # ensure_cors! rule is functioning correctly. - Discourse.warn_exception( - err, - message: "Could not PutBucketCors rules for #{@s3_bucket_name}, rules: #{final_rules}", + rescue Aws::S3::Errors::AccessDenied + Rails.logger.info( + "Could not PutBucketCors rules for #{@s3_bucket_name}, rules: #{final_rules}", ) return false end diff --git a/lib/upload_creator.rb b/lib/upload_creator.rb index d83af7461c9..32922cc468d 100644 --- a/lib/upload_creator.rb +++ b/lib/upload_creator.rb @@ -57,9 +57,6 @@ class UploadCreator true end ) - - # TODO (martin) Validate @opts[:type] to make sure only blessed types are passed - # in, since the clientside can pass any type it wants. end def create_for(user_id) @@ -78,13 +75,13 @@ class UploadCreator is_image ||= @image_info && FileHelper.is_supported_image?("test.#{@image_info.type}") is_image = false if @opts[:for_theme] - # if this is present then it means we are creating an upload record from + # If this is present then it means we are creating an upload record from # an external_upload_stub and the file is > ExternalUploadManager::DOWNLOAD_LIMIT, # so we have not downloaded it to a tempfile. no modifications can be made to the # file in this case because it does not exist; we simply move it to its new location # in S3 # - # TODO (martin) I've added a bunch of external_upload_too_big checks littered + # FIXME: I've added a bunch of external_upload_too_big checks littered # throughout the UploadCreator code. It would be better to have two seperate # classes with shared methods, rather than doing all these checks all over the # place. Needs a refactor. diff --git a/plugins/chat/test/javascripts/unit/lib/chat-emoji-reaction-store-test.js b/plugins/chat/test/javascripts/unit/lib/chat-emoji-reaction-store-test.js index b53a2084dc5..2f34312a6d5 100644 --- a/plugins/chat/test/javascripts/unit/lib/chat-emoji-reaction-store-test.js +++ b/plugins/chat/test/javascripts/unit/lib/chat-emoji-reaction-store-test.js @@ -16,13 +16,10 @@ module("Discourse Chat | Unit | chat-emoji-reaction-store", function (hooks) { this.chatEmojiReactionStore.reset(); }); - // TODO (martin) Remove site setting workarounds after core PR#1290 test("defaults", function (assert) { assert.deepEqual( this.chatEmojiReactionStore.favorites, - (this.siteSettings.default_emoji_reactions || "") - .split("|") - .filter((val) => val) + this.siteSettings.default_emoji_reactions.split("|").filter((val) => val) ); });