Fix URL State management on the Products page #12678

chahmedejaz · 2024-07-16T11:11:23Z

What? Why?

Closes Products_v3: URL state management broken #11640
Turbo was enabled for the affected form submissions, which by default does not update the URL upon submission.
This PR adds the data-turbo-action = advance to the affected form submissions such that the default form submission behavior is not affected as well as turbo remains intact.
Moreover, it fixes the scenario as explained in this comment as well.

What should we test?

Please validate the expected behavior as given in the issue.

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

chahmedejaz · 2024-07-16T11:28:23Z

I'm occupying the au-staging for while, need to validate the fix on staging with real data. Thanks.

dacook

Great, it looks like it resolves the bug, but it removes the dynamic Turbo feature, which makes the page load seem slower :'(

It looks like Turbo has a feature to update the browser history, would this help?
https://turbo.hotwired.dev/handbook/frames#promoting-a-frame-navigation-to-a-page-visit

Could you try that?

- Fix the scenario when per_page is selected as 100 and next page is clicked, then per_page is empty in the request. - Expected behavior should be that it retains the per_page selected previously

chahmedejaz · 2024-07-17T05:27:22Z

Great, it looks like it resolves the bug, but it removes the dynamic Turbo feature, which makes the page load seem slower :'(

It looks like Turbo has a feature to update the browser history, would this help? https://turbo.hotwired.dev/handbook/frames#promoting-a-frame-navigation-to-a-page-visit

Could you try that?

Hi @dacook - Thanks for pointing that out. It's working great! 👍

dacook

Yay! I had a little play with it and it all looks good to me.

filipefurtad0 · 2024-07-17T18:14:19Z

Hey @chahmedejaz ,

This PR correctly introduces URL changes, when filters are used. This is demonstrated on the picture below (upper part, before staging; down part, after staging):

However, it seems the page returns an endless spinner, if one uses the back option, on the browser. Steps to reproduce, the user:

adds a search term, and clicks the search button
selects a pagination option
selects a producer, and clicks the search button
selects a product category, and clicks the search button

Then, click the back option of your browser a few times; wait between each click - notice the page is never really loaded.

I'll try this in another staging server, just to be sure this is not a data issue.

RachL · 2024-07-17T18:43:07Z

I have my fingers crossed this is a data problem! it would explain something the CA instance has been experiencing...

filipefurtad0 · 2024-07-17T18:55:14Z

Unfortunately, I was able to reproduce the endless spinner in another staging server, after having several search filters and using the back option on the browser:

I'm not sure what's better here: do you feel it's best to work on another branch, after merging @chahmedejaz ? I'm a bit concerned about the endless spinner, which will confuse users. So I think it could be a good idea to move this one back to In Dev. What do you think?

chahmedejaz · 2024-07-18T07:30:45Z

Unfortunately, I was able to reproduce the endless spinner in another staging server, after having several search filters and using the back option on the browser:

I'm not sure what's better here: do you feel it's best to work on another branch, after merging @chahmedejaz ? I'm a bit concerned about the endless spinner, which will confuse users. So I think it could be a good idea to move this one back to In Dev. What do you think?

I'll look into it @filipefurtad0. Thanks for raising this one.

filipefurtad0 · 2024-07-18T10:38:48Z

Ok, thanks @chahmedejaz.

I had still another go at testing this, and pulled the branch into my local env.: I found the issue (endless spinner by clicking the browser back option) to be still reproducible as well, which I think removes the hypothesis of bad data (ping @RachL). Moved back to In Dev .

chahmedejaz · 2024-07-18T10:41:49Z

Ok, thanks @chahmedejaz.

I had still another go at testing this, and pulled the branch into my local env.: I found the issue (endless spinner by clicking the browser back option) to be still reproducible as well, which I think removes the hypothesis of bad data (ping @RachL). Moved back to In Dev .

Thanks @filipefurtad0 I'll look into this issue today.

chahmedejaz · 2024-07-22T08:53:47Z

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

@@ -1,6 +1,6 @@
-= form_with url: admin_products_path, id: "filters", method: :get, data: { "search-target": "form", 'turbo-frame': "_self" } do
+= form_with url: admin_products_path, id: "filters", method: :get, data: { "search-target": "form", 'turbo-frame': "_self", 'turbo-action': "advance" } do


Hi @dacook - Here are my findings on the issue raised by the @filipefurtad0:

Due to 'turbo-frame': "_self" on the above form submission, it targets the following frame, hence it gets the aria-busy="true" attribute added when it's being updated.

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

Line 1 in 5caeb16

%turbo-frame#products-content{ target: "_top", refresh: "morph" }

This causes the loader to show up as per our CSS rules

The root cause here is that this same page (with aria-busy="true") is then cached by Turbo, and upon navigation back, the aria-busy="true" is still present on the (cached) page

I think this is a bug in Turbo, what do you think? It should sanitize the page so that no aria-busy is present on any frame. Or cache the page before making it busy.

For the solution, I had many, For example:

remove 'turbo-frame': "_self", 'turbo-action': "advance" from the affected links so that by our default rule, each page will be replaced by "_top" frame

This fixes our current loader problem, however, by this approach, we won't be getting the aria-busy="true" on the above-mentioned frame, so no loader upon UI button clicks.

Another solution is that I think we should opt for this one:

We should disable the Turbo cache because by default, we are also not caching pages in the browser, hence navigation back gets the page by the server.

I've implemented this here, please review: a6efad7

Please let me know what you think about it, if you are okay with this, we are good to move it back to testing. Thanks

Hi Ahmed, this looks tricky. I'm sorry I'm not feeling well today and am not able to fully understand. A few thoughts:

I'm wondering what difference it makes if we remove the morph method?

I'm hesitant to disable cache entirely in case it would be helpful in other circumstances. I'm wondering if choosing no-preview would help (https://turbo.hotwired.dev/handbook/building#opting-out-of-caching)?
But If no-cache is the only way to solve it, then go ahead.

Hi Ahmed, this looks tricky. I'm sorry I'm not feeling well today and am not able to fully understand. A few thoughts:

I'm wondering what difference it makes if we remove the morph method?

I'm hesitant to disable cache entirely in case it would be helpful in other circumstances. I'm wondering if choosing no-preview would help (https://turbo.hotwired.dev/handbook/building#opting-out-of-caching)? But If no-cache is the only way to solve it, then go ahead.

No worries, David. Please take a rest and get well soon.

I've tried using no-preview as cache control and removing morph as well, however, it's of no use. The issue explained here remains the same.

In the context of Turbo, Morphing is a way to handle page reloads/refreshes. When a page/frame refresh is set to morph, then Turbo doesn't fully replace the page/frame. It calculates the changes between the current HTML response and the current page/frame and only updates the changes in the current frame/page, rather than the full replacement.
Ref: https://turbo.hotwired.dev/handbook/page_refreshes#morphing

I'm afraid, disabling Turbo cache seems to be the only solution. 😕

Thanks Ahmed, I guess that's the only option, please go ahead.

dacook

Sorry forgot to approve.
I'm afraid this might limit performance enhancements later, but we will have to deal with that later.

chahmedejaz · 2024-07-25T18:25:48Z

Thanks, David. Sure, If we face any performance issues due to Turbo Cache, then we will see the way to deal with it.

chahmedejaz · 2024-07-25T18:26:17Z

@filipefurtad0 - It's ready for revalidation. Please let me know if the issue still persists. Thanks.

filipefurtad0 · 2024-07-25T19:00:40Z

Thank you so much for your notice, to re-test this PR @chahmedejaz .

It's looking great now:

https://www.loom.com/share/65c93ea3ee9043eea50eff998b205237

I've marked it for release as introducing user-facing changes, as it enables the browsing possibility, throughout different filters.

Awesome! Merging.

github-actions bot assigned chahmedejaz Jul 16, 2024

chahmedejaz changed the title ~~11640 - disable turbo on filter and sort form submissions~~ Fix URL State management on the Products page Jul 16, 2024

chahmedejaz added the pr-staged-au staging.openfoodnetwork.org.au label Jul 16, 2024

chahmedejaz removed the pr-staged-au staging.openfoodnetwork.org.au label Jul 16, 2024

chahmedejaz marked this pull request as ready for review July 16, 2024 12:10

chahmedejaz requested a review from dacook July 16, 2024 12:10

dacook requested changes Jul 17, 2024

View reviewed changes

chahmedejaz added 2 commits July 17, 2024 10:16

11640 - use turbo navigation

1850f29

11640 - fix per_page issue when the page is changed

1af811c

- Fix the scenario when per_page is selected as 100 and next page is clicked, then per_page is empty in the request. - Expected behavior should be that it retains the per_page selected previously

chahmedejaz force-pushed the bugfix/11640-products-page-broken-URL branch from b8e92e8 to 1af811c Compare July 17, 2024 05:17

chahmedejaz requested a review from dacook July 17, 2024 05:26

dacook approved these changes Jul 17, 2024

View reviewed changes

filipefurtad0 self-assigned this Jul 17, 2024

filipefurtad0 added the pr-staged-fr staging.coopcircuits.fr label Jul 17, 2024

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

filipefurtad0 removed pr-staged-au staging.openfoodnetwork.org.au pr-staged-fr staging.coopcircuits.fr labels Jul 17, 2024

filipefurtad0 added the feedback-needed label Jul 17, 2024

filipefurtad0 removed the feedback-needed label Jul 18, 2024

12551 - add no-cache policy for Turbo cache control

a6efad7

chahmedejaz commented Jul 22, 2024

View reviewed changes

chahmedejaz requested a review from dacook July 22, 2024 08:54

chahmedejaz mentioned this pull request Jul 23, 2024

[BUU] 'Back to products list' Button on Edit Product removed filters on Products page #12698

Closed

chahmedejaz added the bug-s3 The bug is stopping a critical or non-critical feature but there is a usable workaround. label Jul 25, 2024

dacook approved these changes Jul 25, 2024

View reviewed changes

filipefurtad0 added pr-staged-fr staging.coopcircuits.fr user facing changes Thes pull requests affect the user experience and removed pr-staged-fr staging.coopcircuits.fr labels Jul 25, 2024

filipefurtad0 merged commit 7bcf320 into openfoodfoundation:master Jul 25, 2024
55 of 56 checks passed

chahmedejaz mentioned this pull request Jul 29, 2024

[BUU] Show per page not working after searching #12712

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix URL State management on the Products page #12678

Fix URL State management on the Products page #12678

chahmedejaz commented Jul 16, 2024 •

edited by filipefurtad0

Loading

chahmedejaz commented Jul 16, 2024

dacook left a comment

chahmedejaz commented Jul 17, 2024

dacook left a comment

filipefurtad0 commented Jul 17, 2024

RachL commented Jul 17, 2024

filipefurtad0 commented Jul 17, 2024

chahmedejaz commented Jul 18, 2024

filipefurtad0 commented Jul 18, 2024 •

edited

Loading

chahmedejaz commented Jul 18, 2024

chahmedejaz Jul 22, 2024 •

edited

Loading

dacook Jul 23, 2024

chahmedejaz Jul 23, 2024 •

edited

Loading

dacook Jul 25, 2024

dacook left a comment

chahmedejaz commented Jul 25, 2024 •

edited

Loading

chahmedejaz commented Jul 25, 2024 •

edited

Loading

filipefurtad0 commented Jul 25, 2024

		@@ -1,6 +1,6 @@
		= form_with url: admin_products_path, id: "filters", method: :get, data: { "search-target": "form", 'turbo-frame': "_self" } do
		= form_with url: admin_products_path, id: "filters", method: :get, data: { "search-target": "form", 'turbo-frame': "_self", 'turbo-action': "advance" } do

Fix URL State management on the Products page #12678

Fix URL State management on the Products page #12678

Conversation

chahmedejaz commented Jul 16, 2024 • edited by filipefurtad0 Loading

What? Why?

What should we test?

Release notes

chahmedejaz commented Jul 16, 2024

dacook left a comment

Choose a reason for hiding this comment

chahmedejaz commented Jul 17, 2024

dacook left a comment

Choose a reason for hiding this comment

filipefurtad0 commented Jul 17, 2024

RachL commented Jul 17, 2024

filipefurtad0 commented Jul 17, 2024

chahmedejaz commented Jul 18, 2024

filipefurtad0 commented Jul 18, 2024 • edited Loading

chahmedejaz commented Jul 18, 2024

chahmedejaz Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

dacook Jul 23, 2024

Choose a reason for hiding this comment

chahmedejaz Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

dacook Jul 25, 2024

Choose a reason for hiding this comment

dacook left a comment

Choose a reason for hiding this comment

chahmedejaz commented Jul 25, 2024 • edited Loading

chahmedejaz commented Jul 25, 2024 • edited Loading

filipefurtad0 commented Jul 25, 2024

chahmedejaz commented Jul 16, 2024 •

edited by filipefurtad0

Loading

filipefurtad0 commented Jul 18, 2024 •

edited

Loading

chahmedejaz Jul 22, 2024 •

edited

Loading

chahmedejaz Jul 23, 2024 •

edited

Loading

chahmedejaz commented Jul 25, 2024 •

edited

Loading

chahmedejaz commented Jul 25, 2024 •

edited

Loading