Merge pull request #370 from openstax/tags

Prevent tags from rarely being randomly deleted when editing an Exercise

app/models/license.rb

@@ -1,6 +1,6 @@
 class License < ApplicationRecord
 
-  has_many :publications, dependent: :destroy
+  has_many :publications
   has_many :class_licenses, dependent: :destroy
 
   has_many :combined_license_compatibilities,

app/models/publication.rb

@@ -12,7 +12,6 @@ class Publication < ApplicationRecord
                               dependent: :destroy,
                               inverse_of: :derived_publication
   has_many :derivations, foreign_key: :source_publication_id,
-                         dependent: :destroy,
                          inverse_of: :source_publication
 
   delegate :group_uuid, :number, :nickname, :nickname=,

app/models/publication_group.rb

@@ -2,11 +2,11 @@ class PublicationGroup < ApplicationRecord
 
   PUSLISHABLE_TYPES_WITH_DEFAULT_PUBLIC_SOLUTIONS = Set[ 'VocabTerm' ]
 
-  has_many :publications, dependent: :destroy, inverse_of: :publication_group, autosave: true
+  has_many :publications, inverse_of: :publication_group, autosave: true
 
   has_many :list_publication_groups, dependent: :destroy
 
-  has_many :vocab_distractors, dependent: :destroy
+  has_many :vocab_distractors
 
   validates :publishable_type, presence: true
   validates :uuid, presence: true, uniqueness: true

app/models/question.rb

@@ -10,7 +10,7 @@ class Question < ApplicationRecord
   sortable_has_many :answers, dependent: :destroy, inverse_of: :question, autosave: true
 
   has_many :collaborator_solutions, dependent: :destroy
-  has_many :community_solutions, dependent: :destroy
+  has_many :community_solutions
 
   sortable_has_many :hints, dependent: :destroy, inverse_of: :question
 

app/models/tag.rb

@@ -1,7 +1,7 @@
 class Tag < ApplicationRecord
 
-  has_many :exercise_tags, dependent: :destroy
-  has_many :vocab_term_tags, dependent: :destroy
+  has_many :exercise_tags
+  has_many :vocab_term_tags
 
   validates :name, presence: true, uniqueness: true, format: /\A[\w\.:#-]*\z/
 

app/models/user.rb

@@ -9,22 +9,19 @@ class User < ApplicationRecord
   has_many :groups_as_member, through: :account
   has_many :groups_as_owner, through: :account
 
-  has_one :administrator, dependent: :destroy
+  has_one :administrator
 
-  has_many :authors, dependent: :destroy
-  has_many :copyright_holders, dependent: :destroy
+  has_many :authors
+  has_many :copyright_holders
 
   has_many :delegations_as_delegator, class_name: 'Delegation',
                                       foreign_key: :delegator_id,
-                                      dependent: :destroy,
                                       inverse_of: :delegator
-  has_many :delegations_as_delegate, class_name: 'Delegation',
-                                     as: :delegate,
-                                     dependent: :destroy
+  has_many :delegations_as_delegate, class_name: 'Delegation', as: :delegate
 
-  has_many :sortings, dependent: :destroy
+  has_many :sortings
 
-  has_many :applications, as: :owner, class_name: 'Doorkeeper::Application', dependent: :destroy
+  has_many :applications, as: :owner, class_name: 'Doorkeeper::Application'
 
   validates :account, uniqueness: true
 

db/migrate/20220523181437_fix_foreign_keys.rb

@@ -0,0 +1,49 @@
+class FixForeignKeys < ActiveRecord::Migration[6.1]
+  def change
+    remove_foreign_key :exercise_tags, :exercises, on_update: :cascade, on_delete: :cascade
+    remove_foreign_key :exercise_tags, :tags, on_update: :cascade, on_delete: :cascade
+    remove_foreign_key :vocab_term_tags, :tags, on_update: :cascade, on_delete: :cascade
+    remove_foreign_key :vocab_term_tags, :vocab_terms, on_update: :cascade, on_delete: :cascade
+
+    add_foreign_key :administrators, :users
+    add_foreign_key :answers, :questions
+    add_foreign_key :authors, :publications
+    add_foreign_key :authors, :users
+    add_foreign_key :class_licenses, :licenses
+    add_foreign_key :collaborator_solutions, :questions
+    add_foreign_key :combo_choice_answers, :combo_choices
+    add_foreign_key :combo_choice_answers, :answers
+    add_foreign_key :combo_choices, :stems
+    add_foreign_key :community_solutions, :questions
+    add_foreign_key :copyright_holders, :publications
+    add_foreign_key :copyright_holders, :users
+    add_foreign_key :delegations, :users, column: :delegator_id
+    add_foreign_key :derivations, :publications, column: :derived_publication_id
+    add_foreign_key :derivations, :publications, column: :source_publication_id
+    add_foreign_key :exercise_tags, :exercises
+    add_foreign_key :exercise_tags, :tags
+    add_foreign_key :exercises, :vocab_terms
+    add_foreign_key :hints, :questions
+    add_foreign_key :license_compatibilities, :licenses, column: :combined_license_id
+    add_foreign_key :license_compatibilities, :licenses, column: :original_license_id
+    add_foreign_key :list_editors, :lists
+    add_foreign_key :list_nestings, :lists, column: :child_list_id
+    add_foreign_key :list_nestings, :lists, column: :parent_list_id
+    add_foreign_key :list_owners, :lists
+    add_foreign_key :list_readers, :lists
+    add_foreign_key :logic_variable_values, :logic_variables
+    add_foreign_key :logic_variables, :logics
+    add_foreign_key :publications, :licenses
+    add_foreign_key :publications, :publication_groups
+    add_foreign_key :question_dependencies, :questions, column: :dependent_question_id
+    add_foreign_key :question_dependencies, :questions, column: :parent_question_id
+    add_foreign_key :questions, :exercises
+    add_foreign_key :stem_answers, :answers
+    add_foreign_key :stem_answers, :stems
+    add_foreign_key :stems, :questions
+    add_foreign_key :users, :openstax_accounts_accounts, column: :account_id
+    add_foreign_key :vocab_distractors, :vocab_terms
+    add_foreign_key :vocab_term_tags, :tags
+    add_foreign_key :vocab_term_tags, :vocab_terms
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 2022_03_02_162344) do
+ActiveRecord::Schema.define(version: 2022_05_23_181437) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "citext"
@@ -565,12 +565,48 @@
 
   add_foreign_key "active_storage_attachments", "active_storage_blobs", column: "blob_id"
   add_foreign_key "active_storage_variant_records", "active_storage_blobs", column: "blob_id"
-  add_foreign_key "exercise_tags", "exercises", on_update: :cascade, on_delete: :cascade
-  add_foreign_key "exercise_tags", "tags", on_update: :cascade, on_delete: :cascade
+  add_foreign_key "administrators", "users"
+  add_foreign_key "answers", "questions"
+  add_foreign_key "authors", "publications"
+  add_foreign_key "authors", "users"
+  add_foreign_key "class_licenses", "licenses"
+  add_foreign_key "collaborator_solutions", "questions"
+  add_foreign_key "combo_choice_answers", "answers"
+  add_foreign_key "combo_choice_answers", "combo_choices"
+  add_foreign_key "combo_choices", "stems"
+  add_foreign_key "community_solutions", "questions"
+  add_foreign_key "copyright_holders", "publications"
+  add_foreign_key "copyright_holders", "users"
+  add_foreign_key "delegations", "users", column: "delegator_id"
+  add_foreign_key "derivations", "publications", column: "derived_publication_id"
+  add_foreign_key "derivations", "publications", column: "source_publication_id"
+  add_foreign_key "exercise_tags", "exercises"
+  add_foreign_key "exercise_tags", "tags"
+  add_foreign_key "exercises", "vocab_terms"
+  add_foreign_key "hints", "questions"
+  add_foreign_key "license_compatibilities", "licenses", column: "combined_license_id"
+  add_foreign_key "license_compatibilities", "licenses", column: "original_license_id"
+  add_foreign_key "list_editors", "lists"
+  add_foreign_key "list_nestings", "lists", column: "child_list_id"
+  add_foreign_key "list_nestings", "lists", column: "parent_list_id"
+  add_foreign_key "list_owners", "lists"
   add_foreign_key "list_publication_groups", "lists"
   add_foreign_key "list_publication_groups", "publication_groups"
+  add_foreign_key "list_readers", "lists"
+  add_foreign_key "logic_variable_values", "logic_variables"
+  add_foreign_key "logic_variables", "logics"
   add_foreign_key "oauth_access_grants", "oauth_applications", column: "application_id"
   add_foreign_key "oauth_access_tokens", "oauth_applications", column: "application_id"
-  add_foreign_key "vocab_term_tags", "tags", on_update: :cascade, on_delete: :cascade
-  add_foreign_key "vocab_term_tags", "vocab_terms", on_update: :cascade, on_delete: :cascade
+  add_foreign_key "publications", "licenses"
+  add_foreign_key "publications", "publication_groups"
+  add_foreign_key "question_dependencies", "questions", column: "dependent_question_id"
+  add_foreign_key "question_dependencies", "questions", column: "parent_question_id"
+  add_foreign_key "questions", "exercises"
+  add_foreign_key "stem_answers", "answers"
+  add_foreign_key "stem_answers", "stems"
+  add_foreign_key "stems", "questions"
+  add_foreign_key "users", "openstax_accounts_accounts", column: "account_id"
+  add_foreign_key "vocab_distractors", "vocab_terms"
+  add_foreign_key "vocab_term_tags", "tags"
+  add_foreign_key "vocab_term_tags", "vocab_terms"
 end

lib/ar_collection_setter.rb

@@ -1,19 +1,5 @@
 AR_COLLECTION_SETTER = ->(input:, binding:, **) do
-   getter = binding[:getter]
-   collection = getter.nil? ? send(binding.getter) : getter.evaluate(self)
-
-  # Hard-delete records that are being replaced
-  # Any further dependent records must be handled with foreign key constraints
-  if collection.is_a?(ActiveRecord::Associations::CollectionProxy)
-    if collection.respond_to?(:loaded?) && !collection.loaded?
-      collection.delete_all :delete_all
-    else
-      collection.group_by(&:class).each do |klass, items|
-        klass.where(id: items.map(&:id)).delete_all
-      end
-    end
-  end
-
-  # Don't use the collection= method (setter) so we can return meaningful errors
-  input.each { |record| collection << record }
+  getter = binding[:getter]
+  collection = getter.nil? ? send(binding.getter) : getter.evaluate(self)
+  collection.replace input
 end

lib/has_tags.rb

@@ -3,19 +3,24 @@ module ActiveRecord
     module Base
       def has_tags(association_name = nil, options = {})
         association_name ||= "#{name.tableize.singularize}_tags"
+        join_association = association_name.to_sym
         inverse_association_name = options[:inverse_of] || name.tableize.singularize
         tagging_class = association_name.to_s.classify.constantize
 
         class_exec do
-          has_many association_name.to_sym, { dependent: :destroy, autosave: true }.merge(options)
-          has_many :tags, through: association_name.to_sym do
+          has_many join_association, { dependent: :destroy, autosave: true }.merge(options)
+          has_many :tags, through: join_association do
             def <<(tag)
-              super(Tag.get(tag))
+              super Tag.get(tag)
+            end
+
+            def replace(other_array)
+              super Tag.get(other_array)
             end
           end
 
           def tags=(tags)
-            super(Tag.get(tags))
+            super Tag.get(tags)
           end
         end
       end

spec/models/license_spec.rb

@@ -4,7 +4,7 @@
 
   subject(:license) { FactoryBot.create(:license) }
 
-  it { is_expected.to have_many(:publications).dependent(:destroy) }
+  it { is_expected.to have_many(:publications) }
   it { is_expected.to have_many(:class_licenses).dependent(:destroy) }
 
   it { is_expected.to have_many(:combined_license_compatibilities).dependent(:destroy) }

spec/models/publication_group_spec.rb

@@ -4,7 +4,7 @@
 
   subject(:publication_group) { FactoryBot.create :publication_group }
 
-  it { is_expected.to have_many(:publications).dependent(:destroy) }
+  it { is_expected.to have_many(:publications) }
 
   it { is_expected.to validate_presence_of(:publishable_type) }
   it { is_expected.to validate_presence_of(:uuid) }

spec/models/publication_spec.rb

@@ -11,7 +11,7 @@
   it { is_expected.to have_many(:copyright_holders).dependent(:destroy) }
 
   it { is_expected.to have_many(:sources).dependent(:destroy) }
-  it { is_expected.to have_many(:derivations).dependent(:destroy) }
+  it { is_expected.to have_many(:derivations) }
 
   it { is_expected.to validate_uniqueness_of(:uuid).case_insensitive }
   it { is_expected.to validate_uniqueness_of(:version).scoped_to(:publication_group_id) }

spec/models/question_spec.rb

@@ -5,7 +5,7 @@
   it { is_expected.to have_many(:stems).dependent(:destroy) }
   it { is_expected.to have_many(:answers).dependent(:destroy) }
   it { is_expected.to have_many(:collaborator_solutions).dependent(:destroy) }
-  it { is_expected.to have_many(:community_solutions).dependent(:destroy) }
+  it { is_expected.to have_many(:community_solutions) }
 
   it { is_expected.to have_many(:hints).dependent(:destroy) }
 

spec/models/tag_spec.rb

@@ -4,7 +4,8 @@
 RSpec.describe Tag, type: :model do
   subject(:tag) { FactoryBot.create :tag }
 
-  it { is_expected.to have_many(:exercise_tags).dependent(:destroy) }
+  it { is_expected.to have_many(:exercise_tags) }
+  it { is_expected.to have_many(:vocab_term_tags) }
 
   it { is_expected.to validate_presence_of(:name) }
 

spec/models/user_spec.rb

@@ -5,15 +5,15 @@
 
   it { is_expected.to belong_to(:account) }
 
-  it { is_expected.to have_one(:administrator).dependent(:destroy) }
+  it { is_expected.to have_one(:administrator) }
 
-  it { is_expected.to have_many(:authors).dependent(:destroy) }
-  it { is_expected.to have_many(:copyright_holders).dependent(:destroy) }
+  it { is_expected.to have_many(:authors) }
+  it { is_expected.to have_many(:copyright_holders) }
 
-  it { is_expected.to have_many(:delegations_as_delegator).dependent(:destroy) }
-  it { is_expected.to have_many(:delegations_as_delegate).dependent(:destroy)  }
+  it { is_expected.to have_many(:delegations_as_delegator) }
+  it { is_expected.to have_many(:delegations_as_delegate)  }
 
-  it { is_expected.to have_many(:applications).dependent(:destroy) }
+  it { is_expected.to have_many(:applications) }
 
   it { is_expected.to validate_uniqueness_of(:account) }
 

spec/representers/api/v1/exercises/question_representer_spec.rb

@@ -55,8 +55,8 @@ module Api::V1::Exercises
 
       it 'can be written' do
         stems = [ Stem.new(content: "Don't you agree?") ]
-        expect(question.stems).to receive(:<<).with(kind_of(Stem)) do |stem|
-          expect(stem.content).to eq "Don't you agree?"
+        expect(question.stems).to receive(:replace).with([kind_of(Stem)]) do |stems|
+          expect(stems.first.content).to eq "Don't you agree?"
         end
 
         described_class.new(question).from_hash(
@@ -77,8 +77,8 @@ module Api::V1::Exercises
       end
 
       it 'can be written' do
-        expect(question.answers).to receive(:<<).with(kind_of(Answer)).exactly(3).times do |answer|
-          expect(answer.content).to be_in [ 'Yes', 'No', 'Maybe so' ]
+        expect(question.answers).to receive(:replace).with([kind_of(Answer)]*3) do |answers|
+          expect(answers.map(&:content)).to eq [ 'Yes', 'No', 'Maybe so' ]
         end
 
         described_class.new(question).from_hash(
@@ -102,10 +102,10 @@ module Api::V1::Exercises
 
       it 'can be written' do
         expect(question.collaborator_solutions).to(
-          receive(:<<).with(kind_of(CollaboratorSolution)) do |collaborator_solution|
-            expect(collaborator_solution.title).to eq 'Test'
-            expect(collaborator_solution.solution_type).to eq 'example'
-            expect(collaborator_solution.content).to eq 'This is a test.'
+          receive(:replace).with([kind_of(CollaboratorSolution)]) do |collaborator_solutions|
+            expect(collaborator_solutions.first.title).to eq 'Test'
+            expect(collaborator_solutions.first.solution_type).to eq 'example'
+            expect(collaborator_solutions.first.content).to eq 'This is a test.'
           end
         )
 
@@ -132,7 +132,7 @@ module Api::V1::Exercises
       end
 
       it 'cannot be written (attempts are silently ignored)' do
-        expect(question.community_solutions).not_to receive(:<<)
+        expect(question.community_solutions).not_to receive(:replace)
 
         described_class.new(question).from_hash(
           {
@@ -155,8 +155,8 @@ module Api::V1::Exercises
       end
 
       it 'can be written' do
-        expect(question.hints).to receive(:<<).with(kind_of(Hint)) do |hint|
-          expect(hint.content).to eq 'A hint'
+        expect(question.hints).to receive(:replace).with([kind_of(Hint)]) do |hints|
+          expect(hints.first.content).to eq 'A hint'
         end
 
         described_class.new(question).from_hash('hints' => [ 'A hint' ])
@@ -170,10 +170,10 @@ module Api::V1::Exercises
 
       it 'can be written' do
         expect(question.parent_dependencies).to(
-          receive(:<<).with(kind_of(QuestionDependency)) do |question_dependency|
-            expect(question_dependency.dependent_question).to eq question
-            expect(question_dependency.parent_question).to eq question
-            expect(question_dependency.is_optional).to eq true
+          receive(:replace).with([kind_of(QuestionDependency)]) do |question_dependencies|
+            expect(question_dependencies.first.dependent_question).to eq question
+            expect(question_dependencies.first.parent_question).to eq question
+            expect(question_dependencies.first.is_optional).to eq true
           end
         )
 

spec/representers/api/v1/exercises/representer_spec.rb

@@ -138,8 +138,8 @@ module Api::V1::Exercises
 
       it 'can be written' do
         expect(exercise.questions).to(
-          receive(:<<).with(kind_of(Question)).exactly(3).times do |question|
-            expect(question.stimulus).to be_in [ 'Question 1', 'Question 2', 'Question 3' ]
+          receive(:replace).with([kind_of(Question)]*3) do |questions|
+            expect(questions.map(&:stimulus)).to eq [ 'Question 1', 'Question 2', 'Question 3' ]
           end
         )