Overhaul target editor

.overcommit.yml

@@ -8,3 +8,8 @@ PreCommit:
     exclude:
       - '**/*.svg'
       - 'app/javascript/**/*.bs.js'
+
+  Stylelint:
+    enabled: true
+    command: ['yarn', 'run', '--silent', 'stylelint']
+    required_executable: './node_modules/stylelint/bin/stylelint.js'

.rubocop.yml

@@ -27,9 +27,9 @@ Style/FormatStringToken:
   Enabled: false
 Style/DoubleNegation:
   Enabled: false
-Layout/AlignParameters:
+Layout/ParameterAlignment:
   EnforcedStyle: with_fixed_indentation
-Layout/AlignArguments:
+Layout/ArgumentAlignment:
   EnforcedStyle: with_fixed_indentation
 Layout/MultilineOperationIndentation:
   EnforcedStyle: indented
@@ -40,6 +40,8 @@ Layout/CaseIndentation:
   IndentOneStep: true
 Layout/EndAlignment:
   EnforcedStyleAlignWith: variable
+Layout/LineLength:
+  Enabled: false
 Naming/VariableNumber:
   Enabled: false
 Metrics/BlockLength:
@@ -48,8 +50,6 @@ Metrics/BlockLength:
     - 'spec/**/*_spec.rb'
     - 'config/**/*'
     - 'spec/factories/*'
-Metrics/LineLength:
-  Enabled: false
 Lint/ShadowingOuterLocalVariable: # shadowing is a language feature - we should use it where appropriate.
   Enabled: false
 
@@ -61,10 +61,6 @@ Bundler/OrderedGems:
   Enabled: false # we have a ton of gems, grouped arbitrarily. Could be tackled later.
 Style/FrozenStringLiteralComment:
   Enabled: false # forces addition of frozen string literal directive to all files. WTF.
-StringLiterals:
-  Enabled: false
-Documentation:
-  Enabled: false
 Metrics/MethodLength:
   Max: 20 # default was 10.
 Metrics/ClassLength:
@@ -75,3 +71,9 @@ Style/GuardClause:
   Enabled: false # this thing is annoying, and can lead to not-as-readable code.
 Style/YodaCondition:
   Enabled: false # buggy - sometimes complains about fixed code, and autocorrect doesn't work.
+Style/StringLiterals:
+  Enabled: false
+Style/Documentation:
+  Enabled: false
+Layout/RescueEnsureAlignment:
+  Enabled: false # buggy

.stylelintrc.json

@@ -0,0 +1,19 @@
+{
+  "extends": "stylelint-config-recommended",
+  "rules": {
+    "at-rule-no-unknown": [
+      true,
+      {
+        "ignoreAtRules": [
+          "tailwind",
+          "apply",
+          "variants",
+          "responsive",
+          "screen"
+        ]
+      }
+    ],
+    "declaration-block-trailing-semicolon": null,
+    "no-descending-specificity": null
+  }
+}

Gemfile

@@ -155,8 +155,8 @@ group :development, :test do
   gem 'coderay', '~> 1.1' # Pretty syntax highlighting on rspec failure snippets.
   gem 'pry-rails', '~> 0.3.5' # Pry debugger.
   gem 'webmock', '~> 3.5' # Mocking web requests.
-  gem 'rubocop', '~> 0.74', require: false # Ruby Style Guide.
-  gem 'rubocop-rails', '~> 2.3', require: false # A RuboCop extension focused on enforcing Rails best practices and coding conventions.
+  gem 'rubocop', '~> 0.79', require: false # Ruby Style Guide.
+  gem 'rubocop-rails', '~> 2.4', require: false # A RuboCop extension focused on enforcing Rails best practices and coding conventions.
   gem 'bundler-audit', '~> 0.5', require: false # Audit gems in gemfile.lock for reported vulnerabilities
   gem 'overcommit', '~> 0.38', require: false # A fully configurable and extendable Git hook manager
   gem 'fuubar', '~> 2.5' # The instafailing RSpec progress bar formatter.

Gemfile.lock

@@ -392,7 +392,7 @@ GEM
     iniparse (1.4.4)
     insensitive_hash (0.3.3)
     intercom (3.8.0)
-    jaro_winkler (1.5.3)
+    jaro_winkler (1.5.4)
     jbuilder (2.9.1)
       activesupport (>= 4.2.0)
     jmespath (1.4.0)
@@ -495,8 +495,8 @@ GEM
     overcommit (0.49.1)
       childprocess (>= 0.6.3, < 2.0)
       iniparse (~> 1.4)
-    parallel (1.17.0)
-    parser (2.6.4.1)
+    parallel (1.19.1)
+    parser (2.7.0.2)
       ast (~> 2.4.0)
     pg (1.1.4)
     polyamorous (2.3.0)
@@ -519,7 +519,7 @@ GEM
       nio4r (~> 2.0)
     pundit (2.1.0)
       activesupport (>= 3.0.0)
-    rack (2.0.8)
+    rack (2.1.1)
     rack-contrib (2.1.0)
       rack (~> 2.0)
     rack-cors (1.0.6)
@@ -675,14 +675,14 @@ GEM
     rspec-retry (0.6.1)
       rspec-core (> 3.3)
     rspec-support (3.9.0)
-    rubocop (0.74.0)
+    rubocop (0.79.0)
       jaro_winkler (~> 1.5.1)
       parallel (~> 1.10)
-      parser (>= 2.6)
+      parser (>= 2.7.0.1)
       rainbow (>= 2.2.2, < 4.0)
       ruby-progressbar (~> 1.7)
       unicode-display_width (>= 1.4.0, < 1.7)
-    rubocop-rails (2.3.2)
+    rubocop-rails (2.4.1)
       rack (>= 1.1)
       rubocop (>= 0.72.0)
     ruby-progressbar (1.10.1)
@@ -758,7 +758,7 @@ GEM
     unf (0.1.4)
       unf_ext
     unf_ext (0.0.7.6)
-    unicode-display_width (1.6.0)
+    unicode-display_width (1.6.1)
     uniform_notifier (1.12.1)
     url_mount (0.2.1)
       rack
@@ -903,8 +903,8 @@ DEPENDENCIES
   rspec-eventually (~> 0.2.2)
   rspec-rails (~> 4.0.0.beta3)
   rspec-retry (~> 0.5)
-  rubocop (~> 0.74)
-  rubocop-rails (~> 2.3)
+  rubocop (~> 0.79)
+  rubocop-rails (~> 2.4)
   sass-rails (>= 6)
   scarf (~> 0.2)
   scout_apm (~> 2.6)

app/controllers/schools/content_blocks_controller.rb

@@ -1,37 +1,22 @@
 module Schools
   class ContentBlocksController < SchoolsController
-    before_action :authorize_target
+    include CamelizeKeys
+    include StringifyIds
 
     # POST /school/targets/:target_id/content_block
     def create
+      target = Target.find(params[:target_id])
+
+      # Let's authorize against the level, since that's the resource we have with a matching action in the policy.
+      authorize(target.level, policy_class: Schools::LevelPolicy)
+
       form = ::Schools::Targets::CreateContentBlockForm.new(ContentBlock.new)
 
-      if form.validate(content_block_params)
-        content_block = form.save
-        render json: content_block_data(content_block)
+      if form.validate(params)
+        render json: camelize_keys(stringify_ids(form.save.merge(error: nil)))
       else
         render json: { error: form.errors.full_messages.join(', ') }
       end
     end
-
-    protected
-
-    def authorize_target
-      authorize(Target.find(params[:target_id]), policy_class: Schools::TargetPolicy)
-    end
-
-    def content_block_data(content_block)
-      content_block_data = { id: content_block.id.to_s, content: content_block.content, error: nil }
-      content_block.file.attached? ? content_block_data.merge!(fileUrl: url_for(content_block.file)) : content_block_data
-      content_block_data.merge(versions: target_versions)
-    end
-
-    def content_block_params
-      params[:content_block].merge(target_id: params[:target_id], content_sort_indices: JSON.parse(params[:content_sort_indices]))
-    end
-
-    def target_versions
-      Target.find(params[:target_id]).content_versions.order('version_on DESC').distinct(:version_on).pluck(:version_on)
-    end
   end
 end

app/controllers/schools/courses_controller.rb

@@ -21,7 +21,7 @@ def attach_images
 
     # GET /courses/:id/curriculum
     def curriculum
-      course = scope.where(id: params[:id]).includes([:evaluation_criteria, :levels, :target_groups, targets: [:evaluation_criteria, :prerequisite_targets, :resources, quiz: { quiz_questions: %I[answer_options correct_answer] }]]).first
+      course = scope.where(id: params[:id]).includes(:evaluation_criteria, :levels, :target_groups, :targets).first
       @course = authorize(course, policy_class: Schools::CoursePolicy)
     end
 

app/controllers/schools/targets_controller.rb

@@ -1,28 +1,18 @@
 module Schools
   class TargetsController < SchoolsController
-    before_action :load_target, only: %w[update]
+    layout 'school'
 
-    # PATCH /school/targets/:id
-    def update
-      form = ::Schools::Targets::UpdateForm.new(@target)
-
-      if form.validate(params[:target])
-        form.save
-        render json: { error: nil }
-      else
-        render json: { error: form.errors.full_messages.join(', ') }
-      end
-    end
-
-    # GET /school/targets/:id/content
+    # GET /school/courses/:course_id/targets/:id/content
     def content
-      render json: camelize_keys(stringify_ids(Targets::FetchContentService.new(@target).details))
+      @course = current_school.courses.find(params[:course_id])
+      authorize(@course.targets.find(params[:id]), policy_class: Schools::TargetPolicy)
+      render 'schools/courses/curriculum'
     end
 
-    protected
+    # GET /school/courses/:course_id/targets/:id/details
+    alias details content
 
-    def load_target
-      @target = authorize(Target.find(params[:id]), policy_class: Schools::TargetPolicy)
-    end
+    # GET /school/courses/:course_id/targets/:id/versions
+    alias versions content
   end
 end

app/controllers/users/omniauth_callbacks_controller.rb

@@ -10,7 +10,7 @@ def oauth_callback
       if oauth_origin.present?
         if @email.blank?
           redirect_to oauth_error_url(host: oauth_origin[:fqdn], error: email_blank_flash)
-          return
+          nil
         else
           sign_in_at_oauth_origin
         end

app/forms/schools/targets/create_content_block_form.rb

@@ -1,30 +1,54 @@
 module Schools
   module Targets
     class CreateContentBlockForm < Reform::Form
-      property :block_type, validates: { presence: true, inclusion: { in: ContentBlock.valid_block_types } }
+      property :block_type, validates: { presence: true, inclusion: { in: %w[image file] } }
       property :target_id, virtual: true, validates: { presence: true }
-      property :url, virtual: true
-      property :content_sort_indices, virtual: true, validates: { presence: true }
       property :file, virtual: true
-      property :markdown, virtual: true
-      property :title, virtual: true
-      property :caption, virtual: true
+      property :above_content_block_id, virtual: true
 
       validates :file, presence: true, image: true, file_size: { less_than: 5.megabytes }, if: :image_block?
       validates :file, presence: true, file_size: { less_than: 10.megabytes }, if: :file_block?
-      validates :markdown, presence: true, if: :markdown_block?
-      validates :url, presence: true, if: :embed_block?
 
       def save
-        ::ContentBlocks::CreateService.new(target, content_block_params).execute
+        ContentBlock.transaction do
+          content_block = create_file_or_image_block
+          shift_content_blocks_below(content_block)
+          json_attributes(content_block)
+        end
       end
 
       private
 
+      def create_file_or_image_block
+        target_version.content_blocks.create!(
+          block_type: block_type,
+          content: content(block_type),
+          file: file,
+          sort_index: sort_index
+        )
+      end
+
+      def content(block_type)
+        filename = file.original_filename
+
+        case block_type
+          when 'image'
+            { caption: filename }
+          when 'file'
+            { title: filename }
+          else
+            raise "Unexpected block type #{block_type} encountered when creating file-based content block for target with ID #{target_id}"
+        end
+      end
+
       def target
         Target.find(target_id)
       end
 
+      def target_version
+        @target_version ||= target.current_target_version
+      end
+
       def image_block?
         block_type == ContentBlock::BLOCK_TYPE_IMAGE
       end
@@ -33,24 +57,46 @@ def file_block?
         block_type == ContentBlock::BLOCK_TYPE_FILE
       end
 
-      def markdown_block?
-        block_type == ContentBlock::BLOCK_TYPE_MARKDOWN
+      def above_content_block
+        @above_content_block ||= begin
+          target.content_blocks.find_by(id: above_content_block_id) if above_content_block_id.present?
+        end
       end
 
-      def embed_block?
-        block_type == ContentBlock::BLOCK_TYPE_EMBED
+      def sort_index
+        @sort_index ||= begin
+          if above_content_block.present?
+            # Put at the same position as 'above_content_block'.
+            above_content_block.sort_index
+          else
+            # Put at the bottom.
+            content_blocks.maximum(:sort_index) + 1
+          end
+        end
       end
 
-      def content_block_params
-        {
-          block_type: block_type,
-          url: url,
-          content_sort_indices: content_sort_indices,
-          file: file,
-          markdown: markdown,
-          title: title,
-          caption: caption
-        }
+      def content_blocks
+        target_version.content_blocks
+      end
+
+      def shift_content_blocks_below(content_block)
+        content_blocks.where.not(id: content_block.id).where('sort_index >= ?', sort_index)
+          .update_all('sort_index = sort_index + 1') # rubocop:disable Rails/SkipsModelValidations
+      end
+
+      def json_attributes(content_block)
+        attributes = content_block.attributes
+          .slice('id', 'block_type', 'content', 'sort_index')
+          .with_indifferent_access
+
+        if content_block.file.attached?
+          attributes.merge(
+            fileUrl: Rails.application.routes.url_helpers.rails_blob_path(content_block.file, only_path: true),
+            filename: content_block.file.filename.to_s
+          )
+        else
+          attributes
+        end
       end
     end
   end