Bug 25266 - Not all vendors are listed in the filters on the late order claims page
Summary: Not all vendors are listed in the filters on the late order claims page
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
: 27102 (view as bug list)
Depends on:
Blocks:
 
Reported: 2020-04-23 12:14 UTC by Martin Renvoize
Modified: 2020-12-01 11:51 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.01


Attachments
Bug 25266: Display vendor with late orders in the dropdown list (2.26 KB, patch)
2020-04-23 13:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25266: Build the vendor list from the order list (6.06 KB, patch)
2020-05-04 11:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25266: Remove C4::Bookseller (34.34 KB, patch)
2020-05-04 11:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25266: Remove C4::Bookseller (34.74 KB, patch)
2020-05-04 11:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25266: Fix error when no later order (902 bytes, patch)
2020-05-25 07:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25266: Build the vendor list from the order list (6.13 KB, patch)
2020-05-25 21:11 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 25266: Remove C4::Bookseller (34.81 KB, patch)
2020-05-25 21:11 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 25266: Fix error when no later order (960 bytes, patch)
2020-05-25 21:11 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 25266: Remove uneeded var (858 bytes, patch)
2020-06-05 14:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25266: Build the vendor list from the order list (6.19 KB, patch)
2020-06-08 08:18 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 25266: Remove C4::Bookseller (34.86 KB, patch)
2020-06-08 08:18 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 25266: Fix error when no later order (1014 bytes, patch)
2020-06-08 08:18 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 25266: Remove uneeded var (912 bytes, patch)
2020-06-08 08:18 UTC, Alex Arnaud
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2020-04-23 12:14:14 UTC
As the title says, for some reason the list of vendors available in the late order claims page is not fully populated.
Comment 1 Jonathan Druart 2020-04-23 13:03:25 UTC
Created attachment 103596 [details] [review]
Bug 25266: Display vendor with late orders in the dropdown list
Comment 2 Jonathan Druart 2020-04-23 13:03:49 UTC
Hum, there is something weird going on here. On test is failing but I don't understand why!
Comment 3 Katrin Fischer 2020-05-02 16:44:38 UTC
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
Comment 4 Katrin Fischer 2020-05-02 16:55:32 UTC
It appears to work in 18.11
Comment 5 Jonathan Druart 2020-05-04 11:18:46 UTC
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.
Comment 6 Jonathan Druart 2020-05-04 11:18:54 UTC
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
Comment 7 Jonathan Druart 2020-05-04 11:19:28 UTC
I went a completely different direction and removed the C4 subroutine.
Comment 8 Jonathan Druart 2020-05-04 11:21:31 UTC
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).
Comment 9 Jonathan Druart 2020-05-04 11:24:19 UTC
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
Comment 10 Katrin Fischer 2020-05-05 21:43:52 UTC
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.
Comment 11 Katrin Fischer 2020-05-05 21:50:26 UTC
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!
Comment 12 Katrin Fischer 2020-05-05 21:51:59 UTC
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?
Comment 13 Jonathan Druart 2020-05-06 07:43:42 UTC
(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.
Comment 14 Katrin Fischer 2020-05-06 08:01:08 UTC
I had a feeling it was too late yesterday to continue on this one, I will try again later!
Comment 15 Katrin Fischer 2020-05-06 14:03:09 UTC
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.
Comment 16 Jonathan Druart 2020-05-06 14:22:41 UTC
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.
Comment 17 Katrin Fischer 2020-05-06 14:33:04 UTC
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.
Comment 18 Katrin Fischer 2020-05-23 11:05:45 UTC
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 

:(
Comment 19 Jonathan Druart 2020-05-25 07:35:00 UTC
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
Comment 20 Katrin Fischer 2020-05-25 21:11:22 UTC
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>
Comment 21 Katrin Fischer 2020-05-25 21:11:26 UTC
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>
Comment 22 Katrin Fischer 2020-05-25 21:11:29 UTC
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>
Comment 23 Katrin Fischer 2020-05-25 21:12:32 UTC
Works like a charm now, thank you.
Comment 24 Alex Arnaud 2020-06-05 14:27:13 UTC
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 ?
Comment 25 Jonathan Druart 2020-06-05 14:59:28 UTC
Created attachment 105592 [details] [review]
Bug 25266: Remove uneeded var
Comment 26 Jonathan Druart 2020-06-05 15:01:31 UTC
(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).
Comment 27 Alex Arnaud 2020-06-08 08:18:30 UTC
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>
Comment 28 Alex Arnaud 2020-06-08 08:18:35 UTC
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>
Comment 29 Alex Arnaud 2020-06-08 08:18:38 UTC
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>
Comment 30 Alex Arnaud 2020-06-08 08:18:42 UTC
Created attachment 105624 [details] [review]
Bug 25266: Remove uneeded var

Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 31 Jonathan Druart 2020-06-15 09:59:32 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 32 Lucas Gass 2020-06-17 22:34:43 UTC
backported to 20.05.x for 20.05.01
Comment 33 Aleisha Amohia 2020-06-22 02:57:54 UTC
Does not apply cleanly on 19.11.x, please rebase if required on 19.11.x
Comment 34 Jonathan Druart 2020-12-01 11:51:58 UTC
*** Bug 27102 has been marked as a duplicate of this bug. ***