Tdd contributor uniqueness (#2010)

Co-authored-by: Samuel Oey <so@capinside.com>

app/controllers/onboarding_controller.rb

@@ -25,8 +25,6 @@ def show
   end
 
   def create
-    # FIXME: there is a inherent inconsistency possible here the overall @organization is taken from
-    # the onboarding paths (if scoped) whereas the context of the contributor is taken from the token itself.
     @contributor = Contributor.new(contributor_params.merge(json_web_token_attributes: { invalidated_jwt: jwt_param },
                                                             organization: @organization))
     @contributor.tag_list = tag_list_from_jwt
@@ -67,7 +65,7 @@ def contributor_exists?
     # for the presence of the `taken` error. This
     # is necessary as custom validators may perform
     # additional normalization.
-    contributor = Contributor.new(attr_name => attr_value)
+    contributor = Contributor.new(attr_name => attr_value, :organization => @organization)
     contributor.valid?
 
     contributor.errors.details[attr_name].pluck(:error).include?(:taken)

app/dashboards/contributor_dashboard.rb

@@ -68,7 +68,7 @@ class ContributorDashboard < Administrate::BaseDashboard
 
   COLLECTION_FILTERS = {
     email: ->(resources) { resources.where.not(email: nil) },
-    telegram: ->(resources) { resources.where.not(telegram_chat_id: nil) },
+    telegram: ->(resources) { resources.where.not(telegram_id: nil) },
     threema: ->(resources) { resources.where.not(threema_id: nil) },
     signal: ->(resources) { resources.where.not(signal_phone_number: nil) }
   }.freeze

app/models/contributor.rb

@@ -39,11 +39,11 @@ class Contributor < ApplicationRecord
   validates :whats_app_phone_number, phony_plausible: true
   validates :data_processing_consent, acceptance: true, unless: proc { |c| c.editor_guarantees_data_consent }
 
-  validates :email, uniqueness: { case_sensitive: false }, allow_nil: true, 'valid_email_2/email': true
-  validates :threema_id, uniqueness: { case_sensitive: false }, allow_blank: true
-  validates :telegram_id, uniqueness: true, allow_nil: true
-  validates :signal_phone_number, uniqueness: true, allow_nil: true
-  validates :whats_app_phone_number, uniqueness: true, allow_nil: true
+  validates :email, uniqueness: { case_sensitive: false, scope: :organization }, allow_nil: true, 'valid_email_2/email': true
+  validates :threema_id, uniqueness: { case_sensitive: false, scope: :organization }, allow_blank: true
+  validates :telegram_id, uniqueness: { scope: :organization }, allow_nil: true
+  validates :signal_phone_number, uniqueness: { scope: :organization }, allow_nil: true
+  validates :whats_app_phone_number, uniqueness: { scope: :organization }, allow_nil: true
 
   validates :avatar, blob: { content_type: ['image/png', 'image/jpg', 'image/jpeg'], size_range: 0..(5.megabytes) }
 

db/migrate/20240831211323_adjust_contributors_indexes.rb

@@ -0,0 +1,18 @@
+# frozen_string_literal: true
+
+class AdjustContributorsIndexes < ActiveRecord::Migration[6.1]
+  def change
+    remove_index :contributors, :email
+    remove_index :contributors, :signal_phone_number
+    remove_index :contributors, :telegram_chat_id
+    remove_index :contributors, :telegram_id
+    remove_index :contributors, :threema_id
+    remove_index :contributors, :whats_app_phone_number
+    add_index :contributors, %i[organization_id email], unique: true, name: 'idx_org_email'
+    add_index :contributors, %i[organization_id signal_phone_number], unique: true, name: 'idx_org_signal_phone_number'
+    add_index :contributors, %i[organization_id telegram_chat_id], unique: true, name: 'idx_org_telegram_chat_id'
+    add_index :contributors, %i[organization_id telegram_id], unique: true, name: 'idx_org_telegram_id'
+    add_index :contributors, %i[organization_id threema_id], unique: true, name: 'idx_org_threema_id'
+    add_index :contributors, %i[organization_id whats_app_phone_number], unique: true, name: 'idx_org_whats_app_phone_number'
+  end
+end

db/migrate/20240905105530_remove_telegram_chat_id_from_contributors.rb

@@ -0,0 +1,7 @@
+# frozen_string_literal: true
+
+class RemoveTelegramChatIdFromContributors < ActiveRecord::Migration[6.1]
+  def change
+    remove_column :contributors, :telegram_chat_id, :bigint
+  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: 2024_08_12_102032) do
+ActiveRecord::Schema.define(version: 2024_09_05_105530) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "pg_trgm"
@@ -92,7 +92,6 @@
 
   create_table "contributors", force: :cascade do |t|
     t.string "email"
-    t.bigint "telegram_chat_id"
     t.datetime "created_at", precision: 6, null: false
     t.datetime "updated_at", precision: 6, null: false
     t.string "username"
@@ -120,13 +119,12 @@
     t.datetime "unsubscribed_at"
     t.string "signal_uuid"
     t.string "signal_onboarding_token"
-    t.index ["email"], name: "index_contributors_on_email", unique: true
+    t.index ["organization_id", "email"], name: "idx_org_email", unique: true
+    t.index ["organization_id", "signal_phone_number"], name: "idx_org_signal_phone_number", unique: true
+    t.index ["organization_id", "telegram_id"], name: "idx_org_telegram_id", unique: true
+    t.index ["organization_id", "threema_id"], name: "idx_org_threema_id", unique: true
+    t.index ["organization_id", "whats_app_phone_number"], name: "idx_org_whats_app_phone_number", unique: true
     t.index ["organization_id"], name: "index_contributors_on_organization_id"
-    t.index ["signal_phone_number"], name: "index_contributors_on_signal_phone_number", unique: true
-    t.index ["telegram_chat_id"], name: "index_contributors_on_telegram_chat_id", unique: true
-    t.index ["telegram_id"], name: "index_contributors_on_telegram_id", unique: true
-    t.index ["threema_id"], name: "index_contributors_on_threema_id", unique: true
-    t.index ["whats_app_phone_number"], name: "index_contributors_on_whats_app_phone_number", unique: true
   end
 
   create_table "data_migrations", primary_key: "version", id: :string, force: :cascade do |t|

spec/models/contributor_spec.rb

@@ -3,6 +3,22 @@
 require 'rails_helper'
 
 RSpec.describe Contributor, type: :model do
+  shared_examples 'unique within an organization' do
+    before { create(:contributor, organization: organization, **attrs) }
+
+    it 'raises errors when not unique' do
+      expect { create(:contributor, organization: organization, **attrs) }.to raise_error(ActiveRecord::RecordInvalid)
+      expect do
+        build(:contributor, organization: organization, **attrs).save!(validate: false)
+      end.to raise_error(ActiveRecord::RecordNotUnique)
+    end
+
+    it 'accepts duplicates for other organizations' do
+      expect { create(:contributor, **attrs) }.not_to raise_error
+      expect { build(:contributor, **attrs).save!(validate: false) }.not_to raise_error
+    end
+  end
+
   let(:the_request) do
     create(:request, title: 'Hitchhiker’s Guide', text: 'What is the answer to life, the universe, and everything?',
                      organization: organization, user: user)
@@ -47,10 +63,8 @@
   end
 
   describe '#email' do
-    it 'must be unique' do
-      create(:contributor, email: 'contributor@example.org')
-      expect { create(:contributor, email: 'contributor@example.org') }.to raise_error(ActiveRecord::RecordInvalid)
-      expect { create(:contributor, email: 'CONTRIBUTOR@example.org') }.to raise_error(ActiveRecord::RecordInvalid)
+    it_behaves_like 'unique within an organization' do
+      let(:attrs) { { email: 'contributor@example.org' } }
     end
 
     describe 'two contributor accounts without email' do
@@ -107,24 +121,24 @@
       expect(build(:contributor, signal_phone_number: nil)).to be_valid
     end
 
-    it 'must be unique' do
-      create(:contributor, signal_phone_number: '+491511234567')
-      contributor = build(:contributor, signal_phone_number: '+491511234567')
-      expect(contributor).not_to be_valid
-      expect { contributor.save!(validate: false) }.to raise_error(ActiveRecord::RecordNotUnique)
+    it_behaves_like 'unique within an organization' do
+      let(:attrs) { { signal_phone_number: '+491511234567' } }
     end
 
     it 'must be a valid phone number' do
       expect(build(:contributor, signal_phone_number: 'A+49151123456789')).not_to be_valid
     end
   end
 
+  describe '#whats_app_phone_number' do
+    it_behaves_like 'unique within an organization' do
+      let(:attrs) { { whats_app_phone_number: '+491511234567' } }
+    end
+  end
+
   describe '#telegram_id' do
-    it 'must be unique' do
-      create(:contributor, telegram_id: 1)
-      contributor = build(:contributor, telegram_id: 1)
-      expect(contributor).not_to be_valid
-      expect { contributor.save!(validate: false) }.to raise_error(ActiveRecord::RecordNotUnique)
+    it_behaves_like 'unique within an organization' do
+      let(:attrs) { { telegram_id: 1 } }
     end
   end
 
@@ -211,21 +225,18 @@
         end
       end
 
-      context 'given a vaild Threema ID' do
+      context 'given a valid Threema ID' do
         before do
           allow(threema_lookup_double).to receive(:key).and_return('PUBLIC_KEY_HEX_ENCODED')
         end
 
-        it 'must be unique' do
-          create(:contributor, threema_id: 'ABCD1234')
-          contributor = build(:contributor, threema_id: 'ABCD1234')
-          expect(contributor).not_to be_valid
-          expect { contributor.save!(validate: false) }.to raise_error(ActiveRecord::RecordNotUnique)
+        it_behaves_like 'unique within an organization' do
+          let(:attrs) { { threema_id: 'abcd1234' } }
         end
 
         it 'must be unique, ignoring case' do
-          create(:contributor, threema_id: 'abcd1234')
-          contributor = build(:contributor, threema_id: 'ABCD1234')
+          create(:contributor, threema_id: 'abcd1234', organization: organization)
+          contributor = build(:contributor, threema_id: 'ABCD1234', organization: organization)
           expect(contributor).not_to be_valid
         end
       end

spec/requests/onboarding/email_spec.rb

@@ -11,6 +11,8 @@
   let(:organization) { create(:organization, onboarding_allowed: onboarding_allowed, users_count: 1) }
   let!(:admin) { create_list(:user, 2, admin: true) }
   let(:onboarding_allowed) { { email: false } }
+  # onboarding should work also with another contributor with same email in another organization
+  let!(:other_contributor) { create(:contributor, email: 'zora@example.org') }
 
   describe 'GET /{organization_id}/onboarding/email' do
     subject { -> { get organization_onboarding_email_path(organization, jwt: jwt) } }
@@ -71,7 +73,7 @@
       it 'creates contributor' do
         expect { subject.call }.to change(Contributor, :count).by(1)
 
-        contributor = Contributor.first
+        contributor = Contributor.unscoped.last
         expect(contributor).to have_attributes(
           first_name: 'Zora',
           last_name: 'Zimmermann',
@@ -156,7 +158,7 @@
         it 'creates contributor without additional consent' do
           expect { subject.call }.to change(Contributor, :count).by(1)
 
-          contributor = Contributor.first
+          contributor = Contributor.unscoped.last
           expect(contributor).to have_attributes(
             first_name: 'Zora',
             last_name: 'Zimmermann',
@@ -167,8 +169,10 @@
         end
       end
 
-      describe 'given an existing email address' do
-        let!(:contributor) { create(:contributor, **attrs.merge(json_web_token: create(:json_web_token, invalidated_jwt: :jwt))) }
+      describe 'given an existing email address for an organization' do
+        let!(:contributor) do
+          create(:contributor, organization: organization, **attrs.merge(json_web_token: create(:json_web_token, invalidated_jwt: :jwt)))
+        end
 
         it 'redirects to success page' do
           subject.call

spec/requests/onboarding/threema_spec.rb

@@ -16,10 +16,13 @@
   let(:params) { { jwt: jwt } }
   let(:threema) { instance_double(Threema) }
   let(:threema_lookup_double) { instance_double(Threema::Lookup) }
+
   before do
     allow(Threema).to receive(:new).and_return(threema)
     allow(Threema::Lookup).to receive(:new).with({ threema: threema }).and_return(threema_lookup_double)
     allow(threema_lookup_double).to receive(:key).and_return('PUBLIC_KEY_HEX_ENCODED')
+    # onboarding works with an existing contributor with the same threema id in another organization
+    create(:contributor, threema_id: 'ABCD1234')
   end
 
   describe 'GET /{organization}/onboarding/threema' do
@@ -95,7 +98,7 @@
       it 'creates contributor' do
         expect { subject.call }.to change(Contributor, :count).by(1)
 
-        contributor = Contributor.first
+        contributor = Contributor.unscoped.last
         expect(contributor).to have_attributes(
           first_name: 'Zora',
           last_name: 'Zimmermann',
@@ -149,8 +152,8 @@
         end
       end
 
-      describe 'given an existing threema ID' do
-        let!(:contributor) { create(:contributor, **attrs, organization: organization) }
+      describe 'given an existing threema ID for an organization' do
+        let!(:contributor) { create(:contributor, organization: organization, **attrs) }
 
         it 'redirects to success page' do
           subject.call
@@ -196,7 +199,7 @@
         it 'creates contributor without additional consent' do
           expect { subject.call }.to change(Contributor, :count).by(1)
 
-          contributor = Contributor.first
+          contributor = Contributor.unscoped.last
           expect(contributor).to have_attributes(
             first_name: 'Zora',
             last_name: 'Zimmermann',

spec/requests/onboarding/whats_app_spec.rb

@@ -22,6 +22,8 @@
   let(:onboarding_allowed) { { whats_app: true } }
   let(:params) { { jwt: jwt } }
   let(:jwt) { JsonWebToken.encode({ invite_code: 'ONBOARDING_TOKEN', action: 'onboarding', organization_id: organization.id }) }
+  # onboarding should work also when a contributor in another organization has the same number
+  let!(:existing_contributor) { create(:contributor, whats_app_phone_number: '+491512454567') }
 
   describe 'GET /{organization_id}/onboarding/whatsapp' do
     subject { -> { get organization_onboarding_whats_app_path(organization), params: params } }
@@ -90,7 +92,7 @@
       {
         first_name: 'Zora',
         last_name: 'Zimmermann',
-        whats_app_phone_number: '01512454567',
+        whats_app_phone_number: '+491512454567',
         data_processing_consent: true
       }
     end
@@ -123,7 +125,7 @@
         it 'creates the contributor' do
           expect { subject.call }.to change(Contributor, :count).by(1)
 
-          contributor = Contributor.first
+          contributor = Contributor.unscoped.last
           expect(contributor).to have_attributes(
             first_name: 'Zora',
             last_name: 'Zimmermann',
@@ -143,7 +145,7 @@
 
           expect(WhatsAppAdapter::TwilioOutbound::Text).to have_been_enqueued.with(
             organization_id: organization.id,
-            contributor_id: Contributor.first.id,
+            contributor_id: Contributor.unscoped.last.id,
             text: welcome_message
           )
         end
@@ -183,7 +185,7 @@
         let(:welcome_message_payload) do
           {
             recipient_type: 'individual',
-            to: Contributor.first.whats_app_phone_number.split('+').last,
+            to: Contributor.unscoped.last.whats_app_phone_number.split('+').last,
             type: 'template',
             template: {
               namespace: organization.three_sixty_dialog_whats_app_template_namespace,
@@ -210,7 +212,7 @@
         it 'creates the contributor' do
           expect { subject.call }.to change(Contributor, :count).by(1)
 
-          contributor = Contributor.first
+          contributor = Contributor.unscoped.last
           expect(contributor).to have_attributes(
             first_name: 'Zora',
             last_name: 'Zimmermann',

spec/requests/telegram/webhook_spec.rb

@@ -116,13 +116,23 @@
 
         context 'and sends telegram_onboarding_token' do
           let(:message) { " \n  GHIJKL  \t " }
+
+          # onboarding should work also with another contributor with same telegram_id in another organization
+          before { create(:contributor, telegram_id: 12_345, organization: create(:organization)) }
+
           it { expect { subject.call }.to respond_with_message "<b>Welcome new contributor!</b>\n" }
           it { expect { subject.call }.to(change { contributor.reload.telegram_id }.from(nil).to(12_345)) }
 
           describe 'treats message case-insensitive' do
             let(:message) { " \n  GhIjKl  \t " }
-            it { expect { subject.call }.to respond_with_message "<b>Welcome new contributor!</b>\n" }
-            it { expect { subject.call }.to(change { contributor.reload.telegram_id }.from(nil).to(12_345)) }
+
+            it 'sends the welcome message' do
+              expect { subject.call }.to respond_with_message "<b>Welcome new contributor!</b>\n"
+            end
+
+            it "updates the contributor's telegram_id" do
+              expect { subject.call }.to(change { contributor.reload.telegram_id }.from(nil).to(12_345))
+            end
           end
 
           context 'even if other contributors are not connected yet' do