Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix RedundantPresenceValidationOnBelongs on some files #12407

Merged
merged 1 commit into from
Apr 23, 2024
Merged

Fix RedundantPresenceValidationOnBelongs on some files #12407

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 1 addition & 6 deletions .rubocop_todo.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -660,15 +660,10 @@ Rails/RedundantActiveRecordAllMethod:
- 'app/models/spree/variant.rb'
- 'spec/system/admin/product_import_spec.rb'

# Offense count: 20
# Offense count: 14
# This cop supports unsafe autocorrection (--autocorrect-all).
Rails/RedundantPresenceValidationOnBelongsTo:
Exclude:
- 'app/models/enterprise_fee.rb'
- 'app/models/exchange.rb'
- 'app/models/inventory_item.rb'
- 'app/models/order_cycle.rb'
- 'app/models/spree/address.rb'
- 'app/models/spree/line_item.rb'
- 'app/models/spree/order.rb'
- 'app/models/spree/product_property.rb'
Expand Down
1 change: 0 additions & 1 deletion app/models/enterprise_fee.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ class EnterpriseFee < ApplicationRecord

validates :fee_type, inclusion: { in: FEE_TYPES }
validates :name, presence: true
validates :enterprise_id, presence: true

before_save :ensure_valid_tax_category_settings

Expand Down
3 changes: 0 additions & 3 deletions app/models/exchange.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,6 @@
# shopfront (outgoing products). But the set of shown products can be smaller
# than all incoming products.
class Exchange < ApplicationRecord
self.belongs_to_required_by_default = false

acts_as_taggable

belongs_to :order_cycle
Expand All @@ -24,7 +22,6 @@ class Exchange < ApplicationRecord
has_many :exchange_fees, dependent: :destroy
has_many :enterprise_fees, through: :exchange_fees

validates :order_cycle, :sender, :receiver, presence: true
validates :sender_id, uniqueness: { scope: [:order_cycle_id, :receiver_id, :incoming] }

before_destroy :delete_related_exchange_variants, prepend: true
Expand Down
4 changes: 0 additions & 4 deletions app/models/inventory_item.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,10 @@
# frozen_string_literal: true

class InventoryItem < ApplicationRecord
self.belongs_to_required_by_default = false

belongs_to :enterprise
belongs_to :variant, class_name: "Spree::Variant"

validates :variant_id, uniqueness: { scope: :enterprise_id }
validates :enterprise, presence: true
validates :variant, presence: true
validates :visible,
inclusion: { in: [true, false], message: I18n.t(:inventory_item_visibility_error) }

Expand Down
4 changes: 1 addition & 3 deletions app/models/order_cycle.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@
require 'open_food_network/scope_variant_to_hub'

class OrderCycle < ApplicationRecord
self.belongs_to_required_by_default = false

searchable_attributes :orders_open_at, :orders_close_at, :coordinator_id
searchable_scopes :active, :inactive, :active_or_complete, :upcoming, :closed, :not_closed,
:dated, :undated, :soonest_opening, :soonest_closing, :most_recently_closed
Expand Down Expand Up @@ -44,7 +42,7 @@ class OrderCycle < ApplicationRecord
before_update :reset_processed_at, if: :will_save_change_to_orders_close_at?
after_save :sync_subscriptions, if: :opening?

validates :name, :coordinator_id, presence: true
validates :name, presence: true
validate :orders_close_at_after_orders_open_at?

preference :product_selection_from_coordinator_inventory_only, :boolean, default: false
Expand Down
6 changes: 2 additions & 4 deletions app/models/spree/address.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,19 +4,17 @@ module Spree
class Address < ApplicationRecord
include AddressDisplay

self.belongs_to_required_by_default = false

searchable_attributes :firstname, :lastname, :phone, :full_name, :full_name_reversed,
:full_name_with_comma, :full_name_with_comma_reversed
searchable_associations :country, :state

belongs_to :country, class_name: "Spree::Country"
belongs_to :state, class_name: "Spree::State"
belongs_to :state, class_name: "Spree::State", optional: true

has_one :enterprise, dependent: :restrict_with_exception
has_many :shipments

validates :address1, :city, :country, :phone, presence: true
validates :address1, :city, :phone, presence: true
validates :company, presence: true, unless: -> { first_name.blank? || last_name.blank? }
validates :firstname, :lastname, presence: true, if: -> do
company.blank? || company == 'unused'
Expand Down
7 changes: 7 additions & 0 deletions db/migrate/20240422140057_require_order_cycle_and_sender_and_receiver_on_exchange.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
class RequireOrderCycleAndSenderAndReceiverOnExchange < ActiveRecord::Migration[7.0]
def change
change_column_null :exchanges, :order_cycle_id, false
change_column_null :exchanges, :sender_id, false
change_column_null :exchanges, :receiver_id, false
end
end
6 changes: 6 additions & 0 deletions db/migrate/20240422145353_require_name_and_coordinator_on_order_cycle.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
class RequireNameAndCoordinatorOnOrderCycle < ActiveRecord::Migration[7.0]
def change
change_column_null :order_cycles, :name, false
change_column_null :order_cycles, :coordinator_id, false
end
end
8 changes: 8 additions & 0 deletions db/migrate/20240422150502_require_address1_and_city_and_phone_and_country_and_on_address.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
class RequireAddress1AndCityAndPhoneAndCountryAndOnAddress < ActiveRecord::Migration[7.0]
def change
change_column_null :spree_addresses, :address1, false
change_column_null :spree_addresses, :city, false
change_column_null :spree_addresses, :phone, false
Comment on lines +3 to +5
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't actually have to enforce these three. If they are null, the records are invalid in Rails but we have some database violations. I ran this:

sql='select count(*) from spree_addresses where address1 is null;'
ansible all_prod -u openfoodnetwork -a "psql -h localhost openfoodnetwork ofn_user -c '$sql'"

Most databases are okay, but some are not:

openfoodnetwork.org.au | CHANGED | rc=0 >>
 count 
-------
    66

csicsoka.org | CHANGED | rc=0 >>
 count 
-------
  1236

Same numbers for city.

And for phone we have the same in AU but two other results:

openfoodnetwork.org.uk | CHANGED | rc=0 >>
 count 
-------
     4

csicsoka.org | CHANGED | rc=0 >>
 count 
-------
     0

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmh, Can/Should I remove this migration file now that this is merged ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am looking at this, we are going to try fix the problematic data, but if it's too much of an issue we might revert some of the migrations

change_column_null :spree_addresses, :country_id, false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, we got a problem here, too:

csicsoka.org | CHANGED | rc=0 >>
 count 
-------
    21

This one should be quite easy to fix by setting the default country on this instance, manually in the database.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would also be good to check if these invalid addresses are actually used. Unfortunately, we have 13 references from other tables to addresses. We should probably script that.

end
end
20 changes: 10 additions & 10 deletions db/schema.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
#
# It's strongly recommended that you check this file into your version control system.

ActiveRecord::Schema[7.0].define(version: 2024_02_13_044159) do
ActiveRecord::Schema[7.0].define(version: 2024_04_22_150502) do
# These are extensions that must be enabled in order to support this database
enable_extension "pg_stat_statements"
enable_extension "plpgsql"
Expand Down Expand Up @@ -258,9 +258,9 @@
end

create_table "exchanges", id: :serial, force: :cascade do |t|
t.integer "order_cycle_id"
t.integer "sender_id"
t.integer "receiver_id"
t.integer "order_cycle_id", null: false
t.integer "sender_id", null: false
t.integer "receiver_id", null: false
t.text "pickup_time"
t.text "pickup_instructions"
t.datetime "created_at", precision: nil, null: false
Expand Down Expand Up @@ -330,10 +330,10 @@
end

create_table "order_cycles", id: :serial, force: :cascade do |t|
t.string "name", limit: 255
t.string "name", limit: 255, null: false
t.datetime "orders_open_at", precision: nil
t.datetime "orders_close_at", precision: nil
t.integer "coordinator_id"
t.integer "coordinator_id", null: false
t.datetime "created_at", precision: nil, null: false
t.datetime "updated_at", precision: nil, null: false
t.datetime "processed_at", precision: nil
Expand Down Expand Up @@ -420,15 +420,15 @@
create_table "spree_addresses", id: :serial, force: :cascade do |t|
t.string "firstname", limit: 255
t.string "lastname", limit: 255
t.string "address1", limit: 255
t.string "address1", limit: 255, null: false
t.string "address2", limit: 255
t.string "city", limit: 255
t.string "city", limit: 255, null: false
t.string "zipcode", limit: 255
t.string "phone", limit: 255
t.string "phone", limit: 255, null: false
t.string "state_name", limit: 255
t.string "alternative_phone", limit: 255
t.integer "state_id"
t.integer "country_id"
t.integer "country_id", null: false
t.datetime "created_at", precision: nil, null: false
t.datetime "updated_at", precision: nil, null: false
t.string "company", limit: 255
Expand Down
Loading