Bug 7162

If you want to participate to the upstreaming:
* assign the bug to yourself
* retrieve the patch(es) on git.biblibre.com
* the branch name can be found using
git branch -a|grep NNNN
Usually, it should be 
  biblibre/customer/ft/MTNNNN
For example:
   remotes/origin/stetienne/ft/MT6522

This patch is in branch number 6539

If you want to participate to the upstreaming:
* assign the bug to yourself
* retrieve the patch(es) on git.biblibre.com
* the branch name can be found using
git branch -a|grep NNNN
Usually, it should be 
  biblibre/customer/ft/MTNNNN
For example:
   remotes/origin/stetienne/ft/MT6522

This patch is in branch number 6845

Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all y
Applying: Bug 7162: Factorize code for order cancellation
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging C4/Acquisition.pm
Auto-merging acqui/addorder.pl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basket.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/acqui/parcel.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/acqui/parcel.tt
Failed to merge in the changes.
Patch failed at 0001 Bug 7162: Factorize code for order cancellation
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".

What functionality needs to be tested to confirm that this patch works? A test plan would be very useful here.

i apply the patch :

when i delete a order since recept sceen i have a message (confirmation) but i can't add a reason for cancellation 

and the order is deleted not cancelled.... so i don't understand the funtions of this patch

when i use the link delete order in the screen parcel.pl i have the erreur 404
and url
../cgi-bin/koha/acqui/cancelorder.pl?ordernumber=13&biblionumber=516&referrer=/cgi-bin/koha/acqui/parcel.pl%3Fsupplierid=%26datereceived=2012-03-12%26invoice=

On receive, when i choose "delete order", the command is cancelled and i can give a reason and refactoring is OK. But i return on a page where i can't see the other lines to recept. I should be redirect on this URL : ... /acqui/parcel.pl?invoice=9767678976&booksellerid=3&gst=0&datereceived=2012-03-19. On receive, if i choose "delete order and catalog record" after giving the reason for cancellation, there's a software error : Undefined subroutine &C4::Search::SimpleSearch called at /home/koha/src/C4/Items.pm line 2465.(URL :acqui/cancelorder.pl?ordernumber=8&biblionumber=6&del_biblio=1&referrer=/cgi-bin/koha/acqui/parcel.pl%3Fsupplierid=%26datereceived=2012-03-19%26invoice=9767678976)

The "Undefined subroutine" error is not directly related to this patch. I've created another bug to fix it (bug 7755)

Bug 7755 has been resolved, so changing status to needs signoff

After submit the cancellation i can't return in the basket 
I am directed in a screen who  display this information
Receipt summary for [ test ] on 06/04/2012

Invoice number: test Received by: test On: 06/04/2012
Pending orders
There are no pending orders.
Already received
There are no received orders.

And when i clik on the button "Finish receiving" i come back in this screen
../cgi-bin/koha/acqui/parcels.pl?booksellerid=
eceive shipment from vendor
Receive a new shipment

   1. Vendor invoice
   2. Shipment date:

ok for cancellation and for the reason given for cancellation on any page of receipt (after filter or not).
But,
 - on the page booksellers.pl, in the table, items count and item expected are not updated
- when I choose "Delete order and catalog record", in basket.pl, in the table "Cancelled orders", the title is "Deleted bibliographic record, can't find title" : it's a bit confusing and I may need to know wich title is deleted.

The first point is blocking according to me

First point is introduced by bug 7169, but since this one is closed, I will fix it in this bug.

Second point is not a bug, the record is deleted so there is no way to display its title. It is how Koha works.

Hi Julian, 
perhaps the title could be retrieved from the deletedbiblio table?

(In reply to comment #17)
> Hi Julian, 
> perhaps the title could be retrieved from the deletedbiblio table?

As it's the actual behaviour to display "Deleted bibliographic record, can't find title" for deleted biblios, and this patch is not related to this, I think this could be a topic for another bug and this should not block this one to be signed off.

I disagree here with you in part. I think especially in acq you need to know what you cancelled, not displaying the deleted title really shoudl be fixed.
This patch is not easy to test, because you have to run through a lot of test cases to do it right (items checked out, subscriptions, items on another order, item title level holds on the items, etc.). So I think it might be worth adding it.

(In reply to comment #19)
> I think especially in acq you need to know what you cancelled, not displaying
> the deleted title really shoudl be fixed.

I agree with that. But it is not the purpose of this bug, which is only to
remove duplicated code (ok, it also adds the possibility to give a reason for
cancellation). It should not block the patch to pass.

> This patch is not easy to test, because you have to run through a lot of test
> cases to do it right (items checked out, subscriptions, items on another
> order, item title level holds on the items, etc.). So I think it might be
> worth adding it.

Order cancellation does not imply deletion of biblio record. You can cancel your
orders and keep your records. I don't understand how it could make testing
easier.

I'll add a new bug for your request.

Ok. I was thinking because there are 2 options fro cancelling, cancel only the order and delete the items, or cancel order and delete the record. So the second option produces the confusing display. But probably you are right about keeping both things separate.

=> Bug 8638 created

Rebased on master

cancel -> ok...but when we click on the link "click ici to return to previous page", we've got this message "Can't use an undefined value as an ARRAY reference at /home/koha/src/acqui/parcel.pl line 160."

Corinne, this patch should fix your problem.

Patch tested with a sandbox, by Corinne Bulac <corinne.hayet@bulac.fr>

Rebased patches (the second one is become useless).

Starting with some tests and a code review for these patches:

1) There are some problems with those patches pointed out by the QA script:

 FAIL	acqui/addorder.pl
   OK	  pod
   FAIL	  forbidden patterns
		forbidden pattern: tab char (line 126)
   OK	  valid
   OK	  critic

 FAIL	acqui/cancelorder.pl
   OK	  pod
   OK	  forbidden patterns
   FAIL	  valid
		Use of qw(...) as parentheses is deprecated 
   OK	  critic

2) cancelorder.pl notes this:
+# FIXME: C4::Search is needed by C4::Items::GetAnalyticsCount, which is called
+# by C4::Acquisition::DelOrder. But C4::Search is not imported by C4::Items.
+# Once this problem is resolved, the following line can be removed.
+# See Bug 7847.
+use C4::Search;
7847 is now marked resolved, can this line be removed?

3) From looking at the code it seems like you no longer have the choice to delete the bibliographic record or to keep it.

4) Some little typos:
canceled -> cancelled
occured -> occurred

5) Just a pet peeve of mine: a linked 'here' is not friendly if you are quickly scanning a page for the right link. It's always better to link the words that actually describe what the link will do:
<p>Click <a href="[% referrer %]">here</a> to return to previous page</p>
Could just be a linked: Return to previous page
Or maybe just a 'OK'?

6) A bad one: Please please, don't add untranslatable strings to the database:

+    if($reason) {
+        $query .= "
+            , notes = IF(notes IS NULL,
+                CONCAT('Cancellation reason: ', ?),
+                CONCAT(notes, ' - Cancellation reason: ', ?)
+            )
+        ";
+    }

We really need to take care of that in a nicer way or at least not have any hardcoded string in there or have it in the template.

It might be worth having an authorized value and a separate field to store cancellation reasons as you might want to look up the reasons in reports in a nice way later on.

Hi Katrin,
Thanks for the review.

Before I sumbit the follow-up, a few remarks:
1) I didn't found any "qw" in the patches.
3) What makes you thinking this? This is still possible, there are two links in basket.pl and parcel.pl: "Delete order" and "Delete order and bibliographic record"

Hi Julian,

I will check if my QA script still gives me that error - I realized a bit later while testing that I didn't have the newest version of it.

3) Sorry, misread the code, all good then.

Applying: Bug 7162: Factorize code for order cancellation
Using index info to reconstruct a base tree...
M	C4/Acquisition.pm
M	acqui/addorder.pl
M	koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basket.tt
M	koha-tmpl/intranet-tmpl/prog/en/modules/acqui/parcel.tt
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/acqui/parcel.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/acqui/parcel.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basket.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basket.tt
Auto-merging acqui/addorder.pl
CONFLICT (content): Merge conflict in acqui/addorder.pl
Auto-merging C4/Acquisition.pm
Patch failed at 0001 Bug 7162: Factorize code for order cancellation
The copy of the patch that failed is found in:
   /home/christopher/git/koha/.git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

All patches rebased on master

Patch applied cleanly, go forth and signoff

Applying: Bug 7162: Factorize code for order cancellation
Using index info to reconstruct a base tree...
M	C4/Acquisition.pm
M	koha-tmpl/intranet-tmpl/prog/en/modules/acqui/parcel.tt
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/acqui/parcel.tt
Auto-merging C4/Acquisition.pm
CONFLICT (content): Merge conflict in C4/Acquisition.pm
Patch failed at 0001 Bug 7162: Factorize code for order cancellation
The copy of the patch that failed is found in:
   /home/christopher/git/koha/.git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

Patches rebased on master

The sandbox you've requested is not ready.
Some problems occurred applying patches from bug 7162:
<h1>Something went wrong !</h1>Applying: Bug 7162: Factorize code for order cancellation
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging C4/Acquisition.pm
CONFLICT (content): Merge conflict in C4/Acquisition.pm
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basket.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/acqui/parcel.tt
Failed to merge in the changes.
Patch failed at 0001 Bug 7162: Factorize code for order cancellation
When you have resolved this problem run git bz apply --continue.
If you would prefer to skip this patch, instead run git bz apply --skip.
To restore the original branch and stop patching run git bz apply --abort.
Bug 7162 - Factorize code for order cancellation

24075 - [SIGNED-OFF] Bug 7162: Factorize code for order cancellation
24076 - [SIGNED-OFF] Bug 7162 [Follow-up] Fix referrer URL when coming from parcel.pl
24077 - Bug 7162; Factorize code for order cancellation (QA fixes)

Apply? [(y)es, (n)o, (i)nteractive] Patch left in /tmp/SIGNED-OFF-Bug-7162-Factorize-code-for-order-cance-CAcZ3U.patch .

Applying: Bug 7162; Factorize code for order cancellation (QA fixes)
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 7162; Factorize code for order cancellation (QA fixes)

So I pass the patch to "Patch doesn't apply" status

I apply the patch against 3.15.00.052 

I find some tab characters in acqui/addorder.pl (I use cat -T , and grep '\^I').

So I pass the patch to "Failed QA" status.

Tabulations don't block you to test I guess, so you can sign it off and let this check for the QA step.

I apply against master 3.17.00.007

Everything's OK.

So I pass the patch to "Signed Off" status.

Julian, could you please provide unit tests for C4::Acquisition::DelOrder?

Working on this now...

Starting with QA script, unit tests and code review:

1) QA script:

 FAIL	t/db_dependent/Acquisition.t
   FAIL	  valid
		"my" variable $order2 masks earlier declaration in same scope 
		"my" variable $error masks earlier declaration in same scope 
   OK	  critic
   OK	  pod
   OK	  forbidden patterns

 FAIL	acqui/cancelorder.pl
   OK	  valid
   FAIL	  forbidden patterns
		forbidden pattern: Koha is now under the GPLv3 license (line 8)
   OK	  pod
   OK	  critic

 FAIL	acqui/addorder.pl
   OK	  valid
   FAIL	  forbidden patterns
		forbidden pattern: tab char (line 126)
   OK	  pod
   OK	  critic

2) Code review

a) Not sure here, but should the copyright year be updated?
+# Copyright 2011 BibLibre SARL

b) Capitalization
+          <input type="submit" class="approve" value="Yes, Cancel (Y)" accesskey="y" />
+          <input type="submit" class="deny" value="No, Don't Cancel (N)" accesskey="n" onclick="window.location='[% referrer %]';return false;" />


Could you please fix in a follow up? I will then try to come back to this as soon as possible.

This works ok, but I'd like to see a follow up removing the (N) and (Y) as the keyboard shortcuts don't seem to work - other option of course is to make them work :)

But, not a blocker.

Possible enhancements:
- Make the cancellation reason searchable
- Make it possible to edit the reason later on

Ah, I am sorry, but the tests are failing. I got the wrong number of tests when I fixed the conflict in that line, but there are also quite a few tests failing.

t/db_dependent/Acquisition.t .. Dubious, test returned 8 (wstat 2048, 0x800)
Failed 8/91 subtests 

Test Summary Report
-------------------
t/db_dependent/Acquisition.t (Wstat: 2048 Tests: 91 Failed: 8)
  Failed tests:  17, 19, 22, 24, 28, 30, 34, 36
  Non-zero exit status: 8

I am going to pass this in order to not risk more conflicts - but there is something small that should be fixed in a follow up on a separate bug. The dialog says (Y) and (N) in the submit buttons, but those keyboard shortcuts don't work and should be removed or made working.

Hello
I know it has passed QA, but I would like to make some remarks, especially regarding bug 10869.
Currently, the 1st parameter passed to DelOrder is the biblionumber of the record. This make the cancelation of an order impossible if the record linked with the order is deleted (and yes, it happens in real life ;-) ).
So bug 10869 tries to fix that by inversing the 2 parameters of DelOrder : ordernumber (mandatory), then biblionumber (optional).
When bug 7162 is pushed, the logic of bug 10869 should be changed, because we will have 4 parameters : $bibnum, $ordernumber, $delete_biblio, $reason
I suppose the best way to make $bibnum optional, as well as $delete_biblio and $reason would be to pass a hashref to DelOrder instead of the 4 separate values?

Could you give me your opinion about that?

M. Saby

Julian, can you please rebase?

Created attachment 32895 [details] [review]
[PASSED QA] Bug 7162: Factorize code for order cancellation

Rebased on master

Created attachment 32896 [details] [review]
[PASSED QA] Bug 7162 [Follow-up] Fix referrer URL when coming from parcel.pl

Rebased on master

Created attachment 32897 [details] [review]
[PASSED QA] Bug 7162; Factorize code for order cancellation (QA fixes)

Rebased on master

Created attachment 32898 [details] [review]
[PASSED QA] Bug 7162: Add unit tests for DelOrder

Rebased on master

Created attachment 32899 [details] [review]
[PASSED QA] Bug 7162: Factorize code for order cancellation (QA fixes)

Rebased on master

Created attachment 32900 [details] [review]
[PASSED QA] Bug 7162: Remove "(Y)" and "(N)" from buttons text

Rebased on master

Created attachment 32902 [details] [review]
[PASSED QA] Bug 7162: (follow-up) Add unit tests for DelOrder

Rebased on master

Patches pushed to master.

Thanks Julian and everyone involved in discussing and testing!