To reproduce, start on detail view for a patron, e.g. /cgi-bin/koha/members/moremember.pl?borrowernumber=5 Click on the "Interlibrary loans" tab on the left of the screen. This takes you to /cgi-bin/koha/ill/ill-requests.pl?borrowernumber=5, which displays all ILL requests, not just the ones for borrowernumber 5.
Part of Bug 7317 (06f9e5fe) adds code to convert GET parameters to template variables ("prefilters"), presumably to pass these values to the DataTables call. But I don't see corresponding changes to the DataTables configuration. Is this an unfinished feature or is there some code which got lost somewhere?
This is fixed by bug 18589, according to Andrew. But since that enh did not make it into 18.11.x, the problem described here is still a bug in 18.11.x that needs to be fixed.
Created attachment 82858 [details] [review] Bug 21460: Fix patron ILL request filtering This patch fixes the non-functioning filtering of ILL requests by borrowernumber. As Magnus mentioned, this fix was originally created as part of a larger peice in bug 18589. So the relevant parts of that bug have been backported here. Important: As such, once this bug is in master, I will need to remove the fix from bug 18589 as, due to the other things that bug does, it will conflict. Test plan: 1) Test on an instance that has ILL requests from multiple borrowers 2) Navigate to a borrower's profile page 3) Click the "Interlibrary loans" link 4) TEST => Note that the requests displayed only apply to that user 5) Navigate to the main ILL requests list page 6) TEST => Note that all requests for all users are displayed
Created attachment 83119 [details] [review] Bug 21460: Fix patron ILL request filtering This patch fixes the non-functioning filtering of ILL requests by borrowernumber. As Magnus mentioned, this fix was originally created as part of a larger peice in bug 18589. So the relevant parts of that bug have been backported here. Important: As such, once this bug is in master, I will need to remove the fix from bug 18589 as, due to the other things that bug does, it will conflict. Test plan: 1) Test on an instance that has ILL requests from multiple borrowers 2) Navigate to a borrower's profile page 3) Click the "Interlibrary loans" link 4) TEST => Note that the requests displayed only apply to that user 5) Navigate to the main ILL requests list page 6) TEST => Note that all requests for all users are displayed Signed-off-by: Magnus Enger <magnus@libriotech.no> Works as advertised.
Created attachment 83184 [details] [review] Bug 21460: Add tests
Created attachment 83185 [details] [review] Bug 21460: Simplify the code to have only 1 assignment
Should not we teach the api definition that there is a new parameter?
(In reply to Jonathan Druart from comment #7) > Should not we teach the api definition that there is a new parameter? Yes, we should. Patch incoming...
OK, well, now I'm really confused. Looking in api/v1/swagger/paths/illrequests.json (https://github.com/PTFS-Europe/koha/blob/master/api/v1/swagger/paths/illrequests.json#L7-L95), the "parameters" object has a bunch of possible parameters, borrowernumber being one of them. Which explains why adding this parameter without modifying the spec didn't break things. However, with the exception of "embed" and "borrowernumber" there's no handling in the API endpoint controller for these parameters. It looks like the majority have been in the spec since the ILL work was originally done, which suggests to me a misunderstanding of how the spec works. So, in short, we don't need to add "borrowernumber" to the spec since it's already there, but we should consider removing the extraneous parameters, though that feels like it's outside the scope of this bug.
Bug added for the extraneous parameters in the definition: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22000
FAIL koha-tmpl/intranet-tmpl/prog/en/modules/ill/ill-requests.tt FAIL filters missing_filter at line 40 ( var prefilters = '[% prefilters %]';)
Created attachment 83272 [details] [review] Bug 21460: (follow-up) Add filter to prefilters Addressing https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21460#c11
(In reply to Andrew Isherwood from comment #12) > Created attachment 83272 [details] [review] [review] > Bug 21460: (follow-up) Add filter to prefilters > > Addressing > https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21460#c11 Hi Andrew, Are you sure about this one? Can you explain why raw?
Hi Jonathan, nope, you're absolutely right, this shouldn't be a $raw. These prefilters are supplied by the client via a URL param, they should most certainly be escaped if appropriate. Patch incoming...
Created attachment 83305 [details] [review] Bug 21460: (follow-up) $raw -> html filter We're filtering a string that is derived from client supplied paramters via a URL, we should be escaping these if appropriate.
You will be stuck, you will not manage to filter all situations correctly. You should either escape in the pl, or split the $var=$value into a structure { var => $var, value => $value } (like how it was before basically), then build the string in the template. Ping me on IRC if you need help.
(In reply to Jonathan Druart from comment #16) > You will be stuck, you will not manage to filter all situations correctly. I'm not clear on how this could be problematic. Whether I escape the values in the .pl or the entire key/value string in the template, I don't think it would make any difference. Looking at the html filter docs: http://www.template-toolkit.org/docs/manual/Filters.html#section_html All it does is escape characters that could be parsed as HTML into their escape sequences. Does it make any difference whether we escape as a string in the template or as a data structure in the .pl?
Created attachment 83435 [details] [review] DO NOT PUSH - example of building filters for uri parameters
Andrew, if it is not clear for you I invite you to apply this (silly) patch. Hit the mainpage and click on the different links it adds. Check the logs and see the content of the variable "a_var", then understand what is happening :)
Thanks for taking the time to post that patch Jonathan, it makes for an interesting experiment! I'll create a patch that will iterate the and escape the prefilters in the .pl then constructs the string in the template.
Created attachment 83674 [details] [review] Bug 21460: (follow-up) Filter params in .pl This patch filters the passed parameters in the .pl, rather than doing it in the template. As per comment 16 (https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21460#c16)
Could you rebase please?
I actually think this bug should depend on 18589 rather than 18589 depend on it. 18589 is at the top of the current QA tree and it makes sense for anything ILL related to be based on it since there are still some significant changes in the pending QA queue that will conflict badly with anything that touches ill-requests.tt. I'll change the dependency accordingly and we can revisit this once 18589 is PQA
Bug 18589 is fixed, so this should be ready for a rebase.
The problem described in this bug appears to have been fixed in Bug 18589 as I can no longer replicate it. The fix implemented in 18589 seems to have been largely the same as what was in this bug's patches. However some of the small enhancement patches are not included. So I am going to implement those and then upload a single patch to this bug which includes them, and obsolete all the existing patches on this bug. It should then be possible to carry out the test plan and sign off this bug.
Created attachment 88444 [details] [review] Bug 21460: Simplify the code to have only 1 assignment
Created attachment 88445 [details] [review] Bug 21460: (follow-up) Filter params in .pl This patch filters the passed parameters in the .pl, rather than doing it in the template. As per comment 16 (https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21460#c16)
The remaining two patches are re-implementations of the enhancements from this bug that hadn't already made it into master as part of Bug 18589. This bug should now apply. Test plan: 1) Test on an instance that has ILL requests from multiple borrowers 2) Navigate to a borrower's profile page 3) Click the "Interlibrary loans" link 4) TEST => Note that the requests displayed only apply to that user 5) Navigate to the main ILL requests list page 6) TEST => Note that all requests for all users are displayed
Sorry, not perfect yet: FAIL Koha/REST/V1/Illrequests.pm FAIL valid Can't use bareword ("args") as a HASH ref while "strict refs" in use FAIL ill/ill-requests.pl FAIL valid Global symbol "@tpl_arr" requires explicit package name (did you forget to declare "my @tpl_arr"?) ill/ill-requests.pl had compilation errors. FAIL koha-tmpl/intranet-tmpl/prog/en/modules/ill/ill-requests.tt FAIL filters missing_filter at line 530 ( var prefilters = '[% prefilters %]';)
Created attachment 88626 [details] [review] But 21460: (follow-up) Fix bugs found in QA Fix silly bugs mentioned in comment #29
Sorry Josef, those are really silly bugs and I'd have expected them to break everything when I was testing it yesterday, but it didn't! I am very confused. Anyway, sorry for wasting your time.
Created attachment 88679 [details] [review] Bug 21460: Simplify the code to have only 1 assignment Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 88680 [details] [review] Bug 21460: (follow-up) Filter params in .pl This patch filters the passed parameters in the .pl, rather than doing it in the template. As per comment 16 (https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21460#c16) Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 88681 [details] [review] Bug 21460: (follow-up) Fix bugs found in QA Fix silly bugs mentioned in comment #29 Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Awesome work all! Pushed to master for 19.05
Bug with enhancement not in 18.11.x series.