From 310fa4c487e4233c235234e5c8663bbba5034abc Mon Sep 17 00:00:00 2001 From: Joseph Simmons Date: Tue, 24 Jan 2023 17:48:19 -0600 Subject: [PATCH 1/7] Adds `active_storage_for_images` feature flag This turns on ActiveStroage, just for the Image module. This PR also sets an enviroment variable for `facility_tile_list` --- app/models/concerns/downloadable_files/image.rb | 15 ++++++++++++--- config/settings.yml | 3 ++- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/app/models/concerns/downloadable_files/image.rb b/app/models/concerns/downloadable_files/image.rb index 38ce1e6626..f68cbb44c5 100644 --- a/app/models/concerns/downloadable_files/image.rb +++ b/app/models/concerns/downloadable_files/image.rb @@ -3,26 +3,35 @@ module DownloadableFiles module Image extend ActiveSupport::Concern - include DownloadableFile + + if SettingsHelper.feature_on?(:active_storage_for_images) + include ActiveStorageFile + else + include PaperclipFile + end included do attr_reader :remove_file before_validation { delete_file if remove_file } - if SettingsHelper.feature_on?(:active_storage) + if SettingsHelper.feature_on?(:active_storage_for_images) validates :file, content_type: ["image/jpg", "image/jpeg", "image/png", "image/gif"] else validates_attachment :file, content_type: { content_type: ["image/jpg", "image/jpeg", "image/png", "image/gif"] } end end + def file_present? + file.present? + end + def remove_file=(value) @remove_file = !value.to_i.zero? end def padded_image(width: 400, height: 200, background_color: "rgb(231, 231, 231)") - if SettingsHelper.feature_on?(:active_storage) + if SettingsHelper.feature_on?(:active_storage_for_images) download_url.variant(resize_and_pad: [width, height, { background: background_color }]) else download_url diff --git a/config/settings.yml b/config/settings.yml index f96c117091..c2b2eaa5e2 100644 --- a/config/settings.yml +++ b/config/settings.yml @@ -130,7 +130,8 @@ feature: bypass_kiosk_auth: false revenue_account_editable: false active_storage: false - facility_tile_list: false + active_storage_for_images: true + facility_tile_list: <%= ENV.fetch("FACILITY_TILE_LIST", false) %> facility_tile_list_admin: false po_require_affiliate_account: true From a2b51e2d585aa151fa816189c10b7982e180d7d1 Mon Sep 17 00:00:00 2001 From: Joseph Simmons Date: Thu, 26 Jan 2023 16:30:27 -0600 Subject: [PATCH 2/7] Sets up `ActiveStorageFile` to work with the `active_storage_for_images` flag --- app/models/concerns/active_storage_file.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/concerns/active_storage_file.rb b/app/models/concerns/active_storage_file.rb index d5f3ae303e..007ed384db 100644 --- a/app/models/concerns/active_storage_file.rb +++ b/app/models/concerns/active_storage_file.rb @@ -9,7 +9,7 @@ module ActiveStorageFile has_one_attached :file def assign_attributes(new_attributes) - if SettingsHelper.feature_on?(:active_storage) + if SettingsHelper.feature_on?(:active_storage) || SettingsHelper.feature_on?(:active_storage_for_images) # Ensure the file name and file_type are set first when attaching a StringIO # see #file= method below assign_first = new_attributes.extract!(:name, :file_type) From 6c45c9bed3ce79338fb48f9c490b46b7b95c2359 Mon Sep 17 00:00:00 2001 From: Joseph Simmons Date: Thu, 26 Jan 2023 17:08:55 -0600 Subject: [PATCH 3/7] Changes feature flag from `active_storage_for_images` to `active_storage_for_images_only` --- app/lib/settings_helper.rb | 4 ++++ app/models/concerns/active_storage_file.rb | 2 +- app/models/concerns/downloadable_files/image.rb | 6 +++--- config/settings.yml | 2 +- 4 files changed, 9 insertions(+), 5 deletions(-) diff --git a/app/lib/settings_helper.rb b/app/lib/settings_helper.rb index 93c1111e17..fff12abea6 100644 --- a/app/lib/settings_helper.rb +++ b/app/lib/settings_helper.rb @@ -29,6 +29,10 @@ def self.relays_enabled_for_reservation? setting "relays.#{Rails.env}.reservation_enabled" end + def self.active_storage_enabled? + feature_on?(:active_storage) || feature_on?(:active_storage_for_images_only) + end + # # Used to query the +Settings+ under feature: # [_feature_] diff --git a/app/models/concerns/active_storage_file.rb b/app/models/concerns/active_storage_file.rb index 007ed384db..2ab884e4ff 100644 --- a/app/models/concerns/active_storage_file.rb +++ b/app/models/concerns/active_storage_file.rb @@ -9,7 +9,7 @@ module ActiveStorageFile has_one_attached :file def assign_attributes(new_attributes) - if SettingsHelper.feature_on?(:active_storage) || SettingsHelper.feature_on?(:active_storage_for_images) + if SettingsHelper.active_storage_enabled? # Ensure the file name and file_type are set first when attaching a StringIO # see #file= method below assign_first = new_attributes.extract!(:name, :file_type) diff --git a/app/models/concerns/downloadable_files/image.rb b/app/models/concerns/downloadable_files/image.rb index f68cbb44c5..764c54f455 100644 --- a/app/models/concerns/downloadable_files/image.rb +++ b/app/models/concerns/downloadable_files/image.rb @@ -4,7 +4,7 @@ module Image extend ActiveSupport::Concern - if SettingsHelper.feature_on?(:active_storage_for_images) + if SettingsHelper.active_storage_enabled? include ActiveStorageFile else include PaperclipFile @@ -15,7 +15,7 @@ module Image before_validation { delete_file if remove_file } - if SettingsHelper.feature_on?(:active_storage_for_images) + if SettingsHelper.active_storage_enabled? validates :file, content_type: ["image/jpg", "image/jpeg", "image/png", "image/gif"] else validates_attachment :file, content_type: { content_type: ["image/jpg", "image/jpeg", "image/png", "image/gif"] } @@ -31,7 +31,7 @@ def remove_file=(value) end def padded_image(width: 400, height: 200, background_color: "rgb(231, 231, 231)") - if SettingsHelper.feature_on?(:active_storage_for_images) + if SettingsHelper.active_storage_enabled? download_url.variant(resize_and_pad: [width, height, { background: background_color }]) else download_url diff --git a/config/settings.yml b/config/settings.yml index c2b2eaa5e2..913c7b72cb 100644 --- a/config/settings.yml +++ b/config/settings.yml @@ -130,7 +130,7 @@ feature: bypass_kiosk_auth: false revenue_account_editable: false active_storage: false - active_storage_for_images: true + active_storage_for_images_only: false facility_tile_list: <%= ENV.fetch("FACILITY_TILE_LIST", false) %> facility_tile_list_admin: false po_require_affiliate_account: true From 25c6f3c5e4748a7067895676b646bc4f8c5d96ce Mon Sep 17 00:00:00 2001 From: Joseph Simmons Date: Thu, 26 Jan 2023 17:09:45 -0600 Subject: [PATCH 4/7] Updates feature flag documentation --- doc/HOWTO_feature_flags.md | 1 + 1 file changed, 1 insertion(+) diff --git a/doc/HOWTO_feature_flags.md b/doc/HOWTO_feature_flags.md index 919653245b..9f9518efe3 100644 --- a/doc/HOWTO_feature_flags.md +++ b/doc/HOWTO_feature_flags.md @@ -56,3 +56,4 @@ * `kiosk_view` Kiosk mode - display a list of actionable reservations without logging in (optionally allow acting w/o auth) * `reservations: grace_period`, `reservations: timeout_period`, `occupancies: timeout_period`, `billing: review_period` various grace periods, time periods, and review periods * `active_storage` use `ActiveStorage` if `true`, or `Paperclip` if `false` +* `active_storage_for_images_only` enables `ActiveStorage` for the `DownloadableFiles::Image` module. This flag does not need to be enabled if `active_storage` is From d12a30eedbc02599ba545e562db6fb6cecb1d4f2 Mon Sep 17 00:00:00 2001 From: Joseph Simmons Date: Fri, 27 Jan 2023 09:39:07 -0600 Subject: [PATCH 5/7] Removes feature flag in `assign_attributes` --- app/models/concerns/active_storage_file.rb | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/app/models/concerns/active_storage_file.rb b/app/models/concerns/active_storage_file.rb index 2ab884e4ff..3e3f1d9f7d 100644 --- a/app/models/concerns/active_storage_file.rb +++ b/app/models/concerns/active_storage_file.rb @@ -9,12 +9,10 @@ module ActiveStorageFile has_one_attached :file def assign_attributes(new_attributes) - if SettingsHelper.active_storage_enabled? - # Ensure the file name and file_type are set first when attaching a StringIO - # see #file= method below - assign_first = new_attributes.extract!(:name, :file_type) - super(assign_first) unless assign_first.empty? - end + # Ensure the file name and file_type are set first when attaching a StringIO + # see #file= method below + assign_first = new_attributes.extract!(:name, :file_type) + # super(assign_first) unless assign_first.empty? super(new_attributes) end end From 61024a5898519b0c5ee278523873d4f1a72590f1 Mon Sep 17 00:00:00 2001 From: Joseph Simmons Date: Fri, 27 Jan 2023 10:03:02 -0600 Subject: [PATCH 6/7] Removes commented out code --- app/models/concerns/active_storage_file.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/app/models/concerns/active_storage_file.rb b/app/models/concerns/active_storage_file.rb index 3e3f1d9f7d..1f00d1fd19 100644 --- a/app/models/concerns/active_storage_file.rb +++ b/app/models/concerns/active_storage_file.rb @@ -12,7 +12,6 @@ def assign_attributes(new_attributes) # Ensure the file name and file_type are set first when attaching a StringIO # see #file= method below assign_first = new_attributes.extract!(:name, :file_type) - # super(assign_first) unless assign_first.empty? super(new_attributes) end end From 235d3b8bffb634a38db25b386aa95d08a03ff6aa Mon Sep 17 00:00:00 2001 From: Joseph Simmons Date: Fri, 27 Jan 2023 15:32:16 -0600 Subject: [PATCH 7/7] Removes `active_storage_enabled?` and makes Image use only the `active_storage_for_images_only flag` --- app/lib/settings_helper.rb | 4 ---- app/models/concerns/active_storage_file.rb | 2 ++ app/models/concerns/downloadable_files/image.rb | 6 +++--- app/models/stored_file.rb | 2 +- doc/HOWTO_feature_flags.md | 2 +- spec/spec_helper.rb | 2 +- spec/system/facility_list_tile_spec.rb | 4 ++-- 7 files changed, 10 insertions(+), 12 deletions(-) diff --git a/app/lib/settings_helper.rb b/app/lib/settings_helper.rb index fff12abea6..93c1111e17 100644 --- a/app/lib/settings_helper.rb +++ b/app/lib/settings_helper.rb @@ -29,10 +29,6 @@ def self.relays_enabled_for_reservation? setting "relays.#{Rails.env}.reservation_enabled" end - def self.active_storage_enabled? - feature_on?(:active_storage) || feature_on?(:active_storage_for_images_only) - end - # # Used to query the +Settings+ under feature: # [_feature_] diff --git a/app/models/concerns/active_storage_file.rb b/app/models/concerns/active_storage_file.rb index 1f00d1fd19..9f5ca9df9e 100644 --- a/app/models/concerns/active_storage_file.rb +++ b/app/models/concerns/active_storage_file.rb @@ -12,6 +12,8 @@ def assign_attributes(new_attributes) # Ensure the file name and file_type are set first when attaching a StringIO # see #file= method below assign_first = new_attributes.extract!(:name, :file_type) + + super(assign_first) unless assign_first.empty? super(new_attributes) end end diff --git a/app/models/concerns/downloadable_files/image.rb b/app/models/concerns/downloadable_files/image.rb index 764c54f455..277c02c406 100644 --- a/app/models/concerns/downloadable_files/image.rb +++ b/app/models/concerns/downloadable_files/image.rb @@ -4,7 +4,7 @@ module Image extend ActiveSupport::Concern - if SettingsHelper.active_storage_enabled? + if SettingsHelper.feature_on?(:active_storage_for_images_only) include ActiveStorageFile else include PaperclipFile @@ -15,7 +15,7 @@ module Image before_validation { delete_file if remove_file } - if SettingsHelper.active_storage_enabled? + if SettingsHelper.feature_on?(:active_storage_for_images_only) validates :file, content_type: ["image/jpg", "image/jpeg", "image/png", "image/gif"] else validates_attachment :file, content_type: { content_type: ["image/jpg", "image/jpeg", "image/png", "image/gif"] } @@ -31,7 +31,7 @@ def remove_file=(value) end def padded_image(width: 400, height: 200, background_color: "rgb(231, 231, 231)") - if SettingsHelper.active_storage_enabled? + if SettingsHelper.feature_on?(:active_storage_for_images_only) download_url.variant(resize_and_pad: [width, height, { background: background_color }]) else download_url diff --git a/app/models/stored_file.rb b/app/models/stored_file.rb index ee9e7bf4d6..26668586da 100644 --- a/app/models/stored_file.rb +++ b/app/models/stored_file.rb @@ -13,7 +13,7 @@ class StoredFile < ApplicationRecord validates_inclusion_of :file_type, in: %w(info user_info template template_result sample_result import_error import_upload) validates :name, uniqueness: { scope: :order_detail_id, case_sensitive: false }, if: :order_detail_id? if SettingsHelper.feature_on?(:active_storage) - validates :file, size: { between: 1.kilobyte..10.megabytes }, if: ->(o) { o.file_type == "user_info" } + validates :file, size: { less_than: 10.megabytes }, if: ->(o) { o.file_type == "user_info" } else validates_with AttachmentSizeValidator, attributes: :file, less_than: 10.megabytes, if: ->(o) { o.file_type == "user_info" } end diff --git a/doc/HOWTO_feature_flags.md b/doc/HOWTO_feature_flags.md index 9f9518efe3..d37fc80a64 100644 --- a/doc/HOWTO_feature_flags.md +++ b/doc/HOWTO_feature_flags.md @@ -56,4 +56,4 @@ * `kiosk_view` Kiosk mode - display a list of actionable reservations without logging in (optionally allow acting w/o auth) * `reservations: grace_period`, `reservations: timeout_period`, `occupancies: timeout_period`, `billing: review_period` various grace periods, time periods, and review periods * `active_storage` use `ActiveStorage` if `true`, or `Paperclip` if `false` -* `active_storage_for_images_only` enables `ActiveStorage` for the `DownloadableFiles::Image` module. This flag does not need to be enabled if `active_storage` is +* `active_storage_for_images_only` enables `ActiveStorage` for the `DownloadableFiles::Image` module. This flag needs to be enabled even if `active_storage` is diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index c96b47ea34..1c6e76a60a 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -123,7 +123,7 @@ # https://relishapp.com/rspec/rspec-core/v/3-0/docs/configuration/global-namespace-dsl config.expose_dsl_globally = false - # for testing paperclip attachments + # for testing attachment validations config.include Paperclip::Shoulda::Matchers config.include ActiveStorageValidations::Matchers end diff --git a/spec/system/facility_list_tile_spec.rb b/spec/system/facility_list_tile_spec.rb index b2a99e272f..5f3c0a33f3 100644 --- a/spec/system/facility_list_tile_spec.rb +++ b/spec/system/facility_list_tile_spec.rb @@ -2,11 +2,11 @@ require "rails_helper" -RSpec.describe "Facility list tile", feature_setting: { facility_tile_list: true } do +RSpec.describe "Facility list tile", feature_setting: { facility_tile_list_admin: true, facility_tile_list: true } do # This spec requires that Facility have an attachement. Currently that can be done using # either Paperclip or ActiveStorage. Because Paperclip requires a migration that may or # may not have occurred, these specs are only set to run if ActiveStorage is being used. - if SettingsHelper.feature_on?(:active_storage) + if SettingsHelper.feature_on?(:active_storage_for_images_only) Facility.include DownloadableFiles::Image let(:admin_user) { create(:user, :administrator) }