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

Move query from variant_overrides_controller to its model scope #2818

Merged
merged 2 commits into from
Oct 24, 2018
Merged

Move query from variant_overrides_controller to its model scope #2818

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3ae3817
Move query from variant_overrides_controller to its model scope
nikobozi Oct 4, 2018
f65d3c2
fix rubocop issue
nikobozi Oct 4, 2018
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
6 changes: 2 additions & 4 deletions app/controllers/admin/variant_overrides_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,10 +61,8 @@ def load_data

def inventory_import_dates
import_dates = VariantOverride.
select('DISTINCT variant_overrides.import_date').
where('variant_overrides.hub_id IN (?)
AND variant_overrides.import_date IS NOT NULL', editable_enterprises.collect(&:id)).
order('import_date DESC')
distinct_import_dates.
for_hubs(editable_enterprises.collect(&:id))

options = [{ id: '0', name: 'All' }]
import_dates.collect(&:import_date).map { |i| options.push(id: i.to_date, name: i.to_date.to_formatted_s(:long)) }
Expand Down
8 changes: 6 additions & 2 deletions app/models/variant_override.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@ class VariantOverride < ActiveRecord::Base

acts_as_taggable

attr_accessor :import_date
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was overriding the ActiveRecord getter/setter, which is why querying didn't work for me. When defining new accessor methods, shouldn't they have a different name than the database field?

Copy link
Contributor

Choose a reason for hiding this comment

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

Great question, I got the opportunity to improve my activerecord basics...
Because import_date is a database column in the variant_override table, this line is useless and I can be deleted because AR already does this for each db table field.
@Matt-Yorkley will confirm if there's any magical stuff going on here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Did we mean attr_accessible instead, perhaps 🤔 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I dont think so, attr_accessible is for mass-assignment. attr_accessor is what you get out of the box in active records. AR adds the getter and setter (like attr_accessor) for each db column.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks fine to delete 👍


belongs_to :hub, class_name: 'Enterprise'
belongs_to :variant, class_name: 'Spree::Variant'

Expand All @@ -21,6 +19,12 @@ class VariantOverride < ActiveRecord::Base
where(hub_id: hubs)
}

scope :distinct_import_dates, lambda {
select('DISTINCT variant_overrides.import_date').
Copy link
Contributor

Choose a reason for hiding this comment

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

A pity that we're still on Rails 3.2 otherwise we could make use of AR's distinct

where('variant_overrides.import_date IS NOT NULL').
order('import_date DESC')
Copy link
Contributor

Choose a reason for hiding this comment

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

just as a minor detail, could we rely on ActiveRecord's hash style? So it'd be like order(import_date: :desc). It decouples the scope from SQL a bit.

See the examples in https://apidock.com/rails/ActiveRecord/QueryMethods/order.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apparently this also works only after Rails 4 :/ as well as .where.not() which could have been used in the line above

Copy link
Contributor

Choose a reason for hiding this comment

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

Really? 😭

}

localize_number :price

def self.indexed(hub)
Expand Down
18 changes: 12 additions & 6 deletions spec/models/variant_override_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,9 @@
describe "scopes" do
let(:hub1) { create(:distributor_enterprise) }
let(:hub2) { create(:distributor_enterprise) }
let(:v) { create(:variant) }
let!(:vo1) { create(:variant_override, hub: hub1, variant: v) }
let!(:vo2) { create(:variant_override, hub: hub2, variant: v) }
let!(:vo3) { create(:variant_override, hub: hub1, variant: v, permission_revoked_at: Time.now) }
let!(:vo1) { create(:variant_override, hub: hub1, variant: variant, import_date: Time.zone.now.yesterday) }
let!(:vo2) { create(:variant_override, hub: hub2, variant: variant, import_date: Time.zone.now) }
let!(:vo3) { create(:variant_override, hub: hub1, variant: variant, permission_revoked_at: Time.now) }

it "ignores variant_overrides with revoked_permissions by default" do
expect(VariantOverride.all).to_not include vo3
Expand All @@ -21,10 +20,17 @@
VariantOverride.for_hubs([hub1, hub2]).should match_array [vo1, vo2]
end

it "fetches import dates for hubs in descending order" do
import_dates = VariantOverride.distinct_import_dates.pluck :import_date

expect(import_dates[0].to_i).to eq(vo2.import_date.to_i)
expect(import_dates[1].to_i).to eq(vo1.import_date.to_i)
end

describe "fetching variant overrides indexed by variant" do
it "gets indexed variant overrides for one hub" do
VariantOverride.indexed(hub1).should == {v => vo1}
VariantOverride.indexed(hub2).should == {v => vo2}
VariantOverride.indexed(hub1).should == {variant => vo1}
VariantOverride.indexed(hub2).should == {variant => vo2}
end
end
end
Expand Down