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

*** Bug 5361 has been marked as a duplicate of this bug. ***

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

First patch no longer applies... probably a conflict in updatedatabase

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.

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

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.

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

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

Ok, I mark it as 'failed QA' and will resubmit a patch soon ;)

Wanted to sign off after my vacancies, first patch does not apply, sorry.

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

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

Hmm, that was my first multi-patch sign-off, hope it is OK.

QA Question:
return unless $ordernumber or $basketno;

Should this not be an AND ?

(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

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

All patches rebased on master

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

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

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

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 ?

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

Hi. It opened in a new tab in my case too.
M. Saby

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

You are true, I wanted to set it to failed QA...
M. Saby

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.

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

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!

Patches rebased on master

Julian, Could you add a unit test for the new subroutine (C4::Acquisition::TransferOrder) please ?

Marked as Failed QA.

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.

Hi Katrin,

I chose another solution: a separate table for order transfers.
I hope this will make everyone happy :)

Fix some typos and useless use of date plugin in last patch.

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

All patches rebased on master.

Patch tested with a sandbox, by Kohateam <koha@univ-lyon3.fr>

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.

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.

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>

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>

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>

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

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>

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>

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

Pushed to master, along with a following that implements some of Owen's suggestions regarding the popup form.  Thanks, Julian!