Add endpoint for accessing variant screenshots

.gitignore

@@ -25,6 +25,7 @@
 /.powenv
 /.env.local
 /db/seed_apps.yml
+/db/seeds/*.yml
 
 /app/assets/builds/*
 !/app/assets/builds/.keep

.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:

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

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

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

app/views/admin/splits/_variants.html.erb

@@ -9,6 +9,9 @@
       <thead>
         <th>Name</th>
         <th>Description</th>
+        <% if TestTrack::AttachmentSettings.attachments_enabled? %>
+          <th>Screenshot</th>
+        <% end %>
         <th>Weight (Assignee Count)</th>
         <th></th>
       </thead>
@@ -17,6 +20,9 @@
           <tr>
             <td><%= variant_detail.display_name %></td>
             <td><%= variant_detail.description %></td>
+            <% if TestTrack::AttachmentSettings.attachments_enabled? %>
+              <td><%= link_to(variant_detail.filename, variant_screenshot_path(variant_detail), target: '_blank') if variant_detail.screenshot_file_name? %></td>
+            <% end %>
             <td><%= variant_detail.weight %>%(<%= variant_detail.assignment_count %>)</td>
             <td><%= link_to 'edit', edit_admin_split_variant_detail_path(split, variant_detail.variant) %></td>
           </tr>

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 %>
 
   <div class="mt-6">

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.

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

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

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

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') }

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

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