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.

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

[DFC Orders] List products to import on screen #13125

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
54 changes: 41 additions & 13 deletions app/controllers/admin/dfc_product_imports_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,32 +5,52 @@

module Admin
class DfcProductImportsController < Spree::Admin::BaseController
before_action :load_enterprise

# Define model class for `can?` permissions:
def model_class
self.class
end

def index
# The plan:
#
# * Fetch DFC catalog as JSON from URL.
enterprise = OpenFoodNetwork::Permissions.new(spree_current_user)
.managed_product_enterprises.is_primary_producer
.find(params.require(:enterprise_id))
# Fetch DFC catalog JSON for preview
api = DfcRequest.new(spree_current_user)
@catalog_url = params.require(:catalog_url)
@catalog_json = api.call(@catalog_url)
graph = DfcIo.import(@catalog_json)
catalog = DfcCatalog.new(graph)

catalog_url = params.require(:catalog_url)
catalog = DfcCatalog.load(spree_current_user, catalog_url)
# Render table and let user decide which ones to import.
@items = catalog.products.map do |subject|
[
subject,
@enterprise.supplied_variants.linked_to(subject.semanticId)&.product
]
end
rescue Faraday::Error,
Addressable::URI::InvalidURIError,
ActionController::ParameterMissing => e
flash[:error] = e.message
redirect_to admin_product_import_path
end

def import
ids = params.require(:semanticIds)

# Load DFC catalog JSON
graph = DfcIo.import(params.require(:catalog_json))
catalog = DfcCatalog.new(graph)
catalog.apply_wholesale_values!

# * First step: import all products for given enterprise.
# * Second step: render table and let user decide which ones to import.
imported = catalog.products.map do |subject|
existing_variant = enterprise.supplied_variants.linked_to(subject.semanticId)
# Import all selected products for given enterprise.
imported = catalog.products.select{ |subject| ids.include?(subject.semanticId) }
.map do |subject|
existing_variant = @enterprise.supplied_variants.linked_to(subject.semanticId)
dacook marked this conversation as resolved.
Show resolved Hide resolved

if existing_variant
SuppliedProductBuilder.update_product(subject, existing_variant)
else
SuppliedProductBuilder.store_product(subject, enterprise)
SuppliedProductBuilder.store_product(subject, @enterprise)
mkllnk marked this conversation as resolved.
Show resolved Hide resolved
end
end

Expand All @@ -41,5 +61,13 @@ def index
flash[:error] = e.message
redirect_to admin_product_import_path
end

private

def load_enterprise
@enterprise = OpenFoodNetwork::Permissions.new(spree_current_user)
.managed_product_enterprises.is_primary_producer
.find(params.require(:enterprise_id))
end
end
end
2 changes: 1 addition & 1 deletion app/models/spree/ability.rb
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,7 @@ def add_product_management_abilities(user)
can [:admin, :index, :guide, :import, :save, :save_data,
:validate_data, :reset_absent_products], ProductImport::ProductImporter

can [:admin, :index], ::Admin::DfcProductImportsController
can [:admin, :index, :import], ::Admin::DfcProductImportsController

# Reports page
can [:admin, :index, :show, :create], ::Admin::ReportsController
Expand Down
7 changes: 7 additions & 0 deletions app/views/admin/dfc_product_imports/import.html.haml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
- content_for :page_title do
#{t(".title")}

= render partial: 'spree/admin/shared/product_sub_menu'

%p= t(".imported_products")
= @count
28 changes: 26 additions & 2 deletions app/views/admin/dfc_product_imports/index.html.haml
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,29 @@

= render partial: 'spree/admin/shared/product_sub_menu'

%p= t(".imported_products")
= @count
%p= t('.catalog_url', count: @items.count, catalog_url: @catalog_url)
%p= t('.enterprise', enterprise_name: @enterprise.name)
%br

= form_with url: main_app.import_admin_dfc_product_imports_path do |form|
-# This is a very inefficient way of holding a json blob. Maybe base64 encode or store as a temporary file
dacook marked this conversation as resolved.
Show resolved Hide resolved
= form.hidden_field :enterprise_id, value: @enterprise.id
= form.hidden_field :catalog_json, value: @catalog_json

%table
%tbody
- @items.each do |supplied_product, existing_product|
%tr{id: supplied_product.semanticId }
%td
%label
= form.check_box 'semanticIds[]', { checked: true }, supplied_product.semanticId, ""
= supplied_product.name
%td
- if existing_product.present?
Update
= link_to(existing_product.id, edit_admin_product_path(existing_product))
- else
New
dacook marked this conversation as resolved.
Show resolved Hide resolved

%br
= form.submit t(".import")
12 changes: 6 additions & 6 deletions app/views/admin/product_import/_dfc_import_form.html.haml
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,15 @@
%br

= form_with url: main_app.admin_dfc_product_imports_path, method: :get do |form|
= form.label :enterprise_id, t(".enterprise")
%span.required *
= form.label :catalog_url, t(".catalog_url")
%br
= form.select :enterprise_id, options_from_collection_for_select(@producers, :id, :name, @producers.first&.id), { "data-controller": "tom-select", class: "primary" }
= form.text_field :catalog_url, size: 60
%br
%br
= form.label :catalog_url, t(".catalog_url")
= form.label :enterprise_id, t(".enterprise")
%span.required *
%br
= form.text_field :catalog_url, size: 60
= form.select :enterprise_id, options_from_collection_for_select(@producers, :id, :name, @producers.first&.id), { "data-controller": "tom-select", class: "primary" }
%br
%br
= form.submit t(".import")
= form.submit t(".preview")
9 changes: 7 additions & 2 deletions config/locales/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -847,7 +847,12 @@ en:

dfc_product_imports:
index:
title: "Importing a DFC product catalog"
title: "DFC product catalog"
catalog_url: "%{count} products to be imported from: %{catalog_url}"
enterprise: "Import to enterprise: %{enterprise_name}"
import: Import
import:
title: "DFC product catalog import"
imported_products: "Imported products:"
enterprise_fees:
index:
Expand Down Expand Up @@ -1036,7 +1041,7 @@ en:
title: "Import from DFC catalog"
enterprise: "Enterprise"
catalog_url: "DFC catalog URL"
import: "Import"
preview: Preview
import:
review: Review
import: Import
Expand Down
4 changes: 3 additions & 1 deletion config/routes/admin.rb
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,9 @@
post '/product_import/save_data', to: 'product_import#save_data', as: 'product_import_save_async'
post '/product_import/reset_absent', to: 'product_import#reset_absent_products', as: 'product_import_reset_async'

resources :dfc_product_imports, only: [:index]
resources :dfc_product_imports, only: [:index] do
post :import, on: :collection
end
Comment on lines +72 to +74
Copy link
Member

Choose a reason for hiding this comment

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

I see the temptation to call the action import but I would prefer to go with Rails defaults, convention over configuration. In this domain, I would say that we create a Product Import. Following that logic, the index action should probably be a new action. 🤔 But the form on the product import screen could also be seen the the new form.

What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't think a lot about it, but thought that Rails' resource/collection model didn't fit well in this case, so simply opted to copy the existing action name on the ProductImportController.
I suppose another way to look at it is an action on the products resource itself, ie: /admin/products/import or /admin/products/batch_create.

Ultimately I don't think it's worth thinking about anymore ;)
But it would be good to have a third opinion.

Copy link
Member Author

Choose a reason for hiding this comment

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

To further clarify my thinking, I didn't think there was much "cost" at all using a non-standard action name.
And I think to "create a Product Import" sounds too abstract, which makes it harder to see what it's for.

Copy link
Collaborator

Choose a reason for hiding this comment

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

When possible I prefer to stick with the defaults Rails action, but I think in this case David's solution is acceptable. After all, we can't really refer to Updating or Deleting a "Product Import", we don't have a tangible model for "Product Import".


constraints FeatureToggleConstraint.new(:admin_style_v3) do
# This might be easier to arrange once we rename the controller to plain old "products"
Expand Down
44 changes: 29 additions & 15 deletions spec/system/admin/dfc_product_import_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@
include AuthorizationHelper

let(:user) { create(:oidc_user, owned_enterprises: [enterprise]) }
let(:enterprise) { create(:supplier_enterprise) }
let(:source_product) { create(:product, supplier_id: enterprise.id) }
let(:enterprise) { create(:supplier_enterprise, name: "Saucy preserves") }
let(:source_product) { create(:product, name: "Sauce", supplier_id: enterprise.id) }
dacook marked this conversation as resolved.
Show resolved Hide resolved

before do
login_as user
Expand All @@ -20,58 +20,72 @@
it "imports from given catalog" do
visit admin_product_import_path

select enterprise.name, from: "Enterprise"

# We are testing against our own catalog for now but we want to replace
# this with the URL of another app when available.
host = Rails.application.default_url_options[:host]
url = "http://#{host}/api/dfc/enterprises/#{enterprise.id}/catalog_items"
fill_in "catalog_url", with: url
select enterprise.name, from: "Enterprise"
click_button "Preview"

expect(page).to have_content "Saucy preserves"
expect(page).to have_content "Sauce - 1g New"

# By feeding our own catalog to the import, we are effectively cloning the
# products. But the DFC product references the spree_product_id which
# make the importer create a variant for that product instead of creating
# a new independent product.
expect {
click_button "Import"
expect(page).to have_content "Imported products: 1"
}.to change {
source_product.variants.count
}.by(1)

expect(page).to have_content "Importing a DFC product catalog"
expect(page).to have_content "Imported products: 1"
end

it "imports from a FDC catalog", vcr: true do
user.update!(oidc_account: build(:testdfc_account))
# One product is existing in OFN
product_id =
"https://env-0105831.jcloud-ver-jpe.ik-server.com/api/dfc/Enterprises/test-hodmedod/SuppliedProducts/44519466467635"
linked_variant = source_product.variants.first
linked_variant.semantic_links << SemanticLink.new(semantic_id: product_id)

visit admin_product_import_path

select enterprise.name, from: "Enterprise"

url = "https://env-0105831.jcloud-ver-jpe.ik-server.com/api/dfc/Enterprises/test-hodmedod/SuppliedProducts"
fill_in "catalog_url", with: url
select enterprise.name, from: "Enterprise"
click_button "Preview"

expect(page).to have_content "4 products to be imported"
expect(page).to have_content "Saucy preserves"
expect(page).not_to have_content "Sauce - 1g" # Does not show other product
expect(page).to have_content "Beans - Retail can, 400g (can) Update" # existing product
expect(page).to have_content "Beans - Case, 12 x 400g (can) New"
expect(page).to have_content "Chia Seed, Organic - Retail pack, 300g"

uncheck "Chia Seed, Organic - Case, 8 x 300g" # don't import this one

expect {
click_button "Import"
expect(page).to have_content "Imported products: 3"
linked_variant.reload
}.to change { enterprise.supplied_products.count }
}.to change { enterprise.supplied_products.count }.by(2) # 1 updated, 2 new
.and change { linked_variant.display_name }
.and change { linked_variant.unit_value }
# 18.85 wholesale variant price divided by 12 cans in the slab.
.and change { linked_variant.price }.to(1.57)
.and change { linked_variant.on_demand }.to(true)
.and change { linked_variant.on_hand }.by(0)

expect(page).to have_content "Importing a DFC product catalog"

product = Spree::Product.last
expect(product.variants[0].semantic_links).to be_present
expect(product.image).to be_present

names = Spree::Product.pluck(:name)
expect(names).to include "Baked British Beans - Case, 12 x 400g (can)"
expect(names).not_to include "Chia Seed, Organic - Case, 8 x 300g"
end

it "fails gracefully" do
Expand All @@ -86,18 +100,18 @@
select enterprise.name, from: "Enterprise"
fill_in "catalog_url", with: url

expect { click_button "Import" }.not_to change { Spree::Variant.count }
expect { click_button "Preview" }.not_to change { Spree::Variant.count }

expect(page).to have_content "the server responded with status 401"

select enterprise.name, from: "Enterprise"
fill_in "catalog_url", with: "badurl"
click_button "Import"
click_button "Preview"
expect(page).to have_content "Absolute URI missing hierarchical segment: 'http://:80'"

select enterprise.name, from: "Enterprise"
fill_in "catalog_url", with: ""
click_button "Import"
click_button "Preview"
expect(page).to have_content "param is missing or the value is empty: catalog_url"
end
end
Loading