From 2b04d72cdd8012f0ac02f1e4ceb250f6498d2b1f Mon Sep 17 00:00:00 2001 From: smudge Date: Tue, 12 Nov 2024 16:55:12 -0500 Subject: [PATCH 01/10] Support fixture files if they are present. --- .gitignore | 1 + db/seeds.rb | 6 ++++++ 2 files changed, 7 insertions(+) diff --git a/.gitignore b/.gitignore index 4521fdd1..ef20de62 100644 --- a/.gitignore +++ b/.gitignore @@ -25,6 +25,7 @@ /.powenv /.env.local /db/seed_apps.yml +/spec/fixtures /app/assets/builds/* !/app/assets/builds/.keep diff --git a/db/seeds.rb b/db/seeds.rb index 59bcffd7..1136231c 100644 --- a/db/seeds.rb +++ b/db/seeds.rb @@ -8,6 +8,12 @@ puts "Ensured #{app_name} app" end end + + require 'active_record/fixtures' + fixtures_dir = File.join(Rails.root, 'spec/fixtures') + fixture_files = Dir.glob('spec/fixtures/*.yml').map { |f| File.basename(f, '.yml') } + + ActiveRecord::FixtureSet.create_fixtures(fixtures_dir, fixture_files) end test_track_app = App.find_or_create_by!(name: 'TestTrack') do |app| From 93e1b0d2e06d44220274edbe29346014e7645c53 Mon Sep 17 00:00:00 2001 From: smudge Date: Tue, 12 Nov 2024 16:57:47 -0500 Subject: [PATCH 02/10] Add an admin user during seeds --- db/seeds.rb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/db/seeds.rb b/db/seeds.rb index 1136231c..bf04dcb6 100644 --- a/db/seeds.rb +++ b/db/seeds.rb @@ -23,3 +23,8 @@ IdentifierType.find_or_create_by!(name: 'app_id') do |identifier_type| identifier_type.owner_app = test_track_app end + +Admin.find_or_create_by!(email: 'admin@example.org') do |user| + user.password = 'password' + user.password_confirmation = 'password' +end From 225f3b270fb4b02a785ece71faedb43501317865 Mon Sep 17 00:00:00 2001 From: smudge Date: Tue, 12 Nov 2024 16:57:59 -0500 Subject: [PATCH 03/10] Increase default max image size to 1 MB --- lib/test_track/attachment_settings.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/test_track/attachment_settings.rb b/lib/test_track/attachment_settings.rb index be7ce871..0a420837 100644 --- a/lib/test_track/attachment_settings.rb +++ b/lib/test_track/attachment_settings.rb @@ -19,7 +19,7 @@ def storage_settings end def max_size - ENV['ATTACHMENT_MAX_SIZE'] || 512.kilobytes + ENV['ATTACHMENT_MAX_SIZE']&.to_i || 1.megabyte end private From b23a8f29e63497efb9bd4133c333bb5768688b9a Mon Sep 17 00:00:00 2001 From: smudge Date: Wed, 13 Nov 2024 11:40:07 -0500 Subject: [PATCH 04/10] New endpoint for viewing variant screenshots --- .../admin/screenshots_controller.rb | 16 ++++++ app/models/variant_detail.rb | 4 ++ app/views/admin/variant_details/edit.html.erb | 3 +- config/routes.rb | 2 +- spec/models/variant_detail_spec.rb | 20 +++++++ spec/requests/admin/screenshots_spec.rb | 54 +++++++++++++++++++ 6 files changed, 97 insertions(+), 2 deletions(-) create mode 100644 app/controllers/admin/screenshots_controller.rb create mode 100644 spec/requests/admin/screenshots_spec.rb diff --git a/app/controllers/admin/screenshots_controller.rb b/app/controllers/admin/screenshots_controller.rb new file mode 100644 index 00000000..506f03da --- /dev/null +++ b/app/controllers/admin/screenshots_controller.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true +class Admin::ScreenshotsController < ApplicationController + def show + split = Split.find(params[:split_id]) + variant_detail = split.variant_details.find_by!(variant: variant) + raise ActiveRecord::RecordNotFound unless variant_detail.screenshot_file_name? + + redirect_to variant_detail.screenshot.expiring_url(300) + end + + private + + def variant + params.fetch(:id).split('.').first + end +end diff --git a/app/models/variant_detail.rb b/app/models/variant_detail.rb index 0367de16..587b5450 100644 --- a/app/models/variant_detail.rb +++ b/app/models/variant_detail.rb @@ -24,6 +24,10 @@ def retirable? weight.zero? && assignment_count.positive? end + def filename + "#{variant}.#{screenshot_file_name.split('.').last}" if screenshot_file_name? + end + private def variant_must_exist diff --git a/app/views/admin/variant_details/edit.html.erb b/app/views/admin/variant_details/edit.html.erb index ac30589f..388aaf09 100644 --- a/app/views/admin/variant_details/edit.html.erb +++ b/app/views/admin/variant_details/edit.html.erb @@ -9,7 +9,8 @@ <%= f.input :description, label: 'Short description', hint: 'For example: There are FAQ links in a sidebar on the Activity Tab' %> <% if TestTrack::AttachmentSettings.attachments_enabled? %> - <%= f.input :screenshot, as: :file, hint: (safe_join(['Current: ', link_to(@variant_detail.screenshot_file_name, @variant_detail.screenshot.expiring_url(300), target: '_blank')]) if @variant_detail.screenshot.present?) %> + <%= f.input :screenshot, as: :file, hint: (safe_join(['Current: ', link_to(@variant_detail.screenshot_file_name, @variant_detail.screenshot.expiring_url(300), target: '_blank')]) if @variant_detail.screenshot_file_name?) %> + <%= image_tag admin_split_screenshot_path(@variant_detail.filename, split_id: @variant_detail.split.id) if @variant_detail.screenshot_file_name? %> <% end %>
diff --git a/config/routes.rb b/config/routes.rb index 2757788f..b8c0f0c0 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -105,7 +105,6 @@ devise_for :admins end - namespace :admin do root 'splits#index' resources :splits, only: [:show] do @@ -117,6 +116,7 @@ resource :retirement, only: [:create], controller: 'variant_retirements' end resources :variant_details, only: [:edit, :update] + resources :screenshots, only: [:show] end end end diff --git a/spec/models/variant_detail_spec.rb b/spec/models/variant_detail_spec.rb index d10fd88a..9bc9a5d3 100644 --- a/spec/models/variant_detail_spec.rb +++ b/spec/models/variant_detail_spec.rb @@ -64,6 +64,26 @@ end end + describe '#filename' do + subject { described_class.new(split: split, variant: 'my_treatment') } + + it 'returns null' do + expect(subject.filename).to be_nil + end + + context 'when a screenshot is attached' do + before do + subject.screenshot_file_name = 'screenshot.png' + subject.screenshot_content_type = 'image/png' + subject.screenshot_file_size = 1_234 + end + + it 'returns the variant name and the file extension' do + expect(subject.filename).to eq 'my_treatment.png' + end + end + end + describe '#valid?' do context 'with a variant that exists' do subject { FactoryBot.build(:variant_detail, split: split, variant: 'true') } diff --git a/spec/requests/admin/screenshots_spec.rb b/spec/requests/admin/screenshots_spec.rb new file mode 100644 index 00000000..cd063bab --- /dev/null +++ b/spec/requests/admin/screenshots_spec.rb @@ -0,0 +1,54 @@ +# frozen_string_literal: true +require 'rails_helper' + +RSpec.describe Admin::ScreenshotsController do + let(:split) { FactoryBot.create(:split, name: 'my_split') } + + around do |example| + env_config = Rails.application.env_config + show_exceptions_was = env_config['action_dispatch.show_exceptions'] + show_detailed_exceptions_was = env_config['action_dispatch.show_detailed_exceptions'] + env_config['action_dispatch.show_exceptions'] = true + env_config['action_dispatch.show_detailed_exceptions'] = false + example.run + ensure + env_config['action_dispatch.show_exceptions'] = show_exceptions_was + env_config['action_dispatch.show_detailed_exceptions'] = show_detailed_exceptions_was + end + + describe 'GET /variant.ext' do + it 'returns a 404' do + get "/admin/splits/#{split.id}/screenshots/hammer_time.jpg" + + expect(response).to have_http_status(:not_found) + end + + context 'when a variant detail record exists' do + let!(:variant_detail) do + FactoryBot.create(:variant_detail, split: split, variant: 'hammer_time') + end + + it 'returns a 404' do + get "/admin/splits/#{split.id}/screenshots/hammer_time.jpg" + + expect(response).to have_http_status(:not_found) + end + + context 'when a screenshot exists' do + before do + variant_detail.screenshot_file_name = '1x1.jpg' + variant_detail.screenshot_content_type = 'image/jpeg' + variant_detail.screenshot_file_size = 123 + variant_detail.save! + end + + it 'redirects to the screenshot' do + get "/admin/splits/#{split.id}/screenshots/hammer_time.jpg" + + expect(response).to have_http_status(:found) + expect(response).to redirect_to(variant_detail.screenshot.url) + end + end + end + end +end From be430fcbcbb4d60d408eff8e901866fecc0661aa Mon Sep 17 00:00:00 2001 From: smudge Date: Wed, 13 Nov 2024 11:41:04 -0500 Subject: [PATCH 05/10] Allow for a .test local URL --- config/environments/development.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/config/environments/development.rb b/config/environments/development.rb index 3bbdb991..272937bc 100644 --- a/config/environments/development.rb +++ b/config/environments/development.rb @@ -1,6 +1,8 @@ Rails.application.configure do # Settings specified here will take precedence over those in config/application.rb. + config.hosts << "testtrack.test" + # In the development environment your application's code is reloaded any time # it changes. This slows down response time but is perfect for development # since you don't have to restart the web server when you make code changes. From 183c79a19312b7c780ff3d31448c41c27bbea222 Mon Sep 17 00:00:00 2001 From: smudge Date: Wed, 13 Nov 2024 12:32:22 -0500 Subject: [PATCH 06/10] Show screenshot on variant overview in split details --- app/helpers/application_helper.rb | 4 ++++ app/views/admin/splits/_variants.html.erb | 6 ++++++ app/views/admin/variant_details/edit.html.erb | 3 +-- 3 files changed, 11 insertions(+), 2 deletions(-) diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index 55aad5ac..3f93437e 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -6,4 +6,8 @@ def identifier_types def percentage(ratio) "#{(ratio * 100).round}%" end + + def variant_screenshot_path(detail) + admin_split_screenshot_path(detail.filename, split_id: detail.split.id) if detail.screenshot_file_name? + end end diff --git a/app/views/admin/splits/_variants.html.erb b/app/views/admin/splits/_variants.html.erb index bd654ef4..2b5508db 100644 --- a/app/views/admin/splits/_variants.html.erb +++ b/app/views/admin/splits/_variants.html.erb @@ -9,6 +9,9 @@ Name Description + <% if TestTrack::AttachmentSettings.attachments_enabled? %> + Screenshot + <% end %> Weight (Assignee Count) @@ -17,6 +20,9 @@ <%= variant_detail.display_name %> <%= variant_detail.description %> + <% if TestTrack::AttachmentSettings.attachments_enabled? %> + <%= link_to(variant_detail.filename, variant_screenshot_path(variant_detail), target: '_blank') if variant_detail.screenshot_file_name? %> + <% end %> <%= variant_detail.weight %>%(<%= variant_detail.assignment_count %>) <%= link_to 'edit', edit_admin_split_variant_detail_path(split, variant_detail.variant) %> diff --git a/app/views/admin/variant_details/edit.html.erb b/app/views/admin/variant_details/edit.html.erb index 388aaf09..77d42785 100644 --- a/app/views/admin/variant_details/edit.html.erb +++ b/app/views/admin/variant_details/edit.html.erb @@ -9,8 +9,7 @@ <%= f.input :description, label: 'Short description', hint: 'For example: There are FAQ links in a sidebar on the Activity Tab' %> <% if TestTrack::AttachmentSettings.attachments_enabled? %> - <%= f.input :screenshot, as: :file, hint: (safe_join(['Current: ', link_to(@variant_detail.screenshot_file_name, @variant_detail.screenshot.expiring_url(300), target: '_blank')]) if @variant_detail.screenshot_file_name?) %> - <%= image_tag admin_split_screenshot_path(@variant_detail.filename, split_id: @variant_detail.split.id) if @variant_detail.screenshot_file_name? %> + <%= f.input :screenshot, as: :file, hint: (safe_join(['Current: ', link_to(@variant_detail.filename, variant_screenshot_path(@variant_detail), target: '_blank')]) if @variant_detail.screenshot_file_name?) %> <% end %>
From 6b18a0daea3d2d503ac40aac8388bbf703eb87e8 Mon Sep 17 00:00:00 2001 From: smudge Date: Wed, 13 Nov 2024 12:39:50 -0500 Subject: [PATCH 07/10] Move to db/seeds since it's not spec-related --- .gitignore | 2 +- db/seeds.rb | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.gitignore b/.gitignore index ef20de62..be8b3c4a 100644 --- a/.gitignore +++ b/.gitignore @@ -25,7 +25,7 @@ /.powenv /.env.local /db/seed_apps.yml -/spec/fixtures +/db/seeds/*.yml /app/assets/builds/* !/app/assets/builds/.keep diff --git a/db/seeds.rb b/db/seeds.rb index bf04dcb6..7ca61c88 100644 --- a/db/seeds.rb +++ b/db/seeds.rb @@ -10,8 +10,8 @@ end require 'active_record/fixtures' - fixtures_dir = File.join(Rails.root, 'spec/fixtures') - fixture_files = Dir.glob('spec/fixtures/*.yml').map { |f| File.basename(f, '.yml') } + fixtures_dir = File.join(Rails.root, 'db/seeds') + fixture_files = Dir.glob('db/seeds/*.yml').map { |f| File.basename(f, '.yml') } ActiveRecord::FixtureSet.create_fixtures(fixtures_dir, fixture_files) end From 0a73ca1599935fdfd4cac8185ab00ca745a155ac Mon Sep 17 00:00:00 2001 From: smudge Date: Wed, 13 Nov 2024 12:45:38 -0500 Subject: [PATCH 08/10] Linter --- .rubocop_todo.yml | 20 ++++++------------- .../admin/screenshots_controller.rb | 1 + spec/requests/admin/screenshots_spec.rb | 1 + 3 files changed, 8 insertions(+), 14 deletions(-) diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 08010175..585fdc8e 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -1,6 +1,6 @@ # This configuration was generated by # `rubocop --auto-gen-config --auto-gen-only-exclude --exclude-limit 99999999999` -# on 2023-03-28 16:02:29 UTC using RuboCop version 1.46.0. +# on 2024-11-13 17:45:18 UTC using RuboCop version 1.46.0. # The point is for the user to remove these configuration records # one by one as the offenses are removed from the code base. # Note that changes in the inspected code, or installation of new @@ -11,11 +11,12 @@ Betterment/NonStandardActions: Exclude: - 'config/routes.rb' -# Offense count: 13 +# Offense count: 14 Betterment/UnscopedFind: Exclude: - 'app/controllers/admin/bulk_assignments_controller.rb' - 'app/controllers/admin/decisions_controller.rb' + - 'app/controllers/admin/screenshots_controller.rb' - 'app/controllers/admin/split_configs_controller.rb' - 'app/controllers/admin/split_details_controller.rb' - 'app/controllers/admin/splits_controller.rb' @@ -44,12 +45,11 @@ Layout/ArgumentAlignment: - 'spec/models/deterministic_assignment_creation_spec.rb' - 'spec/models/visitor_supersession_spec.rb' -# Offense count: 2 +# Offense count: 1 # This cop supports safe autocorrection (--autocorrect). Layout/EmptyLines: Exclude: - 'config/initializers/devise.rb' - - 'config/routes.rb' # Offense count: 1 # This cop supports safe autocorrection (--autocorrect). @@ -76,7 +76,7 @@ Layout/LineLength: - 'config/initializers/devise.rb' - 'config/initializers/simple_form.rb' -# Offense count: 11 +# Offense count: 10 # This cop supports safe autocorrection (--autocorrect). Lint/RedundantCopDisableDirective: Exclude: @@ -88,7 +88,6 @@ Lint/RedundantCopDisableDirective: - 'app/models/arbitrary_assignment_creation.rb' - 'app/models/concerns/delegate_attribute.rb' - 'app/models/deterministic_assignment_creation.rb' - - 'app/models/split_upsert.rb' - 'app/models/visitor_supersession.rb' - 'lib/test_track/attachment_settings.rb' @@ -156,12 +155,11 @@ Rails/EnumHash: Exclude: - 'app/models/split.rb' -# Offense count: 6 +# Offense count: 5 # Configuration parameters: EnforcedStyle. # SupportedStyles: slashes, arguments Rails/FilePath: Exclude: - - 'config/environments/development.rb' - 'lib/tasks/seed_app.rake' - 'spec/rails_helper.rb' - 'spec/requests/api/v1/split_details_spec.rb' @@ -210,12 +208,6 @@ Style/ColonMethodCall: Exclude: - 'config/initializers/airbrake.rb' -# Offense count: 1 -# This cop supports safe autocorrection (--autocorrect). -Style/ExpandPathArguments: - Exclude: - - 'config.ru' - # Offense count: 2 # This cop supports unsafe autocorrection (--autocorrect-all). Style/GlobalStdStream: diff --git a/app/controllers/admin/screenshots_controller.rb b/app/controllers/admin/screenshots_controller.rb index 506f03da..b3ebe964 100644 --- a/app/controllers/admin/screenshots_controller.rb +++ b/app/controllers/admin/screenshots_controller.rb @@ -1,4 +1,5 @@ # frozen_string_literal: true + class Admin::ScreenshotsController < ApplicationController def show split = Split.find(params[:split_id]) diff --git a/spec/requests/admin/screenshots_spec.rb b/spec/requests/admin/screenshots_spec.rb index cd063bab..468d077a 100644 --- a/spec/requests/admin/screenshots_spec.rb +++ b/spec/requests/admin/screenshots_spec.rb @@ -1,4 +1,5 @@ # frozen_string_literal: true + require 'rails_helper' RSpec.describe Admin::ScreenshotsController do From 75bf50b7682eac0a2f26f3dabe0b4702c9653771 Mon Sep 17 00:00:00 2001 From: smudge Date: Wed, 13 Nov 2024 13:11:09 -0500 Subject: [PATCH 09/10] Fix tests --- spec/support/pages/admin/split_show_page.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/spec/support/pages/admin/split_show_page.rb b/spec/support/pages/admin/split_show_page.rb index 7a52bee7..ec2d662d 100644 --- a/spec/support/pages/admin/split_show_page.rb +++ b/spec/support/pages/admin/split_show_page.rb @@ -17,8 +17,9 @@ class AdminSplitShowPage < SitePrism::Page sections :variants, ".fs-VariantsTable tbody tr" do element :name, 'td:nth-of-type(1)' element :description, 'td:nth-of-type(2)' - element :weight, 'td:nth-of-type(3)' - element :edit_link, 'td:nth-of-type(4) a' + element :screenshot, 'td:nth-of-type(3)' + element :weight, 'td:nth-of-type(4)' + element :edit_link, 'td:nth-of-type(5) a' end def edit_variant(text) variants_table.find('tr', text: text).find('a').click From d2d831df14ff811d85d4bc080094430a93218bd1 Mon Sep 17 00:00:00 2001 From: smudge Date: Wed, 13 Nov 2024 17:30:15 -0500 Subject: [PATCH 10/10] Fix test, use original file name on upload form --- app/views/admin/variant_details/edit.html.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/admin/variant_details/edit.html.erb b/app/views/admin/variant_details/edit.html.erb index 77d42785..c01effd4 100644 --- a/app/views/admin/variant_details/edit.html.erb +++ b/app/views/admin/variant_details/edit.html.erb @@ -9,7 +9,7 @@ <%= f.input :description, label: 'Short description', hint: 'For example: There are FAQ links in a sidebar on the Activity Tab' %> <% if TestTrack::AttachmentSettings.attachments_enabled? %> - <%= f.input :screenshot, as: :file, hint: (safe_join(['Current: ', link_to(@variant_detail.filename, variant_screenshot_path(@variant_detail), target: '_blank')]) if @variant_detail.screenshot_file_name?) %> + <%= f.input :screenshot, as: :file, hint: (safe_join(['Current: ', link_to(@variant_detail.screenshot_file_name, variant_screenshot_path(@variant_detail), target: '_blank')]) if @variant_detail.screenshot_file_name?) %> <% end %>