Bug 7291

Summary: claims management improvement
Product: Koha Reporter: claire.hernandez <claire.hernandez>
Component: AcquisitionsAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: corinnebulac, jonathan.druart, katrin.fischer, m.de.rooy, paul.poulain
Version: 3.8   
Hardware: All   
OS: All   
Change sponsored?: Sponsored Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 7293    
Bug Blocks: 7163, 10556, 14535    
Attachments: screenshot
Bug 7291: Adds new field aqbooksellers.deliverytime
Bug 7291: Adds new field aqbooksellers.deliverytime
Bug 7291: Adds new field aqbooksellers.deliverytime
Bug 7291: Adds new field aqbooksellers.deliverytime
csv file
Bug 7291: Adds new field aqbooksellers.deliverytime
Bug 7291: Followup: removes backquotes

Description claire.hernandez@biblibre.com 2011-12-02 09:30:03 UTC
claim: button "check all" & estimated date. Add a button to "check all" on the acquisition claim page. A filter on estimated dates will be added (BibLibre MT6546)
Comment 1 Katrin Fischer 2012-01-22 17:44:18 UTC
Created attachment 7277 [details]
screenshot

Links to "check all/uncheck all" orders from a vendor were implemented with bug 5347.
Comment 2 Jonathan Druart 2012-02-13 14:43:51 UTC
*** Bug 7293 has been marked as a duplicate of this bug. ***
Comment 3 Jonathan Druart 2012-02-13 14:46:30 UTC Comment hidden (obsolete)
Comment 4 Katrin Fischer 2012-02-22 06:03:11 UTC
Hi Jonathan, 
I am sorry, this patch has a lot of conflicts now and will no longer apply.
Could you please rebase and resubmit? Perhaps you will also want to make use of the TT plugin for dates.
Comment 5 Jonathan Druart 2012-02-22 09:32:40 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2012-03-08 08:27:29 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2012-03-08 08:28:32 UTC
Comment on attachment 8074 [details] [review]
Bug 7291: Adds new field aqbooksellers.deliverytime

Rebased patch
Comment 8 Corinne BULAC 2012-03-19 16:12:18 UTC
the filter on "Estimated Delivery date from: "/"to" doesnt't work

Vendor -> OK
Days ago -> OK
Comment 9 Jonathan Druart 2012-03-19 16:49:52 UTC Comment hidden (obsolete)
Comment 10 Koha Team University Lyon 3 2012-03-20 09:20:18 UTC Comment hidden (obsolete)
Comment 11 Corinne BULAC 2012-03-20 10:12:25 UTC
this is still a problem. We have write delivery date as "AAAAMMJJ' if we want if it to work. Or the input form indicate "JJ/AA/MMMM' (ditto for the calendar help)
Comment 12 Jonathan Druart 2012-03-21 09:19:53 UTC
I test on a sandbox and I can not reproduce the problem. Please, can you retest and tell me yours differents actions ?
Comment 13 Corinne BULAC 2012-03-21 14:16:17 UTC
New tests -> OK
Comment 14 Paul Poulain 2012-03-23 10:38:29 UTC
QA comment:
* patch does not apply, there are small merge problem

* dates are formatted without the Date plugin, please fix !
+        estimateddeliverydate => C4::Dates->new( $estimateddeliverydate, 'iso' )->output,

* you use a MYSQLism for getting the current date. Instead of CURDATE( ) use CAST(now() AS date)


Marking failed QA. The rest is OK, will mark passed QA once you've fixed those points
Comment 15 Jonathan Druart 2012-03-23 14:10:17 UTC
Created attachment 8592 [details] [review]
Bug 7291: Adds new field aqbooksellers.deliverytime

Rebased patch.
Remove warnings in tt file.
Replace MYSQLism with standard format
Replace date format
Comment 16 Paul Poulain 2012-03-29 10:31:27 UTC
QA comment:

1- there is a hardcoded variable inside SQL:
-    WHERE (closedate < DATE_SUB(CURDATE( ),INTERVAL $delay DAY)

that is removed => +1 from QA point of view !

2- the dates are displayed using kohaDates plugin => +1

A possible enhancement would be to highlight passed dates, but outside from a QA

proposing passed QA, (not setting it as it's a BibLibre patch)
Comment 17 Marcel de Rooy 2012-04-03 12:21:03 UTC
Additional QA comment:

I have no real objections for pushing this patch, but want to note:

1) Use of adddate, date_sub, ifnull: they are all mysql-isms
I suspect that you could not avoid them here; and they are already present too. But what is the use of replacing curdate's and backquotes if we do not remove typical mysql date/time functions? But this is a discussion on itself, and should be held elsewhere. 

2) BTW You add backtics in kohastructure with this patch (instead of removing them..)

3) Very small note: You edited the part with the cryptic test: not $delay =~ /^\d{1,3}$/
Actually, we do not want a delay>999 (for some reason). If you edit that line, please make the test faster and more readable with delay>999.

Passed QA
Comment 18 Jonathan Druart 2012-04-03 14:16:23 UTC
Created attachment 8809 [details] [review]
Bug 7291: Followup: removes backquotes

Hi Marcel, 

Thanks for your comment.

1,2/ This followup removes backquotes in kohastructure. I would like to remove these mysql specific functions, but there is not yet a way to do this in koha.

3/ the regexp does the same think as 999, even better because it does not warn if $delay is not an integer. I thing it is better, don't you think ?
Comment 19 Paul Poulain 2012-04-03 16:33:06 UTC
(In reply to comment #18)
> Created attachment 8809 [details] [review]
> Bug 7291: Followup: removes backquotes

> 3/ the regexp does the same think as 999, even better because it does not
> warn if $delay is not an integer. I thing it is better, don't you think ?

I agree, as it avoid a risk of a SQL injection.

patch pushed