Bug 5356

Summary: Shipping billing address improvements
Product: Koha Reporter: Paul Poulain <paul.poulain>
Component: AcquisitionsAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: claire.hernandez, jonathan.druart, katrin.fischer, koha, m.de.rooy
Version: 3.10   
Hardware: All   
OS: All   
URL: http://wiki.koha-community.org/wiki/Shipping_billing_acq_RFC
Change sponsored?: Sponsored Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 7302    
Bug Blocks: 7163, 7298    
Deadline: 2011-05-01   
Attachments: Bug 5356: delivery place and billing place centralised in basket management
Bug 5356: delivery place and billing place centralised in basket management
Bug 5356: delivery place and billing place centralised in basket management
Bug 5356: delivery place and billing place centralised in basket management
Bug 5356: delivery place and billing place centralised in basket management
Bug 5356: delivery place and billing place centralised in basket management
Bug 5356: delivery place and billing place centralised in basket management
Bug 5356: delivery place and billing place centralised in basket management
Bug 5356: Followup sort and branchname
[SIGNED-OFF] Bug 7302: Followup: rename "Client number" column with "Account number"
[SIGNED-OFF] Bug 5356: delivery place and billing place centralised in basket management
[SIGNED-OFF] Bug 5356: Followup sort and branchname
Bug 5356: delivery place and billing place centralised in basket management
Bug 5356: Followup: Add basket billing and delivery place on export pdf (layout3pages)
Bug 5356: delivery place and billing place centralised in basket management
Bug 5356: Followup: Add basket billing and delivery place on export pdf (layout3pages)
Bug 5356: Followup: remove useless code
Screenshot of library name too far on the right
Bug 5356: Followup Change font in the layout3pages pdf

Description Paul Poulain 2010-11-02 03:52:39 UTC
The library will be able to choose shipping and billing address at basket level. If those informations are filled at basket address, then they won't have to be filled at basketgroup. If shipping & billing addresses are filled at basket level, the information will be stored in the comment section of the basket. That will be usefull for libraries having a single basketgroup for the supplier, but different shipping/billing address
Comment 1 Jonathan Druart 2012-03-01 16:03:49 UTC
BibLibre ref: MT5833
Comment 2 Jonathan Druart 2012-03-21 11:31:06 UTC Comment hidden (obsolete)
Comment 3 Koha Team University Lyon 3 2012-03-21 13:33:17 UTC
test on sandbox4 :
After saving in http://pro.test4.biblibre.com/cgi-bin/koha/acqui/basketheader.pl?booksellerid=3&op=add_form

Software error:

Template process failed: file error - parse error - /home/koha/src/koha-tmpl/intranet-tmpl/prog/fr-FR/modules/acqui/basket.tt line 309: unexpected token (ELSE)
  [% ELSE %] at /home/koha/src/C4/Templates.pm line 127.

For help, please send mail to the webmaster (webmaster@test4-sandbox), giving this error message and the time and date 
of the error.
Comment 4 Jonathan Druart 2012-03-21 13:46:09 UTC Comment hidden (obsolete)
Comment 5 Koha Team University Lyon 3 2012-03-22 15:02:05 UTC
try to apply on a Lyon3 branch rebase on the git-community/master.
Can't apply :
Applying: Bug 5356: delivery place and billing place centralised in basket management
/home/kohabac/src/.git/rebase-apply/patch:76: trailing whitespace.
            $order->{'entrydate'},
error: patch failed: C4/Acquisition.pm:42
error: C4/Acquisition.pm: patch does not apply
error: patch failed: acqui/basket.pl:129
error: acqui/basket.pl: patch does not apply
error: patch failed: acqui/basketheader.pl:50
error: acqui/basketheader.pl: patch does not apply
error: patch failed: installer/data/mysql/kohastructure.sql:2525
error: installer/data/mysql/kohastructure.sql: patch does not apply
error: patch failed: koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basket.tt:134
error: koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basket.tt: patch does not apply
error: patch failed: koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basketgroup.tt:189
error: koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basketgroup.tt: patch does not apply
error: patch failed: koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basketheader.tt:125
error: koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basketheader.tt: patch does not apply
Patch failed at 0001 Bug 5356: delivery place and billing place centralised in basket management
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".
Patch left in /tmp/Bug-5356-delivery-place-and-billing-place-centrali-MNbz5d.patch
Comment 6 Jonathan Druart 2012-03-22 16:15:20 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2012-04-06 10:31:05 UTC Comment hidden (obsolete)
Comment 8 Koha Team University Lyon 3 2012-04-17 14:11:34 UTC
Applying after 7302.

/home/kohabac/src/.git/rebase-apply/patch:76: trailing whitespace.
            $order->{'entrydate'},
error: patch failed: C4/Acquisition.pm:42
error: C4/Acquisition.pm: patch does not apply
error: patch failed: installer/data/mysql/updatedatabase.pl:5146
error: installer/data/mysql/updatedatabase.pl: patch does not apply
Patch failed at 0001 Bug 5356: delivery place and billing place centralised in basket management
Comment 9 Jonathan Druart 2012-04-17 14:32:58 UTC Comment hidden (obsolete)
Comment 10 Koha Team University Lyon 3 2012-04-17 14:36:35 UTC
Still .....
Applying: Bug 5356: delivery place and billing place centralised in basket management
/home/kohabac/src/.git/rebase-apply/patch:76: trailing whitespace.
            $order->{'entrydate'},
error: patch failed: C4/Acquisition.pm:42
error: C4/Acquisition.pm: patch does not apply
Patch failed at 0001 Bug 5356: delivery place and billing place centralised in basket management
Comment 11 Jonathan Druart 2012-04-17 14:59:20 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2012-06-07 12:31:49 UTC Comment hidden (obsolete)
Comment 13 Katrin Fischer 2012-06-10 14:54:31 UTC
Tried testing this together with 7302 and directly on master, but in both cases patch does not apply.

Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all y
Applying: Bug 5356: delivery place and billing place centralised in basket management
fatal: sha1 information is lacking or useless (C4/Acquisition.pm).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 5356: delivery place and billing place centralised in basket management
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".
Comment 14 Jonathan Druart 2012-06-11 08:36:48 UTC Comment hidden (obsolete)
Comment 15 Katrin Fischer 2012-06-11 10:12:04 UTC
Asking for a follow up for:

1) Changing the display to show the library name instead of branchcode on the basket summary page. Currently shows:

Basketgroup Delivery place: MPL
Basketgroup Billing place: MPL

2) Maybe changing the branchcode to the description in the CSV export file ?
Not sure about this one.

3) On basket header the pull downs for delivery place and billing place don't sort alphabetically by description, but I think by code.

4) Same sorting problem for the pull downs on the basket group page.

Failing QA for:

5) I am testing this on top of 7302 and the colums for basket and basket group delivery and shipping places remain empty in my exported basket group CSV.

Jonathan- could you perhaps fix that in a follow up?
Comment 16 Jonathan Druart 2012-06-11 13:24:52 UTC Comment hidden (obsolete)
Comment 17 Katrin Fischer 2012-06-17 19:32:28 UTC Comment hidden (obsolete)
Comment 18 Katrin Fischer 2012-06-17 19:43:18 UTC Comment hidden (obsolete)
Comment 19 Katrin Fischer 2012-06-17 19:43:26 UTC Comment hidden (obsolete)
Comment 20 Paul Poulain 2012-07-24 16:48:12 UTC
This patch does not apply on C4/Acquisition.pm:
<<<<<<< HEAD
    ModBasketHeader($basket, $basketname || '', $basketnote || '', $basketbooksellernote || '', $basketcontractnumber || undef, $booksellerid);
=======
    ModBasketHeader( $basket, $basketname || '', $basketnote || '', $basketbooksellernote || '', $basketcontractnumber || undef, $deliveryplace || undef, $billingplace || undef );
>>>>>>> Bug 5356: delivery place and billing place centralised in basket management


It seems it's a consequence of bug 8247 patch.

I think the solution is to have:
    ModBasketHeader( $basket, $basketname || '', $basketnote || '', $basketbooksellernote || '', $basketcontractnumber || undef, $deliveryplace || undef, $billingplace || undef, $booksellerid );

and rewrite ModBasketHeader accordingly (ie = reintroduce booksellerid at the end of the API)

QA comments:
perlcritic does not pass:
perlcritic acqui/basket.pl acqui/basketheader.pl
acqui/basket.pl source OK
acqui/basketheader.pl: Variable declared in conditional statement at line 135, column 9.  Declare variables outside of the condition.  (Severity: 5)
acqui/basketheader.pl: Variable declared in conditional statement at line 159, column 9.  Declare variables outside of the condition.  (Severity: 5)
(they were OK before the patch

valid_tt.t, xt/author/translatable-templates.t and xt/author/valid-templates.t ar OK

It would be better to specify where the fields should be added in the table:
+    $dbh->do("ALTER TABLE aqbasket ADD deliveryplace VARCHAR(10) default NULL;");
+    $dbh->do("ALTER TABLE aqbasket ADD billingplace VARCHAR(10) default NULL;");
+    $dbh->do("ALTER TABLE aqbasket ADD freedeliveryplace VARCHAR(10) default NULL;");

add a "AFTER anotherfieldname" to the declaration. I would not have requested that only, but as the patch does not apply and I have other concerns...
Comment 21 Jonathan Druart 2012-07-25 09:14:27 UTC Comment hidden (obsolete)
Comment 22 Marcel de Rooy 2012-08-23 10:09:38 UTC
QA Comment:
Depending on earlier tests: Code looks good to me. Apparently, several older qa comments have been incorporated.
Patch applies.
Passed QA
Comment 23 Paul Poulain 2012-09-03 16:43:14 UTC
QA comment:
Testing this patch, there is something important missing = if you print the basketgroup PDF, you won't get the basket shipping/billing.
Libraries using that will say it's a bug, no doubts. It should not be too hard to add those columns in the tables presenting the baskets on layout2pages and layout3pages (.pm and .pdf)

Entering & exporting those new columns is OK though
Comment 24 Jonathan Druart 2012-09-04 08:05:47 UTC Comment hidden (obsolete)
Comment 25 Paul Poulain 2012-09-19 15:39:57 UTC
(In reply to comment #24)
> Created attachment 11952 [details] [review]
> Bug 5356: Followup: Add basket billing and delivery place on export pdf
> (layout3pages)
> 
> Hi Paul,
> This patch adds the basket billing place and delivery place in the generated
> pdf.
> But only for the layout3pages. Indeed, there are no space to add these
> information.

Sounds logicall not to have the information on layout2pages, as it's all basket mixed (sorted by publisher/title).
So, passed QA !
Comment 26 Paul Poulain 2012-09-19 15:49:55 UTC
Well, testing this feature & QAing the code one more time, a question arise: what is the "freedeliveryplace" done for ?
 * in aqbasketgroup, it was a TEXT, in this patch, for aqbasket it's a varchar(10), that is tiny for a free address !
 * I can't see where I can file this freedeliveryplace field

(not failing QA until i've an anwser to those questions)
Comment 27 Paul Poulain 2012-09-19 15:50:49 UTC
PS: if you switch to a text field, please don't do in a follow-up. updatedatabase conflicts are very very common, and it's a pain to apply a follow-up that update updatedatabase !
Comment 28 Jonathan Druart 2012-09-20 10:01:41 UTC Comment hidden (obsolete)
Comment 29 Jonathan Druart 2012-09-20 10:02:37 UTC Comment hidden (obsolete)
Comment 30 Jonathan Druart 2012-09-20 10:03:33 UTC
(In reply to comment #26)
> Well, testing this feature & QAing the code one more time, a question arise:
> what is the "freedeliveryplace" done for ?
>  * in aqbasketgroup, it was a TEXT, in this patch, for aqbasket it's a
> varchar(10), that is tiny for a free address !
>  * I can't see where I can file this freedeliveryplace field
> 
> (not failing QA until i've an anwser to those questions)

Paul,
In fact the aqbasket.freedeliveryplace is unused.
So I removed it in the last patch.
Comment 31 Paul Poulain 2012-09-20 16:03:27 UTC
QA comment:
I think this line is wrong:
+                basketgroupdeliveryplace => C4::Branch::GetBranchName( $basketgroup->{deliveryplace} ) || C4::Branch::GetBranchName( $basketgroup->{freedeliveryplace} ),

C4::Branch::GetBranchName( $basketgroup->{freedeliveryplace} ) will never work, because freedeliveryplace is free, so not related to a branch code !

do we agree ?

(found this problem when checking if freedeliveryplace had been removed from your patches ;-) )
Comment 32 Jonathan Druart 2012-09-21 08:29:34 UTC Comment hidden (obsolete)
Comment 33 Jonathan Druart 2012-09-21 08:30:32 UTC
(In reply to comment #31)
> QA comment:
> I think this line is wrong:
> +                basketgroupdeliveryplace => C4::Branch::GetBranchName(
> $basketgroup->{deliveryplace} ) || C4::Branch::GetBranchName(
> $basketgroup->{freedeliveryplace} ),
> do we agree ?

Yes. I removed it in the last patch.
Comment 34 Paul Poulain 2012-09-24 19:33:28 UTC
Patch pushed. a small follow-up will be required in the display of layout3pages (screenshot attached)
When the library description is long, it goes to far on the right. We should put things on 2 lines (and/or reduce the font size)
Comment 35 Paul Poulain 2012-09-24 19:34:11 UTC
Created attachment 12484 [details]
Screenshot of library name too far on the right
Comment 36 Jonathan Druart 2012-10-16 07:23:37 UTC
Created attachment 12838 [details] [review]
Bug 5356: Followup Change font in the layout3pages pdf
Comment 37 Paul Poulain 2012-10-17 16:00:40 UTC
tiny follow-up tested, fixes the problem by using a smaller font, well done