Bug 8382 - It is not possible to cancel an order when receiving on a filtered page in parcel.pl
Summary: It is not possible to cancel an order when receiving on a filtered page in pa...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: 3.8
Hardware: All All
: P1 - high normal (vote)
Assignee: Christophe Croullebois
QA Contact: Mason James
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-07-08 14:31 UTC by Christophe Croullebois
Modified: 2013-05-23 06:24 UTC (History)
8 users (show)

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


Attachments
proposed patch (15.72 KB, patch)
2012-07-08 14:34 UTC, Christophe Croullebois
Details | Diff | Splinter Review
proposed patch (15.74 KB, patch)
2012-07-08 15:06 UTC, Christophe Croullebois
Details | Diff | Splinter Review
last patch (16.91 KB, patch)
2012-07-26 08:36 UTC, Christophe Croullebois
Details | Diff | Splinter Review
PREPATCH acquisitions_recieveorder_with filter_withReceiveCancel.png (42.81 KB, image/png)
2012-08-15 21:03 UTC, wajasu
Details
POSTPATCH acquisitions_receiveorder_with filter_withoutReceiveCancel.png (52.95 KB, image/png)
2012-08-15 21:09 UTC, wajasu
Details
[SIGNED-OFF] 0001-bug-8382-It-is-not-possible-to-cancel-an-.patch (17.01 KB, patch)
2012-08-16 23:38 UTC, wajasu
Details | Diff | Splinter Review
Bug 8382 [Follow-up] It is not possible to cancel an order when receiving on a filtered page in parcel.pl (1.33 KB, patch)
2012-09-05 16:35 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 8382 [Follow-up] It is not possible to cancel an order when receiving on a filtered page in parcel.pl (1.36 KB, patch)
2012-09-17 12:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8382: Followup: FIX wrong comparaison (972 bytes, patch)
2012-09-17 12:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
proposed patch to match 3.8.x (16.74 KB, patch)
2012-10-18 14:15 UTC, Christophe Croullebois
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Christophe Croullebois 2012-07-08 14:31:45 UTC
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".
Comment 1 Christophe Croullebois 2012-07-08 14:34:36 UTC Comment hidden (obsolete)
Comment 2 Christophe Croullebois 2012-07-08 14:50:58 UTC
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.
Comment 3 Christophe Croullebois 2012-07-08 15:06:39 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2012-07-24 13:01:45 UTC
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.
Comment 5 Christophe Croullebois 2012-07-26 08:36:57 UTC Comment hidden (obsolete)
Comment 6 wajasu 2012-08-15 21:03:20 UTC
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.
Comment 7 wajasu 2012-08-15 21:09:26 UTC
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
Comment 8 Christophe Croullebois 2012-08-16 07:08:24 UTC
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.
Comment 9 wajasu 2012-08-16 23:38:40 UTC Comment hidden (obsolete)
Comment 10 Ian Walls 2012-08-17 14:42:51 UTC
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.
Comment 11 Paul Poulain 2012-08-31 11:43:13 UTC
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
Comment 12 Paul Poulain 2012-09-05 11:53:17 UTC
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.
Comment 13 Owen Leonard 2012-09-05 16:35:09 UTC Comment hidden (obsolete)
Comment 14 Chris Cormack 2012-09-06 05:02:59 UTC
Ill wait until the follow up is pushed before deciding whether to push this to 3.8.x or not
Comment 15 Jonathan Druart 2012-09-17 07:27:28 UTC
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'
Comment 16 Jonathan Druart 2012-09-17 12:04:29 UTC
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>
Comment 17 Jonathan Druart 2012-09-17 12:04:42 UTC Comment hidden (obsolete)
Comment 18 Kyle M Hall 2012-09-21 12:51:25 UTC
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 19 Jonathan Druart 2012-09-21 13:06:05 UTC
Comment on attachment 12273 [details] [review]
Bug 8382: Followup: FIX wrong comparaison

This patch seems useless now.
Comment 20 Mason James 2012-10-03 00:26:02 UTC
(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 ("&amp;").
> 
> 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
Comment 21 Paul Poulain 2012-10-05 09:56:53 UTC
follow-up pushed
Comment 22 Chris Cormack 2012-10-06 06:59:18 UTC
Original patch doesn't apply to 3.8.x, so the follow up doesn't either. Please resubmit both for 3.8.x
Comment 23 Christophe Croullebois 2012-10-18 14:15:01 UTC
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
Comment 24 Chris Cormack 2012-10-18 19:46:56 UTC
(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?
Comment 25 Christophe Croullebois 2012-10-19 14:30:48 UTC
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"
Comment 26 Chris Cormack 2012-10-22 04:33:07 UTC
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?
Comment 27 Julian Maurice 2012-12-13 11:46:47 UTC
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 ?
Comment 28 Chris Cormack 2012-12-13 19:59:04 UTC
Worked this time, pushed to 3.8.x will be in 3.8.8