Feature/11058 change products order by name #12449

chahmedejaz · 2024-05-08T08:50:22Z

What? Why?

Closes [BUU] Change the order of products in my catalogue by name #11058
This PR adds the feature to sort the products by their name on products page

What should we test?

All the scenarios in the attached issue

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

chahmedejaz · 2024-05-08T09:56:03Z

app/views/spree/admin/shared/_stimulus_sortable_header.html.haml

@@ -1,5 +1,5 @@
 %th
-  %a{ "data-action": "click->search#changeSorting", "data-column": "#{column}", "data-current": sorted.to_s }
+  %a{ "data-controller": "search", "data-action": "click->search#changeSorting", "data-column": "#{column}", "data-current": (sorted || default).to_s }


It's better to associate the search controller here because it doesn't have to depend upon the parent to use it.

chahmedejaz · 2024-05-08T10:02:53Z

spec/system/admin/products_v3/products_spec.rb

+
+          # Sort in descending order
+          name_header.click
+          sleep(0.2)


It requires some delay here otherwise the page doesn't have the updated content.

I think it's something to do with Turbo, as it doesn't technically load the entire page and changes are updated in the existing page. So there's no way for Capybara to know whether the contents are fully loaded or not and it just moves on without waiting enough. 😅

Actually, I think it's because capybara methods will wait until the specified element exists. Eg expect(page).to have_content /Bananas.*Apples/ would wait.

But this code doesn't know how to wait. We need something like:

expect(page).to have_content(/Bananas.*Apples/, include_input_values: true)

But I'm not sure it's worth trying to create that. How about instead of sleep, we wait for:

Suggested change

sleep(0.2)

expect(page).to have_content("Name ▼") # this indicates the re-sorted content has loaded

Thank you so much, David. It worked ❤️

dacook

Great job, it works beautifully. And not a line of javascript added!

I have a suggestion on how we might be able to avoid the sleeps, would you like to try it?

Also if you're interested, to try the rebase for a cleaner history (this makes it easier for the reviewer).

app/views/admin/products_v3/_filters.html.haml

spec/system/admin/products_v3/products_spec.rb

dacook · 2024-05-09T06:27:10Z

spec/system/admin/products_v3/products_spec.rb

+
+          # Sort in descending order
+          name_header.click
+          sleep(0.2)


Actually, I think it's because capybara methods will wait until the specified element exists. Eg expect(page).to have_content /Bananas.*Apples/ would wait.

But this code doesn't know how to wait. We need something like:

expect(page).to have_content(/Bananas.*Apples/, include_input_values: true)

But I'm not sure it's worth trying to create that. How about instead of sleep, we wait for:

Suggested change

sleep(0.2)

expect(page).to have_content("Name ▼") # this indicates the re-sorted content has loaded

dacook · 2024-05-09T06:30:39Z

app/views/admin/products_v3/_content.html.haml

@@ -1,5 +1,5 @@
 %turbo-frame#products-content{ target: "_top", refresh: "morph" }
-  .spinner-overlay.hidden
+  .spinner-overlay{ "data-controller": "loading", "data-products-target": "loading", class: "hidden" }


It would be worth re-basing to revert this on the initial commit. If you're comfortable with interactive rebase, you could try:

git rebase -i master

You can move this commit up two lines, and change to a "fixup" which will combine it into the previous commit

pick 86f88792c2 11058: add sortable products by name fixup ff97794684 11058: fix specs pick b104fd8357 11058: add specs pick 6c65008cf3 11058: add wait for the page to update

Ah, that's nice. Used it for the first time properly. Would use it often now. Thanks :)

A word of caution, only use it when necessary, and only for simple changes if you can.
It can get very difficult very quickly. I find often I need to git rebase --abort!

rioug

Nice one 👍

dacook

Great!

dacook · 2024-05-13T01:02:59Z

app/views/admin/products_v3/_content.html.haml

@@ -1,5 +1,5 @@
 %turbo-frame#products-content{ target: "_top", refresh: "morph" }
-  .spinner-overlay.hidden
+  .spinner-overlay{ "data-controller": "loading", "data-products-target": "loading", class: "hidden" }


A word of caution, only use it when necessary, and only for simple changes if you can.
It can get very difficult very quickly. I find often I need to git rebase --abort!

spec/system/admin/products_v3/products_spec.rb

RachL · 2024-05-13T09:04:35Z

Staging failed but I don't know why :/ @dacook do you have a clue?

dacook · 2024-05-13T10:58:46Z

Looks like the server ran out of memory :(

TASK [deploy : bundle install app dependencies] ********************************
fatal: [staging.coopcircuits.fr]: FAILED! => {"changed": true, "cmd": ["bash", "-lc", "bundle install"], "delta": "0:00:15.082395", "end": "2024-05-13 09:06:42.764295", "msg": "non-zero return code", "rc": 1, "start": "2024-05-13 09:06:27.681900", "stderr": "--- ERROR REPORT TEMPLATE -------------------------------------------------------\n\n```\nNoMemoryError: failed to allocate memory\n

This is probably why fr_staging is failing intermittently. I haven't checked why, but we may need to increase the memory on that server.
You could try again, or try a different server

RachL · 2024-05-13T11:14:36Z

outch, I don't think FR will be able to cover an increase in memory :/ I'm surprised we run into this as I thought staging FR is an old FR production server. I thought it had a large bandwidth.

I think we only have staging AU left then, will try over there as I already tried twice on FR.

RachL · 2024-05-13T13:29:00Z

All scenarios passes kudos @chahmedejaz ! I've also tested the scenario of having several pages of products and check if the sorting was applied on all pages. FYI @mariocarabotta

I wanted to test with a heavy number of products, so tried to use staging AU super admin, but the profile seems stuck in this position:

discarding does not work. This state is prior to staging the PR so has nothing to do with this. I'm merging and will continue testing in production once it's released (not ideal but this is the best to keep things moving forward).

dacook · 2024-05-13T23:35:32Z

Awesome! Thanks Rachel, I will look into the modified issue on staging and try to fix that.

11058: add sortable products by name

86f8879

sigmundpetersen assigned chahmedejaz May 8, 2024

chahmedejaz commented May 8, 2024

View reviewed changes

chahmedejaz marked this pull request as ready for review May 8, 2024 10:03

chahmedejaz requested review from dacook, rioug and mkllnk May 8, 2024 10:03

dacook reviewed May 9, 2024

View reviewed changes

11058: add specs

b5fe19c

chahmedejaz force-pushed the feature/11058-change-products-order-by-name branch from ff97794 to b5fe19c Compare May 9, 2024 21:55

11058: add get_all_input_values

881da0f

chahmedejaz force-pushed the feature/11058-change-products-order-by-name branch from 8e1218a to 881da0f Compare May 9, 2024 23:06

chahmedejaz requested a review from dacook May 9, 2024 23:15

rioug approved these changes May 12, 2024

View reviewed changes

dacook approved these changes May 13, 2024

View reviewed changes

dacook removed the request for review from mkllnk May 13, 2024 01:06

RachL added the pr-staged-fr staging.coopcircuits.fr label May 13, 2024

RachL added pr-staged-fr staging.coopcircuits.fr and removed pr-staged-fr staging.coopcircuits.fr labels May 13, 2024

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

RachL removed the pr-staged-au staging.openfoodnetwork.org.au label May 13, 2024

RachL merged commit bd83977 into openfoodfoundation:master May 13, 2024
54 of 56 checks passed

sigmundpetersen mentioned this pull request May 15, 2024

[staging-FR] Can't stage master/PRs #12485

Closed

dacook added the feature toggled These pull requests' changes are invisible by default and are grouped in release notes label May 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/11058 change products order by name #12449

Feature/11058 change products order by name #12449

chahmedejaz commented May 8, 2024

chahmedejaz May 8, 2024 •

edited

Loading

chahmedejaz May 8, 2024

dacook May 9, 2024

chahmedejaz May 9, 2024

dacook left a comment

dacook May 9, 2024

dacook May 9, 2024

chahmedejaz May 9, 2024

dacook May 13, 2024

rioug left a comment

dacook left a comment

dacook May 13, 2024

RachL commented May 13, 2024

dacook commented May 13, 2024

RachL commented May 13, 2024 •

edited

Loading

RachL commented May 13, 2024

dacook commented May 13, 2024

	sleep(0.2)
	expect(page).to have_content("Name ▼") # this indicates the re-sorted content has loaded

Feature/11058 change products order by name #12449

Feature/11058 change products order by name #12449

Conversation

chahmedejaz commented May 8, 2024

What? Why?

What should we test?

chahmedejaz May 8, 2024 • edited Loading

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

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

rioug left a comment

Choose a reason for hiding this comment

dacook left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RachL commented May 13, 2024

dacook commented May 13, 2024

RachL commented May 13, 2024 • edited Loading

RachL commented May 13, 2024

dacook commented May 13, 2024

chahmedejaz May 8, 2024 •

edited

Loading

RachL commented May 13, 2024 •

edited

Loading