Bug 7175

Summary: Improve order receiving page
Product: Koha Reporter: Julian Maurice <julian.maurice>
Component: AcquisitionsAssignee: Julian Maurice <julian.maurice>
Status: CLOSED FIXED QA Contact: Ian Walls <koha.sekjal>
Severity: enhancement    
Priority: P2 CC: claire.hernandez, corinnebulac, jonathan.druart, katrin.fischer, mathilde.formery, nengard, paul.poulain, srdjan, stephane.delaye
Version: 3.10   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=3966
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 7163, 7896, 14449    
Attachments: orderreceive.pl
Cosmetic modification patch
Patch
[SIGNED-OFF] Bug 7175: orderreceive.pl cosmetic changes
neworder does not show item form
[Rebased] Bug 7175: orderreceive.pl cosmetic changes
[Rebased] Bug 7175: Allow to choose which items to receive
orderreceive.pl cosmetic changes
Allow to choose which items to receive
Bug 7175: Sign OFF Allow to choose which items to receive
[SIGNED-OFF] Bug 7175: orderreceive.pl cosmetic changes
[SIGNED-OFF] Bug 7175: Allow to choose which items to receive
orderreceive.pl cosmetic changes
Allow to choose which items to receive
Allow to choose which items to receive
[SIGNED-OFF] Bug 7175: Allow to choose which items to receive
[SIGNED-OFF] Bug 7175: orderreceive.pl cosmetic changes
[SIGNED-OFF] Bug 7175: orderreceive.pl cosmetic changes
[SIGNED-OFF] Bug 7175: Allow to choose which items to receive

Description Julian Maurice 2011-11-06 04:29:46 UTC
Created attachment 6232 [details]
orderreceive.pl

When syspref AcqCreateItem="ordering", this development allow to choose what items are received by listing all created items and providing a checkbox for each one.
See the screenshot to see what it looks like.

BibLibre infos:
  Repository: git://git.biblibre.com/koha_biblibre.git
  Branch:     stetienne/ft/MT6527
Comment 1 Katrin Fischer 2011-11-06 09:06:11 UTC
'Bug 3966 - receiving items should allow for entering barcode' might be interesting to look at in this context, because there can be a conflict.

It's desirable to be able to change items you have created on order when receiving them. Like adding barcode labels and stocknumbers.
Comment 2 Julian Maurice 2011-12-02 14:12:49 UTC
Editing item is possible by clicking on the "Edit" link on item line. It popups the additem.pl page and close it after clicking on Save
Comment 3 Katrin Fischer 2011-12-02 14:36:44 UTC
Thx Julian, I am adding a see also to bug 3966. I think the work on 3966 was also sponsored by a library - so there needs to be some discussion about the feature perhaps.
Comment 4 Julian Maurice 2012-01-13 16:04:05 UTC Comment hidden (obsolete)
Comment 5 Julian Maurice 2012-01-13 16:11:18 UTC Comment hidden (obsolete)
Comment 6 Katrin Fischer 2012-01-15 18:16:30 UTC
Some notes:

1) The change only changes the behaviour for AcqCreateItem = on order. The other settings still work as before. So all ok.

2) The main difference between this implementation and the implementation of bug 3966 is the presentation of the item information on the order receive screen. 3966 uses the standard item form and pulls in all ordered items. If you order a lot of items, the form can get pretty big and hard to work with. 
This implementation uses a separate table with items, edit links and checkboxes. The editing of items from this screen works very nicely and saving the form redirects correctly to the receive screen. 
I like the way it's done here.

Fields displayed in the table are:
Barcode 	Home branch 	Location 	Call number 

Some notes about the table:

a) Home branch and location display the codes instead of the descriptions.
I would like to see the description displayed here.

b) Collection is never displayed, even if it's set for one of the items. 
It would be nice to make this configurable or display location/collection and some other fields if they are set for one of the items.

3) Workflow change: You now have to use the checkboxes to select how many items you want to receive instead of entering the number in the field. The field is blocked and the number can not be entered there directly. 
Checking and unchecking the checkboxes to select the items works nicely.
Is it a problem that the quantity can not be entered directly?
Suggestion: Make it possible to edit the number manually and check/uncheck items from top to bottom accordingly.

4) It's no longer possible to receive more items than have been ordered. This was possible before the change, but I am not sure it's a good feature. If you receive more items than have been ordered no item records are created. You have to create them in cataloguing and by doing so the items will not be linked to the order.
Suggestion: Add a link to add a new item, redirect like the edit links do and increase the item number on the order accordingly.

I am a bit worried about 4) although I am not sure if this feature is used by many(?) libraries.

I am signing off on the first patch and I think a second opinion for the workflow changes described in 3) and 4) would be good.
Comment 7 Katrin Fischer 2012-01-15 18:28:59 UTC
I checked the logs before signing off the first patch and there are a lot of warnings for finishreceive.pl:

[Sun Jan 15 19:23:47 2012] [error] [client 127.0.0.1] [Sun Jan 15 19:23:47 2012] finishreceive.pl: Use of uninitialized value in pattern match (m//) at /home/katrin/kohaclone/C4/SQLHelper.pm line 361., referer: http://localhost:8080/cgi-bin/koha/acqui/orderreceive.pl?ordernumber=14&invoice=Order%20123&gst=0&supplierid=2
[Sun Jan 15 19:23:47 2012] [error] [client 127.0.0.1] [Sun Jan 15 19:23:47 2012] finishreceive.pl: Use of uninitialized value in pattern match (m//) at /home/katrin/kohaclone/C4/SQLHelper.pm line 361., referer: http://localhost:8080/cgi-bin/koha/acqui/orderreceive.pl?ordernumber=14&invoice=Order%20123&gst=0&supplierid=2
[Sun Jan 15 19:23:47 2012] [error] [client 127.0.0.1] [Sun Jan 15 19:23:47 2012] finishreceive.pl: Use of uninitialized value in pattern match (m//) at /home/katrin/kohaclone/C4/SQLHelper.pm line 361., referer: http://localhost:8080/cgi-bin/koha/acqui/orderreceive.pl?ordernumber=14&invoice=Order%20123&gst=0&supplierid=2
[Sun Jan 15 19:23:47 2012] [error] [client 127.0.0.1] [Sun Jan 15 19:23:47 2012] finishreceive.pl: Use of uninitialized value in pattern match (m//) at /home/katrin/kohaclone/C4/SQLHelper.pm line 361., referer: http://localhost:8080/cgi-bin/koha/acqui/orderreceive.pl?ordernumber=14&invoice=Order%20123&gst=0&supplierid=2
[Sun Jan 15 19:23:47 2012] [error] [client 127.0.0.1] [Sun Jan 15 19:23:47 2012] finishreceive.pl: Use of uninitialized value $error_url_str in concatenation (.) or string at /home/katrin/kohaclone/acqui/finishreceive.pl line 117., referer: http://localhost:8080/cgi-bin/koha/acqui/orderreceive.pl?ordernumber=14&invoice=Order%20123&gst=0&supplierid=2
Comment 8 Katrin Fischer 2012-01-15 18:30:46 UTC Comment hidden (obsolete)
Comment 9 Nicole C. Engard 2012-01-16 12:20:49 UTC
For #3 I think it's a problem that you have to check the boxes to receive.  Many big libraries order 100 or more items at a time and having to check 50 or 60 or 90 checkboxes is very difficult.  The ability to type the number received should remain in this area in some way so that we don't inconvenience those who were using that feature already.
Comment 10 Nicole C. Engard 2012-01-16 12:21:53 UTC
As for bug 3966 As long as this patch allows for the entering of necessary info I'm okay with this change.
Comment 11 Katrin Fischer 2012-01-22 19:34:18 UTC Comment hidden (obsolete)
Comment 12 Katrin Fischer 2012-01-22 19:34:46 UTC
Comment on attachment 7278 [details]
neworder does not show item form

wrong bug
Comment 13 Paul Poulain 2012-02-17 14:39:09 UTC
The script devel/misc/testbugzillapatches.pl has detected that this bug does not apply anymore, so i'm marking this bug as "does not apply".

The script is supposed to handle conflicts related to updatedatabase, but no other cases of merge conflict. So the conflict may be trivial to fix.

If it happens that the patch attached does apply cleanly, please send me an email, there's something wrong in the script. It's possible, as it's a recent script.
If the patch applies does not apply as I think, please rebase and resubmit, thanks
Comment 14 Julian Maurice 2012-02-24 12:48:13 UTC Comment hidden (obsolete)
Comment 15 Julian Maurice 2012-02-24 12:57:32 UTC Comment hidden (obsolete)
Comment 16 Corinne BULAC 2012-03-19 14:20:33 UTC
Check that these items have been received and not the others -> OK

two problems : 

1. column "summary" made a wrong link with a suggestion. I made no suggestion. My order was made from existing bibliographic records from the catalog.

2. I receipted two copies instead of one. Message was good, aditionnal copies are not created. But why not allow this creation while 2 copies are automatically marked in "received summary" ? An other "inconsistency" ? : in spite of this mention "2" in "received summary", order line is still marked with "1" in quantity.
Comment 17 Julian Maurice 2012-03-22 09:15:48 UTC
1. I don't understand. What screen are you talking about? If you're talking about parcel.pl, this patch doesn't modify it so this issue isn't related to this bug.
Maybe you're seeing this link because the biblio you choose to order on was previously created by a suggestion, but I can't confirm that.

2. Not sure to understand. You had an order line with a quantity of 1 (so only 1 item created when ordering), and received 2 ? AcqCreateItem is set to 'ordering' so you create items while ordering, not receiving. Additional received copies must be created manually (in cataloguing module).

What is the "received summary" you're talking about? Is it the table under "Already received"? Where is displayed the "1" and where is the "2"?
Comment 18 MathildeF 2012-03-23 09:08:57 UTC
Tested on sandbox.
it works correct.
Comment 19 Paul Poulain 2012-03-29 09:42:10 UTC
conflict with patch 7178 that has already been pushed. Solving the conflicts is not trivial, could you please rebase & resubmit ?
Comment 20 Julian Maurice 2012-03-29 10:39:17 UTC Comment hidden (obsolete)
Comment 21 Julian Maurice 2012-03-29 10:39:49 UTC Comment hidden (obsolete)
Comment 22 delaye 2012-03-30 15:28:42 UTC
i don't understand what i must test and how test
when i apply the patch in a sandbox (3) i have this message
Bug 7115 - Suggestion form has wrong label if AdvancedSearchTypes 
But the title of the ticket it's   Improve order receiving page 

I read : "When syspref AcqCreateItem="ordering", this development allow to choose what items are received by listing all created items and providing a checkbox for each one."
where is the screenshot ?
After syspref AcqCreateItem="ordering i received an order but when i received i have nothing not choice whith chekbox....
Comment 23 delaye 2012-04-02 13:20:52 UTC
i test the reception. I's work. I can choose the item et i can update the item when i when I receives
Comment 24 Paul Poulain 2012-04-02 13:24:26 UTC Comment hidden (obsolete)
Comment 25 Paul Poulain 2012-04-02 13:32:38 UTC Comment hidden (obsolete)
Comment 26 Paul Poulain 2012-04-02 13:32:51 UTC Comment hidden (obsolete)
Comment 27 Katrin Fischer 2012-04-04 21:44:40 UTC
I found a small bug in this that needs a follow up - the not for loan values show the numeric value instead of the description.

Also the table is a bit too wide - so you have to scroll to see all fields. I know i asked for more fields to be displayed when ordering - perhaps this should be a new configuration option in the future. Possibilities I see are:

- make it a system prefernece with a list of fields
- only show columns that are used by the ordered items
- display table columns according to the ACQ framework
Comment 28 Katrin Fischer 2012-04-04 21:46:54 UTC
Perhaps an idea to make it easier to customize for now - if the th tags had a unique id each, we could use jquery to hide some columns until a better solution is implemented.
Comment 29 Julian Maurice 2012-04-05 11:49:33 UTC
(In reply to comment #27)
> I found a small bug in this that needs a follow up - the not for loan values
> show the numeric value instead of the description.

Did you check the 'ACQ' framework for the link to authorised values list ?
There must be a subfield linked to items.notforloan and an authorised value list for the description to be shown. (It works for me)
Comment 30 Katrin Fischer 2012-04-05 12:20:45 UTC
I think my framework is ok - but I didn't define an ACQ framework. Acquisition used to work with the default framework too (ignoring the message on the screen).
Comment 31 Paul Poulain 2012-04-06 13:24:05 UTC
Katrin, do you think you can add your signoff to this patch ? For now, there's only stephane signoff and author is also from BibLibre so it will require another signature.

thx
Comment 32 Katrin Fischer 2012-04-06 17:08:22 UTC
Hi Paul, 

I wanted to... because I think the interfaces of creating items on order and receiving items should match. Additionally this patch is a big enhancement of the workflow as it allows adding barcodes for existing on order items on receive.

I found some smaller problems that should be addressed:

1) When editing an item after saving the order receive form forgets which items where checked to be received. So each time you add a barcode, you have to recheck all checkboxes.

2) Clicking on cancel from the order receive page doesn't return to the shipment I was working on. But this is wrong on master too, so only noting as an additional bug.

3) The authorized values for notforloan, restricted etc. are only shown in the table, when you are using an ACQ framework. I think this is not ideal.

4) The table is a bit too big and contains lots of values that are not interesting to every library. It would be very nice to have that configurable. An easy option to make it work better now would be adding an id to each th element. Then it will be easier to use jquery to hide unused columns.

I think 1) should be addressed before this can go in.
Comment 33 Julian Maurice 2012-04-12 12:39:45 UTC Comment hidden (obsolete)
Comment 34 Julian Maurice 2012-04-12 12:48:24 UTC Comment hidden (obsolete)
Comment 35 Julian Maurice 2012-04-12 13:49:31 UTC Comment hidden (obsolete)
Comment 36 Nicole C. Engard 2012-05-07 14:52:00 UTC Comment hidden (obsolete)
Comment 37 Nicole C. Engard 2012-05-07 14:52:44 UTC Comment hidden (obsolete)
Comment 38 Paul Poulain 2012-05-15 14:58:30 UTC
I was trying to QA this patch, but it does not apply anymore.
fatal: sha1 information is lacking or useless (acqui/orderreceive.pl).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 7175: Allow to choose which items to receive
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/SIGNED-OFF-Bug-7175-Allow-to-choose-which-items-to-JZbBp6.patch
[new/bug_7175 6688c1a] Bug 8041: lost items are no longer available in search


Could you rebase & resubmit (switch back to signed-off, it's probably an easy rebase)
Comment 39 Julian Maurice 2012-05-15 15:05:04 UTC
Created attachment 9590 [details] [review]
[SIGNED-OFF] Bug 7175: orderreceive.pl cosmetic changes
Comment 40 Julian Maurice 2012-05-15 15:05:49 UTC
Created attachment 9591 [details] [review]
[SIGNED-OFF] Bug 7175: Allow to choose which items to receive
Comment 41 Julian Maurice 2012-05-15 15:08:40 UTC
(In reply to comment #38)
> Could you rebase & resubmit (switch back to signed-off, it's probably an
> easy rebase)

It was very easy in fact, I had no conflict... I wonder if you took patches in the right order. It must be "Bug 7175: orderreceive.pl cosmetic changes" first
Comment 42 Paul Poulain 2012-05-21 16:17:15 UTC
(In reply to comment #41)
> (In reply to comment #38)
> > Could you rebase & resubmit (switch back to signed-off, it's probably an
> > easy rebase)
> 
> It was very easy in fact, I had no conflict... I wonder if you took patches
> in the right order. It must be "Bug 7175: orderreceive.pl cosmetic changes"
> first

That's definetly the cause of my problem = it was not written that the patches were to be applied in reverse order !

QA comments:
 * the code update a line using format_date (+        date                  => format_date($order->{entrydate}),), which is deprecated, but it was here before, so it's OK
 * perlcritic OK for Acquisition.pm acqui/orderreceive.pl acqui/finishreceive.pl acqui/orderreceive.pl catalogue/getitem-ajax.pl cataloguing/additem.pl
 * prove xt/author/translatable-templates.t xt/author/valid-templates.t is OK

passed QA
Comment 43 Paul Poulain 2012-05-21 16:19:06 UTC
works well.

Just a side comment: when you save an item with a (or some) field empty you get "undefined" on each empty column. Is it the intented behaviour ? Why not just let the field empty on the screen ?
Comment 44 Julian Maurice 2012-05-22 08:59:54 UTC
No this is not the correct behaviour. A patch has been proposed on bug 7178.
Comment 45 Julian Maurice 2012-05-22 09:03:48 UTC
This is ok in master, marked as resolved.