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
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
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
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.
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
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>
We should instantiate borrower_prefilter_value, the code is confusing as it.
Can we make this happen before wednesday?
Created attachment 151773 [details] [review] Bug 33786: ILL requests table id
I've rewritten the patch and tested that all works as intended. Please review again. Thanks!
Please don't forget to write good commit messages. Problem description and test plan should be included.
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>
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>
Works well, fixed the commit message :)
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?
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>
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).
This is missing a signature.
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>
This is an important bugfix, all working as described and code is clean. Also passing QA scripts. Passing QA
Pushed to master for 23.11. Nice work everyone, thanks!
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
Updating to 'Pushed to stable' and adding released version before oldstable rmaint activity
Nice work everyone! Pushed to oldstable for 22.11.x