Description
Bernardo Gonzalez Kriegel
2015-10-29 22:57:04 UTC
Created attachment 44181 [details] [review] Bug 15087: make dt_from_string return undef for '0000-00-00' To test: 1) Reproduce problem in description 2) Apply the patch 3) Try again, no more problems I don't know if this could have side effects Hi Bernardo I'll add here Marc and Jonathan, since they have more experience with Koha::DateUtils. I feel scared when you mention that can have side effects. At other places we did return ( eval { dt_from_string($rule->{hardduedate}, 'iso'),$rule->{hardduedatecompare}); }; ...to avoid such errors. I propose to do it here as well and postpone the change to dt_from_string until the C4::Dates removal is finished. It is in C4/Circulation.pm Line 1498 (In reply to Bernardo Gonzalez Kriegel from comment #0) > Current master, define circulation rule All, All any number of days, > but left 'Hard due date' EMPTY. > > Try to checkout something to a patron, you get > > Can't locate object method "truncate" via package "-1" (perhaps you forgot > to load "-1"?) at .... Outch! > Problem comes from GetHardDueDate, it returns > > return (dt_from_string($rule->{hardduedate}, > 'iso'),$rule->{hardduedatecompare}); Yep. > But dt_from_string returns without value in case of '0000-00-00' date, It usually return undef when not called in a list context. (In reply to Bernardo Gonzalez Kriegel from comment #1) > Created attachment 44181 [details] [review] [review] > Bug 15087: make dt_from_string return undef for '0000-00-00' No! :) It is usually a bad idea to explicitly return undef, and you get a perlcritic warning. > I don't know if this could have side effects That's why it's preferable not to change the return of the subroutine but the caller(s?). (In reply to Marc Véron from comment #3) > At other places we did > > return ( eval { dt_from_string($rule->{hardduedate}, > 'iso'),$rule->{hardduedatecompare}); }; It's a weird hack :) dt_from_string does not return an error. > I propose to do it here as well and postpone the change to dt_from_string > until the C4::Dates removal is finished. We cannot wait for a fix here as it makes the application explodes. Bernardo, I don't reproduce the problem you described. My issuingrules table is empty. Please detail. Created attachment 44194 [details] [review] Bug 15087: Fix GetHardDueDate return value This patch fixes GetHardDueDate return value in case of empty ('0000-00-00') hard due date. To test: 1) Reproduce problem in description 2) Apply the patch 3) Try again, no more problems (In reply to Jonathan Druart from comment #5) > Bernardo, I don't reproduce the problem you described. > My issuingrules table is empty. > Please detail. Problem arises when you create a rule mysql kohadev -e "select categorycode,itemtype,hardduedate from issuingrules" +--------------+----------+-------------+ | categorycode | itemtype | hardduedate | +--------------+----------+-------------+ | * | * | 0000-00-00 | +--------------+----------+-------------+ Changed patch and bug description (In reply to Jonathan Druart from comment #4) > (In reply to Bernardo Gonzalez Kriegel from comment #0) > > Current master, define circulation rule All, All any number of days, > > but left 'Hard due date' EMPTY. > > > > Try to checkout something to a patron, you get > > > > Can't locate object method "truncate" via package "-1" (perhaps you forgot > > to load "-1"?) at .... > > Outch! > The problem was discovered by Hector. Bernardo, ok thanks, I am on it! Created attachment 44197 [details] [review] Bug 15087: hardduedate should not be inserted as 0000-00-00 but NULL This has been introduced by bug 14949, recently pushed. So I don't think we need to update the DB, only devs will face this problem. The hardduedate was inserted as 0000-00-00 because was en empty string. To be inserted as NULL, the value should be undef. Test plan: 0/ Don't apply this patch 1/ Edit a circ rules and don't fill the hard due date 2/ Look at the DB, the value is 0000-00-00 3/ Apply this patch and confirm that the value is now NULL To restore the behavior that existed before bug 14949, I think this is the way to go. And this is critical Created attachment 44198 [details] [review] [SIGNED-OFF] Bug 15087: hardduedate should not be inserted as 0000-00-00 but NULL This has been introduced by bug 14949, recently pushed. So I don't think we need to update the DB, only devs will face this problem. The hardduedate was inserted as 0000-00-00 because was en empty string. To be inserted as NULL, the value should be undef. Test plan: 0/ Don't apply this patch 1/ Edit a circ rules and don't fill the hard due date 2/ Look at the DB, the value is 0000-00-00 3/ Apply this patch and confirm that the value is now NULL Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Confirmed +--------------+----------+-------------+ | categorycode | itemtype | hardduedate | +--------------+----------+-------------+ | * | * | NULL | +--------------+----------+-------------+ No koha-qa errors Signed and changed description... again :) (In reply to Jonathan Druart from comment #11) > To restore the behavior that existed before bug 14949, I think this is the > way to go. Agreed Signed and changed summary... again :) Created attachment 44244 [details] [review] [PASSED QA] Bug 15087: hardduedate should not be inserted as 0000-00-00 but NULL This has been introduced by bug 14949, recently pushed. So I don't think we need to update the DB, only devs will face this problem. The hardduedate was inserted as 0000-00-00 because was en empty string. To be inserted as NULL, the value should be undef. Test plan: 0/ Don't apply this patch 1/ Edit a circ rules and don't fill the hard due date 2/ Look at the DB, the value is 0000-00-00 3/ Apply this patch and confirm that the value is now NULL Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Confirmed +--------------+----------+-------------+ | categorycode | itemtype | hardduedate | +--------------+----------+-------------+ | * | * | NULL | +--------------+----------+-------------+ No koha-qa errors Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Patch pushed to master. Thanks Jonathan! Created attachment 52003 [details] [review] Bug 15087 - Follow for koha-create Logic in the koha-create was requiring a DEFAULTSQL to trigger. This patch moves it, such that --create-db will be registered all the time. Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> |