Bug 7162 - Factorize code for order cancellation and add field for giving a reason
Summary: Factorize code for order cancellation and add field for giving a reason
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P3 enhancement (vote)
Assignee: Julian Maurice
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on: 7755
Blocks: 7163 13380
  Show dependency treegraph
 
Reported: 2011-11-05 07:13 UTC by Julian Maurice
Modified: 2015-12-03 22:00 UTC (History)
12 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Proposed patch (16.52 KB, patch)
2011-11-05 07:15 UTC, Julian Maurice
Details | Diff | Splinter Review
Rebased patch (15.39 KB, patch)
2012-01-02 08:25 UTC, Julian Maurice
Details | Diff | Splinter Review
Factorize code for order cancellation (17.92 KB, patch)
2012-04-12 14:18 UTC, Julian Maurice
Details | Diff | Splinter Review
Follow-up: Fix booksellers.pl display (934 bytes, patch)
2012-04-18 13:09 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 7162: Factorize code for order cancellation (15.19 KB, patch)
2012-11-02 13:25 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 7162: Fix booksellers.pl display (870 bytes, patch)
2012-11-02 13:26 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 7162: Factorize code for order cancellation (15.29 KB, patch)
2013-03-19 12:10 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 7162: Fix booksellers.pl display (872 bytes, patch)
2013-03-19 12:10 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 7162 [Follow-up] Fix referrer URL when coming from parcel.pl (2.40 KB, patch)
2013-03-19 15:17 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 7162 [Follow-up] Fix referrer URL when coming from parcel.pl (2.46 KB, patch)
2013-03-20 08:54 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 7162: Factorize code for order cancellation (15.35 KB, patch)
2013-05-29 10:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7162 [Follow-up] Fix referrer URL when coming from parcel.pl (2.46 KB, patch)
2013-05-29 10:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7162; Factorize code for order cancellation (QA fixes) (6.89 KB, patch)
2013-07-01 13:53 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7162: Factorize code for order cancellation (15.53 KB, patch)
2013-09-23 07:48 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7162 [Follow-up] Fix referrer URL when coming from parcel.pl (2.46 KB, patch)
2013-09-23 07:48 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 7162; Factorize code for order cancellation (QA fixes) (6.90 KB, patch)
2013-09-23 07:48 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7162: Factorize code for order cancellation (15.56 KB, patch)
2013-11-05 14:17 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7162 [Follow-up] Fix referrer URL when coming from parcel.pl (2.46 KB, patch)
2013-11-05 14:17 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 7162; Factorize code for order cancellation (QA fixes) (6.21 KB, patch)
2013-11-05 14:17 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7162: Factorize code for order cancellation (15.61 KB, patch)
2014-01-09 13:48 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7162 [Follow-up] Fix referrer URL when coming from parcel.pl (2.46 KB, patch)
2014-01-09 13:48 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 7162; Factorize code for order cancellation (QA fixes) (6.21 KB, patch)
2014-01-09 13:49 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 7162: Factorize code for order cancellation (15.62 KB, patch)
2014-03-13 09:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7162 [Follow-up] Fix referrer URL when coming from parcel.pl (2.46 KB, patch)
2014-03-13 09:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7162; Factorize code for order cancellation (QA fixes) (6.21 KB, patch)
2014-03-13 09:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7162; Factorize code for order cancellation (QA fixes) (6.26 KB, patch)
2014-05-20 11:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7162: Factorize code for order cancellation (16.03 KB, patch)
2014-05-29 16:14 UTC, Paola Rossi
Details | Diff | Splinter Review
Bug 7162 [Follow-up] Fix referrer URL when coming from parcel.pl (2.55 KB, patch)
2014-05-29 16:15 UTC, Paola Rossi
Details | Diff | Splinter Review
Bug 7162 Factorize code for order cancellation (QA fixes) (6.47 KB, patch)
2014-05-29 16:16 UTC, Paola Rossi
Details | Diff | Splinter Review
Bug 7162: Add unit tests for DelOrder (2.37 KB, patch)
2014-06-13 10:56 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 7162: Factorize code for order cancellation (QA fixes) (4.02 KB, patch)
2014-07-07 07:28 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 7162: Factorize code for order cancellation (QA fixes) (5.08 KB, patch)
2014-07-07 07:50 UTC, Julian Maurice
Details | Diff | Splinter Review
[PASSED QA] Bug 7162: Factorize code for order cancellation (15.73 KB, patch)
2014-07-12 20:38 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 7162 [Follow-up] Fix referrer URL when coming from parcel.pl (2.57 KB, patch)
2014-07-12 20:38 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 7162; Factorize code for order cancellation (QA fixes) (6.38 KB, patch)
2014-07-12 20:38 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 7162: Add unit tests for DelOrder (2.43 KB, patch)
2014-07-12 20:38 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 7162: Factorize code for order cancellation (QA fixes) (5.33 KB, patch)
2014-07-12 20:38 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 7162: (follow-up) Add unit tests for DelOrder (1.31 KB, patch)
2014-07-15 12:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
[PASSED QA] Bug 7162: Factorize code for order cancellation (15.75 KB, patch)
2014-07-20 20:19 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 7162 [Follow-up] Fix referrer URL when coming from parcel.pl (2.57 KB, patch)
2014-07-20 20:19 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 7162; Factorize code for order cancellation (QA fixes) (6.38 KB, patch)
2014-07-20 20:20 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 7162: Add unit tests for DelOrder (2.43 KB, patch)
2014-07-20 20:20 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 7162: Factorize code for order cancellation (QA fixes) (5.39 KB, patch)
2014-07-20 20:20 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 7162: (follow-up) Add unit tests for DelOrder (1.44 KB, patch)
2014-07-20 20:21 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 7162: Remove "(Y)" and "(N)" from buttons text (1.29 KB, patch)
2014-07-21 09:31 UTC, Julian Maurice
Details | Diff | Splinter Review
[PASSED QA] Bug 7162: Remove "(Y)" and "(N)" from buttons text (1.40 KB, patch)
2014-07-21 21:55 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 7162: Factorize code for order cancellation (15.73 KB, patch)
2014-10-29 12:56 UTC, Julian Maurice
Details | Diff | Splinter Review
[PASSED QA] Bug 7162 [Follow-up] Fix referrer URL when coming from parcel.pl (2.50 KB, patch)
2014-10-29 12:57 UTC, Julian Maurice
Details | Diff | Splinter Review
[PASSED QA] Bug 7162; Factorize code for order cancellation (QA fixes) (6.34 KB, patch)
2014-10-29 12:57 UTC, Julian Maurice
Details | Diff | Splinter Review
[PASSED QA] Bug 7162: Add unit tests for DelOrder (2.36 KB, patch)
2014-10-29 12:57 UTC, Julian Maurice
Details | Diff | Splinter Review
[PASSED QA] Bug 7162: Factorize code for order cancellation (QA fixes) (5.02 KB, patch)
2014-10-29 12:58 UTC, Julian Maurice
Details | Diff | Splinter Review
[PASSED QA] Bug 7162: Remove "(Y)" and "(N)" from buttons text (1.29 KB, patch)
2014-10-29 12:58 UTC, Julian Maurice
Details | Diff | Splinter Review
[PASSED QA] Bug 7162: (follow-up) Add unit tests for DelOrder (1.43 KB, patch)
2014-10-29 13:02 UTC, Julian Maurice
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Julian Maurice 2011-11-05 07:13:52 UTC
Some code was duplicated, in the following patch all is now in a new file cancelorder.pl. It takes a 'referrer' parameter so it can easily be called from any page of Koha, and then redirect to it when it finish.
Confirmation for cancellation is now done in this page (not in javascript).
Add possibility to give a reason for cancellation (or other things, this is saved in aqorders.notes)
DelOrder is modified to take two new parameters:
  - delete_biblio: if '1', try to delete biblio
  - reason: text to add to aqorders.notes (this note isn't show anywhere since there is no place to view cancelled orders, but another patch will add this feature).
basket.pl and parcel.pl links to this page.
Comment 1 Julian Maurice 2011-11-05 07:15:29 UTC Comment hidden (obsolete)
Comment 2 Paul Poulain 2011-11-05 10:53:18 UTC
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
Comment 3 Paul Poulain 2011-11-05 10:58:38 UTC
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
Comment 4 Katrin Fischer 2011-12-21 20:47:13 UTC
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".
Comment 5 Julian Maurice 2012-01-02 08:25:10 UTC Comment hidden (obsolete)
Comment 6 Jared Camins-Esakov 2012-03-10 16:23:39 UTC
What functionality needs to be tested to confirm that this patch works? A test plan would be very useful here.
Comment 7 delaye 2012-03-12 11:08:22 UTC
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
Comment 8 delaye 2012-03-12 15:51:03 UTC
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=
Comment 9 Koha Team University Lyon 3 2012-03-19 13:26:11 UTC
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)
Comment 10 Julian Maurice 2012-03-20 11:08:48 UTC
The "Undefined subroutine" error is not directly related to this patch. I've created another bug to fix it (bug 7755)
Comment 11 Julian Maurice 2012-04-06 11:43:55 UTC
Bug 7755 has been resolved, so changing status to needs signoff
Comment 12 delaye 2012-04-06 13:16:38 UTC
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:
Comment 13 Julian Maurice 2012-04-12 14:18:36 UTC Comment hidden (obsolete)
Comment 14 Koha Team University Lyon 3 2012-04-16 18:31:38 UTC
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
Comment 15 Julian Maurice 2012-04-18 13:03:50 UTC
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.
Comment 16 Julian Maurice 2012-04-18 13:09:15 UTC Comment hidden (obsolete)
Comment 17 Katrin Fischer 2012-04-18 13:37:51 UTC
Hi Julian, 
perhaps the title could be retrieved from the deletedbiblio table?
Comment 18 Julian Maurice 2012-08-14 09:06:08 UTC
(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.
Comment 19 Katrin Fischer 2012-08-14 10:23:21 UTC
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.
Comment 20 Julian Maurice 2012-08-14 11:57:06 UTC
(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.
Comment 21 Katrin Fischer 2012-08-14 12:05:14 UTC
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.
Comment 22 Julian Maurice 2012-08-14 12:12:24 UTC
=> Bug 8638 created
Comment 23 Julian Maurice 2012-11-02 13:25:32 UTC Comment hidden (obsolete)
Comment 24 Julian Maurice 2012-11-02 13:26:02 UTC Comment hidden (obsolete)
Comment 25 Julian Maurice 2013-03-19 12:10:31 UTC Comment hidden (obsolete)
Comment 26 Julian Maurice 2013-03-19 12:10:38 UTC Comment hidden (obsolete)
Comment 27 Julian Maurice 2013-03-19 12:11:02 UTC
Rebased on master
Comment 28 Corinne BULAC 2013-03-19 14:15:27 UTC
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."
Comment 29 Julian Maurice 2013-03-19 15:17:14 UTC Comment hidden (obsolete)
Comment 30 Julian Maurice 2013-03-19 15:18:05 UTC
Corinne, this patch should fix your problem.
Comment 31 Paul Poulain 2013-03-20 08:54:05 UTC
Patch tested with a sandbox, by Corinne Bulac <corinne.hayet@bulac.fr>
Comment 32 Paul Poulain 2013-03-20 08:54:27 UTC Comment hidden (obsolete)
Comment 33 Jonathan Druart 2013-05-29 10:19:46 UTC Comment hidden (obsolete)
Comment 34 Jonathan Druart 2013-05-29 10:19:53 UTC Comment hidden (obsolete)
Comment 35 Jonathan Druart 2013-05-29 10:20:51 UTC
Rebased patches (the second one is become useless).
Comment 36 Katrin Fischer 2013-06-30 22:05:45 UTC
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.
Comment 37 Julian Maurice 2013-07-01 13:50:52 UTC
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"
Comment 38 Julian Maurice 2013-07-01 13:53:37 UTC Comment hidden (obsolete)
Comment 39 Katrin Fischer 2013-07-01 14:00:41 UTC
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.
Comment 40 I'm just a bot 2013-09-18 09:50:32 UTC
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".
Comment 41 Julian Maurice 2013-09-23 07:48:03 UTC Comment hidden (obsolete)
Comment 42 Julian Maurice 2013-09-23 07:48:15 UTC Comment hidden (obsolete)
Comment 43 Julian Maurice 2013-09-23 07:48:28 UTC Comment hidden (obsolete)
Comment 44 Julian Maurice 2013-09-23 07:48:48 UTC
All patches rebased on master
Comment 45 I'm just a bot 2013-09-23 22:06:47 UTC
Patch applied cleanly, go forth and signoff
Comment 46 I'm just a bot 2013-10-24 19:30:27 UTC
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".
Comment 47 Julian Maurice 2013-11-05 14:17:04 UTC Comment hidden (obsolete)
Comment 48 Julian Maurice 2013-11-05 14:17:28 UTC Comment hidden (obsolete)
Comment 49 Julian Maurice 2013-11-05 14:17:42 UTC Comment hidden (obsolete)
Comment 50 Julian Maurice 2014-01-09 13:48:33 UTC Comment hidden (obsolete)
Comment 51 Julian Maurice 2014-01-09 13:48:46 UTC Comment hidden (obsolete)
Comment 52 Julian Maurice 2014-01-09 13:49:02 UTC Comment hidden (obsolete)
Comment 53 Julian Maurice 2014-01-09 13:49:44 UTC
Patches rebased on master
Comment 54 Koha Team University Lyon 3 2014-03-13 09:17:50 UTC
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 .
Comment 55 Jonathan Druart 2014-03-13 09:59:37 UTC Comment hidden (obsolete)
Comment 56 Jonathan Druart 2014-03-13 09:59:45 UTC Comment hidden (obsolete)
Comment 57 Jonathan Druart 2014-03-13 09:59:51 UTC Comment hidden (obsolete)
Comment 58 Paola Rossi 2014-05-16 15:55:44 UTC
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
Comment 59 Jonathan Druart 2014-05-20 11:50:48 UTC Comment hidden (obsolete)
Comment 60 Paola Rossi 2014-05-20 15:50:40 UTC
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.
Comment 61 Jonathan Druart 2014-05-20 16:07:32 UTC
Tabulations don't block you to test I guess, so you can sign it off and let this check for the QA step.
Comment 62 Paola Rossi 2014-05-29 16:13:55 UTC
I apply against master 3.17.00.007

Everything's OK.

So I pass the patch to "Signed Off" status.
Comment 63 Paola Rossi 2014-05-29 16:14:50 UTC Comment hidden (obsolete)
Comment 64 Paola Rossi 2014-05-29 16:15:46 UTC Comment hidden (obsolete)
Comment 65 Paola Rossi 2014-05-29 16:16:49 UTC Comment hidden (obsolete)
Comment 66 Jonathan Druart 2014-06-11 11:54:27 UTC
Julian, could you please provide unit tests for C4::Acquisition::DelOrder?
Comment 67 Julian Maurice 2014-06-13 10:56:39 UTC Comment hidden (obsolete)
Comment 68 Katrin Fischer 2014-07-06 20:18:10 UTC
Working on this now...
Comment 69 Katrin Fischer 2014-07-06 20:25:44 UTC
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.
Comment 70 Julian Maurice 2014-07-07 07:28:48 UTC Comment hidden (obsolete)
Comment 71 Julian Maurice 2014-07-07 07:50:04 UTC Comment hidden (obsolete)
Comment 72 Katrin Fischer 2014-07-12 20:31:05 UTC
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.
Comment 73 Katrin Fischer 2014-07-12 20:35:04 UTC
Possible enhancements:
- Make the cancellation reason searchable
- Make it possible to edit the reason later on
Comment 74 Katrin Fischer 2014-07-12 20:38:31 UTC Comment hidden (obsolete)
Comment 75 Katrin Fischer 2014-07-12 20:38:36 UTC Comment hidden (obsolete)
Comment 76 Katrin Fischer 2014-07-12 20:38:40 UTC Comment hidden (obsolete)
Comment 77 Katrin Fischer 2014-07-12 20:38:44 UTC Comment hidden (obsolete)
Comment 78 Katrin Fischer 2014-07-12 20:38:48 UTC Comment hidden (obsolete)
Comment 79 Katrin Fischer 2014-07-12 20:50:44 UTC
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
Comment 80 Jonathan Druart 2014-07-15 12:01:38 UTC Comment hidden (obsolete)
Comment 81 Katrin Fischer 2014-07-20 20:18:54 UTC
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.
Comment 82 Katrin Fischer 2014-07-20 20:19:35 UTC Comment hidden (obsolete)
Comment 83 Katrin Fischer 2014-07-20 20:19:54 UTC Comment hidden (obsolete)
Comment 84 Katrin Fischer 2014-07-20 20:20:05 UTC Comment hidden (obsolete)
Comment 85 Katrin Fischer 2014-07-20 20:20:23 UTC Comment hidden (obsolete)
Comment 86 Katrin Fischer 2014-07-20 20:20:35 UTC Comment hidden (obsolete)
Comment 87 Katrin Fischer 2014-07-20 20:21:13 UTC Comment hidden (obsolete)
Comment 88 Julian Maurice 2014-07-21 09:31:26 UTC Comment hidden (obsolete)
Comment 89 Katrin Fischer 2014-07-21 21:55:05 UTC Comment hidden (obsolete)
Comment 90 Mathieu Saby 2014-08-10 14:22:54 UTC
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
Comment 91 Katrin Fischer 2014-10-28 13:41:08 UTC
Julian, can you please rebase?
Comment 92 Julian Maurice 2014-10-29 12:56:52 UTC
Created attachment 32895 [details] [review]
[PASSED QA] Bug 7162: Factorize code for order cancellation

Rebased on master
Comment 93 Julian Maurice 2014-10-29 12:57:11 UTC
Created attachment 32896 [details] [review]
[PASSED QA] Bug 7162 [Follow-up] Fix referrer URL when coming from parcel.pl

Rebased on master
Comment 94 Julian Maurice 2014-10-29 12:57:39 UTC
Created attachment 32897 [details] [review]
[PASSED QA] Bug 7162; Factorize code for order cancellation (QA fixes)

Rebased on master
Comment 95 Julian Maurice 2014-10-29 12:57:58 UTC
Created attachment 32898 [details] [review]
[PASSED QA] Bug 7162: Add unit tests for DelOrder

Rebased on master
Comment 96 Julian Maurice 2014-10-29 12:58:32 UTC
Created attachment 32899 [details] [review]
[PASSED QA] Bug 7162: Factorize code for order cancellation (QA fixes)

Rebased on master
Comment 97 Julian Maurice 2014-10-29 12:58:54 UTC
Created attachment 32900 [details] [review]
[PASSED QA] Bug 7162: Remove "(Y)" and "(N)" from buttons text

Rebased on master
Comment 98 Julian Maurice 2014-10-29 13:02:23 UTC
Created attachment 32902 [details] [review]
[PASSED QA] Bug 7162: (follow-up) Add unit tests for DelOrder

Rebased on master
Comment 99 Tomás Cohen Arazi 2014-10-30 04:00:59 UTC
Patches pushed to master.

Thanks Julian and everyone involved in discussing and testing!