Bug 33786 - ILL requests table pagination in patron ILL history is transposing for different patrons
Summary: ILL requests table pagination in patron ILL history is transposing for differ...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: ILL (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Pedro Amorim
QA Contact:
URL:
Keywords: rel_23_05_candidate
Depends on: 22440
Blocks: 30719
  Show dependency treegraph
 
Reported: 2023-05-19 16:27 UTC by Pedro Amorim
Modified: 2023-12-28 20:47 UTC (History)
9 users (show)

See Also:
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


Attachments
Bug 33786: set stateSave to false (1009 bytes, patch)
2023-05-19 16:54 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 33786: ILL requests table id (2.74 KB, patch)
2023-05-22 10:49 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 33786: ILL requests table id (2.74 KB, patch)
2023-05-22 11:40 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 33786: ILL requests table id (2.80 KB, patch)
2023-05-25 11:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33786: ILL requests table id (3.56 KB, patch)
2023-05-29 11:19 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 33786: ILL requests table id (4.05 KB, patch)
2023-05-29 14:32 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 33786: ILL requests table id (4.17 KB, patch)
2023-05-29 14:33 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 33786: ILL requests table id (4.98 KB, patch)
2023-05-30 09:22 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 33786: ILL requests table id (5.04 KB, patch)
2023-06-06 07:09 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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