[deploy] Add missing foreign keys to ahoy_messages and articles (#10321)

* Add FK between ahoy_messages and feedback_messages

* Add FKs between articles.second_user_id, articles.third_user_id and users

* Fix associations and specs

* Add more validations

* Remove FKs to articles.second_user_id and .third_user_id as they are about to be removed

app/models/article.rb

@@ -19,12 +19,13 @@ class Article < ApplicationRecord
   delegate :name, to: :user, prefix: true
   delegate :username, to: :user, prefix: true
 
-  belongs_to :user
-  belongs_to :organization, optional: true
   # touch: true was removed because when an article is updated, the associated collection
   # is touched along with all its articles(including this one). This causes eventually a deadlock.
   belongs_to :collection, optional: true
 
+  belongs_to :organization, optional: true
+  belongs_to :user
+
   counter_culture :user
   counter_culture :organization
 

app/models/email_message.rb

@@ -1,4 +1,6 @@
 class EmailMessage < Ahoy::Message
+  belongs_to :feedback_message, optional: true
+
   # So far this is mostly used to be compatible with administrate gem,
   # which doesn't seem to play nicely with namespaces. But there could be other
   # reasons to define behavior here, similar to how we use the Tag model.

app/models/feedback_message.rb

@@ -4,6 +4,8 @@ class FeedbackMessage < ApplicationRecord
   belongs_to :offender, class_name: "User", optional: true, inverse_of: :offender_feedback_messages
   belongs_to :reporter, class_name: "User", optional: true, inverse_of: :reporter_feedback_messages
   belongs_to :affected, class_name: "User", optional: true, inverse_of: :affected_feedback_messages
+
+  has_one :email_message, dependent: :nullify
   has_many :notes, as: :noteable, inverse_of: :noteable, dependent: :destroy
 
   validates :feedback_type, :message, presence: true

db/migrate/20200914143753_add_missing_foreign_keys_to_ahoy_messages.rb

@@ -0,0 +1,5 @@
+class AddMissingForeignKeysToAhoyMessages < ActiveRecord::Migration[6.0]
+  def change
+    add_foreign_key :ahoy_messages, :feedback_messages, on_delete: :nullify, validate: false
+  end
+end

db/migrate/20200914144033_validate_add_missing_foreign_keys_to_ahoy_messages.rb

@@ -0,0 +1,5 @@
+class ValidateAddMissingForeignKeysToAhoyMessages < ActiveRecord::Migration[6.0]
+  def change
+    validate_foreign_key :ahoy_messages, :feedback_messages
+  end
+end

db/migrate/20200914144157_add_missing_foreign_keys_to_articles.rb

@@ -0,0 +1,5 @@
+class AddMissingForeignKeysToArticles < ActiveRecord::Migration[6.0]
+  def change
+    add_foreign_key :articles, :collections, on_delete: :nullify, validate: false
+  end
+end

db/migrate/20200914145500_validate_add_missing_foreign_keys_to_articles.rb

@@ -0,0 +1,5 @@
+class ValidateAddMissingForeignKeysToArticles < ActiveRecord::Migration[6.0]
+  def change
+    validate_foreign_key :articles, :collections
+  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: 2020_09_11_140318) do
+ActiveRecord::Schema.define(version: 2020_09_14_145500) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "citext"
@@ -1379,9 +1379,11 @@ ActiveRecord::Schema.define(version: 2020_09_11_140318) do
 
   add_foreign_key "ahoy_events", "ahoy_visits", column: "visit_id", on_delete: :cascade
   add_foreign_key "ahoy_events", "users", on_delete: :cascade
+  add_foreign_key "ahoy_messages", "feedback_messages", on_delete: :nullify
   add_foreign_key "ahoy_messages", "users", on_delete: :cascade
   add_foreign_key "ahoy_visits", "users", on_delete: :cascade
   add_foreign_key "api_secrets", "users", on_delete: :cascade
+  add_foreign_key "articles", "collections", on_delete: :nullify
   add_foreign_key "articles", "organizations", on_delete: :nullify
   add_foreign_key "articles", "users", on_delete: :cascade
   add_foreign_key "audit_logs", "users"

spec/factories/email_messages.rb

@@ -0,0 +1,8 @@
+FactoryBot.define do
+  factory :email_message do
+    to      { Faker::Internet.email }
+    subject { Faker::Lorem.sentence }
+
+    content { Faker::Lorem.paragraph }
+  end
+end

spec/models/article_spec.rb

@@ -14,9 +14,9 @@ RSpec.describe Article, type: :model do
   it_behaves_like "UserSubscriptionSourceable"
 
   describe "validations" do
-    it { is_expected.to belong_to(:user) }
-    it { is_expected.to belong_to(:organization).optional }
     it { is_expected.to belong_to(:collection).optional }
+    it { is_expected.to belong_to(:organization).optional }
+    it { is_expected.to belong_to(:user) }
 
     it { is_expected.to have_many(:buffer_updates).dependent(:destroy) }
     it { is_expected.to have_many(:comments).dependent(:nullify) }

spec/models/email_message_spec.rb

@@ -0,0 +1,9 @@
+require "rails_helper"
+
+RSpec.describe EmailMessage, type: :model do
+  describe "validations" do
+    subject { create(:email_message) }
+
+    it { is_expected.to belong_to(:feedback_message).optional }
+  end
+end

spec/models/feedback_message_spec.rb

@@ -3,6 +3,8 @@ require "rails_helper"
 RSpec.describe FeedbackMessage, type: :model do
   subject(:feedback_message) { create(:feedback_message) }
 
+  it { is_expected.to have_one(:email_message).dependent(:nullify).optional }
+
   it { is_expected.to validate_presence_of(:feedback_type) }
   it { is_expected.to validate_presence_of(:message) }
   it { is_expected.to validate_length_of(:reported_url).is_at_most(250) }

spec/requests/admin/articles_spec.rb

@@ -9,21 +9,28 @@ RSpec.describe "/admin/articles", type: :request do
   context "when updating an Article via /admin/articles" do
     let(:super_admin) { create(:user, :super_admin) }
     let(:article) { create(:article) }
+    let(:second_user) { create(:user) }
+    let(:third_user) { create(:user) }
 
     before { sign_in super_admin }
 
     it "allows an Admin to add a co-author to an individual article" do
       get request
+
       expect do
-        article.update_columns(second_user_id: 1)
-      end.to change(article, :second_user_id).from(nil).to(1)
+        article.update_columns(second_user_id: second_user.id)
+      end.to change(article, :second_user_id).from(nil).to(second_user.id)
     end
 
-    it "allows an Admin to add co-authora to an individual article" do
-      article.update_columns(second_user_id: 2, third_user_id: 3)
+    it "allows an Admin to add multiple co-authors to an individual article" do
+      article.update_columns(second_user_id: second_user.id, third_user_id: third_user.id)
+
       get request
-      expect(article.second_user_id).to eq(2)
-      expect(article.third_user_id).to eq(3)
+
+      article.reload
+
+      expect(article.second_user_id).to eq(second_user.id)
+      expect(article.third_user_id).to eq(third_user.id)
     end
   end
 end

spec/services/users/delete_spec.rb

@@ -163,7 +163,7 @@ RSpec.describe Users::Delete, type: :service do
       described_class.call(user)
       aggregate_failures "associations should not exist" do
         user_associations.each do |user_association|
-          expect { user_association.reload }.to raise_error(ActiveRecord::RecordNotFound)
+          expect { user_association.reload }.to raise_error(ActiveRecord::RecordNotFound), user_association
         end
       end
     end