diff --git a/.gitignore b/.gitignore index 4521fdd1..be8b3c4a 100644 --- a/.gitignore +++ b/.gitignore @@ -25,6 +25,7 @@ /.powenv /.env.local /db/seed_apps.yml +/db/seeds/*.yml /app/assets/builds/* !/app/assets/builds/.keep 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 new file mode 100644 index 00000000..b3ebe964 --- /dev/null +++ b/app/controllers/admin/screenshots_controller.rb @@ -0,0 +1,17 @@ +# 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/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/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/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 ac30589f..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.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_screenshot_path(@variant_detail), target: '_blank')]) if @variant_detail.screenshot_file_name?) %> <% end %>
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. 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/db/seeds.rb b/db/seeds.rb index 59bcffd7..7ca61c88 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, 'db/seeds') + fixture_files = Dir.glob('db/seeds/*.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| @@ -17,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 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 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..468d077a --- /dev/null +++ b/spec/requests/admin/screenshots_spec.rb @@ -0,0 +1,55 @@ +# 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 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