Bug 33786

Summary: ILL requests table pagination in patron ILL history is transposing for different patrons
Product: Koha Reporter: Pedro Amorim <pedro.amorim>
Component: ILLAssignee: Pedro Amorim <pedro.amorim>
Status: RESOLVED FIXED QA Contact:
Severity: major    
Priority: P5 - low CC: fridolin.somers, jonathan.druart, katrin.fischer, martin.renvoize, matt.blenkinsop, nick, paul.derscheid, pedro.amorim, tomascohen
Version: MainKeywords: rel_23_05_candidate
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00,23.05.01,22.11.07
Bug Depends on: 22440    
Bug Blocks: 30719    
Attachments: Bug 33786: set stateSave to false
Bug 33786: ILL requests table id
Bug 33786: ILL requests table id
Bug 33786: ILL requests table id
Bug 33786: ILL requests table id
Bug 33786: ILL requests table id
Bug 33786: ILL requests table id
Bug 33786: ILL requests table id
Bug 33786: ILL requests table id

Description Pedro Amorim 2023-05-19 16:27:56 UTC
Reproduce:

1) Have a borrower with >20 ILL requests in their history
2) Visit cgi-bin/koha/members/ill-requests.pl?borrowernumber=<borrowernumber>
3) On the table, click page 2
4) Visit a different borrower with <20 ILL requests
5) Verify that no requests are shown, this is because the table is using page 2 from step 3)
6) Go back to original borrower, click table page 1
7) Now go back to 2nd borrower, verify is now showing page 1 correctly
Comment 1 Pedro Amorim 2023-05-19 16:54:39 UTC
Created attachment 151487 [details] [review]
Bug 33786: set stateSave to false

This fixes the issue, but I wonder if this is the fix for this, or a better alternative exists
Comment 2 Pedro Amorim 2023-05-22 10:49:31 UTC
Created attachment 151506 [details] [review]
Bug 33786: ILL requests table id

Make sure requests table is unique when visiting patron ILL history so the table state is not shared unintentionally
Comment 3 Pedro Amorim 2023-05-22 10:52:01 UTC
I'm happy with this patch, extra eyes on it are welcome.
Thank you Tomas for your suggestion re making the requests table id unique, it does fix the issue and we can keep the stateSave: true this way.
Test plan is the same as the steps to reproduce the issue, except the issue should not occur anymore after patch is applied.
Comment 4 Pedro Amorim 2023-05-22 11:40:23 UTC
Created attachment 151511 [details] [review]
Bug 33786: ILL requests table id

Make sure requests table is unique when visiting patron ILL history so the table state is not shared unintentionally
Comment 5 Martin Renvoize 2023-05-25 11:24:26 UTC
Created attachment 151670 [details] [review]
Bug 33786: ILL requests table id

Make sure requests table is unique when visiting patron ILL history so the table state is not shared unintentionally

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 6 Jonathan Druart 2023-05-25 11:31:16 UTC
We should instantiate borrower_prefilter_value, the code is confusing as it.
Comment 7 Tomás Cohen Arazi 2023-05-26 12:53:48 UTC
Can we make this happen before wednesday?
Comment 8 Pedro Amorim 2023-05-29 11:19:52 UTC
Created attachment 151773 [details] [review]
Bug 33786: ILL requests table id
Comment 9 Pedro Amorim 2023-05-29 11:21:03 UTC
I've rewritten the patch and tested that all works as intended.
Please review again.
Thanks!
Comment 10 Katrin Fischer 2023-05-29 14:19:43 UTC
Please don't forget to write good commit messages. Problem description and test plan should be included.
Comment 11 Katrin Fischer 2023-05-29 14:32:46 UTC
Created attachment 151776 [details] [review]
Bug 33786: ILL requests table id

Reproduce:

1) Have a borrower with >20 ILL requests in their history
2) Visit cgi-bin/koha/members/ill-requests.pl?borrowernumber=<borrowernumber>
3) On the table, click page 2
4) Visit a different borrower with <20 ILL requests
5) Verify that no requests are shown, this is because the table is using page 2 from step 3)
6) Go back to original borrower, click table page 1
7) Now go back to 2nd borrower, verify is now showing page 1 correctly

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 12 Katrin Fischer 2023-05-29 14:33:58 UTC
Created attachment 151777 [details] [review]
Bug 33786: ILL requests table id

Make sure requests table is unique when visiting patron ILL
history so the table state is not shared unintentionally

Reproduce:
1) Have a borrower with >20 ILL requests in their history
2) Visit cgi-bin/koha/members/ill-requests.pl?borrowernumber=<borrowernumber>
3) On the table, click page 2
4) Visit a different borrower with <20 ILL requests
5) Verify that no requests are shown, this is because the table is using page 2 from step 3)
6) Go back to original borrower, click table page 1
7) Now go back to 2nd borrower, verify is now showing page 1 correctly

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 13 Katrin Fischer 2023-05-29 14:34:30 UTC
Works well, fixed the commit message :)
Comment 14 Jonathan Druart 2023-05-30 07:07:47 UTC
Not blocker, but this:

+    function get_prefilter_value(prefilter_key) {
+        let pre_filter = additional_prefilters.find(e => e.key === prefilter_key);
+        return additional_prefilters.length != 0 && typeof pre_filter !== "undefined" ? pre_filter['value'] : null;
+    }


reads odd.

We should not need 'additional_prefilters.length != 0'.

Also I am wondering how this can be written nicely, Paul maybe?
Comment 15 Pedro Amorim 2023-05-30 09:22:41 UTC
Created attachment 151791 [details] [review]
Bug 33786: ILL requests table id

Make sure requests table is unique when visiting patron ILL
history so the table state is not shared unintentionally

Reproduce:
1) Have a borrower with >20 ILL requests in their history
2) Visit cgi-bin/koha/members/ill-requests.pl?borrowernumber=<borrowernumber>
3) On the table, click page 2
4) Visit a different borrower with <20 ILL requests
5) Verify that no requests are shown, this is because the table is using page 2 from step 3)
6) Go back to original borrower, click table page 1
7) Now go back to 2nd borrower, verify is now showing page 1 correctly

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 16 Pedro Amorim 2023-05-30 09:25:05 UTC
I've simplified the patch (I believe).
Some of the abstraction that was there was preparing for bug 30719 (because there will be a batch_if prefilter on top of the borrowernumber prefilter, but batch_id prefilter will make use of 'placed' and 'updated' filters, whereas borrowernumber does not (patron history page does not have left side filters with date inputs).
Comment 17 Tomás Cohen Arazi 2023-06-01 18:37:25 UTC
This is missing a signature.
Comment 18 Martin Renvoize 2023-06-06 07:09:34 UTC
Created attachment 152036 [details] [review]
Bug 33786: ILL requests table id

Make sure requests table is unique when visiting patron ILL
history so the table state is not shared unintentionally

Reproduce:
1) Have a borrower with >20 ILL requests in their history
2) Visit cgi-bin/koha/members/ill-requests.pl?borrowernumber=<borrowernumber>
3) On the table, click page 2
4) Visit a different borrower with <20 ILL requests
5) Verify that no requests are shown, this is because the table is using page 2 from step 3)
6) Go back to original borrower, click table page 1
7) Now go back to 2nd borrower, verify is now showing page 1 correctly

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 19 Martin Renvoize 2023-06-06 07:10:05 UTC
This is an important bugfix, all working as described and code is clean.  Also passing QA scripts.

Passing QA
Comment 20 Tomás Cohen Arazi 2023-06-06 13:01:14 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 21 Matt Blenkinsop 2023-06-12 10:33:11 UTC
Can this be pushed to 23.05? Its landed in the queue for 22.11.07 but doesn't appear to be in stable yet
Comment 22 Matt Blenkinsop 2023-06-12 10:56:06 UTC
Updating to 'Pushed to stable' and adding released version before oldstable rmaint activity
Comment 23 Matt Blenkinsop 2023-06-12 10:58:18 UTC
Nice work everyone!

Pushed to oldstable for 22.11.x