Prepare product_import_spec.rb for BUU as default #12668

filipefurtad0 · 2024-07-12T19:14:04Z

What? Why?

Closes [BUU] Activate admin_style_v3 for dev, test and new servers #12627

Prepares ./spec/system/admin/product_import_spec.rb for BUU.
Addresses review comment here.
Make admin_style_v3 active by default for new environments
Migrate admin_style_v3 to be active on all dev environments

For reviewing: suggest viewing overall diff.

What should we test?

Feature admin_style_v3 should be activated by default in the:

dev environment (if non-existent)
test environment (build should be green)

Staging and production environments:

should not be affected by this PR, if the feature is activated in some way ⚠️
should affected by this PR, if the feature is fully disabled, and missing from the list of features in the Flipper settings screen ⚠️

Test instructions:

Take a staging server and delete the admin_style_v3 feature
Stage the PR
-> Verify that the feature is fully enabled

Take a staging server make sure the admin_style_v3 feature exists
Set the feature to be partially enabled (for example, for 25% of the users, or for a specific user)
Stage the PR
-> Verify that there are no changes on the admin_style_v3 feature settings (set on the second step)

Release notes

Changelog Category (reviewers may add a label for the release notes):

User facing changes
API changes (V0, V1, DFC or Webhook)
Technical changes only
Feature toggled

The title of the pull request will be included in the release notes.

Dependencies

Documentation updates

As of now, it is not clear whether we wish to re-implement this feature on BUU

Removes TODO, preparing spec for admin_style_3 (2) Uses xpath to point to on hand field

Fixes xpath

db/migrate/20240715103415_enable_admin_style_v3_by_default.rb

dacook · 2024-07-16T01:04:46Z

spec/system/admin/product_import_spec.rb

-        expect(page).to have_content "5" # on_hand
+      carrots = Spree::Product.find_by(name: 'Carrots')
+
+      within "#product_#{carrots.id}" do


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)

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?

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 :)

dacook · 2024-07-16T01:07:32Z

spec/system/admin/product_import_spec.rb

+        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")


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")

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.

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.

dacook

This is looking good! I'm not sure if it's ready, but looks good to me so far.

I think the PR description needs updating; I believe it closes #12627

edit: I just saw your final comment and I think you intended it to be ready, so I'll request a second review and update the description.

dacook · 2024-07-16T23:17:47Z

spec/system/admin/product_import_spec.rb

-        expect(page).to have_content "5" # on_hand
+      carrots = Spree::Product.find_by(name: 'Carrots')
+
+      within "#product_#{carrots.id}" do


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 :)

dacook · 2024-07-16T23:17:59Z

spec/system/admin/product_import_spec.rb

+        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")


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.

rioug

Looks good 👍
I am not really across the plan for BUU testing, but are removing test for the legacy bulk edit product page ? It seems a bit early as it's still in use.

dacook · 2024-07-16T23:39:08Z

I am not really across the plan for BUU testing, but are removing test for the legacy bulk edit product page ? It seems a bit early as it's still in use.

Oh actually that's a really good point, we don't need to be removing it as part of this PR! I think it should still work. @filipefurtad0 would you agree to restore it?

Reverts removal from bulk_product_spec.rb

We don't want to enable this in production just yet

This should have an effect on test environment

filipefurtad0 · 2024-07-17T13:53:49Z

Oh actually that's a really good point, we don't need to be removing it as part of this PR! I think it should still work. @filipefurtad0 would you agree to restore it?

Oh... Of course. I forgot we're still using it in production. Restored the file and squashed it with the commit which was removing it in the first place. Thanks @rioug @dacook.

dacook

Great, I think this is ready to go.

However, as there's currently some blocking bugs on the new product screen, I won't merge right now, because it might cause drama for new developers.

Edit: oh it requires manual testing first anyway :)

RachL · 2024-07-18T14:32:43Z

Scenario 1: All good 🎉

Scenario 2: All good 🎉

In both cases I briefly checked the UI as well. LGTM! Merging

filipefurtad0 added the technical changes only These pull requests do not contain user facing changes and are grouped in release notes label Jul 12, 2024

filipefurtad0 self-assigned this Jul 12, 2024

filipefurtad0 force-pushed the prepare_import_spec_for_admin_style_3 branch 4 times, most recently from 2708202 to 2ae57b8 Compare July 15, 2024 10:31

filipefurtad0 added 3 commits July 15, 2024 11:32

Sets import column test as pending

bdc42de

As of now, it is not clear whether we wish to re-implement this feature on BUU

Removes TODO, preparing spec for admin_style_3 (1)

91daec4

Removes TODO, preparing spec for admin_style_3 (2) Uses xpath to point to on hand field

Brings back xpath selector

e1976c6

Fixes xpath

filipefurtad0 force-pushed the prepare_import_spec_for_admin_style_3 branch 2 times, most recently from e6f5c8d to e1976c6 Compare July 15, 2024 11:34

filipefurtad0 mentioned this pull request Jul 15, 2024

[BUU] Activate admin_style_v3 for dev, test and new servers #12627

Closed

6 tasks

filipefurtad0 commented Jul 15, 2024

View reviewed changes

db/migrate/20240715103415_enable_admin_style_v3_by_default.rb Outdated Show resolved Hide resolved

dacook self-requested a review July 16, 2024 00:26

dacook reviewed Jul 16, 2024

View reviewed changes

filipefurtad0 added pr-staged-fr staging.coopcircuits.fr and removed pr-staged-fr staging.coopcircuits.fr labels Jul 16, 2024

dacook approved these changes Jul 16, 2024

View reviewed changes

rioug approved these changes Jul 16, 2024

View reviewed changes

filipefurtad0 added 4 commits July 17, 2024 14:51

Removes references to enabling admin_style_v3 features

46696df

Reverts removal from bulk_product_spec.rb

Adds a migration to enable the feature by default

24defac

Adds condition for dev environment only

e0d7252

We don't want to enable this in production just yet

Copies admin_style_v3 feature into ACTIVE_BY_DEFAULT section

0123d6f

This should have an effect on test environment

filipefurtad0 force-pushed the prepare_import_spec_for_admin_style_3 branch from 51a493d to 0123d6f Compare July 17, 2024 13:52

dacook approved these changes Jul 17, 2024

View reviewed changes

RachL added pr-staged-fr staging.coopcircuits.fr pr-staged-au staging.openfoodnetwork.org.au and removed pr-staged-fr staging.coopcircuits.fr labels Jul 18, 2024

RachL removed the pr-staged-au staging.openfoodnetwork.org.au label Jul 18, 2024

RachL merged commit de20dd9 into openfoodfoundation:master Jul 18, 2024
54 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prepare product_import_spec.rb for BUU as default #12668

Prepare product_import_spec.rb for BUU as default #12668

filipefurtad0 commented Jul 12, 2024 •

edited

Loading

dacook Jul 16, 2024

filipefurtad0 Jul 16, 2024

dacook Jul 16, 2024

dacook Jul 16, 2024

filipefurtad0 Jul 16, 2024

dacook Jul 16, 2024

dacook left a comment •

edited

Loading

dacook Jul 16, 2024

dacook Jul 16, 2024

rioug left a comment

dacook commented Jul 16, 2024

filipefurtad0 commented Jul 17, 2024 •

edited

Loading

dacook left a comment •

edited

Loading

RachL commented Jul 18, 2024

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

Prepare product_import_spec.rb for BUU as default #12668

Prepare product_import_spec.rb for BUU as default #12668

Conversation

filipefurtad0 commented Jul 12, 2024 • edited Loading

What? Why?

What should we test?

Release notes

Dependencies

Documentation updates

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dacook left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rioug left a comment

Choose a reason for hiding this comment

dacook commented Jul 16, 2024

filipefurtad0 commented Jul 17, 2024 • edited Loading

dacook left a comment • edited Loading

Choose a reason for hiding this comment

RachL commented Jul 18, 2024

filipefurtad0 commented Jul 12, 2024 •

edited

Loading

dacook left a comment •

edited

Loading

filipefurtad0 commented Jul 17, 2024 •

edited

Loading

dacook left a comment •

edited

Loading