[BUU] Activate admin_style_v3 for most system specs #11645

filipefurtad0 · 2023-10-10T11:08:05Z

What? Why?

~~!! Not to be merged just yet, as it activates BUU by default !!~~

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

It does not yet fully address feature parity, I've disconnected the issue #11845

Enables the BUU toggle on base_spec_helper.rb;
Fixes case on text assertions
Fixes failing tests due to overlapping elements; a helper was created to deal with this

In what #11845 concerns, this is the current state (this will be updated as progressed):


As an Administrator
  I want to be able to manage products in bulk
  listing products
    displays a list of products - **DONE**
    displays a message when number of products is zero - **DONE**
    displays a select box for suppliers, with the appropriate supplier selected - **PENDING (should be fixed in #11932)**
    displays an on hand count in a span for each product - **DONE**
    displays 'on demand' for any variant that is available on demand - **DONE**
    displays a select box for the unit of measure for the product's variants - **PENDING**
    displays a text field for the item name when unit is set to 'Items' - **PENDING**

What should we test?

System tests should display the BUU design on all admin pages
Green build

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

This can be done once we have completed all BUU1 features, including:

Documentation updates

filipefurtad0 · 2023-10-10T11:23:34Z

Ok, some /admin specs are failing, moving to in Dev.

dacook · 2023-10-10T22:37:25Z

Thanks Filipe! It looks like probably all of these failures are due to a style change in capitalisation, which results in text no longer matching. Eg:

  expected to find text "DISTRIBUTOR ..." in "Distributor..."

I'm not sure of the most efficient way to solve this, but I've often thought that these expectations shouldn't be case-sensitive (they still convey the same meaning to the user), so I would argue that we change that. I'll raise in #core-devs.

dacook · 2023-10-10T23:34:23Z

I discussed with Maikel, and agreed it's worth updating the text in the specs to be more readable and better match what's on the screen.

We also agreed that it's helpful to optionally add a matcher like have_content_i to better handle this in the future.

@filipefurtad0 would you like to try this, or perhaps hand over for a good-first-issue?

filipefurtad0 · 2023-10-11T10:04:50Z

Happy to have a go at this 👍
Thanks @dacook

filipefurtad0 · 2023-10-24T22:42:11Z

Hum, indeed there are some failing tests now. I've left this one forgotten in In Dev, but now of course the code has changed (after rebasing).

Either way, even after fixing, I guess we'll only be able to merge this PR once we're ready to release the feature, right? So, maybe we should hold this one for a while... And update everything once we're ready to release BUU.

What do you think @dacook ?

dacook · 2023-10-24T22:50:28Z

🤔 Hmm.

We want to continue supporting the old (current) admin styles, so need to ensure it's being tested
We want to also test with the new styles, in case any new problems come up

We could run the specs twice, for both old and new.
But I don't think it's necessary. Apart from the bulk products screen, the only changes are styling (colours, padding, fonts, etc).

So I agree, let's keep this PR in dev, and wait until we get closer to releasing admin_style_v3. We can occasionally refresh this branch when needed until then.

filipefurtad0 · 2023-10-25T14:25:43Z

So I agree, let's keep this PR in dev, and wait until we get closer to releasing admin_style_v3. We can occasionally refresh this branch when needed until then.

Ok, great - thanks. I've updated it with a few changes so it's green again.

filipefurtad0 · 2023-11-22T19:12:20Z

Ok, I've fixed conflicts, rebased, updated a test - it's green again.

It seems to me the challenge now, is to migrate the relevant tests from the legacy products page into the spec for new product page; After that, we can delete the legacy spec file.

Does this sound like a good plan @dacook?

dacook · 2023-11-22T22:38:06Z

Awesome, thanks Filipe! I've created an issue for this:

[BUU] Specs for bulk products feature parity #11845

I think we shouldn't delete the legacy spec until we remove the feature toggle.

on the New variant button This test needs to be improved as, for Capybara, the text seems to be always visible, although it only does become visible by hovering.

This feature does not exist in BUU Replaces previous add variant button click with correct version

These seem to have been changed since the previous rebase.

I'm not sure why it's not appearing on my computer, but it was an unnecessary duplicate message, so I'm happy to remove it.

dacook · 2024-07-01T05:29:27Z

Hi @filipefurtad0 , I went ahead and rebased this to prepare for merge. I condensed some of the past commits, and added some more fixes.
I also added some "TODO" comments where we are still referring to the old Bulk Edit Products screen. I'd like to migrate those over to the new screen soon, but I think it can wait so that we can merge this long-running branch.

It breaks due to a change of the spec environment. There's no point fixing it, it's no longer required.

dacook

Although the latest changes are mine, I'm approving to resolve the "changes requested".

spec/system/admin/product_import_spec.rb

rioug

Looks good !

mkllnk

Huge chunk of work. Well done for taming this.

There are many little bits I would have like to see improved in the history but this needs to be merged as soon as possible to avoid more rebase errors.

spec/system/admin/reports_spec.rb

mkllnk · 2024-07-02T01:06:22Z

spec/system/admin/bulk_product_update_spec.rb

-    it "displays a message when number of products is zero" do
-      visit spree.admin_products_path
-
-      expect(page).to have_text "No products yet. Why don't you add some?"
-    end
-


The main commit message doesn't reflect the commit content.

mkllnk · 2024-07-02T01:09:46Z

spec/system/admin/order_cycles/complex_editing_multiple_updation_spec.rb

+    # Click dismiss on distributor warning
+    click_button 'Dismiss'


Or we could setup the enterprise properly to avoid the warning?

Done in #12652 🙏

mkllnk · 2024-07-02T01:11:07Z

spec/system/admin/order_cycles/simple_spec.rb

+        # overlapping warning, we need to use 'node.trigger("click")'
+        page.find(:button, "Save").trigger("click")


Can we dismiss the warning first? Otherwise this is a UX issue. Users can't "find button and trigger save" with their mouse.

mkllnk · 2024-07-02T01:12:36Z

spec/system/admin/order_spec.rb

+<<<<<<< HEAD
          expect(page).not_to have_content different_shipping_method_for_distributor1.name
+=======
+          expect(page).to_not have_content different_shipping_method_for_distributor1.name
+          dismiss_warning
+>>>>>>> 2ddec2cff4 (Deals with overlapping elements)


Another conflict that could have been squashed.

mkllnk · 2024-07-02T01:14:37Z

spec/system/admin/products_v3/products_spec.rb

-        within(:xpath, '//*[@id="products-form"]/table/tbody[1]/tr[1]/td[5]/div') do
-          expect(page).to have_content "On demand"
-        end
-        within(:xpath, '//*[@id="products-form"]/table/tbody[2]/tr[1]/td[5]/div') do
-          expect(page).to have_content "20" # displays the total stock
-        end
-        within(:xpath, '//*[@id="products-form"]/table/tbody[2]/tr[2]/td[5]/div') 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[5]/div') do
-          expect(page).to have_content "4"  # displays the stock for variant_3
-        end
+        expect(page).to have_content "On demand"
+        expect(page).to_not 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


I think that the previous selectors were to avoid false passes of the spec. Imagine this spec running with Enterprise 4. It would show the number 4 even without displaying that stock.

Do you think we should revert to the xpath code? Maybe an alternative would be to include the count: 1 attribute/condition, or even make the on_hand numbers a bit more specific, like 111, 222, 333 - I know, in theory, we could still have Enterprise 111, but maybe more unlikely.

What do you think @mkllnk @dacook ?

Yes, I would go back to the xpath. If this was a more common use case then I would create helpers for that. Aren't there some helpers for accessing table rows by a displayed name, for example? That would be better.

Yes, in fact the inputs have labels associated with them, so something like this should work:

within row_containing_name("variant name") do expect(page).to have_field "On demand", checked: true expect(page).to_not have_field "On Hand", with: 20 end

(hmm, we need to fix that inconsistent capitalisation..)

Sorry, that was targeting the fields inside the popout.

If we want to check the display, we can target the button (which opens the popout):

within row_containing_name("variant name") do expect(page).to have_button "On Hand", with: "On demand" expect(page).to_not have_button "On Hand", with: 20 end

Even better might be to say something like this. But as far as I know Capybara doesn't have a convenient way to check all text (including button labels). We could make a helper for that, but it's not worth the bother..

within row_containing_name("variant name") do within column("On Hand") do expect(page).to have_text "On demand" expect(page).to_not have_text "20" end end

filipefurtad0 added the technical changes only These pull requests do not contain user facing changes and are grouped in release notes label Oct 10, 2023

filipefurtad0 self-assigned this Oct 11, 2023

filipefurtad0 force-pushed the activate_buu_toggle_by_default_to_run_the_test_suite branch 5 times, most recently from a5e5209 to c50219e Compare October 15, 2023 22:04

This comment was marked as resolved.

Sign in to view

dacook marked this pull request as draft October 24, 2023 22:50

filipefurtad0 force-pushed the activate_buu_toggle_by_default_to_run_the_test_suite branch 2 times, most recently from 50f4af8 to 67c4768 Compare October 25, 2023 12:12

filipefurtad0 force-pushed the activate_buu_toggle_by_default_to_run_the_test_suite branch from 67c4768 to d8313b8 Compare November 21, 2023 18:07

filipefurtad0 force-pushed the activate_buu_toggle_by_default_to_run_the_test_suite branch 2 times, most recently from 66d6103 to 1cc7bc4 Compare November 22, 2023 19:38

filipefurtad0 force-pushed the activate_buu_toggle_by_default_to_run_the_test_suite branch 5 times, most recently from 2f2c300 to 93dd4c8 Compare November 28, 2023 17:26

filipefurtad0 force-pushed the activate_buu_toggle_by_default_to_run_the_test_suite branch 2 times, most recently from e5bdc85 to 4c8f8dc Compare December 13, 2023 11:21

filipefurtad0 and others added 8 commits July 1, 2024 12:24

Adds assertions on table header

9f98dc2

Mouse hovering displays the text

80e8177

on the New variant button This test needs to be improved as, for Capybara, the text seems to be always visible, although it only does become visible by hovering.

Updates case on orders_spec.rb

c226b10

Removes mouse clicks to display variants

ba355fd

This feature does not exist in BUU Replaces previous add variant button click with correct version

Updates spec to create a variant within BUU design

8028610

Sets variant creation scenarios on_hand/on_demand as shared examples

9e25893

Fixes rebase

6dfef81

fixup: Updates spec to create a variant within BUU design

3a8c44d

dacook mentioned this pull request Jul 1, 2024

[BUU] Re-organise products_v3 feature specs #12497

Closed

dacook added 3 commits July 1, 2024 14:58

Fix more uppercase tests after rebase

39cae44

These seem to have been changed since the previous rebase.

Remove unecessary message

5f01bb4

I'm not sure why it's not appearing on my computer, but it was an unnecessary duplicate message, so I'm happy to remove it.

fixup: feature toggle old screen

109432d

dacook force-pushed the activate_buu_toggle_by_default_to_run_the_test_suite branch from 3d9ea3f to eccc6f4 Compare July 1, 2024 05:25

dacook added 3 commits July 1, 2024 15:31

todo: make specs work with admin_style_v3

b1721d6

Use normal capitalisation for translation

c80199e

Remove old migration spec

b1aafbf

It breaks due to a change of the spec environment. There's no point fixing it, it's no longer required.

dacook force-pushed the activate_buu_toggle_by_default_to_run_the_test_suite branch from eccc6f4 to b1aafbf Compare July 1, 2024 05:31

dacook self-assigned this Jul 1, 2024

dacook approved these changes Jul 1, 2024

View reviewed changes

dacook marked this pull request as ready for review July 1, 2024 05:34

dacook requested a review from rioug July 1, 2024 06:47

rioug reviewed Jul 1, 2024

View reviewed changes

spec/system/admin/product_import_spec.rb Show resolved Hide resolved

rioug approved these changes Jul 1, 2024

View reviewed changes

dacook changed the title ~~[BUU] Activate admin_style_v3 for all system specs~~ [BUU] Activate admin_style_v3 for most system specs Jul 2, 2024

mkllnk approved these changes Jul 2, 2024

View reviewed changes

mkllnk merged commit a83daae into openfoodfoundation:master Jul 2, 2024
50 checks passed

filipefurtad0 mentioned this pull request Jul 9, 2024

[BUU] Prepares specs for enabling admin_style_v3 by default #12652

Merged

4 tasks

chahmedejaz mentioned this pull request Jul 11, 2024

[BUU] Re-Organize products_v3 Feature specs #12661

Merged

4 tasks

filipefurtad0 mentioned this pull request Jul 12, 2024

Prepare product_import_spec.rb for BUU as default #12668

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BUU] Activate admin_style_v3 for most system specs #11645

[BUU] Activate admin_style_v3 for most system specs #11645

filipefurtad0 commented Oct 10, 2023 •

edited by dacook

Loading

filipefurtad0 commented Oct 10, 2023

dacook commented Oct 10, 2023

dacook commented Oct 10, 2023

filipefurtad0 commented Oct 11, 2023

This comment was marked as resolved.

filipefurtad0 commented Oct 24, 2023

dacook commented Oct 24, 2023

filipefurtad0 commented Oct 25, 2023

filipefurtad0 commented Nov 22, 2023

dacook commented Nov 22, 2023

dacook commented Jul 1, 2024

dacook left a comment

rioug left a comment

mkllnk left a comment

mkllnk Jul 2, 2024

mkllnk Jul 2, 2024

filipefurtad0 Jul 10, 2024

mkllnk Jul 2, 2024

mkllnk Jul 2, 2024

mkllnk Jul 2, 2024

filipefurtad0 Jul 10, 2024 •

edited

Loading

mkllnk Jul 10, 2024

dacook Jul 10, 2024 •

edited

Loading

dacook Jul 10, 2024

dacook Jul 10, 2024

		# Click dismiss on distributor warning
		click_button 'Dismiss'

		# overlapping warning, we need to use 'node.trigger("click")'
		page.find(:button, "Save").trigger("click")

[BUU] Activate admin_style_v3 for most system specs #11645

[BUU] Activate admin_style_v3 for most system specs #11645

Conversation

filipefurtad0 commented Oct 10, 2023 • edited by dacook Loading

What? Why?

What should we test?

Release notes

Dependencies

Documentation updates

filipefurtad0 commented Oct 10, 2023

dacook commented Oct 10, 2023

dacook commented Oct 10, 2023

filipefurtad0 commented Oct 11, 2023

This comment was marked as resolved.

filipefurtad0 commented Oct 24, 2023

dacook commented Oct 24, 2023

filipefurtad0 commented Oct 25, 2023

filipefurtad0 commented Nov 22, 2023

dacook commented Nov 22, 2023

dacook commented Jul 1, 2024

dacook left a comment

Choose a reason for hiding this comment

rioug left a comment

Choose a reason for hiding this comment

mkllnk left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filipefurtad0 Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dacook Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filipefurtad0 commented Oct 10, 2023 •

edited by dacook

Loading

filipefurtad0 Jul 10, 2024 •

edited

Loading

dacook Jul 10, 2024 •

edited

Loading