Bug 5356

BibLibre ref: MT5833

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.

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

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

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

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".

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?

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...

QA Comment:
Depending on earlier tests: Code looks good to me. Apparently, several older qa comments have been incorporated.
Patch applies.
Passed QA

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

(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 !

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)

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 !

(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.

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 ;-) )

(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.

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)

Created attachment 12484 [details]
Screenshot of library name too far on the right

Created attachment 12838 [details] [review]
Bug 5356: Followup Change font in the layout3pages pdf

tiny follow-up tested, fixes the problem by using a smaller font, well done