Bug 5349 - Moving an order
Summary: Moving an order
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Deadline: 2011-05-01
Assignee: Julian Maurice
QA Contact: Marcel de Rooy
URL: http://wiki.koha-community.org/wiki/M...
Keywords:
: 5361 (view as bug list)
Depends on:
Blocks: 7163
  Show dependency treegraph
 
Reported: 2010-11-02 03:19 UTC by Paul Poulain
Modified: 2014-12-07 20:03 UTC (History)
17 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Large patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Order line transfers (9.34 KB, patch)
2012-04-06 15:00 UTC, Julian Maurice
Details | Diff | Splinter Review
Order line transfers (20.25 KB, patch)
2012-04-06 15:03 UTC, Julian Maurice
Details | Diff | Splinter Review
Order line transfers (20.36 KB, patch)
2012-06-04 09:08 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 5349: Order line transfers (20.88 KB, patch)
2012-06-09 14:33 UTC, Marc Véron
Details | Diff | Splinter Review
Follow-up: run transferorder.pl in a popup (7.47 KB, patch)
2012-06-11 12:23 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 5349: Order line transfers (20.38 KB, patch)
2012-06-20 11:35 UTC, Julian Maurice
Details | Diff | Splinter Review
Follow-up: run transferorder.pl in a popup (7.47 KB, patch)
2012-06-20 11:36 UTC, Julian Maurice
Details | Diff | Splinter Review
Screenshots of processing a transfer, resulting in an empty window (556.43 KB, application/pdf)
2012-06-22 22:03 UTC, Marc Véron
Details
[SIGNED-OFF] Bug 5349: Order line transfers (20.40 KB, patch)
2012-06-25 09:05 UTC, Julian Maurice
Details | Diff | Splinter Review
Follow-up: run transferorder.pl in a popup (7.47 KB, patch)
2012-06-25 09:06 UTC, Julian Maurice
Details | Diff | Splinter Review
Follow-up: run transferorder.pl in a popup (9.31 KB, patch)
2012-07-05 09:14 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 5349: Order line transfers (20.69 KB, patch)
2012-07-25 07:23 UTC, Julian Maurice
Details | Diff | Splinter Review
Follow-up: run transferorder.pl in a popup (9.32 KB, patch)
2012-07-25 07:24 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 5349 follow-up: run transferorder.pl in a popup (9.35 KB, patch)
2012-08-06 13:49 UTC, Marc Véron
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 5349 follow-up: run transferorder.pl in a popup (9.35 KB, patch)
2012-08-06 13:50 UTC, Marc Véron
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 5349: Order line transfers (19.49 KB, patch)
2012-09-14 14:41 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 5349 follow-up: run transferorder.pl in a popup (8.71 KB, patch)
2012-09-14 14:41 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 5349: Fix a logical test in TransferOrder (757 bytes, patch)
2012-09-14 14:42 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 5349: Order line transfers (19.40 KB, patch)
2012-09-21 13:02 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 5349 follow-up: run transferorder.pl in a popup (8.56 KB, patch)
2012-09-21 13:02 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 5349: Fix a logical test in TransferOrder (751 bytes, patch)
2012-09-21 13:02 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 5349: Order line transfers (19.38 KB, patch)
2012-09-24 07:01 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 5349 follow-up: run transferorder.pl in a popup (8.56 KB, patch)
2012-09-24 07:02 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 5349: Fix a logical test in TransferOrder (751 bytes, patch)
2012-09-24 07:02 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 5349: Order line transfers (19.43 KB, patch)
2013-01-05 22:55 UTC, mathieu saby
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 5349 follow-up: run transferorder.pl in a popup (8.62 KB, patch)
2013-01-05 22:59 UTC, mathieu saby
Details | Diff | Splinter Review
[SIGNED-OFF] : Fix a logical test in TransferOrder (751 bytes, patch)
2013-01-05 23:01 UTC, mathieu saby
Details | Diff | Splinter Review
Bug 5349: Order line transfers (19.09 KB, patch)
2013-03-20 11:23 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 5349 follow-up: run transferorder.pl in a popup (8.64 KB, patch)
2013-03-20 11:23 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 5349: Fix a logical test in TransferOrder (753 bytes, patch)
2013-03-20 11:24 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 5349: Order line transfers (19.24 KB, patch)
2013-06-13 12:45 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 5349 follow-up: run transferorder.pl in a popup (8.64 KB, patch)
2013-06-13 12:45 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 5349: Fix a logical test in TransferOrder (753 bytes, patch)
2013-06-13 12:46 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 5349: Add unit tests for TransferOrder (2.98 KB, patch)
2013-06-13 12:46 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 5349: Use DB transaction/rollback in unit tests (1.63 KB, patch)
2013-07-02 10:50 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 5349: Create a table for order line transfers (12.70 KB, patch)
2013-07-02 10:50 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 5349: Create a table for order line transfers (12.41 KB, patch)
2013-07-02 11:35 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 5349: Order line transfers (19.18 KB, patch)
2013-07-24 06:53 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 5349 follow-up: run transferorder.pl in a popup (8.64 KB, patch)
2013-07-24 06:53 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 5349: Fix a logical test in TransferOrder (810 bytes, patch)
2013-07-24 06:53 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 5349: Add unit tests for TransferOrder (2.96 KB, patch)
2013-07-24 06:53 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 5349: Use DB transaction/rollback in unit tests (1.63 KB, patch)
2013-07-24 06:54 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 5349: Create a table for order line transfers (12.40 KB, patch)
2013-07-24 06:54 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 5349: Order line transfers (19.22 KB, patch)
2013-08-19 15:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5349 follow-up: run transferorder.pl in a popup (8.68 KB, patch)
2013-08-19 15:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5349: Fix a logical test in TransferOrder (853 bytes, patch)
2013-08-19 15:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5349: Add unit tests for TransferOrder (3.01 KB, patch)
2013-08-19 15:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5349: Use DB transaction/rollback in unit tests (1.67 KB, patch)
2013-08-19 15:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5349: Create a table for order line transfers (12.44 KB, patch)
2013-08-19 15:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5349: Order line transfers (19.27 KB, patch)
2013-08-28 10:56 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 5349 follow-up: run transferorder.pl in a popup (8.74 KB, patch)
2013-08-28 10:56 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 5349: Fix a logical test in TransferOrder (907 bytes, patch)
2013-08-28 10:57 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 5349: Add unit tests for TransferOrder (3.14 KB, patch)
2013-08-28 10:57 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 5349: Use DB transaction/rollback in unit tests (1.73 KB, patch)
2013-08-28 10:58 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 5349: Create a table for order line transfers (12.49 KB, patch)
2013-08-28 10:58 UTC, Chris Cormack
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Paul Poulain 2010-11-02 03:19:35 UTC
We will add a feature to let the librarian being able to move an order from a basket to another basket, for the same supplier or another. This feature will be useful for orders that can't be done by a given supplier. Note that from a technical point of view, the move will be a « copy / cancel » : the initial order will still be here, but commented as cancelled
Comment 1 Paul Poulain 2011-11-05 10:54:21 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 5854
Comment 2 Julian Maurice 2012-04-06 15:00:22 UTC Comment hidden (obsolete)
Comment 3 Julian Maurice 2012-04-06 15:03:42 UTC Comment hidden (obsolete)
Comment 4 Julian Maurice 2012-04-06 15:05:12 UTC
*** Bug 5361 has been marked as a duplicate of this bug. ***
Comment 5 Koha Team Lyon 3 2012-05-31 09:08:56 UTC
Tested on a sandbox :

Applying: Bug 5349: Order line transfers
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging C4/Acquisition.pm
Auto-merging installer/data/mysql/kohastructure.sql
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basket.tt
Failed to merge in the changes.
Patch failed at 0001 Bug 5349: Order line transfers
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".
Bug 5349 - Moving an order

Order line transfers
Apply? [yn]
Patch left in /tmp/Order-line-transfers-1v2GJM.patch
Comment 6 Julian Maurice 2012-06-04 09:08:54 UTC Comment hidden (obsolete)
Comment 7 Marc Véron 2012-06-09 14:33:52 UTC Comment hidden (obsolete)
Comment 8 Julian Maurice 2012-06-11 12:23:32 UTC Comment hidden (obsolete)
Comment 9 Ian Walls 2012-06-15 17:32:41 UTC
First patch no longer applies... probably a conflict in updatedatabase
Comment 10 Julian Maurice 2012-06-20 11:35:43 UTC Comment hidden (obsolete)
Comment 11 Julian Maurice 2012-06-20 11:36:13 UTC Comment hidden (obsolete)
Comment 12 Marc Véron 2012-06-22 22:03:53 UTC Comment hidden (obsolete)
Comment 13 Julian Maurice 2012-06-25 07:21:01 UTC
1/ Did you apply the follow-up patch?

2/ Do you have any extensions or custom settings that could lead to popup window opening in current tab? (I know Firefox extension Tab Mix Plus can do this)

3/ Javascript enabled?

If answers are 1/ yes 2/ no 3/ yes, please change status to failed qa and I will take a look at it.
Comment 14 Marc Véron 2012-06-25 08:18:04 UTC
Sorry, I get the following error:

Applying: Bug 5349: Order line transfers
/usr/share/kohaclone/.git/rebase-apply/patch:109: new blank line at EOF.
+
error: patch failed: installer/data/mysql/updatedatabase.pl:5369
error: installer/data/mysql/updatedatabase.pl: patch does not apply
Patch failed at 0001 Bug 5349: Order line transfers


I tried to resolve it, maybe it did not work out correctly leading to behaviour described in comment #12

Marc
Comment 15 Julian Maurice 2012-06-25 09:05:33 UTC Comment hidden (obsolete)
Comment 16 Julian Maurice 2012-06-25 09:06:13 UTC Comment hidden (obsolete)
Comment 17 Julian Maurice 2012-06-25 09:08:12 UTC
Tested right now, the transferorder.pl page is opened in a popup and doesn't end on a blank screen.
Please attach any relevant logs if you reproduce the problem.
Comment 18 Marc Véron 2012-06-25 14:35:52 UTC
Both patches applied.

I get the same result as the screenshots show in the PDF attachment:
http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=10441

At the end of koha-error_log I get a couple of following lines:

(...)[Mon Jun 25 16:20:59 2012] transferorder.pl: Use of uninitialized value in pattern match (m//) at /usr/share/kohaclone/C4/SQLHelper.pm line 407.

Additional info:

I tried with Firefox, Firefox  with all plugins disabled and with Chrome.

Could it be a database issue (corrupted data in order tables)? I'm afraid I have no time to puzzle this out right now, that's why I propose that somebody else does a test as well.

Marc
Comment 19 Marc Véron 2012-06-26 12:08:45 UTC
I spent some time to find out what happens.

It seems that the popup functionality is implemented with the JavaScript function transfer_order_popup() in 
koha-tmpl/intranet-tmpl/prog/en/modules/acqui/parcel.tt

but it is *not* implemented in

koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basket.tt
(this is the place where I tested, see the first page in my screenshots 
http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=10441 )

Marc
Comment 20 Julian Maurice 2012-06-26 13:08:07 UTC
Ok, I mark it as 'failed QA' and will resubmit a patch soon ;)
Comment 21 Julian Maurice 2012-07-05 09:14:23 UTC Comment hidden (obsolete)
Comment 22 Marc Véron 2012-07-24 19:15:40 UTC
Wanted to sign off after my vacancies, first patch does not apply, sorry.
Comment 23 Julian Maurice 2012-07-25 07:23:48 UTC Comment hidden (obsolete)
Comment 24 Julian Maurice 2012-07-25 07:24:23 UTC Comment hidden (obsolete)
Comment 25 Marc Véron 2012-07-25 09:27:59 UTC
Both patches applied, and the items can be moved between baskets as expected.

I'm not sure if the following in booksellers.pl is in the scope of this bug:

When I move an item, the basket where the item was removed from still shows the old count of "Items expected".

E.g. you have 3 items, you move one item to an other basket, I would expect "Items expected" now to be 2, but it is still 3.

Marc
Comment 26 Julian Maurice 2012-08-06 12:08:55 UTC
This issue is not related to this bug.
I've created another bug for it: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8575
Comment 27 Marc Véron 2012-08-06 13:49:26 UTC Comment hidden (obsolete)
Comment 28 Marc Véron 2012-08-06 13:50:21 UTC Comment hidden (obsolete)
Comment 29 Marc Véron 2012-08-06 13:53:51 UTC
Hmm, that was my first multi-patch sign-off, hope it is OK.
Comment 30 Marcel de Rooy 2012-08-30 09:48:00 UTC
QA Question:
return unless $ordernumber or $basketno;

Should this not be an AND ?
Comment 31 Julian Maurice 2012-09-14 14:32:23 UTC
(In reply to comment #30)
> QA Question:
> return unless $ordernumber or $basketno;
> 
> Should this not be an AND ?

Yes, this should be AND!
I will submit a follow-up
Comment 32 Julian Maurice 2012-09-14 14:41:15 UTC Comment hidden (obsolete)
Comment 33 Julian Maurice 2012-09-14 14:41:45 UTC Comment hidden (obsolete)
Comment 34 Julian Maurice 2012-09-14 14:42:17 UTC Comment hidden (obsolete)
Comment 35 Marc Véron 2012-09-19 13:40:49 UTC
First patch does not apply.

Applying: Bug 5349: Order line transfers
/usr/share/kohaclone/.git/rebase-apply/patch:109: new blank line at EOF.
+
error: patch failed: installer/data/mysql/updatedatabase.pl:5781
error: installer/data/mysql/updatedatabase.pl: patch does not apply
error: patch failed: koha-tmpl/intranet-tmpl/prog/en/modules/acqui/parcel.tt:241
error: koha-tmpl/intranet-tmpl/prog/en/modules/acqui/parcel.tt: patch does not apply
Patch failed at 0001 Bug 5349: Order line transfers
Comment 36 Julian Maurice 2012-09-21 13:02:21 UTC Comment hidden (obsolete)
Comment 37 Julian Maurice 2012-09-21 13:02:38 UTC Comment hidden (obsolete)
Comment 38 Julian Maurice 2012-09-21 13:02:47 UTC Comment hidden (obsolete)
Comment 39 Julian Maurice 2012-09-21 13:03:09 UTC
All patches rebased on master
Comment 40 Mirko Tietgen 2012-09-21 18:45:05 UTC
Does not apply, sorry :(

Applying: Bug 5349: Order line transfers
/home/pi/koha/.git/rebase-apply/patch:109: new blank line at EOF.
+
error: patch failed: installer/data/mysql/updatedatabase.pl:5966
error: installer/data/mysql/updatedatabase.pl: patch does not apply
Patch failed at 0001 Bug 5349: Order line transfers
Comment 41 Julian Maurice 2012-09-24 07:01:54 UTC Comment hidden (obsolete)
Comment 42 Julian Maurice 2012-09-24 07:02:16 UTC Comment hidden (obsolete)
Comment 43 Julian Maurice 2012-09-24 07:02:35 UTC Comment hidden (obsolete)
Comment 44 mathieu saby 2013-01-05 22:52:27 UTC
Tested on sandbox 3.
Works as expected : 1 line created and transfered in an other basket.
Tested with the same vendor, and 2 different vendors.

But I agree with Marc veron : 
"Small glitch in ergonomy: The page where you can choose the new bookseller
could better be a pop up like in Patrons->Guarantor information->Set to patron.
Otherwise users could mistake and use the Vendors search at the top of the page
(is active by default). If they do so they loose track of the transfer process."

And I add an other "glitch" : I think it could be good to indicate the name (or number?) of the Baskets, and not only of the Vendors.But this could be the fixed in a follow up.

The note in cancelled order line and new orderline created in an other basket are just mentioning vendors : 
"Transfered from L'ARTMADA on 2013-01-05"
and 
"Cancelled and transfered to CDMAIL PRO on 2013-01-05"

=> So I sign off
M. Saby
Rennes 2 university
Comment 45 mathieu saby 2013-01-05 22:55:59 UTC Comment hidden (obsolete)
Comment 46 mathieu saby 2013-01-05 22:59:31 UTC Comment hidden (obsolete)
Comment 47 mathieu saby 2013-01-05 23:01:05 UTC Comment hidden (obsolete)
Comment 48 mathieu saby 2013-01-05 23:06:01 UTC
Oh sorry I did not read well all the history.
I signed-off the 3 patchs, but I suppose "Bug 5349 follow-up: run transferorder.pl in a popup" is NOT working as it should, because I did not see any pop-up during the transfer process...

So I put the bug to "need signoff" again

M. Saby
Comment 49 Julian Maurice 2013-01-08 14:16:03 UTC
I just tried to apply the 3 patches and make a transfer. And it runs in a popup. Maybe you configured your web browser to open popups in a new tab instead of a new window ?
Can someone else try ?
Comment 50 Owen Leonard 2013-01-08 15:06:14 UTC
(In reply to comment #49)
> I just tried to apply the 3 patches and make a transfer. And it runs in a
> popup. Maybe you configured your web browser to open popups in a new tab
> instead of a new window ?

I get the popup too, but it's a full-sized window (opened in a new tab in my case). The page looks like it's designed to be a smaller, standalone popup, so I think more improvements can be made which will help confusion.

- Set a size on the window so that it doesn't potentially open as a new tab:
  
window.open(url,'TransferOrder','width=500,height=300,toolbar=false,scrollbars=yes');

- Alter the markup of transferorder.tt using a popup page like guarantor_search.tt as an example. The grid must be altered to accommodate a smaller window (adjust as necessary):

    <style type="text/css">
    #custom-doc { width:44.46em;*width:43.39em;min-width:578px; margin:auto; text-align:left; }
    </style>
Comment 51 mathieu saby 2013-01-08 16:13:04 UTC
Hi. It opened in a new tab in my case too.
M. Saby
Comment 52 Paul Poulain 2013-01-17 15:26:57 UTC
(In reply to comment #51)
> Hi. It opened in a new tab in my case too.
> M. Saby

Mathieu, I don't understand why you set this bug to "in discussion". I think it's more a "failed QA" for now, because Owen is asking for a suggestion to improve the patch, than "in discussion", where we have at least 2 options that must be discussed & decided.

am I right ?
Comment 53 mathieu saby 2013-01-17 15:53:14 UTC
You are true, I wanted to set it to failed QA...
M. Saby
Comment 54 Julian Maurice 2013-01-17 16:30:41 UTC
Hi Owen and Mathieu,

I agreed this popup page could be improved, but I don't think this should prevent the patch to pass QA.
I reset the status to Signed Off and will provide a follow-up with Owen's suggestions as soon as possible.
Feel free to reset to Failed QA if you really think this patch must not pass as it is.
Comment 55 mathieu saby 2013-01-22 09:31:09 UTC
Hello
By changing the status I just wanted to say I did not want to sign off (QA is not my job...), but after the explanations of Julian, I agree we can sign it off, as cosmetic changes can be make afterwards.

M. Saby
Comment 56 Katrin Fischer 2013-03-20 11:06:24 UTC
Hi Julian,

I have problems applying the patches, even with git am -iu3 - can you please rebase to current master and switch back to signed-off? Thx!
Comment 57 Julian Maurice 2013-03-20 11:23:48 UTC Comment hidden (obsolete)
Comment 58 Julian Maurice 2013-03-20 11:23:59 UTC Comment hidden (obsolete)
Comment 59 Julian Maurice 2013-03-20 11:24:06 UTC Comment hidden (obsolete)
Comment 60 Julian Maurice 2013-03-20 11:25:12 UTC
Patches rebased on master
Comment 61 Jonathan Druart 2013-05-29 13:39:02 UTC
Julian, Could you add a unit test for the new subroutine (C4::Acquisition::TransferOrder) please ?

Marked as Failed QA.
Comment 62 Julian Maurice 2013-06-13 12:45:22 UTC Comment hidden (obsolete)
Comment 63 Julian Maurice 2013-06-13 12:45:58 UTC Comment hidden (obsolete)
Comment 64 Julian Maurice 2013-06-13 12:46:15 UTC Comment hidden (obsolete)
Comment 65 Julian Maurice 2013-06-13 12:46:23 UTC Comment hidden (obsolete)
Comment 66 Katrin Fischer 2013-06-30 19:36:29 UTC
Hi Julian,

It looks to me like the design of internalnotes is not a good idea:

+    $order->{'internalnotes'} = "Transfered from $booksellerfromname on $today";
+    $sth->execute("Cancelled and transfered to $booksellertoname on $today", $ordernumber);

This won't ever be translatable. We should never store composed English strings in the database. 

Also I think that for a column named internalnotes I would expect a different use. Like a internal note that a librarian can set and edit. 

Other solutions I can think of are to store the transferdate and the original vendor in separate columns and compose the message in the templates. 

Or it might be worth having a column for a cancellation reason that is filled via an authorized value list. 

This way we could also give other reasons, like out of print, etc. But the date and vendor would still have to be stored somewhere else if you want to keep that information. Maybe action_log?

Maybe a quick discussion about this could help to find the best solution.
Comment 67 Julian Maurice 2013-07-02 10:50:16 UTC Comment hidden (obsolete)
Comment 68 Julian Maurice 2013-07-02 10:50:22 UTC Comment hidden (obsolete)
Comment 69 Julian Maurice 2013-07-02 10:52:06 UTC
Hi Katrin,

I chose another solution: a separate table for order transfers.
I hope this will make everyone happy :)
Comment 70 Julian Maurice 2013-07-02 11:35:43 UTC Comment hidden (obsolete)
Comment 71 Julian Maurice 2013-07-02 11:36:39 UTC
Fix some typos and useless use of date plugin in last patch.
Comment 72 David Cook 2013-07-24 00:43:31 UTC
Hey Julian,

Could you rebase this patch? (see my error message below)

I'm really keen to see this get into Koha (and hopefully 3.8.x), so I'm willing to test this as much as necessary :). 

GIT ERROR MESSAGE:

Applying: Bug 5349: Order line transfers
Using index info to reconstruct a base tree...
<stdin>:256: new blank line at EOF.
+
warning: 1 line adds whitespace errors.
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/acqui/parcel.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basket.tt
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging installer/data/mysql/kohastructure.sql
Auto-merging C4/Acquisition.pm
CONFLICT (content): Merge conflict in C4/Acquisition.pm
Failed to merge in the changes.
Patch failed at 0001 Bug 5349: Order line transfers
Comment 73 Julian Maurice 2013-07-24 06:53:21 UTC Comment hidden (obsolete)
Comment 74 Julian Maurice 2013-07-24 06:53:34 UTC Comment hidden (obsolete)
Comment 75 Julian Maurice 2013-07-24 06:53:47 UTC Comment hidden (obsolete)
Comment 76 Julian Maurice 2013-07-24 06:53:57 UTC Comment hidden (obsolete)
Comment 77 Julian Maurice 2013-07-24 06:54:05 UTC Comment hidden (obsolete)
Comment 78 Julian Maurice 2013-07-24 06:54:18 UTC Comment hidden (obsolete)
Comment 79 Julian Maurice 2013-07-24 06:54:51 UTC
All patches rebased on master.
Comment 80 sandboxes@biblibre.com 2013-08-19 14:46:19 UTC
Patch tested with a sandbox, by Kohateam <koha@univ-lyon3.fr>
Comment 81 Koha Team Lyon 3 2013-08-19 14:53:16 UTC
Tested on a sandbox. For me, everything is working as expected. I can't see if the translation is ok.
I'm not bothered by the fact of pop-up is a full-sized window in a new tab...
This patch will be really useful for the librarians who manage commands.
Comment 82 Jonathan Druart 2013-08-19 15:20:36 UTC Comment hidden (obsolete)
Comment 83 Jonathan Druart 2013-08-19 15:20:42 UTC Comment hidden (obsolete)
Comment 84 Jonathan Druart 2013-08-19 15:20:48 UTC Comment hidden (obsolete)
Comment 85 Jonathan Druart 2013-08-19 15:20:53 UTC Comment hidden (obsolete)
Comment 86 Jonathan Druart 2013-08-19 15:20:59 UTC Comment hidden (obsolete)
Comment 87 Jonathan Druart 2013-08-19 15:21:05 UTC Comment hidden (obsolete)
Comment 88 Jonathan Druart 2013-08-19 15:23:09 UTC
Note: The sandbox fails to signoff these patchs.

Error from the WWW::Bugzilla package: WWW::Mechanize find_all_links gave us a bogus URL at ./cron/sandbox_signoff.cron line 78.
Comment 89 Chris Cormack 2013-08-28 10:56:40 UTC
Created attachment 20691 [details] [review]
Bug 5349: Order line transfers

On basket.pl and parcel.pl there is a 'Transfer' link which allow you to
transfer order lines from a basket to another.
The link lead to a new page which allow you to search for a bookseller,
then display this bookseller's baskets. Then you can pickup a basket and
the transfer will be done.
This add a message in new column aqorders.internalnotes which is
displayed in basket.pl

Signed-off-by: Marc Veron <veron@veron.ch>
Signed-off-by: Mathieu Saby <mathieu.saby@univ-rennes2.fr>

Signed-off-by: sonia <koha@univ-lyon3.fr>
Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Comment 90 Chris Cormack 2013-08-28 10:56:55 UTC
Created attachment 20692 [details] [review]
Bug 5349 follow-up: run transferorder.pl in a popup

Signed-off-by: Marc Veron <veron@veron.ch>
Signed-off-by: Mathieu Saby <mathieu.saby@univ-rennes2.fr>

Signed-off-by: sonia <koha@univ-lyon3.fr>
Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Comment 91 Chris Cormack 2013-08-28 10:57:09 UTC
Created attachment 20693 [details] [review]
Bug 5349: Fix a logical test in TransferOrder

Signed-off-by: Mathieu Saby <mathieu.saby@univ-rennes2.fr>

Signed-off-by: sonia <koha@univ-lyon3.fr>
Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Comment 92 Chris Cormack 2013-08-28 10:57:22 UTC
Created attachment 20694 [details] [review]
Bug 5349: Add unit tests for TransferOrder

Signed-off-by: sonia <koha@univ-lyon3.fr>
Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Signing off on this because a following patch fixes the test to use
transactions
Comment 93 Chris Cormack 2013-08-28 10:58:09 UTC
Created attachment 20695 [details] [review]
Bug 5349: Use DB transaction/rollback in unit tests

Signed-off-by: sonia <koha@univ-lyon3.fr>
Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Comment 94 Chris Cormack 2013-08-28 10:58:24 UTC
Created attachment 20696 [details] [review]
Bug 5349: Create a table for order line transfers

This allow to keep transfers informations without having untranslatable
strings in database.

Signed-off-by: sonia <koha@univ-lyon3.fr>
Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Comment 95 Chris Cormack 2013-08-28 11:00:48 UTC
It is important to note, do not run the database update until all patches are applied, or you will end up with the db in a messy state
Comment 96 Galen Charlton 2013-09-16 15:25:46 UTC
Pushed to master, along with a following that implements some of Owen's suggestions regarding the popup form.  Thanks, Julian!