Move query from variant_overrides_controller to its model scope #2818
Conversation
| @@ -3,8 +3,6 @@ class VariantOverride < ActiveRecord::Base | |||
|
|
|||
| acts_as_taggable | |||
|
|
|||
| attr_accessor :import_date | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Did we mean attr_accessible instead, perhaps 🤔 ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Looks fine to delete 👍
| scope :distinct_import_dates, lambda { | ||
| select('DISTINCT variant_overrides.import_date'). | ||
| where('variant_overrides.import_date IS NOT NULL'). | ||
| order('import_date DESC') |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Apparently this also works only after Rails 4 :/ as well as .where.not() which could have been used in the line above
| @@ -21,6 +19,12 @@ class VariantOverride < ActiveRecord::Base | |||
| where(hub_id: hubs) | |||
| } | |||
|
|
|||
| scope :distinct_import_dates, lambda { | |||
| select('DISTINCT variant_overrides.import_date'). | |||
There was a problem hiding this comment.
A pity that we're still on Rails 3.2 otherwise we could make use of AR's distinct
|
neat @nikobozi 👏 ! Keep them coming! |
|
What screens could we possible break with this? Do we need some manual testing in staging? |
|
Definitely the inventory page. Perhaps you @RachL know about those import dates. I don't. |
|
"import dates" was something added by @Matt-Yorkley for product import but we are not using this info anymore. Not sure we will use it later either, the filter on import dates should have disappeared as it's not anymore in any place of the product import feature as we just redirect to product or inventory page after import. But as an admin I still see it on product page, so maybe it was not removed. Maybe it's safer to keep the product import info for now and we will see if we delete later on if we don't do anything with it? |
|
According to the code we show an import date filter in the inventory (variant overrides) page. |
|
I am not sure it's clear to everyone, I suggested deleting that import_date line of code because it's technically redundant (the line of code, not the db field). This would not impact on functionality at all. |
|
staged https://staging.openfoodfrance.org The inventory (variant overrides) page needs to be tested. |
|
@sauloperez @luisramos0 is there any issue attached? hard to test without understanding the problem... @RachL Luis said the inventory page (variant override) needs to be tested. Make sure you unhide columns to test all overrides possible. @Matt-Yorkley do you think we need to keep that "import date" at all? I am not sure there will be any use for it, it was useful when you first designed it to display what had been imported but UX was floady aspecially for inventory... Anyway here Rachel you know what to test. |
|
This didn't arise from an existing issue but from an improvement that @nikobozi contributed with which is worth adding @myriamboure . There's no issue. |
|
I've been a bit long to come back on this one, sorry. Ready to go! My testing notes: https://docs.google.com/document/d/1kQXbx_BJb373YBk3U60TzKyJe4owjgP1DRWwJC0eoHw/edit# |
|
Thanks again @nikobozi ! keep 'em coming! 🤘 |
What? Why?
This PR moves query code from
VariantOverrideControllerintoVariantOverridemodel as a new scope query, in order to extract extra logic from controller and also to reuse an existing scopefor_hubsWhat should we test?
The existing rspec tests should pass along with the new one
Release notes
Move query code from
VariantOverrideControllerinto its Model scope and add a specChangelog Category: Changed