From 5bb4d43a4c347f4bcad390b171690eae1364572e Mon Sep 17 00:00:00 2001 From: Sam Davies Date: Wed, 10 Jan 2024 11:19:06 +0700 Subject: [PATCH 1/2] WEB-6757: Checking for image existence in content modules --- app/lib/linting/content_module_linter.rb | 4 +++ app/lib/linting/image/content_module.rb | 23 ++++++++++++++ app/lib/linting/image/lesson.rb | 23 ++++++++++++++ app/lib/linting/image/markdown.rb | 7 +++-- app/lib/linting/image/segment.rb | 38 ++++++++++++++++++++++++ app/lib/linting/image_linter.rb | 11 ++++--- 6 files changed, 100 insertions(+), 6 deletions(-) create mode 100644 app/lib/linting/image/content_module.rb create mode 100644 app/lib/linting/image/lesson.rb create mode 100644 app/lib/linting/image/segment.rb diff --git a/app/lib/linting/content_module_linter.rb b/app/lib/linting/content_module_linter.rb index ec6e411..6f14a67 100644 --- a/app/lib/linting/content_module_linter.rb +++ b/app/lib/linting/content_module_linter.rb @@ -42,6 +42,10 @@ def lint_with_ui(options:, show_ui: true) # rubocop:disable Metrics/AbcSize end return unless annotations.blank? + with_spinner(title: 'Validating image references', show: show_ui) do + annotations.concat(Linting::ImageLinter.new(content_module:).lint) + end + with_spinner(title: 'Validating data models', show: show_ui) do annotations.concat(Linting::Validations::ContentModule.new(content_module:, file:).lint) end diff --git a/app/lib/linting/image/content_module.rb b/app/lib/linting/image/content_module.rb new file mode 100644 index 0000000..1654531 --- /dev/null +++ b/app/lib/linting/image/content_module.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +module Linting + module Image + # Lint the images associated with a content module + class ContentModule + include Linting::Image::Attachable + + def self.lint(content_module) + new(content_module).lint + end + + def lint + [].tap do |annotations| + annotations.concat(lint_attachments) + object.lessons.each do |lesson| + annotations.concat(Linting::Image::Lesson.lint(lesson)) + end + end + end + end + end +end diff --git a/app/lib/linting/image/lesson.rb b/app/lib/linting/image/lesson.rb new file mode 100644 index 0000000..b5ee585 --- /dev/null +++ b/app/lib/linting/image/lesson.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +module Linting + module Image + # Lint the image associated with a lesson + class Lesson + include Linting::Image::Attachable + + def self.lint(lesson) + new(lesson).lint + end + + def lint + [].tap do |annotations| + annotations.concat(lint_attachments) + object.segments.each do |segment| + annotations.concat(Linting::Image::Segment.lint(segment)) + end + end + end + end + end +end diff --git a/app/lib/linting/image/markdown.rb b/app/lib/linting/image/markdown.rb index aee9541..7cfd5a2 100644 --- a/app/lib/linting/image/markdown.rb +++ b/app/lib/linting/image/markdown.rb @@ -8,8 +8,11 @@ module Markdown attr_reader :markdown_file - def lint_markdown_images - _md_generate_annotations(_md_non_existent_images, :non_existent) + _md_generate_annotations(_md_images_missing_width, :missing_width) + def lint_markdown_images(check_width: true) + annotations = _md_generate_annotations(_md_non_existent_images, :non_existent) + return annotations unless check_width + + annotations + _md_generate_annotations(_md_images_missing_width, :missing_width) end private diff --git a/app/lib/linting/image/segment.rb b/app/lib/linting/image/segment.rb new file mode 100644 index 0000000..eb21016 --- /dev/null +++ b/app/lib/linting/image/segment.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +module Linting + module Image + # Lint the images associated with a segment + class Segment + include Linting::Image::Attachable + include Linting::Image::Markdown + + def self.lint(segment) + new(segment).lint + end + + def lint + [].tap do |annotations| + annotations.concat(lint_attachments) if object.class < Concerns::ImageAttachable + annotations.concat(lint_markdown_images(check_width: false)) if markdown_file.present? + end + end + + def markdown_file + case segment_type + when 'video' + object.script_file + when 'text' + object.markdown_file + else + nil + end + end + + # What is the segment type + def segment_type + object.class.name.demodulize.underscore + end + end + end +end diff --git a/app/lib/linting/image_linter.rb b/app/lib/linting/image_linter.rb index 0db6875..7577f9b 100644 --- a/app/lib/linting/image_linter.rb +++ b/app/lib/linting/image_linter.rb @@ -1,16 +1,19 @@ # frozen_string_literal: true module Linting - # Lints the images in a book + # Lints the images in a book or a content_module class ImageLinter - attr_reader :book + attr_reader :book, :content_module - def initialize(book:) + def initialize(book: nil, content_module: nil) @book = book + @content_module = content_module end def lint - Linting::Image::Book.lint(book) + return Linting::Image::Book.lint(book) if book.present? + + Linting::Image::ContentModule.lint(content_module) if content_module.present? end end end From a0d7ed2078518cf03d56a4cd2c66b2bcdb9d653f Mon Sep 17 00:00:00 2001 From: Sam Davies Date: Wed, 10 Jan 2024 15:41:30 +0700 Subject: [PATCH 2/2] WEB-6757: Improving lesson and segment validation --- app/models/lesson.rb | 1 + app/models/lessons_validator.rb | 20 +++++++--------- app/models/segments_validator.rb | 39 ++++++++++++++++++++++++++++++++ 3 files changed, 48 insertions(+), 12 deletions(-) create mode 100644 app/models/segments_validator.rb diff --git a/app/models/lesson.rb b/app/models/lesson.rb index 7f5499d..ce70093 100644 --- a/app/models/lesson.rb +++ b/app/models/lesson.rb @@ -12,6 +12,7 @@ class Lesson attr_markdown :description, source: :description_md, file: false attr_markdown :learning_objectives, source: :learning_objectives_md, file: false validates :title, :ordinal, :ref, presence: true + validates :segments, length: { minimum: 1 }, allow_blank: false, segments: true def initialize(attributes = {}) super diff --git a/app/models/lessons_validator.rb b/app/models/lessons_validator.rb index aca3586..cbeb196 100644 --- a/app/models/lessons_validator.rb +++ b/app/models/lessons_validator.rb @@ -19,24 +19,20 @@ def check_correct_class(record, attribute, value) def check_unique_refs(record, attribute, value) return unless value.is_a?(Array) - value.each do |lesson| - ref_counts = Hash.new(0) - lesson.segments.each { |segment| ref_counts[segment.ref] += 1 } - non_unique_refs = ref_counts.select { |_, count| count > 1 }.keys + ref_counts = Hash.new(0) + value.each { |lesson| ref_counts[lesson.ref] += 1 } + non_unique_refs = ref_counts.select { |_, count| count > 1 }.keys - non_unique_refs.each { |ref| record.errors.add(attribute, "(#{lesson.title}) segment ref #{ref} is not unique") } - end + non_unique_refs.each { |ref| record.errors.add(attribute, "=> ref '#{ref}' is not unique") } end def check_unique_title(record, attribute, value) return unless value.is_a?(Array) - value.each do |lesson| - title_counts = Hash.new(0) - lesson.segments.each { |segment| title_counts[segment.title] += 1 } - non_unique_titles = title_counts.select { |_, count| count > 1 }.keys + title_counts = Hash.new(0) + value.each { |lesson| title_counts[lesson.title] += 1 } + non_unique_titles = title_counts.select { |_, count| count > 1 }.keys - non_unique_titles.each { |title| record.errors.add(attribute, "(#{lesson.title}) segment title #{title} is not unique") } - end + non_unique_titles.each { |title| record.errors.add(attribute, "=> Title '#{title}' is not unique") } end end diff --git a/app/models/segments_validator.rb b/app/models/segments_validator.rb new file mode 100644 index 0000000..02fabfc --- /dev/null +++ b/app/models/segments_validator.rb @@ -0,0 +1,39 @@ +# frozen_string_literal: true + +# A validator that will check an array of choices for uniqueness of ref +class SegmentsValidator < ActiveModel::EachValidator + def validate_each(record, attribute, value) + return unless value.is_a?(Array) + + check_correct_class(record, attribute, value) + check_unique_refs(record, attribute, value) + check_unique_title(record, attribute, value) + end + + def check_correct_class(record, attribute, value) + value.each do |segment| + record.errors.add(attribute, "segment #{segment} needs to be a quiz, video or text") unless [Assessment::Quiz, Video, Text].any? { |klass| segment.is_a?(klass) } + end + end + + def check_unique_refs(record, attribute, value) + return unless value.is_a?(Array) + + ref_counts = Hash.new(0) + value.each { |segment| ref_counts[segment.ref] += 1 } + non_unique_refs = ref_counts.select { |_, count| count > 1 }.keys + + non_unique_refs.each { |ref| record.errors.add(attribute, "(#{record.title}) => Segment ref '#{ref}' is not unique") } + end + + def check_unique_title(record, attribute, value) + return unless value.is_a?(Array) + + title_counts = Hash.new(0) + + value.each { |segment| title_counts[segment.title] += 1 } + non_unique_titles = title_counts.select { |_, count| count > 1 }.keys + + non_unique_titles.each { |title| record.errors.add(attribute, "(#{record.title}) => Segment title '#{title}' is not unique") } + end +end