openfoodfacts / openfoodfacts-server

Open Food Facts database, API server and web interface - 🐪🦋 Perl, CSS and JS coders welcome 😊 For helping in Python, see Robotoff or taxonomy-editor
GNU Affero General Public License v3.0
633 stars 371 forks source link

Sort by button in search results does not work #9392

Open stephanegigandet opened 9 months ago

stephanegigandet commented 9 months ago

What

Resulting url has 2 sort_by fields, with the last one with a ? instead of & :

http://world.openfoodfacts.localhost/cgi/search.pl?action=process&search_terms=coca&sort_by=unique_scans_n&page_size=24?sort_by=last_modified_t

image

Reported by Olaf Görlitz on Slack: https://openfoodfacts.slack.com/archives/CUZK65DGE/p1700749241876539 (includes video)

Part of

himanshisrestha commented 5 months ago

Can I get to know more about this issue

anaritadauane commented 5 months ago

@himanshisrestha Have you looked at the video?

himanshisrestha commented 5 months ago

Hi @anaritadauane , yes I've looked at the video. Thank you

himanshisrestha commented 5 months ago

@stephanegigandet , I've refered to the code and found that the related code is in Display.pm around here: `if ((defined $options{product_type}) and ($options{product_type} eq "food")) {

    push @{$template_data_ref->{sort_options}},
        {
        value => "popularity",
        link => $request_ref->{current_link} . "?sort_by=popularity",
        name => lang("sort_by_popularity")
        };
    push @{$template_data_ref->{sort_options}},`

and this causes the url to contain the '?' instead of '&' Could you give me some more idea about this to proceed on it

stephanegigandet commented 5 months ago

@himanshisrestha Instead of generating an url like https://world.openfoodfacts.org/cgi/search.pl?action=process&search_terms=test&sort_by=unique_scans_n&page_size=24?sort_by=created_t we want to generate an URL like https://world.openfoodfacts.org/cgi/search.pl?action=process&search_terms=test&sort_by=created_t&page_size=24 when the sort button is used on the search results page.

Of course for other pages like the home page, we still want to have URL that work with the search button

himanshisrestha commented 5 months ago
Screenshot 2024-03-24 at 8 41 31 PM Screenshot 2024-03-24 at 8 41 34 PM

@stephanegigandet I we changed the code here and in effect

Could you review this changes and is it that I should make some changes in the test script files (.t) for this ?

alexgarel commented 5 months ago

@himanshisrestha I think your change is fine, also did you verify it does not break

This way of adding parameter is in my opinion not quite good (we should instead collect parameters in a dict and at the end create the corresponding url parameters) but it would be maybe beyond the scope of this fix.

Maybe adding a method in Web.pm like add_params($url, $params) where you verify if you already have a '?' to decide wether to concatenate $params with a '?' or a '&' would be great and easy to add. (and we could use it ubiquitously)

Regarding tests you could complete current integration test to verify if the links to sort the page as expected are present in page HTML.