Bug 21460

Summary: Filtering ILL requests on borrowernumber does not work
Product: Koha Reporter: Magnus Enger <magnus>
Component: ILLAssignee: Andrew Isherwood <bugzilla>
Status: CLOSED FIXED QA Contact: Josef Moravec <josef.moravec>
Severity: normal    
Priority: P5 - low CC: jonathan.druart, josef.moravec, m.de.rooy, martin.renvoize, nick
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18589
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
19.05.00
Attachments: Bug 21460: Fix patron ILL request filtering
Bug 21460: Fix patron ILL request filtering
Bug 21460: Add tests
Bug 21460: Simplify the code to have only 1 assignment
Bug 21460: (follow-up) Add filter to prefilters
Bug 21460: (follow-up) $raw -> html filter
DO NOT PUSH - example of building filters for uri parameters
Bug 21460: (follow-up) Filter params in .pl
Bug 21460: Simplify the code to have only 1 assignment
Bug 21460: (follow-up) Filter params in .pl
But 21460: (follow-up) Fix bugs found in QA
Bug 21460: Simplify the code to have only 1 assignment
Bug 21460: (follow-up) Filter params in .pl
Bug 21460: (follow-up) Fix bugs found in QA

Description Magnus Enger 2018-10-01 09:43:01 UTC
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.
Comment 1 Owen Leonard 2018-11-20 16:22:32 UTC
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?
Comment 2 Magnus Enger 2018-12-05 08:38:23 UTC
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.
Comment 3 Andrew Isherwood 2018-12-05 10:47:31 UTC Comment hidden (obsolete)
Comment 4 Magnus Enger 2018-12-12 14:01:53 UTC
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.
Comment 5 Jonathan Druart 2018-12-13 20:06:23 UTC
Created attachment 83184 [details] [review]
Bug 21460: Add tests
Comment 6 Jonathan Druart 2018-12-13 20:06:41 UTC
Created attachment 83185 [details] [review]
Bug 21460: Simplify the code to have only 1 assignment
Comment 7 Jonathan Druart 2018-12-13 20:08:13 UTC
Should not we teach the api definition that there is a new parameter?
Comment 8 Andrew Isherwood 2018-12-14 13:49:55 UTC
(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...
Comment 9 Andrew Isherwood 2018-12-14 14:14:51 UTC
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.
Comment 10 Andrew Isherwood 2018-12-14 14:48:10 UTC
Bug added for the extraneous parameters in the definition: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22000
Comment 11 Jonathan Druart 2018-12-16 15:48:02 UTC
 FAIL   koha-tmpl/intranet-tmpl/prog/en/modules/ill/ill-requests.tt
   FAIL   filters
                missing_filter at line 40 (        var prefilters = '[% prefilters %]';)
Comment 12 Andrew Isherwood 2018-12-17 09:42:56 UTC
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
Comment 13 Jonathan Druart 2018-12-17 15:30:02 UTC
(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?
Comment 14 Andrew Isherwood 2018-12-17 16:03:35 UTC
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...
Comment 15 Andrew Isherwood 2018-12-17 16:04:38 UTC
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.
Comment 16 Jonathan Druart 2018-12-19 18:41:14 UTC
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.
Comment 17 Andrew Isherwood 2018-12-20 11:41:27 UTC
(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?
Comment 18 Jonathan Druart 2018-12-20 22:25:03 UTC
Created attachment 83435 [details] [review]
DO NOT PUSH - example of building filters for uri parameters
Comment 19 Jonathan Druart 2018-12-20 22:26:42 UTC
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 :)
Comment 20 Andrew Isherwood 2019-01-04 12:20:32 UTC
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.
Comment 21 Andrew Isherwood 2019-01-07 09:35:56 UTC
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)
Comment 22 Josef Moravec 2019-02-28 12:41:57 UTC
Could you rebase please?
Comment 23 Andrew Isherwood 2019-02-28 13:55:09 UTC
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
Comment 24 Owen Leonard 2019-04-22 13:40:12 UTC
Bug 18589 is fixed, so this should be ready for a rebase.
Comment 25 Andrew Isherwood 2019-04-23 08:24:57 UTC
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.
Comment 26 Andrew Isherwood 2019-04-23 09:20:32 UTC
Created attachment 88444 [details] [review]
Bug 21460: Simplify the code to have only 1 assignment
Comment 27 Andrew Isherwood 2019-04-23 09:20:35 UTC
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)
Comment 28 Andrew Isherwood 2019-04-23 09:23:42 UTC
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
Comment 29 Josef Moravec 2019-04-24 13:54:31 UTC
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 %]';)
Comment 30 Andrew Isherwood 2019-04-24 13:59:51 UTC
Created attachment 88626 [details] [review]
But 21460: (follow-up) Fix bugs found in QA

Fix silly bugs mentioned in comment #29
Comment 31 Andrew Isherwood 2019-04-24 14:00:48 UTC
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.
Comment 32 Josef Moravec 2019-04-25 05:59:40 UTC
Created attachment 88679 [details] [review]
Bug 21460: Simplify the code to have only 1 assignment

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 33 Josef Moravec 2019-04-25 05:59:43 UTC
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>
Comment 34 Josef Moravec 2019-04-25 05:59:47 UTC
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>
Comment 35 Nick Clemens 2019-04-25 11:52:53 UTC
Awesome work all!

Pushed to master for 19.05
Comment 36 Martin Renvoize 2019-04-26 15:26:24 UTC
Bug with enhancement not in 18.11.x series.