Description
Martin Renvoize (ashimema)
2020-04-23 12:14:14 UTC
Created attachment 103596 [details] [review] Bug 25266: Display vendor with late orders in the dropdown list Hum, there is something weird going on here. On test is failing but I don't understand why! I think the behaviour was: - Show the vendors from the 'results' in the pull down - When filtered on a vendor, show the 'check all' link - Then you could proceed to claim all for that vendor It appears to work in 18.11 Created attachment 104254 [details] [review] Bug 25266: Build the vendor list from the order list This patch is removing the use of GetBooksellersWithLateOrders and use the late order list to generate the vendors. Test plan: Create several vendors, several orders, close the baskets Also create vendors that do not have orders (or basket not closed) Go to the late orders page and confirm that the list of vendors is correctly filled, select one, you can see the "select all" link in the header of the table QA: that would be good to test this with production data and compare execution time. Created attachment 104255 [details] [review] Bug 25266: Remove C4::Bookseller This was the only occurrence of GetBooksellersWithLateOrders and it was the only subroutine of C4::Bookseller I went a completely different direction and removed the C4 subroutine. I am not sure it's the correctly fix, as the vendor list will not have all the vendors with late orders, but the vendors with late orders that match the existing filters (date/delay). Created attachment 104257 [details] [review] Bug 25266: Remove C4::Bookseller This was the only occurrence of GetBooksellersWithLateOrders and it was the only subroutine of C4::Bookseller There is something odd with this filter on 18.11 already. Without any filters I have late orders from 2 vendors showing, but only one of them shows in the filter list... so 18.11 might not be a good reference point. I feel like I should know why this happens, but the tests fail rather spectactularly for me: kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ prove t/Bookseller.t Cannot detect source of 't/Bookseller.t'! at /usr/share/perl/5.24/TAP/Parser/IteratorFactory.pm line 261. TAP::Parser::IteratorFactory::detect_source(TAP::Parser::IteratorFactory=HASH(0x55d019f812b8), TAP::Parser::Source=HASH(0x55d019f813a8)) called at /usr/share/perl/5.24/TAP/Parser/IteratorFactory.pm line 211 ... Also: Template process failed: undef error - The method Koha::Acquisition::Orders->size is not covered by tests! Ok, second problem fixed by a restart all - looks much better on first glance, but will need a bit more testing. Could you check on the tests meanwhile? (In reply to Katrin Fischer from comment #11) > kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ prove t/Bookseller.t > Cannot detect source of 't/Bookseller.t'! at The test file (and the whole C4::Bookseller module) has been deleted. I had a feeling it was too late yesterday to continue on this one, I will try again later! In 17.11 the list is not 'all vendors', but it also seems to not be only vendors with late orders... I wonder if it's vendors with open baskets/orders. There is a lot of data in the db I checked, but samples seem to point that way. The last change made to this subroutine is commit b76eac9ca2c32a5db51b9b8ca32a2cf4fa6397b5 Bug 14535: Update the supplier filter too which is in 3.22, so I would not expect recent behaviour changes. The way it works, reading the code is: get the vendor which have orders not fully received yet (datereceived is null or quantityreceived < quantity) and the basket is closed. If delay is passed (>=0), then the devery time is taken into account and the date the basket has been closed is compared (now + delivery time >= close date). I don't think it makes sense to have 2 ways (especially if different) to get the "lates". If the order list is correctly generated, it makes sense to build the vendor list from it. I agree, it acts as a filter on the list, only offering what is IN the list makes a lot of sense. Especially since whe have some special libraries with a lot of vendors. Keeping the list sensibly short. Template process failed: undef error - SQL::Abstract::puke(): [SQL::Abstract::__ANON__] Fatal: Argument passed to the 'IN' operator can not be undefined at /home/vagrant/kohaclone/Koha/Objects.pm line 372 at /home/vagrant/kohaclone/C4/Templates.pm line 122 :( Created attachment 105321 [details] [review] Bug 25266: Fix error when no later order If no late order we got: "Argument passed to the 'IN' operator can not be undefined" It highlights a context error Created attachment 105329 [details] [review] Bug 25266: Build the vendor list from the order list This patch is removing the use of GetBooksellersWithLateOrders and use the late order list to generate the vendors. Test plan: Create several vendors, several orders, close the baskets Also create vendors that do not have orders (or basket not closed) Go to the late orders page and confirm that the list of vendors is correctly filled, select one, you can see the "select all" link in the header of the table QA: that would be good to test this with production data and compare execution time. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 105330 [details] [review] Bug 25266: Remove C4::Bookseller This was the only occurrence of GetBooksellersWithLateOrders and it was the only subroutine of C4::Bookseller Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 105331 [details] [review] Bug 25266: Fix error when no later order If no late order we got: "Argument passed to the 'IN' operator can not be undefined" It highlights a context error Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Works like a charm now, thank you. Seems that @parameters variable is now useless in acqui/lateorders.pl: 126 my @parameters = ( $delay ); 127 push @parameters, $estimateddeliverydatefrom_dt 128 ? $estimateddeliverydatefrom_dt->ymd() 129 : undef; 130 131 push @parameters, $estimateddeliverydateto_dt 132 ? $estimateddeliverydateto_dt->ymd() 133 : undef; Also, you remove the file t/db_dependent/Bookseller.t. That file contains a lot of test that are not related to C4::Bookseller and GetBooksellersWithLateOrders (AddBookseller, DelBookseller, get bookseller etc...). Are you sure these tests exist elsewhere ? Created attachment 105592 [details] [review] Bug 25266: Remove uneeded var (In reply to Alex Arnaud from comment #24) > Seems that @parameters variable is now useless in acqui/lateorders.pl: > > 126 my @parameters = ( $delay ); > 127 push @parameters, $estimateddeliverydatefrom_dt > 128 ? $estimateddeliverydatefrom_dt->ymd() > 129 : undef; > 130 > 131 push @parameters, $estimateddeliverydateto_dt > 132 ? $estimateddeliverydateto_dt->ymd() > 133 : undef; Indeed, fixed! > Also, you remove the file t/db_dependent/Bookseller.t. That file contains a > lot of test that are not related to C4::Bookseller and > GetBooksellersWithLateOrders (AddBookseller, DelBookseller, get bookseller > etc...). > > Are you sure these tests exist elsewhere ? Those CRUD subroutines have been moved to Koha::Acquisition::Booksellers. The whole test file was there to test GetBooksellersWithLateOrders, and a left over of the previous subroutines. There is some tests in t/db_dependent/Koha/Acquisition/Booksellers.t, that's enough to test Koha::Acquisition::Booksellers (most of CRUD methods rely on highly tested Koha::Object[s] anyway). Created attachment 105621 [details] [review] Bug 25266: Build the vendor list from the order list This patch is removing the use of GetBooksellersWithLateOrders and use the late order list to generate the vendors. Test plan: Create several vendors, several orders, close the baskets Also create vendors that do not have orders (or basket not closed) Go to the late orders page and confirm that the list of vendors is correctly filled, select one, you can see the "select all" link in the header of the table QA: that would be good to test this with production data and compare execution time. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Created attachment 105622 [details] [review] Bug 25266: Remove C4::Bookseller This was the only occurrence of GetBooksellersWithLateOrders and it was the only subroutine of C4::Bookseller Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Created attachment 105623 [details] [review] Bug 25266: Fix error when no later order If no late order we got: "Argument passed to the 'IN' operator can not be undefined" It highlights a context error Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Created attachment 105624 [details] [review] Bug 25266: Remove uneeded var Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Pushed to master for 20.11, thanks to everybody involved! backported to 20.05.x for 20.05.01 Does not apply cleanly on 19.11.x, please rebase if required on 19.11.x *** Bug 27102 has been marked as a duplicate of this bug. *** |