Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: sort_by param issue in the url with "?" instead of "&" #10024

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

fix: sort_by param issue in the url with "?" instead of "&" #10024

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a0847ea
fix: sort_by param issue in the url
himanshisrestha Mar 25, 2024
9089d8d
fix: sort_by add_params added to search.pl
himanshisrestha Mar 26, 2024
ba45c6a
Merge branch 'main' into fix-sort-by-issue
hangy Mar 30, 2024
ab2072d
Merge branch 'main' into fix-sort-by-issue
himanshisrestha Apr 20, 2024
84cd70d
Merge branch 'main' into fix-sort-by-issue
teolemon May 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions cgi/search.pl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
use ProductOpener::PackagerCodes qw/:all/;
use ProductOpener::Text qw/:all/;
use ProductOpener::Lang qw/:all/;
use ProductOpener::Web qw/:all/;
hangy marked this conversation as resolved.
Show resolved Hide resolved

use CGI qw/:cgi :form escapeHTML/;
use URI::Escape::XS;
Expand Down Expand Up @@ -665,11 +666,13 @@
}
}

if (defined $sort_by) {
$current_link .= "&sort_by=$sort_by";
if (defined $sort_by && $current_link !~ /[?&]sort_by=/) {
add_params($current_link, "sort_by=$sort_by");
}

$current_link .= "\&page_size=$limit";
if (defined $limit && $current_link !~ /[?&]page_size=/) {
add_params($current_link, "page_size=$limit");
}

# Graphs

Expand Down
10 changes: 5 additions & 5 deletions lib/ProductOpener/Display.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5002,13 +5002,13 @@ sub search_and_display_products ($request_ref, $query_ref, $sort_by, $limit, $pa
push @{$template_data_ref->{sort_options}},
{
value => "popularity",
link => $request_ref->{current_link} . "?sort_by=popularity",
link => add_params($request_ref->{current_link}, "sort_by=popularity") . "&page_size=$limit",
name => lang("sort_by_popularity")
};
push @{$template_data_ref->{sort_options}},
{
value => "nutriscore_score",
link => $request_ref->{current_link} . "?sort_by=nutriscore_score",
link => add_params($request_ref->{current_link}, "sort_by=nutriscore_score") . "&page_size=$limit",
name => lang("sort_by_nutriscore_score")
};

Expand All @@ -5017,7 +5017,7 @@ sub search_and_display_products ($request_ref, $query_ref, $sort_by, $limit, $pa
push @{$template_data_ref->{sort_options}},
{
value => "ecoscore_score",
link => $request_ref->{current_link} . "?sort_by=ecoscore_score",
link => add_params($request_ref->{current_link}, "sort_by=ecoscore_score") . "&page_size=$limit",
name => lang("sort_by_ecoscore_score")
};
}
Expand All @@ -5026,13 +5026,13 @@ sub search_and_display_products ($request_ref, $query_ref, $sort_by, $limit, $pa
push @{$template_data_ref->{sort_options}},
{
value => "created_t",
link => $request_ref->{current_link} . "?sort_by=created_t",
link => add_params($request_ref->{current_link}, "sort_by=created_t") . "&page_size=$limit",
name => lang("sort_by_created_t")
};
push @{$template_data_ref->{sort_options}},
{
value => "last_modified_t",
link => $request_ref->{current_link} . "?sort_by=last_modified_t",
link => add_params($request_ref->{current_link}, "sort_by=last_modified_t") . "&page_size=$limit",
name => lang("sort_by_last_modified_t")
};

Expand Down
32 changes: 31 additions & 1 deletion lib/ProductOpener/Web.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,8 @@ BEGIN {
&display_knowledge_panel
&get_languages_options_list
&get_countries_options_list
); #the fucntions which are called outside this file
&add_params
); #the functions which are called outside this file
%EXPORT_TAGS = (all => [@EXPORT_OK]);
}

Expand Down Expand Up @@ -378,4 +379,33 @@ sub get_countries_options_list ($target_lc, $exclude_world = 1) {
return \@countries_list;
}

=head2 add_params( $url, $params )

Adds parameters to a URL while properly handling the existing query string.

=head3 Arguments

=head4 $url - The URL to which parameters need to be added.

=head4 $params - The parameters to be added in the format "key=value".

=head3 Return value

The modified URL with added parameters.

=cut

sub add_params ($url, $params) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you put a more specific name, like add_params_to_link?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@himanshisrestha we can even consider having

sub add_params_to_link ($url, @params) {

@params is a list of parameters you want to add.

This enables calling the function using:

add_params_to_link($my_url, "test=foo", "othe=bar"); with as many parameters as we want.


# Check if the URL already contains parameters
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to check if the URL already contains the same parameter we want to add, in which case we want to remove the old one before adding the new one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to add unit tests for this function with various cases: param already there, other params, no other params etc.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to check if the URL already contains the same parameter we want to add, in which case we want to remove the old one before adding the new one.

Do we always explicitly limit it to one value when reading the parameters? cgi can do arrays

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stephanegigandet where should the test cases be written .. Is it in a file in tests/unit or should I create a .t file there for the same ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can create a tests/unit/web.t file.

if ($url =~ /\?/) {
$url .= "&$params"; # Append using '&' if parameters already exist
}
else {
$url .= "?$params"; # Append using '?' if no parameters exist
}

return $url;
}

1;
Loading