Bug 19593 - [16.11.x] "Delete vendor" button is always set
Summary: [16.11.x] "Delete vendor" button is always set
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: 16.11
Hardware: All All
: P5 - low major (vote)
Assignee: Bugs List
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-11-08 15:58 UTC by Gwendal JONCOUR
Modified: 2017-12-19 21:12 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Rennes 2 local correction for basketcount and subscriptioncount calls (807 bytes, patch)
2017-11-16 09:30 UTC, Gwendal JONCOUR
Details | Splinter Review
Bug 19593 Basketcount correction on Bookseller.pm (1.07 KB, patch)
2017-12-12 08:32 UTC, Chris Cormack
Details | Splinter Review
Bug 19593 Basketcount correction on Bookseller.pm (1.12 KB, patch)
2017-12-13 07:34 UTC, Katrin Fischer
Details | Splinter Review
Bug 19593 Basketcount correction on Bookseller.pm (1.14 KB, patch)
2017-12-13 09:10 UTC, Gwendal JONCOUR
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Gwendal JONCOUR 2017-11-08 15:58:42 UTC
On our freshly updated 16.11 instance, we noticed that on pages /cgi-bin/koha/acqui/booksellers.pl and /cgi-bin/koha/acqui/supplier.pl, the acquisition toolbar always shows the "Delete vendor" button instead of the "Receive shipments", no matter the number of baskets attached to the vendor.

When I looking on conditions on koha-tmpl/intranet-tmpl/prog/en/include/acquisitions-toolbar.inc, I saw that basketcount variable is not set. acqui/booksellers.pl and acqui/supplier.pl calling $suppliers[0]->{'basketcount'} and $supplier->{'basketcount'} from Koha::Acquisition::Bookseller while Bookseller.pm has a basket_count method to return it, explaining why the "Delete vendor" button is always shown. 

The problem is mainly cosmetic but with it, the test on vendor deletion in supplier.pl line 95...
if ( $supplier->{'basketcount'} == 0 ) {
...always returns true so the only thing preventing deletion of vendor with baskets attached on is the database foreign key contraint.
Comment 1 Katrin Fischer 2017-11-15 21:53:39 UTC
I verified the problem in 16.11.x, but might need some help in order to fix it.


The template expects a variable basketcount to contain the number of linked baskets. It's undefined in 16.11.x.

The problem is in the API rewrite:

Bug 12896: Move the bookseller-related code into Koha::Acquisition::Bookseller

Changed the code from using GetBookSellerFromId which returned the count, to use  the new API:

$supplier = Koha::Acquisition::Bookseller->fetch({ id => $booksellerid });
This doesn't return the counts.

The problem doesn't appear in 17.05, because in December after 16.11.x was released, another heavy change was made that fixed things in the 17.05 branch:

Bug 13726 - Koha::Acquisition::Bookseller should use Koha::Object

It added Koha::Acquisition::Booksellers and fixed the count variables for baskets and subscriptions.

How to fix?

GetBookSellerFromId is no longer there, but I am also missing the newer methods used in 17.05:
+    $supplier = Koha::Acquisition::Booksellers->find( $booksellerid );
basketcount   => $supplier->baskets->count,

Dependency trees from that far back are hard to unravel and trying to do so might cause new bad side effects. I think a 16.11.x specific fix for this problem only would be preferrable at this point in time.

Can someone help?
Comment 2 Gwendal JONCOUR 2017-11-16 09:30:42 UTC
Created attachment 69178 [details] [review]
Rennes 2 local correction for basketcount and subscriptioncount calls

Here's my local correction. A little bit nasty but it make its job and works on each calls of basketcount and subscriptioncount.
Comment 3 Chris Cormack 2017-12-12 08:32:02 UTC
Created attachment 69708 [details] [review]
Bug 19593 Basketcount correction on Bookseller.pm

To test:
1/ go to /cgi-bin/koha/acqui/booksellers.pl or
/cgi-bin/koha/acqui/supplier.pl
2/ Note that it displays the delete vendor button instead of the
receive shipments button
3/ Apply patch
4/ Reload the page and notice the button is fixed

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Comment 4 Katrin Fischer 2017-12-12 09:35:59 UTC
Thx Gwendal and Chris!
Comment 5 Katrin Fischer 2017-12-13 07:34:43 UTC
Created attachment 69761 [details] [review]
Bug 19593 Basketcount correction on Bookseller.pm

To test:
1/ go to /cgi-bin/koha/acqui/booksellers.pl or
/cgi-bin/koha/acqui/supplier.pl
2/ Note that it displays the delete vendor button instead of the
receive shipments button
3/ Apply patch
4/ Reload the page and notice the button is fixed

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 6 Katrin Fischer 2017-12-13 07:36:00 UTC
Tested:
- vendor with no orders and subscriptions (button)
- vendor with orders (no button)
- vendor with subscriptions (no button)

Just noticed something, author line reads:
Author: KohaR2 <koha@listes.uhb.fr>

Gwendal, do you want us to adjust that to your name and personal email?
Comment 7 Gwendal JONCOUR 2017-12-13 09:10:13 UTC
Created attachment 69769 [details] [review]
Bug 19593 Basketcount correction on Bookseller.pm

Arf, default user/email on our preprod server. Here's the corrected version.
Comment 8 Chris Cormack 2017-12-18 02:45:40 UTC
Pushed to 16.11.x will be in 16.11.15

Thats Gwendal
Comment 9 Chris Cormack 2017-12-18 02:46:03 UTC
> Thats Gwendal

That should be Thanks Gwendal :)
Comment 10 Jonathan Druart 2017-12-18 18:11:30 UTC
This change broke 3 tests:
https://jenkins.koha-community.org/job/Koha_16.11_D8/lastCompletedBuild/testReport/
Comment 11 Chris Cormack 2017-12-18 23:36:29 UTC
(In reply to Jonathan Druart from comment #10)
> This change broke 3 tests:
> https://jenkins.koha-community.org/job/Koha_16.11_D8/lastCompletedBuild/
> testReport/

Yup, the tests are wrong so they should break. I'll fix them this afternoon ;)
Comment 12 Chris Cormack 2017-12-19 20:40:27 UTC
Interestingly the test fails without this patch

#   at t/db_dependent/Bookseller.t line 121.
#     Structures begin differing at:
#          $got->{discount} = '2'
#     $expected->{discount} = '2.0000'

Just in a different way. 

Ill try to fix both issues
Comment 13 Chris Cormack 2017-12-19 21:12:44 UTC
Ok, discount is a float(6,4) .. and tax_rate is a decimal(6,4), 

$VAR1 = {
          'bookselleremail' => undef,
          'notes' => 'notes2',
          'address2' => 'address2-2',
          'booksellerurl' => undef,
          'booksellerfax' => undef,
          'url' => 'url2',
          'address1' => 'address1_2',
          'phone' => 'phone2',
          'listprice' => undef,
          'fax' => 'fax2',
          'id' => 44,
          'invoiceincgst' => 1,
          'currency' => '',
          'accountnumber' => 'accountnumber2',
          'name' => 'Name2',
          'deliverytime' => 2,
          'postal' => 'postal2',
          'tax_rate' => '2.0000',
          'gstreg' => 1,
          'listincgst' => 1,
          'invoiceprice' => undef,
          'address4' => 'address4_2',
          'discount' => '2',
          'othersupplier' => undef,
          'address3' => 'address3_2',
          'active' => 1
        };

It's handing back 2 instead of 2.0000 but fine for tax_rate, if I change the db to be decimal, it works
Not to do with this bug, ill fix the basketcount thing, and push that, but this test needs looking into more