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

Prepare product_import_spec.rb for BUU as default #12668

Merged
merged 7 commits into from
Jul 18, 2024
Merged

Prepare product_import_spec.rb for BUU as default #12668

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
bdc42de
Sets import column test as pending
filipefurtad0 Jul 12, 2024
91daec4
Removes TODO, preparing spec for admin_style_3 (1)
filipefurtad0 Jul 12, 2024
e1976c6
Brings back xpath selector
filipefurtad0 Jul 12, 2024
46696df
Removes references to enabling admin_style_v3 features
filipefurtad0 Jul 15, 2024
24defac
Adds a migration to enable the feature by default
filipefurtad0 Jul 15, 2024
e0d7252
Adds condition for dev environment only
filipefurtad0 Jul 16, 2024
0123d6f
Copies admin_style_v3 feature into ACTIVE_BY_DEFAULT section
filipefurtad0 Jul 16, 2024
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: 7 additions & 0 deletions db/migrate/20240715103415_enable_admin_style_v3_by_default.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
class EnableAdminStyleV3ByDefault < ActiveRecord::Migration[7.0]
def up
if Rails.env.development?
Flipper.enable(:admin_style_v3)
end
end
end
2 changes: 1 addition & 1 deletion 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_07_10_013128) do
ActiveRecord::Schema[7.0].define(version: 2024_07_15_103415) do
# These are extensions that must be enabled in order to support this database
enable_extension "pg_stat_statements"
enable_extension "plpgsql"
Expand Down
3 changes: 3 additions & 0 deletions lib/open_food_network/feature_toggle.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,9 @@ module FeatureToggle
ACTIVE_BY_DEFAULT = {
# Copy features here that were activated in a migration so that new
# instances, development and test environments have the feature active.
"admin_style_v3" => <<~DESC,
Test the work-in-progress design updates.
DESC
}.freeze

def self.setup!
Expand Down
3 changes: 0 additions & 3 deletions spec/base_spec_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,9 +148,6 @@
config.before(:each) do
Flipper.features.each(&:remove)
OpenFoodNetwork::FeatureToggle.setup!

# activate feature toggle admin_style_v3 to use new admin interface and run the build
Flipper.enable(:admin_style_v3)
end

config.before(:each, :feature) do |example|
Expand Down
49 changes: 30 additions & 19 deletions spec/system/admin/product_import_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,8 @@
end

it "records a timestamp on import that can be viewed and filtered under Bulk Edit Products" do
pending "This feature was removed, see:
https://github.com/openfoodfoundation/openfoodnetwork/issues/10694#issuecomment-1578097339"
csv_data = <<~CSV
name, producer, category, on_hand, price, units, unit_type, shipping_category_id
Carrots, User Enterprise, Vegetables, 5, 3.20, 500, g, #{shipping_category_id_str}
Expand All @@ -209,15 +211,18 @@
potatoes = Spree::Product.find_by(name: 'Potatoes')
expect(potatoes.variants.first.import_date).to be_within(1.minute).of Time.zone.now

puts "TODO: migrate to v3"
Flipper.disable(:admin_style_v3) # disabling BUU for legacy products page
click_link 'Go To Products Page'

wait_until { page.find("#p_#{carrots.id}").present? }
# displays product list
expect(page).to have_field("_products_2_name", with: carrots.name.to_s)
expect(page).to have_field("_products_5_name", with: potatoes.name.to_s)

expect(page).to have_field "product_name", with: carrots.name
expect(page).to have_field "product_name", with: potatoes.name
toggle_columns "Import"
click_button "Save changes"

ofn_drop_down("Columns").click
within ofn_drop_down("Columns") do
check "Import"
end

within "tr#p_#{carrots.id} td.import_date" do
expect(page).to have_content Time.zone.now.year
Expand Down Expand Up @@ -675,14 +680,20 @@
expect(page).to have_selector '.created-count', text: '2'
expect(page).not_to have_selector '.updated-count'

puts "TODO: migrate to v3"
Flipper.disable(:admin_style_v3) # disabling BUU for legacy products page
default_variant_selector = "tr:has(input[aria-label=Name][value='Carrots'])"

visit spree.admin_products_path

within "#p_#{Spree::Product.find_by(name: 'Carrots').id}" do
expect(page).to have_input "product_name", with: "Carrots"
expect(page).to have_select "variant_unit_with_scale", selected: "Weight (lb)"
expect(page).to have_content "5" # on_hand
carrots = Spree::Product.find_by(name: 'Carrots')

within "#product_#{carrots.id}" do
Copy link
Member

Choose a reason for hiding this comment

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

Instead of asserting backend values, we can use ProductsHelper#row_containing_name to find the table row containing a field in column labelled "name" with the text:

Suggested change
within "#product_#{carrots.id}" do
within row_containing_name("Carrots") do

(You'll need to include ProductsHelper at the top of the file)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I can make it work with row_containing_name, as it finds the first row which contains the name "Carrots" (the product row - TR[1]):

[14] pry(#<RSpec::ExampleGroups::ProductImport::WhenImportingProductsFromUploadedFile>)> page.find(row_containing_name("Carrots"))
=> #<Capybara::Node::Element tag="tr" path="//HTML[1]/BODY[1]/DIV[1]/DIV[4]/DIV[1]/DIV[1]/DIV[1]/DIV[1]/TURBO-FRAME[1]/DIV[3]/DIV[1]/FORM[1]/TABLE[1]/TBODY[3]/TR[1]">

I think we're aiming to look at the second row (i.e., the first variant row - TR[2]). The current implementation looks at the whole section, without specifying the row:

[8] pry(#<RSpec::ExampleGroups::ProductImport::WhenImportingProductsFromUploadedFile>)> page.find("#product_#{carrots.id}")
=> #<Capybara::Node::Element tag="tbody" path="//HTML[1]/BODY[1]/DIV[1]/DIV[4]/DIV[1]/DIV[1]/DIV[1]/DIV[1]/TURBO-FRAME[1]/DIV[3]/DIV[1]/FORM[1]/TABLE[1]/TBODY[3]">

(You'll need to include ProductsHelper at the top of the file)

I've added it to the base_spec_helper so we don't need to include it; do you think it's better to have it loaded only when needed?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I can make it work with row_containing_name

Oh well, thanks for trying, sorry to have made more work for you.

I've added it to the base_spec_helper so we don't need to include it; do you think it's better to have it loaded only when needed?

Oh I see. As it's for only a specific part of the system, I personally would have only included it on the relevant tests (I guess I forgot that from a prior review). This is fine too :)

expect(page).to have_input("[products][2][variants_attributes][0][display_name]",
text: "Carrots")
expect(page).to have_input("[products][2][variants_attributes][][0][unit_to_display]",
text: "1 lb")
Comment on lines +690 to +693
Copy link
Member

Choose a reason for hiding this comment

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

Also here we can use the column labels to refer:

Suggested change
expect(page).to have_input("[products][2][variants_attributes][0][display_name]",
text: "Carrots")
expect(page).to have_input("[products][2][variants_attributes][][0][unit_to_display]",
text: "1 lb")
expect(page).to have_input("Name", text: "Carrots") # (This is now already asserted by row_containing_name above)
expect(page).to have_input("Unit", text: "1 lb")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't seem to make that work either... I get:

Failures:

  1) Product Import when importing products from uploaded file imports lines with all allowed units
     Failure/Error: expect(page).to have_input("Name", text: "Carrots") # (This is now already asserted by row_containing_name above)
       expected to find css "[name='Name']" within #<Capybara::Node::Element tag="tbody" path="//HTML[1]/BODY[1]/DIV[1]/DIV[4]/DIV[1]/DIV[1]/DIV[1]/DIV[1]/TURBO-FRAME[1]/DIV[3]/DIV[1]/FORM[1]/TABLE[1]/TBODY[3]"> but there were no matches

The element looks like this:

<input aria-label="Name" placeholder="Carrots" type="text" name="[products][2][variants_attributes][0][display_name]" id="_products_2_variants_attributes_0_display_name">

So it fails to find css with name='Name'. Hence I could make it work with name="[products][2][variants_attributes][0][display_name]"

Sorry if I'm missing something obvious - this looks simple, but has been quite a challenge.

Copy link
Member

Choose a reason for hiding this comment

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

I find that the biggest challenge is to write simple code ;)

And it's a balance deciding how much time to spend on it! I think that is fine as it is.

within(:xpath, '//*[@id="products-form"]/table/tbody[3]/tr[2]/td[7]') do
expect(page).to have_content("5")
end
end
end

Expand Down Expand Up @@ -716,15 +727,15 @@
expect(page).to have_content "Go To Products Page"
expect(page).to have_content "Upload Another File"

puts "TODO: migrate to v3"
Flipper.disable(:admin_style_v3) # disabling BUU for legacy products page
visit spree.admin_products_path

within "#p_#{Spree::Product.find_by(name: 'Cupcake').id}" do
expect(page).to have_input "product_name", with: "Cupcake"
expect(page).to have_select "variant_unit_with_scale", selected: "Items"
expect(page).to have_input "variant_unit_name", with: "Bunch"
expect(page).to have_content "5" # on_hand
expect(page).to have_input("[products][2][variants_attributes][0][display_name]",
text: "Cupcake")
expect(page).to have_select "_products_2_variant_unit_with_scale", selected: "Items"
expect(page).to have_input("[products][2][variant_unit_name]",
text: "Bunch")
within(:xpath, '//*[@id="products-form"]/table/tbody[3]/tr[2]/td[7]') do
expect(page).to have_content("5")
end
end

Expand Down
55 changes: 0 additions & 55 deletions spec/system/admin/products_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,61 +250,6 @@
end
end

describe "legacy products page (TODO: migrate/combine specs with v3 specs)" do
before { Flipper.disable(:admin_style_v3) }

describe "deleting" do
let!(:product1) {
create(:simple_product, name: 'a product to keep', supplier_id: supplier.id)
}

context 'a simple product' do
let!(:product2) {
create(:simple_product, name: 'a product to delete', supplier_id: supplier.id)
}

before do
login_as_admin
visit spree.admin_products_path

within "#p_#{product2.id}" do
accept_alert { page.find("[data-powertip=Remove]").click }
end
visit current_path
end

it 'removes it from the product list' do
expect(page).not_to have_selector "#p_#{product2.id}"
expect(page).to have_selector "#p_#{product1.id}"
end
end
end

describe 'cloning' do
let!(:product1) {
create(:simple_product, name: 'a weight product', supplier_id: supplier.id,
variant_unit: "weight")
}

context 'products' do
before do
login_as_admin
visit spree.admin_products_path
end

it 'creates a copy of the product' do
within "#p_#{product1.id}" do
page.find("[data-powertip=Clone]").click
end
visit current_path
within "#p_#{product1.id + 1}" do
expect(page).to have_input "product_name", with: 'COPY OF a weight product'
end
end
end
end
end

context "as an enterprise user" do
let!(:tax_category) { create(:tax_category) }
let(:filter) { { producerFilter: 2 } }
Expand Down
2 changes: 1 addition & 1 deletion spec/system/admin/products_v3/actions_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

require "system_helper"

RSpec.describe 'As an enterprise user, I can manage my products', feature: :admin_style_v3 do
RSpec.describe 'As an enterprise user, I can manage my products' do
include AdminHelper
include WebHelper
include AuthenticationHelper
Expand Down
2 changes: 1 addition & 1 deletion spec/system/admin/products_v3/create_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

require "system_helper"

RSpec.describe 'As an enterprise user, I can manage my products', feature: :admin_style_v3 do
RSpec.describe 'As an enterprise user, I can manage my products' do
include AuthenticationHelper
include WebHelper

Expand Down
15 changes: 10 additions & 5 deletions spec/system/admin/products_v3/index_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

require "system_helper"

RSpec.describe 'As an enterprise user, I can manage my products', feature: :admin_style_v3 do
RSpec.describe 'As an enterprise user, I can manage my products' do
include AdminHelper
include WebHelper
include AuthenticationHelper
Expand Down Expand Up @@ -76,10 +76,15 @@
end

it "displays an on hand count in a span for each product" do
expect(page).to have_content "On demand"
expect(page).not_to have_content "20" # does not display the total stock
expect(page).to have_content "16" # displays the stock for variant_2
expect(page).to have_content "4" # displays the stock for variant_3
within(:xpath, '//*[@id="products-form"]/table/tbody[1]/tr[2]/td[7]') do
expect(page).to have_content "On demand"
end
within(:xpath, '//*[@id="products-form"]/table/tbody[2]/tr[2]/td[7]') do
expect(page).to have_content "16" # displays the stock for variant_2
end
within(:xpath, '//*[@id="products-form"]/table/tbody[2]/tr[3]/td[7]') do
expect(page).to have_content "4" # displays the stock for variant_3
end
end
end

Expand Down
2 changes: 1 addition & 1 deletion spec/system/admin/products_v3/update_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

require "system_helper"

RSpec.describe 'As an enterprise user, I can update my products', feature: :admin_style_v3 do
RSpec.describe 'As an enterprise user, I can update my products' do
include AdminHelper
include WebHelper
include AuthenticationHelper
Expand Down
2 changes: 1 addition & 1 deletion spec/system/admin/subscriptions/smoke_tests_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

require 'system_helper'

RSpec.describe 'Subscriptions', feature: :admin_style_v3 do
RSpec.describe 'Subscriptions' do
include AdminHelper
include AuthenticationHelper
include WebHelper
Expand Down
Loading