diff --git a/app/controllers/onboarding_controller.rb b/app/controllers/onboarding_controller.rb index fabbe0762..6e3ec9ee4 100644 --- a/app/controllers/onboarding_controller.rb +++ b/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) diff --git a/app/dashboards/contributor_dashboard.rb b/app/dashboards/contributor_dashboard.rb index 3320c913f..e0332606e 100644 --- a/app/dashboards/contributor_dashboard.rb +++ b/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 diff --git a/app/models/contributor.rb b/app/models/contributor.rb index 668525c17..f58e67e61 100644 --- a/app/models/contributor.rb +++ b/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) } diff --git a/db/migrate/20240831211323_adjust_contributors_indexes.rb b/db/migrate/20240831211323_adjust_contributors_indexes.rb new file mode 100644 index 000000000..2920b7f3f --- /dev/null +++ b/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 diff --git a/db/migrate/20240905105530_remove_telegram_chat_id_from_contributors.rb b/db/migrate/20240905105530_remove_telegram_chat_id_from_contributors.rb new file mode 100644 index 000000000..b4e2007de --- /dev/null +++ b/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 diff --git a/db/schema.rb b/db/schema.rb index 31e50d632..119275801 100644 --- a/db/schema.rb +++ b/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| diff --git a/spec/models/contributor_spec.rb b/spec/models/contributor_spec.rb index 8e4121433..4410b1182 100644 --- a/spec/models/contributor_spec.rb +++ b/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,11 +121,8 @@ 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 @@ -119,12 +130,15 @@ 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 diff --git a/spec/requests/onboarding/email_spec.rb b/spec/requests/onboarding/email_spec.rb index c6076de1a..9303c7e17 100644 --- a/spec/requests/onboarding/email_spec.rb +++ b/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 diff --git a/spec/requests/onboarding/threema_spec.rb b/spec/requests/onboarding/threema_spec.rb index 3e38bc783..7c731f4af 100644 --- a/spec/requests/onboarding/threema_spec.rb +++ b/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', diff --git a/spec/requests/onboarding/whats_app_spec.rb b/spec/requests/onboarding/whats_app_spec.rb index 4339d3c3e..311f2b26b 100644 --- a/spec/requests/onboarding/whats_app_spec.rb +++ b/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', diff --git a/spec/requests/telegram/webhook_spec.rb b/spec/requests/telegram/webhook_spec.rb index 7fe119f57..4b3854c30 100644 --- a/spec/requests/telegram/webhook_spec.rb +++ b/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 "Welcome new contributor!\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 "Welcome new contributor!\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 "Welcome new contributor!\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