diff --git a/.travis.yml b/.travis.yml index 5a77c33..e16eac7 100644 --- a/.travis.yml +++ b/.travis.yml @@ -60,12 +60,9 @@ name: "Brakeman check" script: bundle exec brakeman -w2 --except=Redirect,CrossSiteScripting - # Removed this temporarily until this issue is fixed: - # https://github.com/rubyzip/rubyzip/issues/369 - # - # - stage: security - # name: "Bundle audit" - # script: bundle exec bundle-audit check --update + - stage: security + name: "Bundle audit" + script: bundle exec bundle-audit check --update - stage: hygiene name: "Check seeds are valid" diff --git a/Gemfile b/Gemfile index c05979c..da0ea4b 100644 --- a/Gemfile +++ b/Gemfile @@ -193,6 +193,11 @@ end group :development do + + gem "progress_bar", require: false + + gem "text", require: false + # Better error page for Rails and other Rack apps (https://github.com/charliesome/better_errors) gem "better_errors" diff --git a/Gemfile.lock b/Gemfile.lock index 0f2186a..61f84fc 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -158,6 +158,7 @@ rspec (>= 2.99.0, < 4.0) hashdiff (0.3.7) hashie (3.5.7) + highline (1.7.10) htmltoword (1.0.0) actionpack nokogiri @@ -232,6 +233,7 @@ ruby_dig (~> 0.0.2) omniauth-shibboleth (1.3.0) omniauth (>= 1.0.0) + options (2.3.2) orm_adapter (0.5.0) parallel (1.12.1) parser (2.5.1.2) @@ -240,6 +242,9 @@ po_to_json (1.0.1) json (>= 1.6.0) powerpack (0.1.2) + progress_bar (1.2.0) + highline (~> 1.6) + options (~> 2.3.0) pry (0.11.3) coderay (~> 1.1.0) method_source (~> 0.9.0) @@ -327,7 +332,7 @@ ruby-progressbar (1.9.0) ruby_dep (1.5.0) ruby_dig (0.0.2) - rubyzip (1.2.1) + rubyzip (1.2.2) safe_yaml (1.0.4) sax-machine (1.3.2) selenium-webdriver (3.13.1) @@ -427,6 +432,7 @@ omniauth-orcid omniauth-shibboleth pg (~> 0.19.0) + progress_bar pundit rack-mini-profiler rails (~> 4.2.10) @@ -442,6 +448,7 @@ simplecov spring spring-commands-rspec + text thin web-console webmock diff --git a/app/models/annotation.rb b/app/models/annotation.rb index 6c7d7b5..51e0d51 100644 --- a/app/models/annotation.rb +++ b/app/models/annotation.rb @@ -2,17 +2,19 @@ # # Table name: annotations # -# id :integer not null, primary key -# text :text -# type :integer default(0), not null -# created_at :datetime -# updated_at :datetime -# org_id :integer -# question_id :integer +# id :integer not null, primary key +# text :text +# type :integer default(0), not null +# created_at :datetime +# updated_at :datetime +# org_id :integer +# question_id :integer +# versionable_id :string(36) # # Indexes # -# index_annotations_on_question_id (question_id) +# index_annotations_on_question_id (question_id) +# index_annotations_on_versionable_id (versionable_id) # # Foreign Keys # @@ -22,6 +24,7 @@ class Annotation < ActiveRecord::Base include ValidationMessages + include VersionableModel ## # I liked type as the name for the enum so overriding inheritance column diff --git a/app/models/concerns/acts_as_sortable.rb b/app/models/concerns/acts_as_sortable.rb index f5ebc6e..6112b79 100644 --- a/app/models/concerns/acts_as_sortable.rb +++ b/app/models/concerns/acts_as_sortable.rb @@ -30,12 +30,12 @@ connection.execute(query) end - def self.update_numbers_mysql2!(ids) + def update_numbers_mysql2!(ids) ids_string = ids.map { |id| "'#{id}'" }.join(",") update_all(%Q{ number = FIELD(id, #{sanitize_sql(ids_string)}) }) end - def self.update_numbers_sequentially!(ids) + def update_numbers_sequentially!(ids) ids.each_with_index.map do |id, number| find(id).update_attribute(:number, number + 1) end diff --git a/app/models/concerns/versionable_model.rb b/app/models/concerns/versionable_model.rb new file mode 100644 index 0000000..154dc1d --- /dev/null +++ b/app/models/concerns/versionable_model.rb @@ -0,0 +1,19 @@ +module VersionableModel + + extend ActiveSupport::Concern + + included do + + attr_readonly :versionable_id + + before_validation :set_versionable_id, unless: :versionable_id? + + end + + private + + def set_versionable_id + self.versionable_id = SecureRandom.uuid + end + +end \ No newline at end of file diff --git a/app/models/phase.rb b/app/models/phase.rb index f56edf6..84c216c 100644 --- a/app/models/phase.rb +++ b/app/models/phase.rb @@ -2,18 +2,20 @@ # # Table name: phases # -# id :integer not null, primary key -# description :text -# modifiable :boolean -# number :integer -# title :string -# created_at :datetime -# updated_at :datetime -# template_id :integer +# id :integer not null, primary key +# description :text +# modifiable :boolean +# number :integer +# title :string +# created_at :datetime +# updated_at :datetime +# template_id :integer +# versionable_id :string(36) # # Indexes # -# index_phases_on_template_id (template_id) +# index_phases_on_template_id (template_id) +# index_phases_on_versionable_id (versionable_id) # # Foreign Keys # @@ -29,6 +31,8 @@ include ValidationMessages include ValidationValues include ActsAsSortable + include VersionableModel + ## # Sort order: Number ASC @@ -115,4 +119,5 @@ value = Rational(num_answered_questions(plan), plan.num_questions) * 100 value >= Rails.application.config.default_plan_percentage_answered.to_f end + end diff --git a/app/models/question.rb b/app/models/question.rb index 801660d..7b82119 100644 --- a/app/models/question.rb +++ b/app/models/question.rb @@ -1,5 +1,4 @@ # frozen_string_literal: true - # == Schema Information # # Table name: questions @@ -14,10 +13,12 @@ # updated_at :datetime # question_format_id :integer # section_id :integer +# versionable_id :string(36) # # Indexes # -# index_questions_on_section_id (section_id) +# index_questions_on_section_id (section_id) +# index_questions_on_versionable_id (versionable_id) # # Foreign Keys # @@ -26,9 +27,9 @@ # class Question < ActiveRecord::Base - include ValidationMessages include ActsAsSortable + include VersionableModel # ============== # = Attributes = @@ -98,7 +99,6 @@ delegate :option_based?, to: :question_format - # =========================== # = Public instance methods = # =========================== diff --git a/app/models/section.rb b/app/models/section.rb index 49f9237..66fc2f3 100644 --- a/app/models/section.rb +++ b/app/models/section.rb @@ -1,21 +1,22 @@ # frozen_string_literal: true - # == Schema Information # # Table name: sections # -# id :integer not null, primary key -# description :text -# modifiable :boolean -# number :integer -# title :string -# created_at :datetime -# updated_at :datetime -# phase_id :integer +# id :integer not null, primary key +# description :text +# modifiable :boolean +# number :integer +# title :string +# created_at :datetime +# updated_at :datetime +# phase_id :integer +# versionable_id :string(36) # # Indexes # -# index_sections_on_phase_id (phase_id) +# index_sections_on_phase_id (phase_id) +# index_sections_on_versionable_id (versionable_id) # # Foreign Keys # @@ -23,10 +24,11 @@ # class Section < ActiveRecord::Base - include ValidationMessages include ValidationValues include ActsAsSortable + include VersionableModel + # ================ # = Associations = @@ -116,7 +118,7 @@ end # Can't be modified as it was duplicatd over from another Phase. - def template? + def unmodifiable? !modifiable? end diff --git a/app/models/section_sorter.rb b/app/models/section_sorter.rb new file mode 100644 index 0000000..092e3b5 --- /dev/null +++ b/app/models/section_sorter.rb @@ -0,0 +1,107 @@ +# frozen_string_literal: true + +# Private: Takes a list of Sections and sorts them in the correct display order based on +# the number, modifiable, and id attributes. +# +# Examples: +# +# SectionSorter.new(*@phase.sections).sort! # => Array of sorted Sections +# +# +class SectionSorter + + ## + # Access the array of Sections + # + # Returns Array + attr_accessor :sections + + ## + # Initialize a new SectionSorter + # + # sections - A set of Section records + # + def initialize(*sections) + @sections = sections + end + + # Re-order {#sections} into the correct order. + # + # Returns Array of Sections + def sort! + if all_sections_unmodifiable? + sort_as_homogenous_group + elsif all_sections_modifiable? + sort_as_homogenous_group + else + # If there are duplicates in the #1 position + if duplicate_number_values.include?(1) + + mod_1 = sections.select { |section| section.modifiable? && section.number == 1 } + + # There should only be, if any, one prefixed modifiable Section + prefix = mod_1.shift + + # In the off-chance that there is more than one prefix Section, stick them + # after the unmodifiable block + erratic = mod_1 + + # Collect the unmodifiable Section ids in the order the should be displayed + unmodifiable = sections + .select { |section| section.unmodifiable? && section.number > 1 } + .sort_by { |section| [section.number, section.id] } + + # Then any additional Sections that come after the main block... + modifiable = sections + .select { |section| section.modifiable? && section.number > 1 } + .sort_by { |section| [section.number, section.id] } + + # Create one Array with all of the ids in the correct order. + self.sections = [prefix] + unmodifiable + erratic + modifiable + else + prefix = sections.detect { |s| s.modifiable? && s.number == 1 } + remaining_sections = sections - [prefix] + unmodifiable = remaining_sections.select(&:unmodifiable?) + .sort_by { |s| [s.number, s.id] } + modifiable = remaining_sections.select(&:modifiable?) + .sort_by { |s| [s.number, s.id] } + + self.sections = [prefix] + unmodifiable + modifiable + end + sections.uniq.compact + end + end + + private + + def modifiable_values + @modifiable_values ||= sections.map(&:modifiable?).uniq + end + + def number_values_with_count + @number_values_with_count ||= begin + hash = Hash.new { |hash, key| hash[key] = 0 } + sections.map(&:number).each { |number| hash[number] += 1 } + hash + end + end + + def duplicate_number_values + @duplicate_number_values ||= number_values_with_count.select do |number, count| + count > 1 + end.keys + end + + def all_sections_unmodifiable? + modifiable_values == [false] + end + + def all_sections_modifiable? + modifiable_values == [true] + end + + def sort_as_homogenous_group + sections.sort_by { |section| [section.number, section.id] } + end + +end diff --git a/app/models/template.rb b/app/models/template.rb index 5822eb9..ae57810 100644 --- a/app/models/template.rb +++ b/app/models/template.rb @@ -394,106 +394,7 @@ # Generates a new copy of self including latest changes from the funder this # template is customized_of def upgrade_customization! - if customization_of.blank? - raise _("upgrade_customization! requires a customised template") - end - funder_template = Template.published(customization_of).first - - if funder_template.blank? - # rubocop:disable Metrics/LineLength - raise _("upgrade_customization! cannot be carried out since there is no published template of its current funder") - # rubocop:enable Metrics/LineLength - end - - # preserves modifiable flags from the self template copied - source = deep_copy(attributes: { version: version + 1, published: false }) - - # Creates a new customisation for the published template whose family_id is - # self.customization_of - customization = funder_template.deep_copy( - attributes: { - version: source.version, - published: source.published, - family_id: source.family_id, - customization_of: source.customization_of, - org: source.org, - visibility: Template.visibilities[:organisationally_visible], - is_default: false - }, modifiable: false, save: true - ) - - # Sorts the phases from the source template, i.e. self - sorted_phases = source.phases.sort_by(&:number) - - # Merges modifiable sections or questions from source into customization - # template object - # rubocop:disable Metrics/BlockLength - customization.phases.each do |customization_phase| - # Search for the phase in the source template whose number matches the - # customization_phase - - candidate_phase = sorted_phases.bsearch do |phase| - customization_phase.number <=> phase.number - end - - # The funder could have added this new phase after the customisation took - # place - next if candidate_phase.blank? - # Selects modifiable sections from the candidate_phase - modifiable_sections = candidate_phase.sections.select(&:modifiable) - - # Attaches modifiable sections into the customization_phase - modifiable_sections.each do |modifiable_section| - customization_phase.sections << modifiable_section - end - - # Sorts the sections for the customization_phase - sorted_sections = customization_phase.sections.sort_by(&:number) - - # Selects unmodifiable sections from the candidate_phase - unmodifiable_sections = candidate_phase.sections.reject(&:modifiable) - - unmodifiable_sections.each do |unmodifiable_section| - # Search for modifiable questions within the unmodifiable_section - # from candidate_phase - modifiable_questions = unmodifiable_section.questions.select(&:modifiable) - customization_section = sorted_sections.bsearch do |section| - unmodifiable_section.number <=> section.number - end - # The funder could have deleted the section - if customization_section.present? - modifiable_questions.each do |modifiable_question| - customization_section.questions << modifiable_question - end - end - # Search for unmodifiable questions within the unmodifiable_section in case - # source template added annotations - unmodifiable_questions = unmodifiable_section.questions.reject(&:modifiable) - sorted_questions = customization_section.questions.sort_by(&:number) - unmodifiable_questions.each do |unmodifiable_question| - customization_question = sorted_questions.bsearch do |question| - unmodifiable_question.number <=> question.number - end - # The funder could have deleted the question - if customization_question.present? - annotations_added_by_customiser = unmodifiable_question.annotations - .select do |annotation| - annotation.org_id == source.org_id - end - annotations_added_by_customiser.each do |annotation| - customization_question.annotations << annotation - end - end - end - end - end - # rubocop:enable Metrics/BlockLength - - # Appends the modifiable phases from source - source.phases.select(&:modifiable).each do |modifiable_phase| - customization.phases << modifiable_phase - end - customization + Template::UpgradeCustomizationService.call(self) end private diff --git a/app/services/template/upgrade_customization_service.rb b/app/services/template/upgrade_customization_service.rb new file mode 100644 index 0000000..ab96a2e --- /dev/null +++ b/app/services/template/upgrade_customization_service.rb @@ -0,0 +1,186 @@ +# frozen_string_literal: true + +class Template + + # Service object to upgrade a customization Template with new changes from the original + # funder Template. Remember: {target_template} is a customization of funder Template. + # + # - Duplicate the init template (Duplication called {#source_template}) + # + # - Create a new customisation of funder template (Customization called + # {#target_template}) + # + # - Take each phase on the {#target_template} and iterate to find if there's a + # corresponding one in {#source_template} + # - Test for each of a) no corresponding phase in source, + # b) corresponding found + # a) Create a duplicate phase on the source_template + # b) Do nothing at this point + # - Copy over each of the modifiable sections from source to the target + # - Re-number the sections if necessary to keep the display order (number) the same + # - Copy each of the questions and annotations exactly + # - For each unmodifiable section, copy over any modifiable questions from target + # + # - Copy each of the modifiable sections from the {#source_template} to the + # {#target_template} + # + class UpgradeCustomizationService + + # Exception raised when the Template is not a customization. + class NotACustomizationError < StandardError + end + + # Exception raised when no published funder Template can be found. + class NoFunderTemplateError < StandardError + end + + + ## + # The Template we're upgrading + # + # Returns {Template} + attr_reader :init_template + + # Initialize a new instance and run the script + # + # template - The Template we're upgrading + # + # Returns {Template} + def self.call(template) + new(template).call + end + + private_class_method :new + + # Initialize a new record + # + # template - The Template we're upgrading. Sets the value for {#init_template} + # + def initialize(template) + @init_template = template + end + + # Run the script + # + # Returns {Template} + def call + if init_template.customization_of.blank? + raise NotACustomizationError, + _("upgrade_customization! requires a customised template") + end + if funder_template.nil? + # rubocop:disable Metrics/LineLength + raise NoFunderTemplateError, + _("upgrade cannot be carried out since there is no published template of its current funder") + # rubocop:enable Metrics/LineLength + end + + # Merges modifiable sections or questions from source into target_template object + target_template.phases.map do |target_phase| + # Search for the phase in the source template whose versionable_id matches the + # customization_phase + # + # a) If the Org's template ({#source_template}) has the Phase... + if source_phase = find_matching_record_in_collection( + record: target_phase, + collection: source_template.phases) + + # b) If the Org's template ({#source_template}) doesn't have this Phase. + # This is not a problem, since {#customization_template} should have this Phase + # copied over from {#template_phase}. + else + next + end + + copy_modifiable_sections_for_phase(source_phase, target_phase) + sort_sections_within_phase(target_phase) + end + + copy_custom_annotations_for_questions + + return target_template + end + + private + + # The funder Template for this {#template} + # + # Returns Template + def funder_template + @funder_template ||= Template.published(init_template.customization_of).first + end + + # A copy of the Template we're currently upgrading. Preserves modifiable flags from + # the self template copied + # + # + # Returns {Template} + def source_template + @source_template ||= init_template.deep_copy(attributes: { + version: init_template.version + 1, + published: false + }) + end + + # Creates a new customisation for the published template whose family_id {#template} + # is a customization of + # + # Returns {Template} + def target_template + @target_template ||= funder_template.deep_copy( + attributes: { + version: source_template.version, + published: source_template.published, + family_id: source_template.family_id, + customization_of: source_template.customization_of, + org: source_template.org, + visibility: Template.visibilities[:organisationally_visible], + is_default: false + }, modifiable: false, save: true + ) + end + + # Find an item within collection that has the same versionable_id as record + # + # record - The record we're searching for a match of + # collection - The collection of records we're searching in + # + # Returns Positionable + # + # Returns nil + def find_matching_record_in_collection(record:, collection:) + collection.detect { |item| item.versionable_id == record.versionable_id } + end + + # Attach modifiable sections into the customization_phase + # + # source_phase - A Phase to copy sections for. + # target_phase - A Phase to copy Sections to. + # + # Returns Array of Sections + def copy_modifiable_sections_for_phase(source_phase, target_phase) + source_phase.sections.select(&:modifiable?).each do |section| + target_phase.sections << section + end + end + + def copy_custom_annotations_for_questions + init_template.annotations.where(org: template_org).each do |custom_annotation| + target_question = target_template.questions.find_by( + versionable_id: custom_annotation.question.versionable_id + ) + target_question.annotations << custom_annotation + end + end + + def sort_sections_within_phase(phase) + phase.sections = SectionSorter.new(*phase.sections).sort! + end + + def template_org + init_template.org + end + + end + +end diff --git a/app/views/paginable/plans/_publicly_visible.html.erb b/app/views/paginable/plans/_publicly_visible.html.erb index 475f2a2..fb74566 100644 --- a/app/views/paginable/plans/_publicly_visible.html.erb +++ b/app/views/paginable/plans/_publicly_visible.html.erb @@ -17,7 +17,8 @@ <%= (plan.owner.nil? || plan.owner.org.nil? ? _('Not Applicable') : plan.owner.org.name) %> <%= (plan.owner.nil? ? _('Unknown') : plan.owner.name(false)) %> - <%= link_to _('PDF'), plan_export_path(plan, format: :pdf), class: "dmp_table_link", target: '_blank' %> + <%= link_to _('PDF'), plan_export_path(plan, format: :pdf), + class: "dmp_table_link", target: '_blank' %> <% end %> diff --git a/db/migrate/20180903104704_add_versionable_id_to_phases_sections_and_questions.rb b/db/migrate/20180903104704_add_versionable_id_to_phases_sections_and_questions.rb new file mode 100644 index 0000000..004321f --- /dev/null +++ b/db/migrate/20180903104704_add_versionable_id_to_phases_sections_and_questions.rb @@ -0,0 +1,13 @@ +class AddVersionableIdToPhasesSectionsAndQuestions < ActiveRecord::Migration + def change + add_column :phases, :versionable_id, :string, limit: 36 + add_column :sections, :versionable_id, :string, limit: 36 + add_column :questions, :versionable_id, :string, limit: 36 + add_column :annotations, :versionable_id, :string, limit: 36 + + add_index :phases, :versionable_id + add_index :sections, :versionable_id + add_index :questions, :versionable_id + add_index :annotations, :versionable_id + end +end diff --git a/db/schema.rb b/db/schema.rb index 974c823..0927f21 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20180901095920) do +ActiveRecord::Schema.define(version: 20180903131335) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -20,12 +20,14 @@ t.integer "question_id" t.integer "org_id" t.text "text" - t.integer "type", default: 0, null: false + t.integer "type", default: 0, null: false t.datetime "created_at" t.datetime "updated_at" + t.string "versionable_id", limit: 36 end add_index "annotations", ["question_id"], name: "index_annotations_on_question_id", using: :btree + add_index "annotations", ["versionable_id"], name: "index_annotations_on_versionable_id", using: :btree create_table "answers", force: :cascade do |t| t.text "text" @@ -181,9 +183,11 @@ t.datetime "created_at" t.datetime "updated_at" t.boolean "modifiable" + t.string "versionable_id", limit: 36 end add_index "phases", ["template_id"], name: "index_phases_on_template_id", using: :btree + add_index "phases", ["versionable_id"], name: "index_phases_on_versionable_id", using: :btree create_table "plans", force: :cascade do |t| t.string "title" @@ -248,11 +252,13 @@ t.datetime "created_at" t.datetime "updated_at" t.integer "question_format_id" - t.boolean "option_comment_display", default: true + t.boolean "option_comment_display", default: true t.boolean "modifiable" + t.string "versionable_id", limit: 36 end add_index "questions", ["section_id"], name: "index_questions_on_section_id", using: :btree + add_index "questions", ["versionable_id"], name: "index_questions_on_versionable_id", using: :btree create_table "questions_themes", id: false, force: :cascade do |t| t.integer "question_id", null: false @@ -288,9 +294,11 @@ t.datetime "updated_at" t.integer "phase_id" t.boolean "modifiable" + t.string "versionable_id", limit: 36 end add_index "sections", ["phase_id"], name: "index_sections_on_phase_id", using: :btree + add_index "sections", ["versionable_id"], name: "index_sections_on_versionable_id", using: :btree create_table "settings", force: :cascade do |t| t.string "var", null: false diff --git a/lib/data_cleanup/rules/section/fix_duplicate_number.rb b/lib/data_cleanup/rules/section/fix_duplicate_number.rb index aa58c85..12fc3be 100644 --- a/lib/data_cleanup/rules/section/fix_duplicate_number.rb +++ b/lib/data_cleanup/rules/section/fix_duplicate_number.rb @@ -19,62 +19,9 @@ phase_ids.each do |phase_id| log("Re-setting number order for Sections in Phase##{phase_id}") - phase = ::Phase.find(phase_id) - sections = phase.sections - - # When every section within a Phase is modifiable, or when none is... - if sections.all?(&:modifiable?) || sections.all?(&:template?) - update_homogenous_set(phase) - - # When some of the sections are modifiable then... - else - update_heterogenous_set(phase) - end - end - end - - private - - # Re-sort the Phase's Sections by number, then by the ID - def update_homogenous_set(phase) - ids = phase.sections.order("number ASC, id ASC").ids - ::Section.update_numbers!(*ids, parent: phase) - end - - def update_heterogenous_set(phase) - # Array of duplicate Section numbers within this Phase - numbers = phase.sections - .group(:number) - .count - .select { |number, count| count > 1 } - .collect(&:first) - - # If there are duplicates in the #1 position - if numbers.include?(1) - # There should only be, if any, one prefixed modifiable Section - prefix = phase.sections.modifiable.where(number: 1).limit(1).ids - - # In the off-chance that there is more than one prefix Section, stick them - # after the unmodifiable block - erratic = phase.sections.modifiable.where(number: 1).offset(1).ids - - # Collect the unmodifiable Section ids in the order the should be displayed - unmodifiable = phase.sections.not_modifiable.order('number, id').ids - - # Then any additional Sections that come after the main block... - modifiable = phase.sections.modifiable.order('number, id').ids - - # Create one Array with all of the ids in the correct order. - ids = prefix + erratic + unmodifiable + modifiable - ::Section.update_numbers!(*ids, parent: phase) - - else - # Sort ids based on the number order. If there are duplicates, then take the - # earliest first. Unmodifiable sections from the template should be grouped - # together before the additional, modifiable sections are appended afterwards. - ids = phase.sections.not_modifiable.order("number, id").ids - ids += phase.sections.modifiable.order("number, id").ids - ::Section.update_numbers!(*ids, parent: phase) + phase = ::Phase.find(phase_id) + sorted_sections = SectionSorter.new(*phase.sections).sort! + ::Section.update_numbers!(*sorted_sections.map(&:id), parent: phase) end end diff --git a/lib/tasks/migrate.rake b/lib/tasks/migrate.rake index 9430994..998250c 100644 --- a/lib/tasks/migrate.rake +++ b/lib/tasks/migrate.rake @@ -1,12 +1,125 @@ namespace :migrate do - desc "migrate to 1.0" + desc "Add verisoning_id to published Templates" + task :add_versioning_id_to_templates => :environment do + safe_require 'text' + safe_require 'progress_bar' + + template_count = Template.latest_version.where(customization_of: nil) + .includes(phases: { sections: { questions: :annotations }}) + .count + bar = ProgressBar.new(template_count) + + + # Remove attr_readonly restrictions form these models + Phase.attr_readonly.delete('versionable_id') + Section.attr_readonly.delete('versionable_id') + Question.attr_readonly.delete('versionable_id') + Annotation.attr_readonly.delete('versionable_id') + + + # Get each of the funder templates... + Template.latest_version.where(customization_of: nil) + .includes(phases: { sections: { questions: :annotations }}) + .each do |funder_template| + + bar.increment!(1) + + Rails.logger.info "Updating versionable_id for Template: #{funder_template.id}" + + funder_template.phases.each do |funder_phase| + Rails.logger.info "Updating versionable_id for Phase: #{funder_phase.id}" + funder_phase.update! versionable_id: SecureRandom.uuid + + Phase.joins(:template) + .where(templates: { customization_of: funder_template.family_id }) + .where(number: funder_phase.number).each do |phase| + + if fuzzy_match?(phase.title, funder_phase.title) + phase.update! versionable_id: funder_phase.versionable_id + end + end + + funder_phase.sections.each do |funder_section| + Rails.logger.info "Updating versionable_id for Section: #{funder_section.id}" + funder_section.update! versionable_id: SecureRandom.uuid + + Section.joins(:template).where(templates: { + customization_of: funder_template.family_id + }).each do |section| + + # Prefix the match text with the number. This will make it easier to match + # Sections where the number hasn't changed + text_a = "#{section.number} - #{section.description}" + text_b = "#{funder_section.number} - #{funder_section.description}" + if fuzzy_match?(text_a, text_b) + section.update! versionable_id: funder_section.versionable_id + end + end + + funder_section.questions.each do |funder_question| + Rails.logger.info "Updating versionable_id for Question: #{funder_question.id}" + + funder_question.update! versionable_id: SecureRandom.uuid + + Question.joins(:template).where(templates: { + customization_of: funder_template.family_id + }).each do |question| + + # Prefix the match text with the number. This will make it easier to match + # Questions where the number hasn't changed + text_a = "#{question.number} - #{question.text}" + text_b = "#{funder_question.number} - #{funder_question.text}" + + if fuzzy_match?(text_a, text_b) + question.update! versionable_id: funder_question.versionable_id + end + end + + funder_question.annotations.each do |funder_annotation| + Rails.logger.info "Updating versionable_id for Annotation: #{funder_annotation.id}" + + funder_annotation.update! versionable_id: SecureRandom.uuid + + Annotation.joins(:template).where(templates: { + customization_of: funder_template.family_id, + }).where(type: funder_annotation.type).each do |ann| + + if fuzzy_match?(ann.text, funder_annotation.text) + ann.update! versionable_id: funder_annotation.versionable_id + end + end + end + end + end + end + end + + # Add versionable_id to any customized Sections... + Section.joins(:template) + .includes(questions: :annotations) + .where(templates: { id: Template.latest_version.ids }) + .where(versionable_id: nil, modifiable: true).each do |section| + + section.update! versionable_id: SecureRandom.uuid + + section.questions.each do |question| + question.update! versionable_id: SecureRandom.uuid + question.annotations.each do |annotation| + annotation.update! versionable_id: SecureRandom.uuid + end + end + end + + end + + desc "migrate to 1.0" task prep_for_1_0: :environment do # Convert existing orgs.target_url to the orgs.links JSON arrays Rake::Task['migrate:org_target_url_to_links'].execute - end + end - desc "migrate to 0.4" + desc "migrate to 0.4" task to_04: :environment do # Default all plans.visibility to the value specified in application.rb Rake::Task['migrate:init_plan_visibility'].execute @@ -274,8 +387,8 @@ contact = contact.gsub(' , ', '').strip contact = contact[0..(contact.length - 2)] if contact.ends_with?(',') contact = nil if contact == ',' - - p.update_attributes(data_contact_email: email, data_contact_phone: phone, + + p.update_attributes(data_contact_email: email, data_contact_phone: phone, data_contact: contact) end end @@ -283,25 +396,25 @@ desc "Move old ORCID from users table to user_identifiers" task move_orcids: :environment do users = User.includes(:user_identifiers).where('users.orcid_id IS NOT NULL') - + # If we have users with orcid ids if users.length > 0 # If orcid isn't defined in the identifier_schemes table add it if IdentifierScheme.find_by(name: 'orcid').nil? - IdentifierScheme.create!(name: 'orcid', - description: 'ORCID', + IdentifierScheme.create!(name: 'orcid', + description: 'ORCID', active: true, logo_url: 'http://orcid.org/sites/default/files/images/orcid_16x16.png', user_landing_url: 'https://orcid.org') end - + scheme = IdentifierScheme.find_by(name: 'orcid') - + unless scheme.nil? users.each do |u| if u.orcid_id.gsub('orcid.org/', '').match(/^[\d-]+/) schemes = u.user_identifiers.collect{|i| i.identifier_scheme_id} - + unless schemes.include?(scheme.id) UserIdentifier.create(user: u, identifier_scheme: scheme, identifier: u.orcid_id.gsub('orcid.org/', '')) @@ -311,27 +424,27 @@ end end end - + desc "Move old Shibboleth Ids from users table to user_identifiers" task move_shibs: :environment do if Rails.configuration.shibboleth_enabled users = User.includes(:user_identifiers).where('users.shibboleth_id IS NOT NULL') - + # If we have users with orcid ids if users.length > 0 # If orcid isn't defined in the identifier_schemes table add it if IdentifierScheme.find_by(name: 'shibboleth').nil? - IdentifierScheme.create!(name: 'shibboleth', - description: "Your institution credentials", + IdentifierScheme.create!(name: 'shibboleth', + description: "Your institution credentials", active: true) end - + scheme = IdentifierScheme.find_by(name: 'shibboleth') - + unless scheme.nil? users.each do |u| schemes = u.user_identifiers.collect{|i| i.identifier_scheme_id} - + unless schemes.include?(scheme.id) # TODO: Add logic to move shib identifiers over # UserIdentifier.create(user: u, identifier_scheme: scheme, @@ -364,10 +477,28 @@ org.links = [{link: org.target_url, text: ''}] org.target_url = nil - # Running with validations off because Dragonfly will fail if it cannot find the + # Running with validations off because Dragonfly will fail if it cannot find the # org logos which live on the deployed instance org.save!(validate: false) end end end + + + private + + def fuzzy_match?(text_a, text_b, min = 3) + Text::Levenshtein.distance(text_a, text_b) <= min + end + + def safe_require(libname) + begin + require libname + rescue LoadError + puts "Please install the #{libname} gem locally and try again: + gem install #{libname}" + exit 1 + end + end + end diff --git a/spec/factories/annotations.rb b/spec/factories/annotations.rb index c698c89..37fce14 100644 --- a/spec/factories/annotations.rb +++ b/spec/factories/annotations.rb @@ -2,17 +2,19 @@ # # Table name: annotations # -# id :integer not null, primary key -# text :text -# type :integer default(0), not null -# created_at :datetime -# updated_at :datetime -# org_id :integer -# question_id :integer +# id :integer not null, primary key +# text :text +# type :integer default(0), not null +# created_at :datetime +# updated_at :datetime +# org_id :integer +# question_id :integer +# versionable_id :string(36) # # Indexes # -# index_annotations_on_question_id (question_id) +# index_annotations_on_question_id (question_id) +# index_annotations_on_versionable_id (versionable_id) # # Foreign Keys # diff --git a/spec/factories/phases.rb b/spec/factories/phases.rb index 345edc4..0a1dea7 100644 --- a/spec/factories/phases.rb +++ b/spec/factories/phases.rb @@ -2,18 +2,20 @@ # # Table name: phases # -# id :integer not null, primary key -# description :text -# modifiable :boolean -# number :integer -# title :string -# created_at :datetime -# updated_at :datetime -# template_id :integer +# id :integer not null, primary key +# description :text +# modifiable :boolean +# number :integer +# title :string +# created_at :datetime +# updated_at :datetime +# template_id :integer +# versionable_id :string(36) # # Indexes # -# index_phases_on_template_id (template_id) +# index_phases_on_template_id (template_id) +# index_phases_on_versionable_id (versionable_id) # # Foreign Keys # @@ -27,7 +29,6 @@ sequence(:number) template modifiable false - transient do sections 0 end diff --git a/spec/factories/questions.rb b/spec/factories/questions.rb index 6015297..da006e6 100644 --- a/spec/factories/questions.rb +++ b/spec/factories/questions.rb @@ -12,10 +12,12 @@ # updated_at :datetime # question_format_id :integer # section_id :integer +# versionable_id :string(36) # # Indexes # -# index_questions_on_section_id (section_id) +# index_questions_on_section_id (section_id) +# index_questions_on_versionable_id (versionable_id) # # Foreign Keys # diff --git a/spec/factories/sections.rb b/spec/factories/sections.rb index b7272ae..b56369e 100644 --- a/spec/factories/sections.rb +++ b/spec/factories/sections.rb @@ -2,18 +2,20 @@ # # Table name: sections # -# id :integer not null, primary key -# description :text -# modifiable :boolean -# number :integer -# title :string -# created_at :datetime -# updated_at :datetime -# phase_id :integer +# id :integer not null, primary key +# description :text +# modifiable :boolean +# number :integer +# title :string +# created_at :datetime +# updated_at :datetime +# phase_id :integer +# versionable_id :string(36) # # Indexes # -# index_sections_on_phase_id (phase_id) +# index_sections_on_phase_id (phase_id) +# index_sections_on_versionable_id (versionable_id) # # Foreign Keys # diff --git a/spec/models/annotation_spec.rb b/spec/models/annotation_spec.rb index 08d2671..a9e13fb 100644 --- a/spec/models/annotation_spec.rb +++ b/spec/models/annotation_spec.rb @@ -2,6 +2,8 @@ RSpec.describe Annotation, type: :model do + it_behaves_like "VersionableModel" + context "validations" do subject { build(:annotation) } diff --git a/spec/models/phase_spec.rb b/spec/models/phase_spec.rb index d93d19a..530e443 100644 --- a/spec/models/phase_spec.rb +++ b/spec/models/phase_spec.rb @@ -2,6 +2,8 @@ RSpec.describe Phase, type: :model do + it_behaves_like "VersionableModel" + context "validations" do it { is_expected.to validate_presence_of(:title) } @@ -9,9 +11,12 @@ it { is_expected.to validate_presence_of(:template) } - it { is_expected.to validate_uniqueness_of(:number) - .scoped_to(:template_id) - .with_message("must be unique") } + it "validates uniqueness of number" do + subject.versionable_id = SecureRandom.uuid + expect(subject).to validate_uniqueness_of(:number) + .scoped_to(:template_id) + .with_message("must be unique") + end it { is_expected.to allow_values(true, false).for(:modifiable) } diff --git a/spec/models/question_spec.rb b/spec/models/question_spec.rb index eefd514..8c5c92f 100644 --- a/spec/models/question_spec.rb +++ b/spec/models/question_spec.rb @@ -2,15 +2,20 @@ RSpec.describe Question, type: :model do + it_behaves_like "VersionableModel" + context "validations" do it { is_expected.to validate_presence_of(:text) } it { is_expected.to validate_presence_of(:number) } - it { is_expected.to validate_uniqueness_of(:number) + it "validates uniqueness of number" do + subject.versionable_id = SecureRandom.uuid + expect(subject).to validate_uniqueness_of(:number) .scoped_to(:section_id) - .with_message("must be unique") } + .with_message("must be unique") + end it { is_expected.to validate_presence_of(:section) } diff --git a/spec/models/section_sorter_spec.rb b/spec/models/section_sorter_spec.rb new file mode 100644 index 0000000..db88378 --- /dev/null +++ b/spec/models/section_sorter_spec.rb @@ -0,0 +1,147 @@ +require "spec_helper" + +RSpec.describe SectionSorter do + + class StubSection < Struct.new(:number, :modifiable, :id) + alias modifiable? modifiable + + def unmodifiable? + !modifiable? + end + + def has_number?(value) + number == value + end + + def has_id?(value) + id == value + end + + end + + describe "#sort!" do + + let!(:sections_array) do + [ + StubSection.new(1, true, 105), + StubSection.new(2, false, 108), + StubSection.new(3, false, 111), + StubSection.new(4, true, 19), + StubSection.new(5, false, 1009), + StubSection.new(6, true, 999), + ].shuffle + end + + subject { SectionSorter.new(*sections_array).sort! } + + it "returns an Array" do + expect(subject).to be_an_instance_of(Array) + end + + it "moves the prefix section to the front" do + expect(subject.first).to have_number(1) + end + + it "groups unmodifiable sections together" do + expect(subject[1..3].map(&:number)).to eql([2,3,5]) + end + + it "groups modifiable sections together, last" do + expect(subject[4..5].map(&:number)).to eql([4,6]) + end + + context "when duplicate prefix exists" do + + let!(:sections_array) do + [ + StubSection.new(1, true, 34), + StubSection.new(1, false, 12), + StubSection.new(2, false, 54), + StubSection.new(3, false, 199), + StubSection.new(4, true, 84), + StubSection.new(5, false, 129), + StubSection.new(6, true, 555), + ].shuffle + end + + it "moves the modifiable one to the front" do + expect(subject.first).to have_number(1) + expect(subject.first).to be_modifiable + end + + it "moves the unmodifiable one to the second position" do + expect(subject.second).to have_number(2) + expect(subject.second).to be_unmodifiable + end + + end + + context "when duplicate section exists" do + + let!(:sections_array) do + [ + StubSection.new(1, true, 34), + StubSection.new(2, false, 54), + StubSection.new(3, true, 199), + StubSection.new(3, true, 205), + StubSection.new(3, true, 84), + ].shuffle + end + + it "sorts the duplicates by id" do + expect(subject[2]).to have_id(84) + expect(subject[3]).to have_id(199) + expect(subject[4]).to have_id(205) + end + + end + + context "when all sections are modifiable" do + + let!(:sections_array) do + [ + StubSection.new(1, true, 105), + StubSection.new(2, true, 108), + StubSection.new(3, true, 111), + StubSection.new(4, true, 19), + StubSection.new(5, true, 1009), + StubSection.new(5, true, 999), + ].shuffle + end + + it "sorts all sections by number" do + expect(subject.map(&:number)).to eql([1,2,3,4,5,5]) + end + + it "sorts duplicates by id" do + expect(subject.last).to have_id(1009) + end + + end + + context "when all sections are unmodifiable" do + + let!(:sections_array) do + [ + StubSection.new(1, false, 105), + StubSection.new(2, false, 108), + StubSection.new(3, false, 111), + StubSection.new(4, false, 109), + StubSection.new(4, false, 10), + StubSection.new(5, false, 999), + ].shuffle + end + + it "sorts all sections by number" do + expect(subject.map(&:number)).to eql([1,2,3,4,4,5]) + end + + it "sorts duplicates by id" do + expect(subject[4]).to have_id(109) + end + + end + + end + +end diff --git a/spec/models/section_spec.rb b/spec/models/section_spec.rb index 03b2655..904c9ae 100644 --- a/spec/models/section_spec.rb +++ b/spec/models/section_spec.rb @@ -2,6 +2,8 @@ RSpec.describe Section, type: :model do + it_behaves_like "VersionableModel" + context "validations" do it { is_expected.to validate_presence_of(:title) } @@ -9,9 +11,12 @@ it { is_expected.to validate_presence_of(:phase) } - it { is_expected.to validate_uniqueness_of(:number) + it "validates uniqueness of number" do + subject.versionable_id = SecureRandom.uuid + expect(subject).to validate_uniqueness_of(:number) .scoped_to(:phase_id) - .with_message("must be unique") } + .with_message("must be unique") + end it { is_expected.to allow_values(true, false).for(:modifiable) } diff --git a/spec/services/template/upgrade_customization_service_spec.rb b/spec/services/template/upgrade_customization_service_spec.rb new file mode 100644 index 0000000..483d0b3 --- /dev/null +++ b/spec/services/template/upgrade_customization_service_spec.rb @@ -0,0 +1,220 @@ +require 'rails_helper' + +RSpec.describe "Template::UpgradeCustomizationService", type: :service do + + describe ".call" do + + let!(:funder_template) do + ft = create(:template, :published, :default, org: create(:org, :funder)) + phase = create(:phase, template: ft) + create_list(:section, 4, phase: phase).each do |section| + create_list(:question, 2, section: section).each do |question| + create_list(:annotation, 2, question: question) + end + end + ft + end + + let!(:template) { funder_template.customize!(create(:org, :funder)) } + + subject { Template::UpgradeCustomizationService.call(template) } + + context "when template is a customization of a published funder template" do + + it "returns a new Template" do + expect(subject).to be_an_instance_of(Template) + end + + it "returns a persisted Template" do + expect(subject).to be_persisted + end + + it "increments the version number" do + template.update!(version: 2) + expect(subject.version).to eql(3) + end + + it "returns a draft Template" do + expect(subject.published).to eql(false) + end + + it "sets the customization_of to the family_id" do + expect(subject.customization_of).to eql(template.customization_of) + expect(subject.customization_of).to eql(funder_template.family_id) + end + + it "sets the org to the template org" do + expect(subject.org).to eql(template.org) + end + + it "creates new phases for this Template" do + expect { subject }.to change { Phase.count }.by(1) + end + + it "creates new sections for this Template" do + expect { subject }.to change { Section.count }.by(4) + end + + it "creates new questions for this Template" do + expect { subject }.to change { Question.count }.by(8) + end + + it "creates new annotations for this Template" do + expect { subject }.to change { Annotation.count }.by(16) + end + + end + + context "when a new section is present in funder template" do + + let!(:org) { create(:org) } + + let!(:template) { funder_template.customize!(org) } + + before do + # Shuffle the sections + phase = funder_template.phases.first + Section.update_numbers!(*phase.sections.ids.shuffle, parent: phase) + end + + it "preserves the versionable_id" do + template.sections.each do |section| + matching_section = funder_template.sections.detect do |s| + s.description == section.description + end + expect(section.number).not_to eql(matching_section.number) + expect(section.versionable_id).to eql(matching_section.versionable_id) + expect(section.description).to eql(matching_section.description) + end + end + + end + + context "when a new question is present in funder template" do + + let!(:org) { create(:org) } + + let!(:template) { funder_template.customize!(org) } + + before do + funder_template.sections.first.questions << create(:question) + end + + it "copies the new question" do + expect(subject.questions).to have_exactly(9).items + end + + end + + context "when a new phase is present in funder template" do + + let!(:org) { create(:org) } + + let!(:template) { funder_template.customize!(org) } + + before do + funder_template.phases << create(:phase) + end + + it "copies the new phase" do + expect(subject.phases).to have_exactly(2).items + end + + end + + context "when a new section is present in funder template" do + + let!(:org) { create(:org) } + + let!(:template) { funder_template.customize!(org) } + + before do + funder_template.phases.first.sections << create(:section) + end + + it "copies the new sections" do + expect(subject.sections).to have_exactly(5).items + end + + end + + context "when a new question is present in funder template" do + + let!(:org) { create(:org) } + + let!(:template) { funder_template.customize!(org) } + + before do + funder_template.sections.first.questions << create(:question) + end + + it "copies the new question" do + expect(subject.questions).to have_exactly(9).items + end + + end + + context "when a new annotation is present in funder template" do + + let!(:org) { create(:org) } + + let!(:template) { funder_template.customize!(org) } + + before do + funder_template.questions.first.annotations << create(:annotation) + end + + it "copies the new annotation" do + expect(subject.annotations).to have_exactly(17).items + end + + end + + context "when a new annotation is present in Org template" do + + + let!(:org) { create(:org) } + + let!(:template) { funder_template.customize!(org) } + + before do + template.questions.first.annotations << create(:annotation, org: org) + @annotation = Annotation.last + end + + it "copies the new annotation" do + expect(subject.annotations).to have_exactly(17).items + annotation_vals = [@annotation.text, @annotation.versionable_id] + expected_vals = subject.annotations.pluck(:text, :versionable_id) + expect(expected_vals).to include(annotation_vals) + end + + end + + context "when template is not a customization of a published funder template" do + + let!(:template) { create(:template) } + + it "raises an exception" do + expect { + subject + }.to raise_error(Template::UpgradeCustomizationService::NotACustomizationError) + end + + end + + context "when no published funder template exists" do + + let!(:funder_template) { create(:template, :archived, org: create(:org, :funder)) } + + it "raises an exception" do + expect { + subject + }.to raise_error(Template::UpgradeCustomizationService::NoFunderTemplateError) + end + + end + + end + +end \ No newline at end of file diff --git a/spec/support/locales.rb b/spec/support/locales.rb index cfafdcd..83b4092 100644 --- a/spec/support/locales.rb +++ b/spec/support/locales.rb @@ -1,7 +1,12 @@ +# frozen_string_literal: true + require 'shoulda/matchers' +AVAILABLE_TEST_LOCALES = %w[ en en_GB fr de ] + RSpec.configure do |config| - config.before(:each, type: :feature, js: true) do - Rails.application.config.i18n.available_locales = ['en', 'en_GB', 'fr', 'de'] + config.before(:each, type: :feature) do + I18n.config.enforce_available_locales = false + I18n.config.available_locales = AVAILABLE_TEST_LOCALES end end diff --git a/spec/support/mixins/versionable_model_spec.rb b/spec/support/mixins/versionable_model_spec.rb new file mode 100644 index 0000000..563496f --- /dev/null +++ b/spec/support/mixins/versionable_model_spec.rb @@ -0,0 +1,36 @@ +require 'rails_helper' + +UUID_REGEX = /\A[\w\d]{8}(\-[\w\d]{4}){3}-[\w\d]{12}\Z/i + +shared_examples_for "VersionableModel" do + + + context "attributes" do + + it { is_expected.to have_readonly_attribute(:versionable_id) } + + end + + context "#versionable_id" do + + + before do + subject.valid? + end + + it "is set on validation" do + expect(subject.versionable_id).to be_present + end + + it "is set to a random UUID" do + expect(subject.versionable_id).to match(UUID_REGEX) + end + + it "doesn't change if already set" do + subject.versionable_id = SecureRandom.uuid + expect { subject.valid? }.not_to change { subject.versionable_id } + end + + end + +end