Description
Aleisha Amohia
2020-01-23 03:58:29 UTC
Created attachment 97773 [details] [review] Bug 24488: Show correct first patron details on Holds to pull This patch rewrites the complex search query to reduce the risk of reordering the results multiple times. This patch includes a change to schema files so may need to upgrade schema and/or restart memcached To test: 1) Reproduce problem following test plan in Description 2) Apply patch and refresh page 3) Notice the correct patron is now shown 4) Play with date selection, confirm correct results are still shown 5) Test cancelling holds 6) Test filtering table results Sponsored by: IHC New Zealand Aleisha- something odd- nothing is showing in the Holds To Pull report. I have tried this multiple times and unable to get the holds to populate. Both biblio and Item level holds. (In reply to Kelly McElligott from comment #2) > Aleisha- something odd- nothing is showing in the Holds To Pull report. I > have tried this multiple times and unable to get the holds to populate. Both > biblio and Item level holds. Hi Kelly, is this before or after you apply the patch? If after, did you try upgrading the schema files and restarting memcached? Hi Aleisha, I have done what you have recommended. I am now getting an error when I try to go to the Holds to Pull report DBIx::Class::Storage::DBI::_dbh_execute(): 'koha_hold.reserve.reserve_id' isn't in GROUP BY at /kohadevbox/koha/Koha/Objects.pm line 130 at /usr/share/perl5/DBIx/Class/Exception.pm line 77 (In reply to Kelly McElligott from comment #4) > Hi Aleisha, > I have done what you have recommended. I am now getting an error when I try > to go to the Holds to Pull report > > DBIx::Class::Storage::DBI::_dbh_execute(): 'koha_hold.reserve.reserve_id' > isn't in GROUP BY at /kohadevbox/koha/Koha/Objects.pm line 130 > at /usr/share/perl5/DBIx/Class/Exception.pm line 77 Hi Kelly, I can't reproduce this error. There might be a problem in your database? Sorry! Hi Aleisha, I think this is related to bug 21941/bug 17258 - a DBMS version problem. Can you please check? (In reply to Katrin Fischer from comment #6) > Hi Aleisha, I think this is related to bug 21941/bug 17258 - a DBMS version > problem. Can you please check? Hi Katrin, sorry I'm not really sure how I'm meant to check? It could be related, but also that query is pretty horrible and I think this fix is nicer code to work with in the long term. Setting back to Needs signoff because I'm not getting the aforementioned error, and wonder if it happened when the reservedate was manually changed in the database? Regardless, patch still works for me. > DBIx::Class::Storage::DBI::_dbh_execute(): 'koha_hold.reserve.reserve_id'
> isn't in GROUP BY at /kohadevbox/koha/Koha/Objects.pm line 130
> at /usr/share/perl5/DBIx/Class/Exception.pm line 77
Issues with GROUP BY usually point to problems with stricter DB settings - that's what I meant. It's possible the tester had a different DBMS version / different settings which made it explode - trying to help because you said you cannot reproduce.
To summarise.. try setting `<strict_sql_modes>1</strict_sql_modes>` in your koha-conf.xml Aleisha.. it's on by default in sandboxes and dev environments to weed out queries that would fail we strict features enabled at the SQL level. Looking at the error Kelly is reporting it sounds like strict SQL is enabled and she's catching an error in your reworked SQL query that you're not seeing with sql in 'forgiving' mode ;). I've not looked at the code yet, but this usually arrises from a group by on a query with a join where you're trying to group by an id field in on table.. the fix, as horrible as it is, is to add all fields in that table to the group by. I've misunderstood here - I thought that Katrin's comment was talking about the potential cause of the original bug reported, not in relation to reproducing Kelly's error. Also I'm more than happy to write tests. I had simply forgotten in the first instance. Will take another look when I get a chance. Kelly's error exists with and without the patch (if strict_sql_modes is set). (In reply to Jonathan Druart from comment #11) > Kelly's error exists with and without the patch (if strict_sql_modes is set). Well, at least without the patch, I did not try with the patch applied. *** Bug 24301 has been marked as a duplicate of this bug. *** Hello Aleisha, is this patch ready to test? I applied patch, but name of first patron was same after change of holds order as before. Tested on Kohadevbox after restart_all. Hi Michal, not ready to test yet - needs some work on master and some tests. OK, I'll wait on "Needs signoff" status :-) But I think that similar bug has Holds queue page. Created attachment 106379 [details] [review] Bug 24488: Show correct first patron details on Holds to pull This patch rewrites the complex search query to reduce the risk of reordering the results multiple times. This patch includes a change to schema files so may need to upgrade schema and/or restart memcached To test: 1) Reproduce problem following test plan in Description 2) Apply patch and refresh page 3) Notice the correct patron is now shown 4) Play with date selection, confirm correct results are still shown 5) Test cancelling holds 6) Test filtering table results 7) Test with biblios with multiple items 8) Test with making items unavailable (i.e. not for loan, checked out) Sponsored by: IHC New Zealand Just rebasing - still need to try with strict_sql_mode. Will come back to this tomorrow. Yes it's failing with strict mode: 'koha_kohadev.reserve.reserve_id' isn't in GROUP BY Aleisa, if you could compare the performance on a production DB that would be really useful. Created attachment 106882 [details] [review] Bug 24488: Show correct first patron details on Holds to pull This patch rewrites the complex search query to reduce the risk of reordering the results multiple times. This patch includes a change to schema files so may need to upgrade schema and/or restart memcached To test: 1) Reproduce problem following test plan in Description 2) Apply patch and refresh page 3) Notice the correct patron is now shown 4) Play with date selection, confirm correct results are still shown 5) Test cancelling holds 6) Test filtering table results 7) Test with biblios with multiple items 8) Test with making items unavailable (i.e. not for loan, checked out) Sponsored by: IHC New Zealand I enabled strict_sql_modes but unfortunately I still can't reproduce the errors. Can someone give me some instructions for this? I'm happy to keep working on it. Actually I think we have a bigger problem here: I was confusing holds to pull and holds queue earlier. Now testing another patch... Holds to pull fails for me with and without this patch in strict mode (error on bug 26020) Hi Aleisha, I'm so sorry but this patch didn't solve problem with first patron on my kohadevbox. If I change order of borrower with hold, list without change. I've problems with 'dbic' on kohadevbox, so I tried make schema change manually by !sudo koha-upgrade-schema kohadev'. The patch applies for me - did you mean to set Failed QA? Hi Katrin, yes,QA faild for me. Thank you. I saw on IRC, that you fight with 'dbic' too. Did you solve it? (In reply to Michal Denar from comment #25) > Hi Katrin, > yes,QA faild for me. Thank you. I saw on IRC, that you fight with 'dbic' > too. Did you solve it? No, I ended up running the normal command in koha-shell - so doing it manually instead of taking the shortcut. (In reply to Katrin Fischer from comment #22) > Actually I think we have a bigger problem here: I was confusing holds to > pull and holds queue earlier. Now testing another patch... > > Holds to pull fails for me with and without this patch in strict mode (error > on bug 26020) Thanks for pointing this out and reporting it, Katrin. I still can't seem to enable strict mode, can you please tell me how to do this? I suppose we want to fix the bug on Bug 26020 before getting this patch through (in fact fixing Bug 26020 might fix this problem) so I'll set 26020 as blocking this bug. Hi Aleiha, all I do is changing the setting in the koha-conf.xml file and restarted all the things including the mysql server. Not sure if that's needed. I later turend it off again so I coudl continue testing the other patch and get around the bug. (In reply to Katrin Fischer from comment #28) > Hi Aleiha, all I do is changing the setting in the koha-conf.xml file and > restarted all the things including the mysql server. Not sure if that's > needed. I later turend it off again so I coudl continue testing the other > patch and get around the bug. Oh, and your patch didn't fix it for me - still showing the same problem with the group by. (it's not changed, I think) + itemnumber => { -not_in => \'SELECT itemnumber FROM branchtransfers WHERE datearrived IS NULL', -not_in => \'SELECT itemnumber FROM issues' }, There are twice the same key in the hash "-not_in", only the second one will be used. As you are reusing this set in different ->search I think you should retrieve them once to have the itemnumber list in a perl array. Created attachment 107206 [details] [review] Bug 24488: Show correct first patron details on Holds to pull This patch rewrites the complex search query to reduce the risk of reordering the results multiple times. This patch includes a change to schema files so may need to upgrade schema and/or restart memcached To test: 1) Reproduce problem following test plan in Description 2) Apply patch and refresh page 3) Notice the correct patron is now shown 4) Play with date selection, confirm correct results are still shown 5) Test cancelling holds 6) Test filtering table results 7) Test with biblios with multiple items 8) Test with making items unavailable (i.e. not for loan, checked out) Sponsored by: IHC New Zealand Sorry team I really cannot seem to enable strict_sql_modes and reproduce that error, I tried a bunch of different things including MariaDB specific things but I couldn't reproduce. I've reworked the patch a bit, and really curious if it stops that error from showing. Please test and let me know how it goes. (In reply to Aleisha Amohia from comment #27) > (In reply to Katrin Fischer from comment #22) > > Actually I think we have a bigger problem here: I was confusing holds to > > pull and holds queue earlier. Now testing another patch... > > > > Holds to pull fails for me with and without this patch in strict mode (error > > on bug 26020) > > Thanks for pointing this out and reporting it, Katrin. I still can't seem to > enable strict mode, can you please tell me how to do this? > > I suppose we want to fix the bug on Bug 26020 before getting this patch > through (in fact fixing Bug 26020 might fix this problem) so I'll set 26020 > as blocking this bug. I have the hope to fix the problematic group by with your patches, Aleisha. We already tried to fix it on bug 22431 but failed, so I don't think we should depend on another bug report. Created attachment 107213 [details] [review] Bug 24488: Fix reserve.reserve_id' isn't in GROUP BY Hi, sorry Aleisha and Jonathan. But patch don't work for me. If you change priority of holds, name on List to Pull is still same as befere change. It's not on test plan, I know. But I should work too. Maybe someone test it too. Katrin? https://recordit.co/RA8bCjXekD (In reply to Michal Denar from comment #35) > Hi, > sorry Aleisha and Jonathan. But patch don't work for me. If you change > priority of holds, name on List to Pull is still same as befere change. It's > not on test plan, I know. But I should work too. Maybe someone test it too. > Katrin? > https://recordit.co/RA8bCjXekD Hi Michal, I don't think it's about priority, it's about the earliest place hold. As far as I know the Holds to Pull shows the 'first patron' as based on date, I don't think it ever considered priority. But I may have that wrong, perhaps Jonathan or Katrin can confirm? I am expecting the first patron in the queue to be displayed. The query did not deal with the priority indeed, but this query is plain wrong :D Hi, we'll change status on Failed QA? For me is best solution make 'first patron' based on wating queue (smallest non suspended priority number)? So to confirm, this patch will not only fix the group_by error but we also want to fix the Holds to Pull to show the highest priority hold instead of the earliest placed hold? Because currently it shows the earliest placed hold. Happy to make this change. Hi Aleisha, from my point of view focus on priority solve both scenarios. If borrower make hold, Koha set priority to "1". Next hold "2", etc. If case that priority will be changed, Hold too pull list show correct borrower too. Win-win for me. Hi Aleisha, any updates? Thank you. Unfortunately, not yet sorry. It's on my list to get to when I have time. OK Created attachment 111341 [details] [review] Bug 24488: Show correct first patron details on Holds to pull This patch rewrites the complex search query to reduce the risk of reordering the results multiple times. This patch includes a change to schema files so may need to upgrade schema and/or restart memcached To test: 1) Reproduce problem following test plan in Description 2) Apply patch and refresh page 3) Notice the correct patron is now shown 4) Play with date selection, confirm correct results are still shown 5) Test cancelling holds 6) Test filtering table results 7) Test with biblios with multiple items 8) Test with making items unavailable (i.e. not for loan, checked out) Sponsored by: IHC New Zealand Created attachment 111342 [details] [review] Bug 24488: Fix reserve.reserve_id' isn't in GROUP BY Created attachment 111343 [details] [review] Bug 24488: Display patron with the highest priority hold Patches have been rebased (and I've added some modifications to the first patch, especially related to bug 23485 and bug 25534). I have also added a patch to display the patron with the highest priority hold. Reading for testing again. Created attachment 111347 [details] [review] Bug 24488: (follow-up) Reduce repeated code Created attachment 111348 [details] [review] Bug 24488: Simplify searches It also: Use dtf for date handling add missing td Aleisha, what were you trying to achieve with the following code? 196 foreach my $bibnum ( @biblionumbers ){ 197 198 my @items = Koha::Items->search({ biblionumber => $bibnum }); 199 foreach my $i ( @items ){ 200 if ( $i->checkout ){ 201 next; 202 } 203 } Created attachment 111349 [details] [review] Bug 24488: [DISCUSSION] For comparaison I'd like someone to run a benchmark with and without this patch on a quite big database and see which approach is the best Created attachment 111350 [details] [review] Bug 24488: [DISCUSSION] For comparaison I'd like someone to run a benchmark with and without this patch on a quite big database and see which approach is the best Created attachment 111358 [details] [review] Bug 24488: Show correct first patron details on Holds to pull This patch rewrites the complex search query to reduce the risk of reordering the results multiple times. This patch includes a change to schema files so may need to upgrade schema and/or restart memcached To test: 1) Reproduce problem following test plan in Description 2) Apply patch and refresh page 3) Notice the correct patron is now shown 4) Play with date selection, confirm correct results are still shown 5) Test cancelling holds 6) Test filtering table results 7) Test with biblios with multiple items 8) Test with making items unavailable (i.e. not for loan, checked out) Sponsored by: IHC New Zealand Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 111359 [details] [review] Bug 24488: Fix reserve.reserve_id' isn't in GROUP BY Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 111360 [details] [review] Bug 24488: Display patron with the highest priority hold Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 111361 [details] [review] Bug 24488: Simplify searches It also: Use dtf for date handling add missing td Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 111362 [details] [review] Bug 24488: [DISCUSSION] For comparaison I'd like someone to run a benchmark with and without this patch on a quite big database and see which approach is the best Signed-off-by: Michal Denar <black23@gmail.com> Neither the original patches nor Joubu's alternative(?) apply at the moment. I feel like we got a bit stuck here. Aleisha, could you take a look at comment#50 too when rebasing and maybe Joubu's patch? Created attachment 112815 [details] [review] Bug 24488: Show correct first patron details on Holds to pull This patch rewrites the complex search query to reduce the risk of reordering the results multiple times. This patch includes a change to schema files so may need to upgrade schema and/or restart memcached To test: 1) Reproduce problem following test plan in Description 2) Apply patch and refresh page 3) Notice the correct patron is now shown 4) Play with date selection, confirm correct results are still shown 5) Test cancelling holds 6) Test filtering table results 7) Test with biblios with multiple items 8) Test with making items unavailable (i.e. not for loan, checked out) Sponsored by: IHC New Zealand Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 112816 [details] [review] Bug 24488: Fix reserve.reserve_id' isn't in GROUP BY Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 112817 [details] [review] Bug 24488: Display patron with the highest priority hold Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 112818 [details] [review] Bug 24488: Simplify searches It also: Use dtf for date handling add missing td Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 112819 [details] [review] Bug 24488: [DISCUSSION] For comparaison I'd like someone to run a benchmark with and without this patch on a quite big database and see which approach is the best Signed-off-by: Michal Denar <black23@gmail.com> (In reply to Katrin Fischer from comment #58) > Neither the original patches nor Joubu's alternative(?) apply at the moment. > I feel like we got a bit stuck here. Easy conflict solved. Created attachment 113695 [details] [review] Bug 24488: Show correct first patron details on Holds to pull This patch rewrites the complex search query to reduce the risk of reordering the results multiple times. This patch includes a change to schema files so may need to upgrade schema and/or restart memcached To test: 1) Reproduce problem following test plan in Description 2) Apply patch and refresh page 3) Notice the correct patron is now shown 4) Play with date selection, confirm correct results are still shown 5) Test cancelling holds 6) Test filtering table results 7) Test with biblios with multiple items 8) Test with making items unavailable (i.e. not for loan, checked out) Sponsored by: IHC New Zealand Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 113696 [details] [review] Bug 24488: Fix reserve.reserve_id' isn't in GROUP BY Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 113697 [details] [review] Bug 24488: Display patron with the highest priority hold Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 113698 [details] [review] Bug 24488: Simplify searches It also: Use dtf for date handling add missing td Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 113699 [details] [review] Bug 24488: [DISCUSSION] For comparaison I'd like someone to run a benchmark with and without this patch on a quite big database and see which approach is the best Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 113700 [details] [review] Bug 24488: (QA follow-up): Fix small bugs, add checkk for waiting holds Signed-off-by: Josef Moravec <josef.moravec@gmail.com> I have done a little benchmark Testing DB - copy from production 85k biblios 125k items 15k reserves I ran twice a batch of 10 runs of pendingreserves.pl and measure clean time of this script running (from begin to end of whole script) Original version from Aleisha: Run 1: 7.57 s avg Run 1: 8.1 s avg Joubu's alternative: Run 1: 5,96 s Run 2: 6,04 s That is between 20 and 25 percent gain for Joubu's alternative patch (In reply to Jonathan Druart from comment #63) > I'd like someone to run a benchmark with and without this patch on a > quite big database and see which approach is the best I placed 1000 holds in my system: INSERT INTO reserves (borrowernumber,reservedate,biblionumber,branchcode,priority,itemnumber) SELECT CEIL(rand()*50),CURDATE(),biblionumber,'CPL',1,NULL FROM biblio; On master (strict mode disabled): < 10s Without discusson patch and followup: > 1 min 20s With discussion and followup: < 50 s While the follow-up offers great improvement, I am not impressed with the slowdown overall (In reply to Josef Moravec from comment #71) > I have done a little benchmark > > Testing DB - copy from production > 85k biblios > 125k items > 15k reserves > > I ran twice a batch of 10 runs of pendingreserves.pl and measure clean time > of this script running (from begin to end of whole script) > > Original version from Aleisha: > Run 1: 7.57 s avg > Run 1: 8.1 s avg > > Joubu's alternative: > Run 1: 5,96 s > Run 2: 6,04 s > > That is between 20 and 25 percent gain for Joubu's alternative patch How many holds were reported on the report for you? Created attachment 113756 [details] [review] Bug 24488: (QA follow-up) Make pending reserves faster Signed-off-by: Josef Moravec <josef.moravec@gmail.com> (In reply to Nick Clemens from comment #72) > (In reply to Jonathan Druart from comment #63) > > I'd like someone to run a benchmark with and without this patch on a > > quite big database and see which approach is the best > > I placed 1000 holds in my system: > INSERT INTO reserves > (borrowernumber,reservedate,biblionumber,branchcode,priority,itemnumber) > SELECT CEIL(rand()*50),CURDATE(),biblionumber,'CPL',1,NULL FROM biblio; > > On master (strict mode disabled): > < 10s > > Without discusson patch and followup: > > 1 min 20s > > With discussion and followup: > < 50 s > > While the follow-up offers great improvement, I am not impressed with the > slowdown overall There is in fact no need to select all itemnumbers from checkouts... I added a follow-up, for me it is now significantly faster, even something like 10 times faster then before, could you test on your data? Created attachment 113757 [details] [review] Bug 24488: (QA follow-up) Remove two queries out of foreach cycle Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 113763 [details] [review] Bug 24488: (QA follow-up) Remove two queries out of foreach cycle Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 113764 [details] [review] Bug 24488: (QA follow-up) Move getting items out of cycle Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Josef, I am expecting this last patch to improve perf only in the case where all items are coming from the same biblio. Which don't happen on a real-life situation :) And the memory usage will be worst (maybe negligible however). 17-18s (josef) 18-19 (mine) 5s (master) Working on this. Comment on attachment 113696 [details] [review] Bug 24488: Fix reserve.reserve_id' isn't in GROUP BY Review of attachment 113696 [details] [review]: ----------------------------------------------------------------- ::: circ/pendingreserves.pl @@ +190,2 @@ > ); > +my @biblionumbers = uniq $distinct_holds->get_column('biblionumber'); We could do this at the DB level: `my @biblionumbers = $distinct_holds->_resultset->get_column('biblionumber')->func('DISTINCT');` Created attachment 113774 [details] [review] Bug 24488: Fix count distinct patron on a given hold Created attachment 113775 [details] [review] Bug 24488: perf - Group by at DBMS level Created attachment 113776 [details] [review] Bug 24488: perf - move patrons_count out of the loop Created attachment 113777 [details] [review] Bug 24488: Simplify structure passed to the template Created attachment 113778 [details] [review] Bug 24488: perf - Move holds search out of the loop This will need attention from QA. Are we sure it's doing the same thing (especially the order by!) With the last patches I am at ~9sec (the last patch is doing the best part) I am afraid this one won't hit 20.11. Setting back to NSO as I'd like a full retest of the whole patch stack. Hi, I get this errors on Hold to pull page https://lh3.googleusercontent.com/-Ka99Dm0l93w/X7VZsqPoAoI/AAAAAAAAHvM/btn2wdfWqWYKe7zgA6xu62B5DAd0c_PIgCK8BGAsYHg/s0/2020-11-18.png This does not work correctly - in fact the behaviour is same as before patch (only slower) When we get all holds at once, it will be the same as before... we need to do one of this things: - get hold by biblionumber in each loop - revert last patch - bad performance - add 'priority' => 1, to where clause - best performance, but can we - add subquery and get only the holds with priority = min(priority) for given biblionumber - probably the optimal solution? (In reply to Josef Moravec from comment #90) > This does not work correctly - in fact the behaviour is same as before patch > (only slower) > > When we get all holds at once, it will be the same as before... we need to > do one of this things: > > - get hold by biblionumber in each loop - revert last patch - bad performance > - add 'priority' => 1, to where clause - best performance, but can we > - add subquery and get only the holds with priority = min(priority) for > given biblionumber - probably the optimal solution? Sorry, commited comment too early ;) ad second point: - add 'priority' => 1, to where clause - best performance, but can we assume the lowest priority is always 1? Created attachment 113798 [details] [review] Bug 24488: perf - Move holds search out of the loop This will need attention from QA. We are assuming that the hold with the highest priority has priority=1 Sorry, the last patch was silly indeed. I hadn't turned on the strict mode. Your suggestion, Josef, of assuming that the highest priority hold has priority=1 is great actually. I think we never wanted to assume that (in case the data is corrupted?) in this script, but we should go for it IMO, that simplifies the code, the queries, and it makes sense to me! (In reply to Jonathan Druart from comment #93) > Sorry, the last patch was silly indeed. I hadn't turned on the strict mode. > > Your suggestion, Josef, of assuming that the highest priority hold has > priority=1 is great actually. I think we never wanted to assume that (in > case the data is corrupted?) in this script, but we should go for it IMO, > that simplifies the code, the queries, and it makes sense to me! I believe in the past we had issues with the priorities not being correct, but believe it was fixed (hopefully). Nick or Kyle might be able to tell. Wondering what happens with the local holds priority settings - do they change the priority in the db or do they check highest priority for local? (In reply to Katrin Fischer from comment #94) > (In reply to Jonathan Druart from comment #93) > > Sorry, the last patch was silly indeed. I hadn't turned on the strict mode. > > > > Your suggestion, Josef, of assuming that the highest priority hold has > > priority=1 is great actually. I think we never wanted to assume that (in > > case the data is corrupted?) in this script, but we should go for it IMO, > > that simplifies the code, the queries, and it makes sense to me! > > I believe in the past we had issues with the priorities not being correct, > but believe it was fixed (hopefully). Nick or Kyle might be able to tell. > > Wondering what happens with the local holds priority settings - do they > change the priority in the db or do they check highest priority for local? I checked in our production sites, there are few cases where the highest priority is not 1, and they mostly seem to be related to other issues (suspended holds, holds in the future, missing items) so nothing that should show on holds to pull. Local holds don't alter the priority, they get checked first when filling holds or building the queue, so this report won't catch them, but I think that is fine as this report isn't branch specific Other options (for another bug) would be: 1 - List all the holds - just show the first and maybe add a modal to see all the others 2 - Create a cron like the holds queue, build this page on the backend and simply display - it adds a delay to new holds showing, but allows us to calculate the exact holds to be pulled 3 - Add a branch limiter to the report and default to local branches items Created attachment 113829 [details] [review] Bug 24488: Show correct first patron details on Holds to pull This patch rewrites the complex search query to reduce the risk of reordering the results multiple times. This patch includes a change to schema files so may need to upgrade schema and/or restart memcached To test: 1) Reproduce problem following test plan in Description 2) Apply patch and refresh page 3) Notice the correct patron is now shown 4) Play with date selection, confirm correct results are still shown 5) Test cancelling holds 6) Test filtering table results 7) Test with biblios with multiple items 8) Test with making items unavailable (i.e. not for loan, checked out) Sponsored by: IHC New Zealand Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Michal Denar <black23@gmail.comm> Created attachment 113830 [details] [review] Bug 24488: Fix reserve.reserve_id' isn't in GROUP BY Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Michal Denra <black23@gmail.comm> Created attachment 113831 [details] [review] Bug 24488: Display patron with the highest priority hold Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Michal Denra <black23@gmail.comm> Created attachment 113832 [details] [review] Bug 24488: Simplify searches It also: Use dtf for date handling add missing td Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Michal Denra <black23@gmail.comm> Created attachment 113833 [details] [review] Bug 24488: [DISCUSSION] For comparaison I'd like someone to run a benchmark with and without this patch on a quite big database and see which approach is the best Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Michal Denra <black23@gmail.comm> Created attachment 113834 [details] [review] Bug 24488: (QA follow-up): Fix small bugs, add checkk for waiting holds Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Michal Denra <black23@gmail.comm> Created attachment 113835 [details] [review] Bug 24488: (QA follow-up) Make pending reserves faster Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Michal Denra <black23@gmail.comm> Created attachment 113836 [details] [review] Bug 24488: (QA follow-up) Remove two queries out of foreach cycle Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Michal Denra <black23@gmail.comm> Created attachment 113837 [details] [review] Bug 24488: (QA follow-up) Move getting items out of cycle Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Michal Denra <black23@gmail.comm> Created attachment 113838 [details] [review] Bug 24488: Fix count distinct patron on a given hold Signed-off-by: Michal Denra <black23@gmail.comm> Created attachment 113839 [details] [review] Bug 24488: perf - Group by at DBMS level Signed-off-by: Michal Denra <black23@gmail.comm> Created attachment 113840 [details] [review] Bug 24488: perf - move patrons_count out of the loop Signed-off-by: Michal Denra <black23@gmail.comm> Created attachment 113841 [details] [review] Bug 24488: Simplify structure passed to the template Signed-off-by: Michal Denra <black23@gmail.comm> Created attachment 113842 [details] [review] Bug 24488: perf - Move holds search out of the loop This will need attention from QA. We are assuming that the hold with the highest priority has priority=1 Signed-off-by: Michal Denra <black23@gmail.comm> (In reply to Michal Denar from comment #108) > Created attachment 113841 [details] [review] [review] > Bug 24488: Simplify structure passed to the template > > Signed-off-by: Michal Denra <black23@gmail.comm> Thanks Michal for testing, just a note, you have misspelled your name and email in your git configuration Created attachment 113850 [details] [review] Bug 24488: Show correct first patron details on Holds to pull This patch rewrites the complex search query to reduce the risk of reordering the results multiple times. This patch includes a change to schema files so may need to upgrade schema and/or restart memcached To test: 1) Reproduce problem following test plan in Description 2) Apply patch and refresh page 3) Notice the correct patron is now shown 4) Play with date selection, confirm correct results are still shown 5) Test cancelling holds 6) Test filtering table results 7) Test with biblios with multiple items 8) Test with making items unavailable (i.e. not for loan, checked out) Sponsored by: IHC New Zealand Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 113851 [details] [review] Bug 24488: Fix reserve.reserve_id' isn't in GROUP BY Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 113852 [details] [review] Bug 24488: Display patron with the highest priority hold Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 113853 [details] [review] Bug 24488: Simplify searches It also: Use dtf for date handling add missing td Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 113854 [details] [review] Bug 24488: [DISCUSSION] For comparaison I'd like someone to run a benchmark with and without this patch on a quite big database and see which approach is the best Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 113855 [details] [review] Bug 24488: (QA follow-up): Fix small bugs, add checkk for waiting holds Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 113856 [details] [review] Bug 24488: (QA follow-up) Make pending reserves faster Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 113857 [details] [review] Bug 24488: (QA follow-up) Remove two queries out of foreach cycle Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 113858 [details] [review] Bug 24488: (QA follow-up) Move getting items out of cycle Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 113859 [details] [review] Bug 24488: Fix count distinct patron on a given hold Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 113860 [details] [review] Bug 24488: perf - Group by at DBMS level Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 113861 [details] [review] Bug 24488: perf - move patrons_count out of the loop Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 113862 [details] [review] Bug 24488: Simplify structure passed to the template Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 113863 [details] [review] Bug 24488: perf - Move holds search out of the loop This will need attention from QA. We are assuming that the hold with the highest priority has priority=1 Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 113864 [details] [review] Bug 24488: (QA follow-up) Go to next loop as early as possible Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Looks good now I think Nick or Jonathan, can you please review my last follow-up? It does not give much speed gain, but still measurable on my testing machine. This looks like a great improvement, movin us to searching via objects etc... but... dare I say it, can we move this to a module and thus add tests? There's a lot of functionality here in filtering down to the right level, improving performance etc.. be great to have that solidly unit/regression tested ;) I thought a bit more about the priority=1 thing this morning. And actually I have the feeling that the original issue described on this bug report could be fixed with the following (...one line!) change: diff --git a/circ/pendingreserves.pl b/circ/pendingreserves.pl index 01cf877b55..962ebe5609 100755 --- a/circ/pendingreserves.pl +++ b/circ/pendingreserves.pl @@ -230,7 +230,7 @@ my $strsth = AND items.itemnumber NOT IN (SELECT itemnumber FROM branchtransfers where datearrived IS NULL) AND items.itemnumber NOT IN (SELECT itemnumber FROM reserves WHERE found IS NOT NULL AND itemnumber IS NOT NULL) AND issues.itemnumber IS NULL - AND reserves.priority <> 0 + AND reserves.priority = 1 AND reserves.suspend = 0 AND notforloan = 0 AND itemlost = 0 AND withdrawn = 0 AND ( circulation_rules.rule_value IS NULL OR circulation_rules.rule_value != 0 ) (In reply to Jonathan Druart from comment #128) > I thought a bit more about the priority=1 thing this morning. > > And actually I have the feeling that the original issue described on this > bug report could be fixed with the following (...one line!) change: > > diff --git a/circ/pendingreserves.pl b/circ/pendingreserves.pl > index 01cf877b55..962ebe5609 100755 > --- a/circ/pendingreserves.pl > +++ b/circ/pendingreserves.pl > @@ -230,7 +230,7 @@ my $strsth = > AND items.itemnumber NOT IN (SELECT itemnumber FROM branchtransfers > where datearrived IS NULL) > AND items.itemnumber NOT IN (SELECT itemnumber FROM reserves WHERE > found IS NOT NULL AND itemnumber IS NOT NULL) > AND issues.itemnumber IS NULL > - AND reserves.priority <> 0 > + AND reserves.priority = 1 > AND reserves.suspend = 0 This doesn't work because then rcount i.e. "Pull this many items" would only show 1 when in reality you need to pull many more items also to fill those other reserves. PS. unrelated, but rcount is also incorrect, it should be the number of holds, not amount of borrowers. (In reply to Martin Renvoize from comment #127) > This looks like a great improvement, movin us to searching via objects > etc... but... dare I say it, can we move this to a module and thus add tests? I think this definitely needs to use a module, the code population of @all_items should be replaced with an object's method call that returns items that can fill holds. Then it will make super easy to read code where you just iterate over all the items and add the callnumber, shelving location, etc. info to the $reserves variable that goes. Btw, to resolve the original issue, can't we just order by reserve priority? This refactoring could be in a separate bug. (In reply to Joonas Kylmälä from comment #130) > (In reply to Martin Renvoize from comment #127) > > This looks like a great improvement, movin us to searching via objects > > etc... but... dare I say it, can we move this to a module and thus add tests? > > I think this definitely needs to use a module, the code population of > @all_items should be replaced with an object's method call that returns > items that can fill holds. Then it will make super easy to read code where > you just iterate over all the items and add the callnumber, shelving > location, etc. info to the $reserves variable that goes. Yes, it would be nice to have tests and make the code even more clean. But given from when we started, I think it's already a great improvement. I really would like to have this into master at the beginning of the release cycle, and I am not sure we are going to find volunteers to write tests soon... > Btw, to resolve the original issue, can't we just order by reserve priority? > This refactoring could be in a separate bug. No, I don't think it will work, because of the group by. I had a (quick) look at how we could move the @all_items construction into a Koha::Items->filter_by_ method. I didn't find a simply way to do it, keeping code readable and without duplication. (In reply to Jonathan Druart from comment #132) > I had a (quick) look at how we could move the @all_items construction into a > Koha::Items->filter_by_ method. I didn't find a simply way to do it, keeping > code readable and without duplication. Having it under Koha::Items was not my intention and would not lead to any clearer result. It should be something like Koha::Holds->find($biblionumber)->items_that_can_fill and Koha::Hold->items_that_can_fill. But for this we need the CanItemFillHold from https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26659#c6 I think. And of course some other naming scheme might be better, take items_that_can_fill as just an example. I've opened a new bug report. I don't think we should add more patch to this one. See bug 27131. Created attachment 114835 [details] [review] Bug 24488: Show correct first patron details on Holds to pull This patch rewrites the complex search query to reduce the risk of reordering the results multiple times. This patch includes a change to schema files so may need to upgrade schema and/or restart memcached To test: 1) Reproduce problem following test plan in Description 2) Apply patch and refresh page 3) Notice the correct patron is now shown 4) Play with date selection, confirm correct results are still shown 5) Test cancelling holds 6) Test filtering table results 7) Test with biblios with multiple items 8) Test with making items unavailable (i.e. not for loan, checked out) Sponsored by: IHC New Zealand Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 114836 [details] [review] Bug 24488: Fix reserve.reserve_id' isn't in GROUP BY Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 114837 [details] [review] Bug 24488: Display patron with the highest priority hold Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 114838 [details] [review] Bug 24488: Simplify searches It also: Use dtf for date handling add missing td Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 114839 [details] [review] Bug 24488: [DISCUSSION] For comparaison I'd like someone to run a benchmark with and without this patch on a quite big database and see which approach is the best Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 114840 [details] [review] Bug 24488: (QA follow-up): Fix small bugs, add checkk for waiting holds Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 114841 [details] [review] Bug 24488: (QA follow-up) Make pending reserves faster Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 114842 [details] [review] Bug 24488: (QA follow-up) Remove two queries out of foreach cycle Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 114843 [details] [review] Bug 24488: (QA follow-up) Move getting items out of cycle Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 114844 [details] [review] Bug 24488: Fix count distinct patron on a given hold Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 114845 [details] [review] Bug 24488: perf - Group by at DBMS level Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 114846 [details] [review] Bug 24488: perf - move patrons_count out of the loop Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 114847 [details] [review] Bug 24488: Simplify structure passed to the template Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 114848 [details] [review] Bug 24488: perf - Move holds search out of the loop This will need attention from QA. We are assuming that the hold with the highest priority has priority=1 Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 114849 [details] [review] Bug 24488: (QA follow-up) Go to next loop as early as possible Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 114850 [details] [review] Bug 24488: create a loop to avoid copy paste Patches rebased. I have provided a small enhancement on bug 27131 to move some code to a method. I don't think we could do more here, if you look at the code it read good and is pretty trivial (it's just a matter a few ->search calls). I have done all what I could do get this ready, hopefully it will be approved and pushed soon :) Sending back to the NQA queue. (In reply to Jonathan Druart from comment #151) > Patches rebased. > > I have provided a small enhancement on bug 27131 to move some code to a > method. > I don't think we could do more here, if you look at the code it read good > and is pretty trivial (it's just a matter a few ->search calls). > > I have done all what I could do get this ready, hopefully it will be > approved and pushed soon :) > > Sending back to the NQA queue. My first impression here is: I knew that the query was ugly but replacing it by a huge number of DBIx calls will probably degrade the performance significantly. And if we make so many changes in a script, we should really go to a module. my ( $k, $f ) = each %$fff ) Clear variables names btw :) QA: Still looking here Created attachment 116362 [details] [review] Bug 24488: Show correct first patron details on Holds to pull This patch rewrites the complex search query to reduce the risk of reordering the results multiple times. This patch includes a change to schema files so may need to upgrade schema and/or restart memcached To test: 1) Reproduce problem following test plan in Description 2) Apply patch and refresh page 3) Notice the correct patron is now shown 4) Play with date selection, confirm correct results are still shown 5) Test cancelling holds 6) Test filtering table results 7) Test with biblios with multiple items 8) Test with making items unavailable (i.e. not for loan, checked out) Sponsored by: IHC New Zealand Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 116363 [details] [review] Bug 24488: Fix reserve.reserve_id' isn't in GROUP BY Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 116364 [details] [review] Bug 24488: Display patron with the highest priority hold Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 116365 [details] [review] Bug 24488: Simplify searches It also: Use dtf for date handling add missing td Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 116366 [details] [review] Bug 24488: [DISCUSSION] For comparaison I'd like someone to run a benchmark with and without this patch on a quite big database and see which approach is the best Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 116367 [details] [review] Bug 24488: (QA follow-up): Fix small bugs, add checkk for waiting holds Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 116368 [details] [review] Bug 24488: (QA follow-up) Make pending reserves faster Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 116369 [details] [review] Bug 24488: (QA follow-up) Remove two queries out of foreach cycle Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 116370 [details] [review] Bug 24488: (QA follow-up) Move getting items out of cycle Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 116371 [details] [review] Bug 24488: Fix count distinct patron on a given hold Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 116372 [details] [review] Bug 24488: perf - Group by at DBMS level Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 116373 [details] [review] Bug 24488: perf - move patrons_count out of the loop Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 116374 [details] [review] Bug 24488: Simplify structure passed to the template Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 116375 [details] [review] Bug 24488: perf - Move holds search out of the loop This will need attention from QA. We are assuming that the hold with the highest priority has priority=1 Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 116376 [details] [review] Bug 24488: (QA follow-up) Go to next loop as early as possible Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 116377 [details] [review] Bug 24488: create a loop to avoid copy paste Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Some timings from the browser: 500 holds -> only 6 selected 150 ms OLD 300 ms NEW 2500 holds -> 40 selected 500 ms OLD 800 ms NEW 20000 holds -> 1000 selected Gateway timeouts or unclear results. In view of the attention given already, passing QA. This should get further follow-up. Move essential code into a module, test it. The original query was terrible, but the new code is far from ideal too. The performance has been degraded. Hard to say when it is no longer acceptable. (In reply to Marcel de Rooy from comment #152) > (In reply to Jonathan Druart from comment #151) > > Patches rebased. > > > > I have provided a small enhancement on bug 27131 to move some code to a > > method. > > I don't think we could do more here, if you look at the code it read good > > and is pretty trivial (it's just a matter a few ->search calls). > > > > I have done all what I could do get this ready, hopefully it will be > > approved and pushed soon :) > > > > Sending back to the NQA queue. > > My first impression here is: I knew that the query was ugly but replacing it > by a huge number of DBIx calls will probably degrade the performance > significantly. And if we make so many changes in a script, we should really > go to a module. Have you seen bug 27131? Thanks for the review, Marcel! Created attachment 116384 [details] [review] Bug 24488: create a loop to avoid copy paste Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> JD amended patch: Rename variables Pushed to master for 21.05, thanks to everybody involved! *** Bug 22431 has been marked as a duplicate of this bug. *** Do you think this is safe for backport on 20.11.x ? (In reply to Fridolin Somers from comment #177) > Do you think this is safe for backport on 20.11.x ? No, it's too big. Not backported to 20.11.x |