fix: sort_by param issue in the url with "?" instead of "&" #10024
Conversation
cgi/search.pl
Outdated
| # if (defined $sort_by) { | ||
| # $current_link .= "&sort_by=$sort_by"; | ||
| # } | ||
|
|
||
| $current_link .= "\&page_size=$limit"; | ||
| # $current_link .= "\&page_size=$limit"; |
There was a problem hiding this comment.
I'm not sure we want to remove it because we may let user sort on whatever they want through the API.
There was a problem hiding this comment.
In search.pl we also have different sort possibilities.
So I would say: use add_param here also, and maybe verify first that sort_by or page_size is not already in current_link.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10024 +/- ##
==========================================
- Coverage 49.54% 48.59% -0.96%
==========================================
Files 67 71 +4
Lines 20650 20987 +337
Branches 4980 5029 +49
==========================================
- Hits 10231 10198 -33
- Misses 9131 9514 +383
+ Partials 1288 1275 -13 ☔ View full report in Codecov by Sentry. |
|
@himanshisrestha the code looks good per se, but there seems to be an error. Try http://world.openfoodfacts.localhost/cgi/search.pl and look at the permalink It should contains the sort_by, it does not have it (while it's working on staging |
|
@alexgarel I've tried to replicate but I'vent seen what you mentioned. and the local host is here: |
|
|
||
| =cut | ||
|
|
||
| sub add_params ($url, $params) { |
There was a problem hiding this comment.
Can you put a more specific name, like add_params_to_link?
|
|
||
| sub add_params ($url, $params) { | ||
|
|
||
| # Check if the URL already contains parameters |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
It would be good to add unit tests for this function with various cases: param already there, other params, no other params etc.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@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 ?
There was a problem hiding this comment.
You can create a tests/unit/web.t file.
|
|
||
| =cut | ||
|
|
||
| sub add_params ($url, $params) { |
There was a problem hiding this comment.
@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.
|
@himanshisrestha we would be glad if you can finish this PR ! |
|
@alexgarel Sure I'm working on it !! 😄 |
|
What
After searching for a product, and then applying a sort_by filter, the resulting url contained 2 sort_by params and "?" instead of "&"
A new subroutine add_params($url, $param) was added to the Web.pm which checks whether we have a "?" in the current url and if yes then concatenates the sort_by param with "&" else with "?"
The urls now looks like this:
http://world.openfoodfacts.localhost/?sort_by=nutriscore_score&page_size=100
http://world.openfoodfacts.localhost/cgi/search.pl?action=process&search_terms=sugar&sort_by=nutriscore_score&page_size=24
Related issue(s) and discussion