Because in parcel.pl page the filter (on the left) makes a table using jscript and a very different workflow, the "cancel" link does nothing just asking for cancelling. Principaly due to the use of the "filter" js function that call the parcel.pl page and does all the job in a separate block with a separate perl function (SearchOrder) that sends all to js, that constructs html to finally append it to table. So I have decided to rebuild entirely the filter. I have choosen to overload the function "GetPendingOrders" to enable it to accept new arguments. To test : when you are in "parcel.pl" ready to receive orders, simply select a filter on the left and on the filtered page try to cancel a line, you'll have the warning message but no more, the line will be not canceled. Instead having "Delete order" and "Delete order and catalog record", you have "cancel".
Created attachment 10701 [details] [review] proposed patch thx for testing
in parcel.tt, line 172, I corrected an oversight of Owen Leonard made in bug 7120. More, not linked with this bug, I noticed that in this page the jquery datatable was poorly inserted with the code above, so the results shown in the table are poorly sorted. I will create a separate bug report.
Created attachment 10702 [details] [review] proposed patch
QA Comments: You can't modify the prototype of a routine without modifying all the occurrences ! grep is your friend: acqui/uncertainprice.pl:my $pendingorders = &GetPendingOrders($booksellerid,0,$owner,$basketno); + Modify POD In the majority of cases, it is preferable to add the new parameters at the end of the parameter's list.
Created attachment 11142 [details] [review] last patch right Jonathan. So I think it's ok now. thx
Created attachment 11630 [details] PREPATCH acquisitions_recieveorder_with filter_withReceiveCancel.png I attempted a test, but to clarify where this applies, I attached this PREPATCH image.
Created attachment 11631 [details] POSTPATCH acquisitions_receiveorder_with filter_withoutReceiveCancel.png After applying the patch, I could not see the Receive/Cancel link to try to Cancel. I wasn't sure if this is what you wanted, so I attached a POSTPATH image of what I see. Maybe a clearer test plan, or a further fix is needed. wajasu
Hello wajasu, sorry if I was not clear for the test, your snapshots are useful to explain: 1° without patch: if you look at the first snapshot you see a column without title on the right of 'Order cost' column, this column have Receive/Cancel, two explicit links. But if you click on Cancel, this link doesn't cancel the order. That is the bug. 2° with the patch: we can see on your second snapshot we have now two columns, the second one offers the same choice than the normal page without filter, I mean 'Delete order' and depending on the situation of the biblio record, 'Delete order and catalog record' or 'Can't delete...' in our case. And of course if you click on the link 'Delete...' it does its job, no more bug. I hope I am clear, so the first check of my patch is when you look at your snapshots (the right column(s)) changes, and the second is to try to cancel an order without my patch (it doesn't cancel) and with my patch (cancel successful). So thx for re testing wajasu.
Created attachment 11653 [details] [review] [SIGNED-OFF] 0001-bug-8382-It-is-not-possible-to-cancel-an-.patch I think I understand. In aquisitions, when recieving orders, one may want to cancel and order, or specifically, an order line. After finding the vendor for the order, and clicking receive shipment button, and keying in the vendor's invoice number and Next button, you see the order lines. If you use the javascipt filter widget at the left of the page, and put the order line number in it and clock Filter button, a NEW column containing Receive/Cancel links shows up. (see PREPATCH attachment image) The bug is when you click "Cancel" the order is not cancelled. The patch writer, reworked this so now with the new patch, after using a filter, no Receive/Cancel links appear anymore, and you can just click "Delete Order" to cancel the order entry. I used the patch and the resulting screen showed the cancelled order entry in a section below. So it seems to work fine. Note: I had to add funds, budgets, vendors, order books, a new staff user (since sysadmin get warning it must define a budget), plus figure out how to get into this workflow point. I hope the workflow gets improved in the future. wajasu
Reimplements search as server-side, rather than client side with Javascript and JSON, so two large chunks of code are removed. Changes to GetPendingOrders function are clear and safe; new parameters are added at the end, so existing function calls do not need to be changed, and while $booksellerid is renamed to $supplierid internally, it's kept consistent within the function. POD for GetPendingOrders is updated, but the new parameters are not sufficiently described. Please provide more details on what these parameters actually do (a followup patch would be appropriate for this). Furthermore, unit tests should be expanded to cover the addition of the new filter options. A followup can be provided for this, as well. I'm still defrosting after a long absence, so all my tests are not yet working for other reasons, but to the best of my knowledge, this patch is safe and can pass QA.
This is not a blocker, as we can deal with recieving without filtering (even if it's a major problem I agree). Due to the number of passed QA bugs, i'll deal with this one later
Patch pushed to master I've tested it, it's OK, patch pushed. But i'm a little bit uncomfortable with this patch. I can't say why, so I've pushed it.
Created attachment 11982 [details] [review] Bug 8382 [Follow-up] It is not possible to cancel an order when receiving on a filtered page in parcel.pl In order to be valid XHTML ampersands must be encoded ("&").
Ill wait until the follow up is pushed before deciding whether to push this to 3.8.x or not
Christophe: A followup is mandatory ! This line does not do what we want: +my $datereceived = ($input->param('op') eq ('new' or "search")) ? C4::Dates->new($input->param('datereceived')) + : C4::Dates->new($input->param('datereceived'), 'iso'); $s eq ('foo' or 'bar') does not return true if $s = 'bar'
Created attachment 12272 [details] [review] Bug 8382 [Follow-up] It is not possible to cancel an order when receiving on a filtered page in parcel.pl In order to be valid XHTML ampersands must be encoded ("&"). Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 12273 [details] [review] Bug 8382: Followup: FIX wrong comparaison
Applying: Bug 8382 [Follow-up] It is not possible to cancel an order when receiving on a filtered page in parcel.pl Bug 8382: Followup: FIX wrong comparaison Apply? [yn] y Applying: Bug 8382: Followup: FIX wrong comparaison error: patch failed: acqui/parcel.pl:79 error: acqui/parcel.pl: patch does not apply Patch failed at 0001 Bug 8382: Followup: FIX wrong comparaison
Comment on attachment 12273 [details] [review] Bug 8382: Followup: FIX wrong comparaison This patch seems useless now.
(In reply to comment #16) > Created attachment 12272 [details] [review] > Bug 8382 [Follow-up] It is not possible to cancel an order when receiving on > a filtered page in parcel.pl > > In order to be valid XHTML ampersands must be encoded ("&"). > > Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> passing QA, trival patch $ koha-qa.pl -c 1 testing 1 commit(s) (applied to commit 0a35b26) * 74d0ff6 Bug 8382 [Follow-up] It is not possible to cancel an order when receiving on a filtered page in parcel.pl koha-tmpl/intranet-tmpl/prog/en/modules/acqui/parcel.tt * koha-tmpl/intranet-tmpl/prog/en/modules/acqui/parcel.tt OK
follow-up pushed
Original patch doesn't apply to 3.8.x, so the follow up doesn't either. Please resubmit both for 3.8.x
Created attachment 12913 [details] [review] proposed patch to match 3.8.x Hello Chris, done, I hope this one is ok. For the follow up I had no conflicts so it must be ok. thx
(In reply to comment #23) > Created attachment 12913 [details] [review] > proposed patch to match 3.8.x > > Hello Chris, > done, I hope this one is ok. > For the follow up I had no conflicts so it must be ok. > thx Unfortunately still has a conflict. Falling back to patching base and 3-way merge... Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/acqui/parcel.tt Auto-merging acqui/parcel.pl CONFLICT (content): Merge conflict in acqui/parcel.pl Auto-merging C4/Acquisition.pm Looks like something to do search has snuck in my $datereceived = ($input->param('op') eq ('new' or "search")) Should that be in 3.8.x?
Hi Chris, this is amazing, I have retried and for me it's ok on a branch 3.8.x (the last one is : commit e7b91d6a823401f73d4ef69ae80e045031995dfc Author: Fridolyn SOMERS <fridolyn.somers@biblibre.com> Date: Wed Sep 19 11:45:19 2012 +0200 Bug 8791: OPAC UNIMARC XSLT details: subject with subfield 3) And Fridolyn Somers has tried too, it's ok for him.. Have you take the right one ? => "proposed patch to match 3.8.x"
Yep, You can see in the patch this bit -my $datereceived = ($input->param('op') eq 'new') ? C4::Dates->new($input->param('datereceived')) - : C4::Dates->new($input->param('datereceived'), 'iso') ; +my $datereceived = ($input->param('op') eq ('new' or "search")) ? C4::Dates->new($input->param('datereceived')) + : C4::Dates->new($input->param('datereceived'), 'iso'); This is the bit that is conflicting, I can fix the conflict if you are sure the search bit is to do with this change, not something that has already gone into master?
I just tried to apply both patches on up-to-date 3.8.x branch in this order proposed patch to match 3.8.x Bug 8382 [Follow-up] It is not possible to cancel an order when receiving on a filtered page in parcel.pl and had no conflicts. Furthermore, it works :) Can you give it a try again Chris ?
Worked this time, pushed to 3.8.x will be in 3.8.8