[BUU, Bulk product edit page] Refreshing the page resets pagination options and search strings to default values

[filipefurtad0]

Description

Affects both legacy and BUU admin/products page.

Relevant only for BUU design (feature toggle admin_style_v3 enabled, see "Steps to reproduce" section for details).

When the page is refreshed, all pagination options are reset, i.e. if the user:

• for example, is viewing page number 2 -> the selection will be lost back to the default page nr. 1
• has selected the option to display 50 or 100 results per page -> the selection will be lost, and the default option of displaying 15 results per page will be selected.

Edit: Also, refreshing the page removes any search string used to search products.

An error will be displayed in the console.

Expected Behavior

Other pages (like the orders page) keep these selections, after page refresh; search strings should be kept as well.

Actual Behaviour

Refreshing the page resets pagination options and search strings to default values.

Steps to Reproduce

0. Enable admin_style_v3:
   • as superadmin, visit /admin/feature-toggle/features/admin_style_v3
   • click Fully Enable

For pagination:

2. Visit admin/products. Make sure you have more than 15 products.
3. Under the pagination feature, select page 2.
4. Refresh the page -> notice you're viewing page 1.
5. Under the pagination feature, select 50 per page.
6. Refresh the page -> notice you're the first 15 results only.

For search strings:

1. Visit admin/products.
2. Make a search using a given string; expect the results to match that string
3. Refresh the page
4. Notice the search field is empty and all products are returned

Animated Gif/Screenshot

Workaround

Re-select the desired page or number of results per page.

Severity

bug-s4: it's annoying, but you can use it

Your Environment

• Version used: v4.4.24
• Browser name and version: Firefox 121
• Operating System and version (desktop or mobile): Ubuntu 22.04

Possible Fix

[abdulazizali77]

@filipefurtad0 would like to take a stab at this, please assign to me

[filipefurtad0]

Hey @abdulazizali77, I've assigned you - thanks for your interest! Please let us know if you run into any issues.

[abdulazizali77]

thanks @filipefurtad0 First thought, http://localhost:3000/api/v0/products/bulk_products.json?per_page=50&page=2 returns an empty array, should it? or should the endpoint logic actually decide that 'page=2' is redundant/invalid here and return 'page=1' ?

[filipefurtad0]

Hey @abdulazizali77 ,

returns an empty array, should it?

I'm not entirely sure. It sounds like this is an implementation decision, I'm afraid I don't have a grounded opinion on that. Maybe @openfoodfoundation/core-devs can best answer here?

[filipefurtad0]

One additional comment on this issue: in addition to the pagination selection, it has been spotted that the search strings also also disappear upon page refresh.

This appears to be related - what do you think @abdulazizali77, can you confirm this assumption? If so, I'll edit the issue description above.

[abdulazizali77]

This appears to be related - what do you think @abdulazizali77, can you confirm this assumption? If so, I'll edit the issue description above.

@filipefurtad0 that is plausible, ill get back to you on that by this wednesday latest

[filipefurtad0]

Great, thank you @abdulazizali77 - I'll add this info. It is of course totally fine, if your PR does not fix it all at once, we can later decide to keep this issue open or open a new one. Thanks again.

[rioug]

@abdulazizali77

First thought, http://localhost:3000/api/v0/products/bulk_products.json?per_page=50&page=2 returns an empty array, should it? or should the endpoint logic actually decide that 'page=2' is redundant/invalid here and return 'page=1' ?

We don't support the version 0 of the API , so ideally we wouldn't update it. I had a quick play with it and it returns some pagination data, so you should be able to infer which page to display in the front end from that. ie:

{"products":[],"pagination":{"results":6,"pages":1,"page":3,"per_page":50}}

[dacook]

Thanks all for looking into this. I'd like to clarify that we should deal with the new product page (as pictured above, with admin_style_v3 turned on) separately from the old one, which uses a completely different implementation. I don't believe we should prioritise a fix for the old one. @filipefurtad0 would it be ok to update the description to cover only the new page?

So, some information about the new one: This has been built with StimulusReflex, and loads the paginated product data with ProductsReflex#fetch, and templates under app/views/admin/products_v3 The new page doesn't actually use the API endpoints, that was for the old one.

I hope this helps steer you in the right direction, please let us know if you have more questions, and I will endeavour to improve our documentation!

Edit: I've added this and other information to a new wiki page here: https://github.com/openfoodfoundation/openfoodnetwork/wiki/Bulk-Edit-Products-admin-interface

[filipefurtad0]

which uses a completely different implementation.

Ahh, I did not stress this in the issue as the bug was observed in both legacy and new display. Now edited :+1: Thank you for pointing that out @dacook.

[abdulazizali77]

Thanks for chiming in @dacook @rioug i actually was investigating the empty results with the V0::ProductsController implementation which i surmise to either be an issue with the frontend not resetting the page param when the per_page is set or the V0::ProductsController using the page and per_page 'blindly'

I have yet to look at the V3 behaviour so i dont have any conclusions yet. Im guessing for both V3 and V0 the missing search strings and pagination option could/should be fixable with either passing in query strings to the admin/products#index controller which should get passed to any subsequent API calls OR changing admin/products#index to a POST (?). Also possible, usage of cookies (not so preferable). I will get back with more findings ~tomorrow~ in the next couple of days on V3

[dacook]

Hi @abdulazizali77 , were you able to find anything out for V3?

Regarding the old one with V0, I appreciate your investigation so far, but you don't need to continue because we don't plan on supporting that in the long term.

[abdulazizali77]

Apologies for the delay @dacook after further investigation it seems with V3, all the 'reset' defects do not manifest anymore with Chrome, Firefox and Edge. However what is interesting to note is when i zoom in on @filipefurtad0's attached screenshot (i cant seem to click on it "This private-user-images.githubusercontent.com page can’t be found") there is no query string in the url whereas in my instance there is always a query string (_page=1&_per_page=15). However with my instance there were times that when the per_page was changed the per_page query string param could not be changed fast enough. Couldnt reproduce this consistently though.

Currently im running master 979071736b61d3922f9b3f42fe261a3325c03117 (6 Dec 2023) Its very likely Bellet's changes fixed the issue? 1a8a4ee72bc18de90326f0ab4a9e4191c552f9e6 574adb88d2f031a86c2b9bd92ab1a62d41af7a76 - ProductsReflex#change_per_page

The only difference i see with the current screenshot and V0 is that with ProductsReflex the query_string is always updated whenever the _per_page, _page and _search_term are changed and are not reset to default with Ctrl+R/F5. I didnt test on OSX though

Chrome

Edge

FF

Is there something im missing here that im not able to reproduce on V3?

[dacook]

That's odd that you can't replicate it, using the version that Filipe probably used when he raised this issue. (Although it would be best to replicate against the current master branch, which is updated almost every day) I'm sure I've seen these query string parameters too, when I was working on it earlier this week.

But I just checked out the latest master, and I was able to replicate with the steps in the description above, and saw no query string parameters at all:

So there must be some conditions to make it work..

[abdulazizali77]

that is odd! i can guess the problem would manifest without a query string. i guess it is odd that my version has query string being appended, i will continue investigating

[rioug]

This is the code that adds the query parameters (on master as of the 26th of Feb): https://github.com/openfoodfoundation/openfoodnetwork/blob/676f64cc4baaf916145ad6bbdfd87b3892f35974/app/reflexes/products_reflex.rb#L121-L123 I wonder if the broadcast_later could be causing some race condition ? maybe it would be better if this was handled on the client side. I believe there is something along the line of after_reflex event that we could use in a stimulus controller to do such things

[mkllnk]

Can we simply change broadcast_later to broadcast? This discussion was held before:

• https://github.com/openfoodfoundation/openfoodnetwork/pull/11163#discussion_r1260531844

But maybe that error was just during work-in-progress? Shall we test again?

[abdulazizali77]

thanks @mkllnk @rioug ~it seems to be that this potentially is a cors issue? with StimulusReflex/config?~ (or not going by conversation on #11163 ) Looking at the screenshot again its likely that the ProductsReflex#current_url basepath is https://rails whereas the application(?) basepath is https://staging.openfoodnetwork.org.uk. Which is why the replace_state failed and query string absent in the screenshot and causing the defect. So im guessing changing broadcast_later to broadcast might not make much of a difference. (obviously i might be wrong about that)

@dacook its weird that in your test video, theres no similar replace_state error thrown by chrome even though you dont get any query_string appended. Was wondering whether this is a non-localhost specific problem. Just to analyze this from a different env, is it possible to get an admin account on staging.openfoodnetwork.org.uk or what are options to deploy on staging/remote?

The only thing ive narrowed down is that the defect will manifest because the query_string cant get appended to the url (im sorry if this was obvious from the beginning)

[abdulazizali77]

@dacook @filipefurtad0 do you guys have the development log (/usr/src/app/app/log/development.log) from the instances which you ran with the defect? im trying to look for this line

web_1      | StimulusReflex::Channel#receive({"attrs"=>{"data-controller"=>"products", "id"=>"products_v3_page", "checked"=>false, "selected"=>false, "tag_name"=>"DIV"}, "dataset"=>{"dataset"=>{"data-controller"=>"products"}, "datasetAll"=>{}}, "selectors"=>[], "id"=>"425d3795-122a-44ff-a950-e32ad356f839", "resolveLate"=>false, "suppressLogging"=>false, "xpathController"=>"//*[@id='products_v3_page']", "xpathElement"=>"//*[@id='products_v3_page']", "inner_html"=>"", "text_content"=>"", "reflexController"=>"products", "permanentAttributeName"=>"data-reflex-permanent", "target"=>"Products#fetch", "args"=>[], "url"=>"http://localhost:3000/admin/products", "tabId"=>"96652a65-9d8d-4004-87a5-8fe8a8ef0943", "version"=>"3.5.0-rc3", "formData"=>""})

most pertinent is the url which gets passed to the StimulusReflex::Reflex#initialize

[dacook]

I just tried to replicate it on my environment, but now it's working!?!

So I looked for old entries in development.log, and found StimulusReflex::Channel#receive near the time of my last post, here it is:

 417348 StimulusReflex::Channel#receive({"attrs"=>{"name"=>"per_page", "id"=>"per_page", "class"=>"no-input per-page tomselected ts-hidden-accessible", "data-reflex"=>"change->products#change_per_page", "data-controller"=>"tom-select", "data-tom-select-options-value"=>"{ \"plugins\": [] }", "tabindex"=>"-1", "data-action"=>"change->products#__perform", "checked"=>false, "selected"=>false, "tag_name"=>"SELECT", "values"=>["25"], "value"=>"25"}, "dataset"=>{"dataset"=>{"data-reflex"=>"change->products#change_per_page", "data-controller"=>"tom-select", "data-tom-select-options-value"=>"{ \"plugins\": [] }", "data-action"=>"change->products#__perform"}, "datasetAll"=>{}}, "selectors"=>[], "id"=>"e630a276-a76e-422d-82fa-4745ffab8a58", "resolveLate"=>false, "suppressLogging"=>false, "xpathController"=>"//*[@id='products_v3_page']", "xpathElement"=>"//*[@id='per_page']", "inner_html"=>"", "text_content"=>"", "reflexController"=>"products", "permanentAttributeName"=>"data-reflex-permanent", "target"=>"products#change_per_page", "args"=>[], "url"=>"http://localhost:3000/admin/products", "tabId"=>"f0739ef9-766e-4b5b-a571-b9b1a5020fa7", "version"=>"3.5.0-rc3", "formData"=>"per_page=25"})c2a367b544f6a7d", api_key: nil, enterprise_limit: 5, locale: "en", disabled_at: nil, show_api_key_view: false, provider: nil, uid: nil, terms_of_service_accepted_at: nil>, @value="Spree::User;1"> gate_name=boolean ]

Most notably: "url"=>"http://localhost:3000/admin/products", which you can see in the screen recording is the URL that I was using in the browser.

[dacook]

But I'll just point out that I think Gaetan's point was very good:

maybe it would be better if this was handled on the client side. I believe there is something along the line of after_reflex event that we could use in a stimulus controller to do such things

I strongly agree, that if we could simply set the URL with Javascript, using the parameters that were sent, it might be a lot more reliable.

[mkllnk]

Looking at the screenshot again its likely that the ProductsReflex#current_url basepath is https://rails whereas the application(?) basepath is https://staging.openfoodnetwork.org.uk. Which is why the replace_state failed and query string absent in the screenshot and causing the defect.

This is interesting and maybe pointing to a configuration bug on our staging and production servers. On our servers, we use nginx as web proxy communicating with Rails via a socket. Here are the relevant config lines:

upstream rails {
     server unix:/home/openfoodnetwork/apps/openfoodnetwork/shared/sock/puma.openfoodnetwork.sock fail_timeout=0;

}

   try_files $uri/index.html $uri @rails;
   location @rails {
     limit_except GET POST PUT PATCH DELETE OPTIONS { deny all; }

     if (-f /etc/nginx/maintenance.html) {
       return 503;
     }

     gzip_proxied no-cache no-store private expired auth;
     proxy_http_version 1.1;
     proxy_set_header X-Real-IP $remote_addr;
     proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
     proxy_set_header Host $host;
     proxy_set_header X-Forwarded-Proto $scheme;
     proxy_set_header X-Request-Start "t=${msec}";
     proxy_redirect off;
     proxy_pass http://rails;
   }

   location /cable {
     limit_except GET POST PUT PATCH DELETE OPTIONS { deny all; }
     proxy_pass http://rails;
     proxy_http_version 1.1;
     proxy_set_header X-Forwarded-Proto https;
     proxy_set_header X-Forwarded-Ssl on;
     proxy_set_header Upgrade $http_upgrade;
     proxy_set_header Connection "upgrade";
   }

Do we need to set the Host header?

[abdulazizali77]

@mkllnk thanks! ill try and replicate on my end with an nginx setup (im unsure whether setting proxy_set_header Host is the correct thing here it could be!) @dacook @rioug regardless of outcome, ill see how i can reimplement hte query string on the frontend instead

[abdulazizali77]

Looking at the screenshot again its likely that the ProductsReflex#current_url basepath is https://rails whereas the application(?) basepath is https://staging.openfoodnetwork.org.uk. Which is why the replace_state failed and query string absent in the screenshot and causing the defect.

This is interesting and maybe pointing to a configuration bug on our staging and production servers. On our servers, we use nginx as web proxy communicating with Rails via a socket. Here are the relevant config lines:

upstream rails {
     server unix:/home/openfoodnetwork/apps/openfoodnetwork/shared/sock/puma.openfoodnetwork.sock fail_timeout=0;

}

   try_files $uri/index.html $uri @rails;
   location @rails {
     limit_except GET POST PUT PATCH DELETE OPTIONS { deny all; }

     if (-f /etc/nginx/maintenance.html) {
       return 503;
     }

     gzip_proxied no-cache no-store private expired auth;
     proxy_http_version 1.1;
     proxy_set_header X-Real-IP $remote_addr;
     proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
     proxy_set_header Host $host;
     proxy_set_header X-Forwarded-Proto $scheme;
     proxy_set_header X-Request-Start "t=${msec}";
     proxy_redirect off;
     proxy_pass http://rails;
   }

   location /cable {
     limit_except GET POST PUT PATCH DELETE OPTIONS { deny all; }
     proxy_pass http://rails;
     proxy_http_version 1.1;
     proxy_set_header X-Forwarded-Proto https;
     proxy_set_header X-Forwarded-Ssl on;
     proxy_set_header Upgrade $http_upgrade;
     proxy_set_header Connection "upgrade";
   }

Do we need to set the Host header?

oh i think its that line proxy_pass http://rails; should it be proxy_pass $host instead?

[mkllnk]

oh i think its that line proxy_pass http://rails; should it be proxy_pass $host instead?

No, with $host we would go into an infinite loop. Nginx receives the request and needs to forward it to the Rails application defined as upstream rails. With $host it would try to forward it to itself.

[mkllnk]

I found this pull request which is supposed to set the URL but I think that the environment variable OFN_URL is never set anywhere. It's empty.

• https://github.com/openfoodfoundation/openfoodnetwork/pull/9804

I also found this post in which they needed the X-Forwarded-For header:

• https://stackoverflow.com/questions/67696655/what-do-i-need-to-do-to-hook-up-actioncable-on-nginx-and-puma

[abdulazizali77]

@mkllnk what are your immediate thoughts, should we rewrite the query_string replace or address this from an nginx/configuration view?

[mkllnk]

I think that addressing the nginx configuration is very important because we have several pages with intermittent bugs pointing to cable ready.

[abdulazizali77]

ok, i will investigate the deployment configurations for admin endpoints first then

[abdulazizali77]

@filipefurtad0 @mkllnk am unsure of this fix as i have yet to test it https://github.com/openfoodfoundation/ofn-install/pull/920 the changes are far reaching or at least all the endpoints which use ActionCable. Would it be possible on your end to test the nginx directive changes on staging? Else i will take a few more days to verify this

[sigmundpetersen]

I think this should stay open with a prod-test to check if the issue is fixed

[RachL]

It looks like we are good here :tada: Let's reopen if we hear anything new