diff --git a/.database_consistency.yml b/.database_consistency.yml new file mode 100644 index 000000000..002f47783 --- /dev/null +++ b/.database_consistency.yml @@ -0,0 +1,9 @@ +# Invoice#human_id is populated deterministically by a before_create callback +# (Invoice#set_human_id) rather than being set directly, so it is never nil at +# INSERT time even though the column is NOT NULL. Adding a presence validator +# would run before that callback and would break specs built with +# FactoryBot's build_stubbed (which never runs on: :create callbacks). +Invoice: + human_id: + NullConstraintChecker: + enabled: false diff --git a/Gemfile b/Gemfile index 7453c7c6e..6429a6e22 100644 --- a/Gemfile +++ b/Gemfile @@ -51,7 +51,7 @@ group :development, :test do gem 'brakeman', '~> 8.0.1', require: false gem 'bullet', '~> 8.1' gem 'colorize', '~> 1.1.0' - gem 'database_consistency', '~> 2.1.1' + gem 'database_consistency', '~> 3.0.12' gem 'dotenv-rails', '~> 3.2.0' gem 'guard-livereload', '~> 2.5.2' gem 'guard-rspec', '~> 4.7.3', require: false diff --git a/Gemfile.lock b/Gemfile.lock index 934366c31..81e1a7cfe 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -119,14 +119,14 @@ GEM coderay (1.1.3) colorize (1.1.0) concurrent-ruby (1.3.8) - connection_pool (2.5.5) + connection_pool (3.0.2) crass (1.0.7) cssbundling-rails (1.4.3) railties (>= 6.0.0) dartsass-rails (0.5.1) railties (>= 6.0.0) sass-embedded (~> 1.63) - database_consistency (2.1.1) + database_consistency (3.0.12) activerecord (>= 3.2) date (3.5.1) devise (5.0.4) @@ -218,7 +218,7 @@ GEM domain_name (~> 0.5) http-form_data (2.3.0) http_parser.rb (0.8.0) - i18n (1.14.8) + i18n (1.15.2) concurrent-ruby (~> 1.0) io-console (0.8.2) irb (1.16.0) @@ -621,7 +621,7 @@ DEPENDENCIES colorize (~> 1.1.0) cssbundling-rails (~> 1.4, >= 1.4.3) dartsass-rails (~> 0.5.1) - database_consistency (~> 2.1.1) + database_consistency (~> 3.0.12) devise (~> 5.0.0) devise-i18n (~> 1.15.0) dotenv-rails (~> 3.2.0) diff --git a/app/jobs/payment_poll_job.rb b/app/jobs/payment_poll_job.rb index 7202644d9..1119874bd 100644 --- a/app/jobs/payment_poll_job.rb +++ b/app/jobs/payment_poll_job.rb @@ -3,6 +3,13 @@ class PaymentPollJob < ApplicationJob def perform Payment.not_completed.each do |payment| + if payment.mollie_id.nil? + # Mollie::Payment.create can fail to return an id (e.g. the API call errored after + # the record was created), leaving the payment stuck in `not_completed` forever + payment.update(status: 'failed') if payment.created_at < 15.minutes.ago + next + end + payment.update(status: payment.mollie_payment.status) rescue ActiveRecord::StaleObjectError # If the payment has not changed to paid with this concurrent update, diff --git a/app/models/activity.rb b/app/models/activity.rb index de04f2785..142419862 100644 --- a/app/models/activity.rb +++ b/app/models/activity.rb @@ -3,6 +3,7 @@ class Activity < ApplicationRecord has_many :orders, dependent: :destroy has_many :credit_mutations, dependent: :destroy + has_many :invoices, dependent: :destroy has_many :ordering_users, through: :orders, source: :user belongs_to :price_list belongs_to :created_by, class_name: 'User' diff --git a/app/models/invoice.rb b/app/models/invoice.rb index 2637d08b2..956fa84e4 100644 --- a/app/models/invoice.rb +++ b/app/models/invoice.rb @@ -6,8 +6,11 @@ class Invoice < ApplicationRecord belongs_to :user belongs_to :activity has_many :rows, class_name: 'InvoiceRow', dependent: :destroy + has_many :payments, dependent: :destroy accepts_nested_attributes_for :rows + validates :token, uniqueness: true + validate :activity_is_locked before_create :set_human_id diff --git a/app/models/order_row.rb b/app/models/order_row.rb index a330b0974..84c03789f 100644 --- a/app/models/order_row.rb +++ b/app/models/order_row.rb @@ -3,18 +3,20 @@ class OrderRow < ApplicationRecord belongs_to :product validates :product_count, presence: true, numericality: { only_integer: true, greater_than_or_equal_to: 0 } - validates :price_per_product, numericality: { greater_than: 0, allow_nil: true } + validates :price_per_product, presence: true, numericality: { greater_than: 0 } validates :product, inclusion: { in: :available_products } validate :no_changes_of_product_count_allowed validate :no_changes_of_price_per_product_allowed - before_create :copy_product_price + before_validation :copy_product_price, on: :create before_destroy -> { throw(:abort) } def copy_product_price - self.price_per_product = order.activity.price_list.product_price_for(product).price + return if order.blank? || product.blank? + + self.price_per_product = order.activity.price_list.product_price_for(product)&.price end def no_changes_of_product_count_allowed diff --git a/app/models/product.rb b/app/models/product.rb index adb3dca04..6d830a3cf 100644 --- a/app/models/product.rb +++ b/app/models/product.rb @@ -4,6 +4,7 @@ class Product < ApplicationRecord has_many :product_prices, dependent: :destroy has_many :price_lists, through: :product_prices, dependent: :restrict_with_error + has_many :order_rows, dependent: :destroy attribute :color, :string, default: '#f8f9fa' diff --git a/app/models/roles_users.rb b/app/models/roles_users.rb index 5015f75f4..88ef03350 100644 --- a/app/models/roles_users.rb +++ b/app/models/roles_users.rb @@ -2,5 +2,7 @@ class RolesUsers < ApplicationRecord belongs_to :user belongs_to :role + validates :user_id, uniqueness: { scope: %i[role_id created_at] } + delegate :name, to: :role end diff --git a/app/models/sofia_account.rb b/app/models/sofia_account.rb index ad49a725d..ba72f3339 100644 --- a/app/models/sofia_account.rb +++ b/app/models/sofia_account.rb @@ -3,8 +3,9 @@ class SofiaAccount < OmniAuth::Identity::Models::ActiveRecord belongs_to :user - validates :user, uniqueness: true # rubocop:disable Rails/UniqueValidationWithoutIndex + validates :user, uniqueness: true validates :username, presence: true, uniqueness: true + validates :password_digest, presence: true # the presence of :password is already checked by omniauth-sofia-account itself validates :password, length: { minimum: 12, maximum: 128 }, allow_nil: true diff --git a/app/models/user.rb b/app/models/user.rb index de744e3ea..8c5922566 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -4,6 +4,8 @@ class User < ApplicationRecord # rubocop:disable Metrics/ClassLength has_many :order_rows, through: :orders, dependent: :destroy has_many :credit_mutations, dependent: :destroy has_many :activities, dependent: :destroy, foreign_key: 'created_by_id', inverse_of: :created_by + has_many :payments, dependent: :destroy + has_many :invoices, dependent: :destroy has_many :roles_users, class_name: 'RolesUsers', dependent: :destroy has_many :roles, through: :roles_users @@ -13,6 +15,10 @@ class User < ApplicationRecord # rubocop:disable Metrics/ClassLength validate :no_deactivation_when_nonzero_credit validates :email, format: { with: Devise.email_regexp }, allow_blank: true validates :email, presence: true, if: ->(user) { !user.deactivated && user.sofia_account.present? } + validates :email, uniqueness: { + case_sensitive: false, + conditions: -> { where(deleted_at: nil).where.not(email: nil).where('provider IS DISTINCT FROM ?', 'amber_oauth2') } + }, if: ->(user) { user.email.present? && user.deleted_at.nil? && user.provider != 'amber_oauth2' } scope :in_amber, -> { where(provider: 'amber_oauth2') } scope :sofia_account, -> { where(provider: 'sofia_account') } diff --git a/bin/ci.sh b/bin/ci.sh index 26101cfb6..729a63c56 100755 --- a/bin/ci.sh +++ b/bin/ci.sh @@ -14,8 +14,7 @@ if [ "${TYPE}" = "lint" ] || [ "${TYPE}" = "" ]; then gem install bundler-audit bundle-audit update && bundle-audit check --ignore CVE-2015-9284 || true RAILS_ENV=test bundle exec rails db:create db:environment:set db:schema:load - # uncomment when it does not fail anymore :) - # bundle exec database_consistency + RAILS_ENV=test bundle exec database_consistency echo "--- :eslint: Yarn lint" yarn install # Why do I need to do this again? This was done in Dockerfile, rite? diff --git a/db/migrate/20260916095221_enforce_database_consistency_constraints.rb b/db/migrate/20260916095221_enforce_database_consistency_constraints.rb new file mode 100644 index 000000000..f6c0863c0 --- /dev/null +++ b/db/migrate/20260916095221_enforce_database_consistency_constraints.rb @@ -0,0 +1,80 @@ +class EnforceDatabaseConsistencyConstraints < ActiveRecord::Migration[7.2] + def change + remove_redundant_indexes + fix_sofia_accounts_unique_index + add_missing_foreign_keys + add_missing_not_null_constraints + add_missing_check_constraints + add_email_uniqueness_index + end + + private + + def remove_redundant_indexes + # Both are fully covered by the composite index index_roles_users_on_user_id_and_role_id_and_created_at + remove_index :roles_users, :user_id, name: 'index_roles_users_on_user_id' + # Both are fully covered by the composite index index_product_prices_on_product_id_and_price_list_id + remove_index :product_prices, :product_id, name: 'index_product_prices_on_product_id' + end + + def fix_sofia_accounts_unique_index + remove_index :sofia_accounts, :user_id, name: 'index_sofia_accounts_on_user_id' + add_index :sofia_accounts, :user_id, unique: true + end + + def add_missing_foreign_keys + add_foreign_key :roles_users, :users + add_foreign_key :roles_users, :roles + add_foreign_key :product_prices, :products + add_foreign_key :product_prices, :price_lists + add_foreign_key :payments, :users + add_foreign_key :payments, :invoices + add_foreign_key :order_rows, :orders + add_foreign_key :order_rows, :products + add_foreign_key :orders, :activities + add_foreign_key :orders, :users + add_foreign_key :invoice_rows, :invoices + add_foreign_key :invoices, :users + add_foreign_key :invoices, :activities + add_foreign_key :credit_mutations, :users + add_foreign_key :credit_mutations, :activities + add_foreign_key :activities, :price_lists + end + + def add_missing_not_null_constraints + change_column_null :roles, :role_type, false + + change_table :product_prices, bulk: true do |t| + t.change_null :product_id, false + t.change_null :price_list_id, false + t.change_null :price, false + end + + change_column_null :payments, :amount, false + change_column_null :orders, :created_by_id, false + change_column_null :invoice_rows, :invoice_id, false + change_column_null :credit_mutations, :created_by_id, false + + change_table :activities, bulk: true do |t| + t.change_null :price_list_id, false + t.change_null :created_by_id, false + end + + change_column_null :users, :name, false + end + + def add_missing_check_constraints + add_check_constraint :order_rows, 'product_count >= 0', name: 'order_rows_product_count_check' + add_check_constraint :order_rows, 'price_per_product > 0', name: 'order_rows_price_per_product_check' + add_check_constraint :credit_mutations, 'amount <= 5000', name: 'credit_mutations_amount_check' + end + + def add_email_uniqueness_index + # Only sofia_account and manually created users sign up with their own email address. + # Amber-synced accounts (provider: 'amber_oauth2') are managed externally and are excluded. + add_index :users, 'lower(email)', + name: 'index_users_on_lower_email_for_sofia_and_manual', + unique: true, + where: "deleted_at IS NULL AND email IS NOT NULL AND provider IS DISTINCT FROM 'amber_oauth2'" + end +end diff --git a/db/schema.rb b/db/schema.rb index 8e64bb1a8..b0790c167 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[7.2].define(version: 2025_12_12_000001) do +ActiveRecord::Schema[7.2].define(version: 2026_09_16_095221) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -21,8 +21,8 @@ t.datetime "deleted_at", precision: nil t.datetime "created_at", precision: nil, null: false t.datetime "updated_at", precision: nil, null: false - t.bigint "price_list_id" - t.bigint "created_by_id" + t.bigint "price_list_id", null: false + t.bigint "created_by_id", null: false t.bigint "locked_by_id" t.index ["created_by_id"], name: "index_activities_on_created_by_id" t.index ["locked_by_id"], name: "index_activities_on_locked_by_id" @@ -37,14 +37,15 @@ t.datetime "deleted_at", precision: nil t.datetime "created_at", precision: nil, null: false t.datetime "updated_at", precision: nil, null: false - t.bigint "created_by_id" + t.bigint "created_by_id", null: false t.index ["activity_id"], name: "index_credit_mutations_on_activity_id" t.index ["created_by_id"], name: "index_credit_mutations_on_created_by_id" t.index ["user_id"], name: "index_credit_mutations_on_user_id" + t.check_constraint "amount <= 5000::numeric", name: "credit_mutations_amount_check" end create_table "invoice_rows", force: :cascade do |t| - t.bigint "invoice_id" + t.bigint "invoice_id", null: false t.string "name", null: false t.integer "amount", null: false t.decimal "price", precision: 8, scale: 2, null: false @@ -78,6 +79,8 @@ t.datetime "deleted_at", precision: nil t.index ["order_id"], name: "index_order_rows_on_order_id" t.index ["product_id"], name: "index_order_rows_on_product_id" + t.check_constraint "price_per_product > 0::numeric", name: "order_rows_price_per_product_check" + t.check_constraint "product_count >= 0", name: "order_rows_product_count_check" end create_table "orders", force: :cascade do |t| @@ -86,7 +89,7 @@ t.datetime "deleted_at", precision: nil t.datetime "created_at", precision: nil, null: false t.datetime "updated_at", precision: nil, null: false - t.bigint "created_by_id" + t.bigint "created_by_id", null: false t.boolean "paid_with_cash", default: false, null: false t.boolean "paid_with_pin", default: false, null: false t.decimal "order_total", precision: 8, scale: 2 @@ -97,7 +100,7 @@ create_table "payments", force: :cascade do |t| t.string "mollie_id" - t.decimal "amount", precision: 8, scale: 2 + t.decimal "amount", precision: 8, scale: 2, null: false t.integer "status", default: 0, null: false t.bigint "user_id" t.datetime "deleted_at", precision: nil @@ -118,15 +121,14 @@ end create_table "product_prices", force: :cascade do |t| - t.bigint "product_id" - t.bigint "price_list_id" - t.decimal "price", precision: 8, scale: 2 + t.bigint "product_id", null: false + t.bigint "price_list_id", null: false + t.decimal "price", precision: 8, scale: 2, null: false t.datetime "deleted_at", precision: nil t.datetime "created_at", precision: nil, null: false t.datetime "updated_at", precision: nil, null: false t.index ["price_list_id"], name: "index_product_prices_on_price_list_id" t.index ["product_id", "price_list_id", "deleted_at"], name: "index_product_prices_on_product_id_and_price_list_id", unique: true - t.index ["product_id"], name: "index_product_prices_on_product_id" end create_table "products", force: :cascade do |t| @@ -143,7 +145,7 @@ t.datetime "deleted_at", precision: nil t.datetime "created_at", precision: nil, null: false t.datetime "updated_at", precision: nil, null: false - t.integer "role_type" + t.integer "role_type", null: false end create_table "roles_users", force: :cascade do |t| @@ -154,7 +156,6 @@ t.datetime "updated_at", precision: nil, null: false t.index ["role_id"], name: "index_roles_users_on_role_id" t.index ["user_id", "role_id", "created_at"], name: "index_roles_users_on_user_id_and_role_id_and_created_at", unique: true - t.index ["user_id"], name: "index_roles_users_on_user_id" end create_table "sofia_accounts", force: :cascade do |t| @@ -166,7 +167,7 @@ t.datetime "deleted_at" t.datetime "created_at", null: false t.datetime "updated_at", null: false - t.index ["user_id"], name: "index_sofia_accounts_on_user_id" + t.index ["user_id"], name: "index_sofia_accounts_on_user_id", unique: true t.index ["username"], name: "index_sofia_accounts_on_username", unique: true end @@ -174,7 +175,7 @@ t.datetime "deleted_at", precision: nil t.datetime "created_at", precision: nil, null: false t.datetime "updated_at", precision: nil, null: false - t.string "name" + t.string "name", null: false t.string "provider" t.string "uid" t.string "avatar_thumb_url" @@ -184,6 +185,7 @@ t.string "activation_token" t.datetime "activation_token_valid_till" t.string "sub_provider" + t.index "lower((email)::text)", name: "index_users_on_lower_email_for_sofia_and_manual", unique: true, where: "((deleted_at IS NULL) AND (email IS NOT NULL) AND ((provider)::text IS DISTINCT FROM 'amber_oauth2'::text))" t.index ["sub_provider"], name: "index_users_on_sub_provider" t.index ["uid"], name: "index_users_on_uid", unique: true end @@ -198,9 +200,25 @@ t.index ["item_type", "item_id"], name: "index_versions_on_item_type_and_item_id" end + add_foreign_key "activities", "price_lists" add_foreign_key "activities", "users", column: "created_by_id" add_foreign_key "activities", "users", column: "locked_by_id" + add_foreign_key "credit_mutations", "activities" + add_foreign_key "credit_mutations", "users" add_foreign_key "credit_mutations", "users", column: "created_by_id" + add_foreign_key "invoice_rows", "invoices" + add_foreign_key "invoices", "activities" + add_foreign_key "invoices", "users" + add_foreign_key "order_rows", "orders" + add_foreign_key "order_rows", "products" + add_foreign_key "orders", "activities" + add_foreign_key "orders", "users" add_foreign_key "orders", "users", column: "created_by_id" + add_foreign_key "payments", "invoices" + add_foreign_key "payments", "users" + add_foreign_key "product_prices", "price_lists" + add_foreign_key "product_prices", "products" + add_foreign_key "roles_users", "roles" + add_foreign_key "roles_users", "users" add_foreign_key "sofia_accounts", "users" end diff --git a/spec/models/sofia_account_spec.rb b/spec/models/sofia_account_spec.rb index 7b1c9a045..c2a18c84e 100644 --- a/spec/models/sofia_account_spec.rb +++ b/spec/models/sofia_account_spec.rb @@ -74,7 +74,7 @@ end context 'with user' do - subject(:duplicate_sofia_account) { build_stubbed(:sofia_account, user: sofia_account.user) } + subject(:duplicate_sofia_account) { build(:sofia_account, user: sofia_account.user) } it { expect(duplicate_sofia_account).not_to be_valid } end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 2714046b1..42d22abae 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -18,6 +18,30 @@ it { expect(user).not_to be_valid } end + context 'when email is already taken by another manual or sofia_account user' do + before { create(:user, :manual, email: 'duplicate@example.com') } + + subject(:user) { build(:user, :sofia_account, email: 'DUPLICATE@example.com') } + + it { expect(user).not_to be_valid } + end + + context 'when email is already taken but only by an amber user' do + before { create(:user, :from_amber, email: 'duplicate@example.com') } + + subject(:user) { build(:user, :manual, email: 'duplicate@example.com') } + + it { expect(user).to be_valid } + end + + context 'when email is already taken by an amber user with the same email' do + before { create(:user, :from_amber, email: 'duplicate@example.com') } + + subject(:user) { build(:user, :from_amber, email: 'duplicate@example.com') } + + it { expect(user).to be_valid } + end + context 'when deactivating with credit' do subject(:user) { create(:user, deactivated: true) }