Bug 5549 - Hourly Loans
Summary: Hourly Loans
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: 3.8
Hardware: All All
: P1 - high enhancement with 10 votes (vote)
Assignee: Chris Cormack
QA Contact: Jonathan Druart
URL:
Whiteboard:
Keywords:
Depends on: 5872 6430 6431 6432 6438 7359
Blocks: 7751 7820 17679
  Show dependency treegraph
 
Reported: 2010-12-28 22:08 UTC by Nicole C. Engard
Modified: 2016-11-25 11:01 UTC (History)
12 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Fixed Bracketing Error (912 bytes, patch)
2011-08-17 22:08 UTC, Elliott Davis
Details | Diff | Splinter Review
Patch to correct calculation of large durations (4.84 KB, patch)
2011-09-15 11:55 UTC, Colin Campbell
Details | Diff | Splinter Review
Git Branch for hourly loans (50 bytes, text/plain)
2012-03-16 14:22 UTC, Elliott Davis
Details
Branc to pull from (82 bytes, text/plain)
2012-03-20 00:47 UTC, Chris Cormack
Details
Bug 5549 : Fixing the test to only run if the test data exists (2.02 KB, patch)
2012-03-22 19:44 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 5549 : Fixing the test to only run if the test data exists (2.14 KB, patch)
2012-03-22 20:27 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 5549 followup: fix suspension in days further (2.79 KB, patch)
2012-03-22 21:36 UTC, Ian Walls
Details | Diff | Splinter Review
bug 5549: change to kohastructure.sql missing from commit 8cea8e5cc05075dc391194248d31a54586389601 (945 bytes, patch)
2012-04-05 17:56 UTC, MJ Ray (software.coop)
Details | Diff | Splinter Review
bug 5549: change to kohastructure.sql missing from commit 8cea8e5cc05075dc391194248d31a54586389601 (1.72 KB, patch)
2012-04-05 18:11 UTC, MJ Ray (software.coop)
Details | Diff | Splinter Review
bug 5549: change to kohastructure.sql missing from commit 8cea8e5cc05075dc391194248d31a54586389601 (1.71 KB, patch)
2012-04-05 18:20 UTC, MJ Ray (software.coop)
Details | Diff | Splinter Review
bug 5549: change to kohastructure.sql missing from commit 8cea8e5cc05075dc391194248d31a54586389601 (1.84 KB, patch)
2012-04-06 13:58 UTC, MJ Ray (software.coop)
Details | Diff | Splinter Review
bug 5549: change to kohastructure.sql missing from commit 8cea8e5cc05075dc391194248d31a54586389601 (2.41 KB, patch)
2012-04-06 14:09 UTC, MJ Ray (software.coop)
Details | Diff | Splinter Review
[SIGNED-OFF] bug 5549: change to kohastructure.sql missing from commit 8cea8e5cc05075dc391194248d31a54586389601 (2.70 KB, patch)
2012-04-06 14:23 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 5549 followup: fix suspension in days further (2.84 KB, text/plain)
2012-04-09 02:51 UTC, Chris Cormack
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Nicole C. Engard 2010-12-28 22:08:20 UTC
Koha 3.2's circulation loans are currently measured only in units of days. However, because some libraries, especially academic libraries, perform loans that are measured in minutes or hours, particularly for course reserves and short loans, it is necessary to modify Koha to perform all circulation and related transactions in units of minutes

RFC on Wiki: http://wiki.koha-community.org/wiki/Hourly_Loans_RFC
Comment 1 Chris Cormack 2011-05-30 22:19:41 UTC
Work is in progress, you can track progress here

http://git.catalyst.net.nz/gw?p=koha.git;a=shortlog;h=refs/heads/bug_5549

When It comes time for sign off I will break it into smaller testable pieces
Comment 2 Chris Cormack 2011-05-31 01:41:47 UTC
Have started breaking it into smaller pieces, db changes are on 6430
Comment 3 Colin Campbell 2011-06-27 14:37:35 UTC
Hourly loans as currently being tested at Staffordshire University is in my branch rolling_loans at git://github.com/colinsc/koha.git Needs some work still on the admin interface. (Staffs loan to 10::00 am if previous day is closed needs to be an option). Uses the same db changes as Chris' work and an extra column in issuing-rules to specify loan length units as days/hours
Comment 4 Chris Cormack 2011-07-03 22:05:56 UTC
Colin

Did you see the work I was doing on moving the Circulation code out into the Koha:: and into object oriented modules?
Comment 5 Chris Cormack 2011-07-20 23:35:12 UTC
Have merged to a branch based on master, and fixed conflicts

Then added in the db changes.

Things to note, 
Koha::DateUtils
Introduces a new dependency  DateTime::Format::DateParse
Also binds us to perl 5.10 which is no bad thing, but something to note

Koha::Calendar
Introduces
DateTime::Set (libdatetime-set-perl)

new/awaiting_qa/bug_5549
Comment 6 Brendan Gallagher 2011-07-21 03:04:15 UTC
I was just testing origin/new/awaiting_qa/bug_5549

I don't see an updatedatabase.pl for adding "unit" to the issuingrules table.
Comment 7 Colin Campbell 2011-07-21 09:12:44 UTC
installer/data/mysql/atomicupdate/hourlyloans.sql 
needs to be called to add the lengthunit


Testing shows that fines handling needs to be rethought to deal with production size databases. In fact for hourly loans its probably more paractical to recalculate as required.

We need to maintain branch opening and closing times as well. If fining on an hourly basis closed times need to be taken into account much as closed days are.
It should be possible with a bit of refactoring to have a more consistent approach to closed days/times
Comment 8 Elliott Davis 2011-07-27 02:14:52 UTC
When I was testing I also needed libreadonly-perl.
Comment 9 Elliott Davis 2011-08-03 02:39:27 UTC
My library (UT Tyler) is about to put this into production and would like to know what things we should look for to test.
Comment 10 Elliott Davis 2011-08-12 01:28:54 UTC
Also needed this dependency libdatetime-format-dateparse-perl
Comment 11 Elliott Davis 2011-08-16 17:44:00 UTC
I have discovered an error that happens in this branch during testing.  When you check in a book in dropbox mode the following error occurs:


Software Error
Can’t locate object method ‘new’ via package “koha” at /home/koha/kohaclone/c4/circulation, pm line 1798, <DATA> line 522
Comment 12 Chris Cormack 2011-08-16 23:00:46 UTC
I have fixed the typo and updated new/awaiting_qa/bug_5549
Comment 13 Elliott Davis 2011-08-17 13:26:42 UTC
found another error in when checking in via bookdrop 

Can't locate object method "strftimei" via package "DateTime" at /home/koha/kohaclone/C4/Circulation.pm line 1801, <DATA> line 522.
Comment 14 Elliott Davis 2011-08-17 22:08:47 UTC Comment hidden (obsolete)
Comment 15 Chris Cormack 2011-08-17 23:20:58 UTC
Pushed the fix for the bracket to new/awaiting_qa/bug_5549
Comment 16 Chris Cormack 2011-08-17 23:23:47 UTC
Fix for the strftime typo pushed
Comment 17 Elliott Davis 2011-08-30 03:25:19 UTC
after having this in production for 3 weeks and working out a few kinks that have been patches I can confidently say we are happy to sign off on this.
Comment 18 Elliott Davis 2011-09-01 15:17:42 UTC
Areas we tested:
  - Hourly checkouts
    - checking out
    - returning
    - defining overdue rules
    - defining checkout rules
  - fines
Areas we didn't test
  - messaging
Comment 19 Elliott Davis 2011-09-14 19:51:36 UTC
CalcFine is not calculating the date diff correctly.  For large date ranges (months/years) the calc returns a duration of 0 days.  I believe this is due to a limitation of the DateTime::Duration module.  

Since this is used in Koha::Calendar I am hesitant to commit any changes but I would recommend looking into using Date::Calc instead.  I am going to be testing that locally and will report the results.
Comment 20 Colin Campbell 2011-09-15 10:20:12 UTC
Elliott the solution of the problem with large date differences in CalcFine is to use the DateTime delta_days method rather than subtract in the date comparison. That way we avoid creating the duration object in units that don't have a fixed conversion to days. Patch to follow
Comment 21 Colin Campbell 2011-09-15 11:55:03 UTC Comment hidden (obsolete)
Comment 22 Elliott Davis 2011-09-15 17:15:15 UTC
Currently Testing.  Looks good but I could really use a second signoff.
Comment 23 Elliott Davis 2011-09-19 12:57:07 UTC
Tested and seems to fix the Fines issue.  I would recommend that we visit the issue of fines being managed by the cron for hourly loans.  I think this is a bad idea because of the delay in the script running.  I do however believe that falls outside the scope of this ticket.
Comment 24 Ian Walls 2011-10-14 16:01:48 UTC
Attempting to merge master into the new/awaiting_qa/bug_5549 yields the following:

Auto-merging C4/Budgets.pm
Auto-merging C4/Circulation.pm
Auto-merging C4/ILSDI/Services.pm
Auto-merging C4/Installer/PerlDependencies.pm
Auto-merging C4/Items.pm
Auto-merging C4/Members.pm
CONFLICT (content): Merge conflict in C4/Members.pm
Auto-merging C4/Overdues.pm
Auto-merging C4/SIP/ILS.pm
Auto-merging C4/SIP/Sip.pm
Auto-merging C4/TTParser.pm
Auto-merging C4/TmplToken.pm
Auto-merging C4/TmplTokenType.pm
Auto-merging acqui/addorderiso2709.pl
Auto-merging acqui/neworderempty.pl
Auto-merging admin/aqbudgets.pl
Auto-merging admin/aqplan.pl
Auto-merging catalogue/detail.pl
Auto-merging catalogue/moredetail.pl
Auto-merging cataloguing/additem.pl
Auto-merging circ/circulation.pl
CONFLICT (content): Merge conflict in circ/circulation.pl
Auto-merging circ/overdue.pl
CONFLICT (content): Merge conflict in circ/overdue.pl
Auto-merging circ/returns.pl
Auto-merging circ/ypattrodue-attr-search-authvalue.pl
CONFLICT (content): Merge conflict in circ/ypattrodue-attr-search-authvalue.pl
Auto-merging circ/ysearch.pl
CONFLICT (content): Merge conflict in circ/ysearch.pl
Removing debian/scripts/koha-create.8
Removing install_misc/apt-get-debian-lenny.sh
Removing installer/data/mysql/de-DE/mandatory/sysprefs.sql
Removing installer/data/mysql/fr-FR/1-Obligatoire/unimarc_standard_systemprefs.sql
Removing installer/data/mysql/it-IT/necessari/sysprefs.sql
Auto-merging installer/data/mysql/kohastructure.sql
CONFLICT (content): Merge conflict in installer/data/mysql/kohastructure.sql
Removing installer/data/mysql/nb-NO/1-Obligatorisk/sysprefs.sql
Removing installer/data/mysql/pl-PL/mandatory/sysprefs.sql
Removing installer/data/mysql/ru-RU/mandatory/system_preferences_only_translation_updating.sql
Removing installer/data/mysql/ru-RU/mandatory/system_preferences_only_translation_updating.txt
Auto-merging installer/data/mysql/sysprefs.sql
Removing installer/data/mysql/uk-UA/mandatory/system_preferences_only_translation_updating.sql
Removing installer/data/mysql/uk-UA/mandatory/system_preferences_only_translation_updating.txt
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Removing koha-tmpl/intranet-tmpl/prog/en/css/intranet-print.css
Removing koha-tmpl/intranet-tmpl/prog/en/css/intranet.css
Removing koha-tmpl/intranet-tmpl/prog/en/css/intranet2.css
Removing koha-tmpl/intranet-tmpl/prog/en/css/intranet2popup.css
Removing koha-tmpl/intranet-tmpl/prog/en/css/pages/addbiblio.css
Removing koha-tmpl/intranet-tmpl/prog/en/css/staff-global2.css
Removing koha-tmpl/intranet-tmpl/prog/en/includes/acquisitions-history-search.inc
Removing koha-tmpl/intranet-tmpl/prog/en/includes/error-bottom.inc
Removing koha-tmpl/intranet-tmpl/prog/en/includes/error-top.inc
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt
Removing koha-tmpl/intranet-tmpl/prog/en/modules/help/acqui/newbiblio.tt
Removing koha-tmpl/intranet-tmpl/prog/en/modules/help/admin/aqbookfund.tt
Removing koha-tmpl/intranet-tmpl/prog/en/modules/help/admin/aqbudget.tt
Removing koha-tmpl/intranet-tmpl/prog/en/modules/help/members/messaging.tt
Removing koha-tmpl/intranet-tmpl/prog/en/modules/maint/catmaintain.tt
Removing koha-tmpl/intranet-tmpl/prog/en/modules/members/messaging.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/serials/subscription-add.tt
Removing koha-tmpl/intranet-tmpl/prog/img/csv_icon.gif
Removing koha-tmpl/intranet-tmpl/prog/img/icon_xml.gif
Removing koha-tmpl/intranet-tmpl/prog/img/itemtypeimg/vokal/BOOK-32px.png
Removing koha-tmpl/intranet-tmpl/prog/img/itemtypeimg/vokal/BOOK.png
Removing koha-tmpl/intranet-tmpl/prog/img/pdficon_large.gif
Removing koha-tmpl/opac-tmpl/prog/itemtypeimg/vokal/BOOK-32px.png
Removing koha-tmpl/opac-tmpl/prog/itemtypeimg/vokal/BOOK.png
Auto-merging members/member.pl
Removing members/messaging.pl
Auto-merging members/moremember.pl
CONFLICT (content): Merge conflict in members/moremember.pl
Auto-merging members/notices.pl
Auto-merging members/readingrec.pl
Auto-merging misc/cronjobs/advance_notices.pl
Auto-merging misc/cronjobs/overdue_notices.pl
Removing misc/spellcheck_suggest/make_spellcheck_suggest.pl
Auto-merging opac/opac-detail.pl
Auto-merging opac/opac-reserve.pl
Auto-merging opac/opac-user.pl
CONFLICT (content): Merge conflict in opac/opac-user.pl
Auto-merging reserve/request.pl
Auto-merging t/00-testcritic.t
CONFLICT (content): Merge conflict in t/00-testcritic.t
Removing xt/check_sysprefs.t
Removing xt/syspref.t
Comment 25 Elliott Davis 2011-10-15 18:31:41 UTC
merged with master https://github.com/libsysguy/koha-hourly
Comment 26 Ian Walls 2011-10-17 18:14:50 UTC
Notes from testing Elliott's merge work:


Unit Type is free text; should be dropdown.  Empty values can be supplied, as well as nonsense values like "widgets".  We need to tighten this up for usability, as well as be sure to provide explicit default value of 'Days' in case the form is submitted without a value unittype param.

References to 'decreaseLoanHighHoldsDuration' and 'decreaseLoanHighHoldsValue'.  This is happening in the checkHighHolds() subroutine (incorrectly documented as CheckHighHolds in POD).  Looks like this is an additional feature that will shorten an item's loan period based on whether it has many holds.  An interesting and valuable feature to be sure, but not part of of Hourly Loans.  I also fail to see the preference in the Syspref Editor.

Seems to also include the changes for bug 6321, which has not yet passed QA.

Also includes 3.4.1 release notes, which are not appropriate for this base, as well as a ton of .po files.

Other included work seems to include SIP2 Authentication changes (adding institution code to login)

Use of CURDATE() in SQL circulation functions almost entirely eradicated in favour of NOW(), except in longoverdue.pl.


Given the usability issue in the circ rules, as well as the several other bug fixes/enhancements which seem to be mixed in, I'm marking this as Failed QA for now.
Comment 27 Elliott Davis 2011-10-18 13:52:55 UTC
I have fixed the selection option in the smart-rules.tt file to only allow for accepted values.  It will also select correctly based on what you currently have.  

As for the .po files I'm not sure how to correct that on my branch.  

With the sysprefs I would like the original devel to chime in and let us know what he was planning or if it is even finished.  

Let me know what to do with the .po files and I'll try and correct whatever the issue is.
Comment 28 Ian Walls 2011-10-18 19:52:09 UTC
.po files removed.

Dropdowns have been added for existing circ rules; since these rules cannot be edited on their own lines, these dropdowns are misleading.  I don't think they properly save, either; after saving an hourly rule, the database did not record an hourly rule.

More importantly, I'm unable to view a patron with checkouts; I get:

Can't call method "clone" without a package or object reference at /home/sekjal/kohaclone/members/moremember.pl line 500.

which is in build_issue_data:

my $issuedate;
if ($issue->{issuedate} ) {
  $issuedate = $issue->{issuedate}->clone();
}

Since $issuedate is just a string from the issues table, it's clear that calling an object reference on it would fail... I don't know why this is working in Elliott's production system.  Something must be different there.  I cannot even tell what "clone" is supposed to do for me.

Failed QA persists.
Comment 29 Elliott Davis 2011-10-20 01:51:28 UTC
Fixed the dropdown issue and also fixed the clone issue.

The problem resided in Member.pm when the datetime wasn't converted to an object.  

The solutions have been pushed to my github branch for further testing.
Comment 30 Ian Walls 2011-10-20 02:32:27 UTC
Elliott's latest patches fix my issues with saving rules, and loading patron's with checkouts.

One small issue around the data conversion: old issues are translated to being due at 23:00 on the day in question, but new materials checked out under daily loan rules are due at 23:59.  This is inconsistent, and should be changed as a matter of the conversion.

I've set up my system to use the standard fines.pl crontab, running at 01:20 every day.  I'll check in the morning to see how it's handled an hourly loan with a fine interval of '1' (which I would assume means hours in this case).

Minutes should be supported as a valid interval, per the RFC.  This can be added in a follow up patch, but it should come soon, so libraries can 30 min loan periods (or less, if need be).

On this GitHub branch, please notice that DB rev 3.05.00.020 is used for Hourly Loans work instead of the value it is actually assigned in master.  This will be a small merge issue with master, when the time comes.  Also, including all the hourly loans database changes in updatedatabase.pl will be required.

Thank you to Elliott Davis for his hard work on fixing and adapting this code to meet my testing requirements.  I shall continue testing.  Even if we cannot have this ready by 3.6, we will have a very strong candidate for inclusion early in 3.8 cycle.
Comment 31 Katrin Fischer 2011-10-20 04:18:14 UTC
Found another little problem: changes from updatedatabase have to be added to kohastructure.sql
Comment 32 Elliott Davis 2011-10-20 14:06:57 UTC
Kohastructure.sql patched and pushed
Comment 33 Katrin Fischer 2011-11-02 20:34:18 UTC
Hm, I think the recent changes perhaps need another sign-off. Changing the patch status to reflect that.
Comment 34 Elliott Davis 2011-11-30 17:18:59 UTC
After having the new patches in production for over a month now with no errors I think we can say with relative confidence that it is functional with the new patches.  I am marking it signed off for now to see if we need a secondary sign off.

Latest rebased branch is at: https://github.com/libsysguy/koha-hourly
Comment 35 Paul Poulain 2011-12-13 13:50:35 UTC
Sorry, but it's unclear to me how to test/QA/push this patch. The 2 small patches here are not enough, i'm sure. which git repo should I pull from ? patch attached must be applied on the git repo ?

explanations welcomed !
Comment 36 Elliott Davis 2011-12-13 15:19:44 UTC
Paul,

There are currently two options for applying this patch.  You can either:

A) Pull from the repo I created https://github.com/libsysguy/koha-hourly.  This should be at most one week behind master but i generally try and update it everyday.

B) Checkout origin/new/awaiting_qa/but_5549 and apply the two patches listed here.  This version will not be merged with master and all the rebase work will have to be done yourself.  

I run my rebased branch in production so there shouldn't be any issues.  With that being said, I don't claim to be a rebasing expert by any stretch of the imagination so if there is a bug in my branch there is a good chance that is a rebase bug.  

You will also need to install the packages 
libdatetime-set-perl
libdatetime-format-dateparse-perl

And Apply the atomic update found at installer/data/mysql/atomicupdate/hourlyloans.sql
Comment 37 Paul Poulain 2012-01-20 16:38:12 UTC
Testing & QAing at the same time (even if I won't signoff & passed QA myself)


Here are the commands i've entered to do some test:
git remote add libsysguy git://github.com/libsysguy/koha-hourly.git
git remote update
git checkout -b BZ5549
git merge libsysguy/hourly

Auto-merging C4/Circulation.pm
Auto-merging C4/Context.pm
Auto-merging C4/Print.pm
Auto-merging C4/SIP/ILS/Patron.pm
Auto-merging catalogue/detail.pl
Auto-merging catalogue/issuehistory.pl
Auto-merging circ/circulation.pl
Auto-merging circ/overdue.pl
Auto-merging circ/returns.pl
Auto-merging installer/data/mysql/kohastructure.sql
Auto-merging installer/data/mysql/updatedatabase.pl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt
Auto-merging misc/cronjobs/overdue_notices.pl
Removing misc/translator/po/am-Ethi-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/am-Ethi-i-staff-t-prog-v-3006000.po
Removing misc/translator/po/am-Ethi-pref.po
Removing misc/translator/po/ar-Arab-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/ar-Arab-i-staff-t-prog-v-3006000.po
Removing misc/translator/po/ar-Arab-pref.po
Removing misc/translator/po/az-AZ-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/az-AZ-i-staff-t-prog-v-3006000.po
Removing misc/translator/po/az-AZ-pref.po
Removing misc/translator/po/ben-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/ben-i-staff-t-prog-v-3006000.po
Removing misc/translator/po/ben-pref.po
Removing misc/translator/po/bg-Cyrl-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/bg-Cyrl-i-staff-t-prog-v-3006000.po
Removing misc/translator/po/bg-Cyrl-pref.po
Removing misc/translator/po/ca-VA-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/ca-VA-i-staff-t-prog-v-3006000.po
Removing misc/translator/po/ca-VA-pref.po
Removing misc/translator/po/cs-CZ-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/cs-CZ-i-staff-t-prog-v-3006000.po
Removing misc/translator/po/cs-CZ-pref.po
Removing misc/translator/po/da-DK-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/da-DK-i-staff-t-prog-v-3006000.po
Removing misc/translator/po/da-DK-pref.po
Removing misc/translator/po/de-DE-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/de-DE-i-staff-t-prog-v-3006000.po
Removing misc/translator/po/de-DE-pref.po
Removing misc/translator/po/el-GR-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/el-GR-i-staff-t-prog-v-3006000.po
Removing misc/translator/po/el-GR-pref.po
Removing misc/translator/po/en-GB-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/en-GB-i-staff-t-prog-v-3006000.po
Removing misc/translator/po/en-GB-pref.po
Removing misc/translator/po/en-NZ-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/en-NZ-i-staff-t-prog-v-3006000.po
Removing misc/translator/po/en-NZ-pref.po
Removing misc/translator/po/en-USACADEMIC-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/es-ES-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/es-ES-i-staff-t-prog-v-3006000.po
Removing misc/translator/po/es-ES-pref.po
Removing misc/translator/po/eu-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/eu-pref.po
Removing misc/translator/po/fa-Arab-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/fa-Arab-pref.po
Removing misc/translator/po/fi-FI-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/fi-FI-i-staff-t-prog-v-3006000.po
Removing misc/translator/po/fi-FI-pref.po
Removing misc/translator/po/fr-CA-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/fr-CA-i-staff-t-prog-v-3006000.po
Removing misc/translator/po/fr-CA-pref.po
Removing misc/translator/po/fr-FR-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/fr-FR-i-staff-t-prog-v-3006000.po
Removing misc/translator/po/fr-FR-pref.po
Removing misc/translator/po/gl-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/gl-pref.po
Removing misc/translator/po/he-Hebr-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/he-Hebr-pref.po
Removing misc/translator/po/hi-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/hi-i-staff-t-prog-v-3006000.po
Removing misc/translator/po/hi-pref.po
Removing misc/translator/po/hr-HR-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/hr-HR-i-staff-t-prog-v-3006000.po
Removing misc/translator/po/hr-HR-pref.po
Removing misc/translator/po/hu-HU-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/hu-HU-i-staff-t-prog-v-3006000.po
Removing misc/translator/po/hu-HU-pref.po
Removing misc/translator/po/hy-Armn-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/hy-Armn-i-staff-prog-v-3006000.po
Removing misc/translator/po/hy-Armn-i-staff-t-prog-v-3006000.po
Removing misc/translator/po/hy-Armn-pref.po
Removing misc/translator/po/id-ID-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/id-ID-i-staff-t-prog-v-3006000.po
Removing misc/translator/po/id-ID-pref.po
Removing misc/translator/po/is-IS-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/is-IS-pref.po
Removing misc/translator/po/it-IT-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/it-IT-i-staff-t-prog-v-3006000.po
Removing misc/translator/po/it-IT-pref.po
Removing misc/translator/po/ja-Jpan-JP-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/ja-Jpan-JP-pref.po
Removing misc/translator/po/kn-Knda-i-staff-t-prog-v-3006000.po
Removing misc/translator/po/ko-Kore-KP-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/ko-Kore-KP-pref.po
Removing misc/translator/po/lo-Laoo-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/lo-Laoo-i-staff-t-prog-v-3006000.po
Removing misc/translator/po/lo-Laoo-pref.po
Removing misc/translator/po/mi-NZ-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/mi-NZ-pref.po
Removing misc/translator/po/mon-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/mon-i-staff-t-prog-v-3006000.po
Removing misc/translator/po/mon-pref.po
Removing misc/translator/po/mr-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/mr-i-staff-t-prog-v-3006000.po
Removing misc/translator/po/mr-pref.po
Removing misc/translator/po/ms-MY-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/ms-MY-i-staff-t-prog-v-3006000.po
Removing misc/translator/po/ms-MY-pref.po
Removing misc/translator/po/nb-NO-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/nb-NO-i-staff-t-prog-v-3006000.po
Removing misc/translator/po/nb-NO-pref.po
Removing misc/translator/po/ne-NE-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/ne-NE-i-staff-t-prog-v-3006000.po
Removing misc/translator/po/ne-NE-pref.po
Removing misc/translator/po/nl-BE-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/nl-BE-i-staff-t-prog-v-3006000.po
Removing misc/translator/po/nl-BE-pref.po
Removing misc/translator/po/nl-NL-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/nl-NL-i-staff-t-prog-v-3006000.po
Removing misc/translator/po/nl-NL-pref.po
Removing misc/translator/po/nn-NO-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/nn-NO-i-staff-prog-v-3006000.po
Removing misc/translator/po/nn-NO-i-staff-t-prog-v-3006000.po
Removing misc/translator/po/nn-NO-pref.po
Removing misc/translator/po/pbr-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/pbr-i-staff-t-prog-v-3006000.po
Removing misc/translator/po/pbr-pref.po
Removing misc/translator/po/pl-PL-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/pl-PL-i-staff-t-prog-v-3006000.po
Removing misc/translator/po/pl-PL-pref.po
Removing misc/translator/po/prs-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/prs-i-staff-t-prog-v-3006000.po
Removing misc/translator/po/prs-pref.po
Removing misc/translator/po/pt-BR-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/pt-BR-i-staff-t-prog-v-3006000.po
Removing misc/translator/po/pt-BR-pref.po
Removing misc/translator/po/pt-PT-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/pt-PT-i-staff-t-prog-v-3006000.po
Removing misc/translator/po/pt-PT-pref.po
Removing misc/translator/po/ro-RO-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/ro-RO-i-staff-t-prog-v-3006000.po
Removing misc/translator/po/ro-RO-pref.po
Removing misc/translator/po/ru-RU-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/ru-RU-i-staff-t-prog-v-3006000.po
Removing misc/translator/po/ru-RU-pref.po
Removing misc/translator/po/sd-PK-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/sd-PK-i-staff-t-prog-v-3006000.po
Removing misc/translator/po/sd-PK-pref.po
Removing misc/translator/po/sk-SK-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/sk-SK-i-staff-t-prog-v-3006000.po
Removing misc/translator/po/sk-SK-pref.po
Removing misc/translator/po/sl-SI-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/sl-SI-i-staff-t-prog-v-3006000.po
Removing misc/translator/po/sl-SI-pref.po
Removing misc/translator/po/sq-AL-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/sq-AL-i-staff-t-prog-v-3006000.po
Removing misc/translator/po/sq-AL-pref.po
Removing misc/translator/po/sr-Cyrl-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/sr-Cyrl-i-staff-t-prog-v-3006000.po
Removing misc/translator/po/sr-Cyrl-pref.po
Removing misc/translator/po/sv-SE-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/sv-SE-i-staff-t-prog-v-3006000.po
Removing misc/translator/po/sv-SE-pref.po
Removing misc/translator/po/ta-LK-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/ta-LK-i-staff-t-prog-v-3006000.po
Removing misc/translator/po/ta-LK-pref.po
Removing misc/translator/po/tet-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/tet-i-staff-t-prog-v-3006000.po
Removing misc/translator/po/tet-pref.po
Removing misc/translator/po/th-THA-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/th-THA-i-staff-t-prog-v-3006000.po
Removing misc/translator/po/th-THA-pref.po
Removing misc/translator/po/tl-PH-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/tl-PH-pref.po
Removing misc/translator/po/tr-TR-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/tr-TR-i-staff-t-prog-v-3006000.po
Removing misc/translator/po/tr-TR-pref.po
Removing misc/translator/po/uk-UA-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/uk-UA-i-staff-t-prog-v-3006000.po
Removing misc/translator/po/uk-UA-pref.po
Removing misc/translator/po/ur-Arab-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/ur-Arab-i-staff-t-prog-v-3006000.po
Removing misc/translator/po/ur-Arab-pref.po
Removing misc/translator/po/vi-VN-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/vi-VN-i-staff-t-prog-v-3006000.po
Removing misc/translator/po/vi-VN-pref.po
Removing misc/translator/po/zh-Hans-CN-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/zh-Hans-CN-i-staff-t-prog-v-3006000.po
Removing misc/translator/po/zh-Hans-CN-pref.po
Removing misc/translator/po/zh-Hans-TW-i-opac-t-prog-v-3006000.po
Removing misc/translator/po/zh-Hans-TW-i-staff-t-prog-v-3006000.po
Removing misc/translator/po/zh-Hans-TW-pref.po
Auto-merging opac/opac-detail.pl
Auto-merging opac/opac-user.pl
CONFLICT (content): Merge conflict in opac/opac-user.pl
Automatic merge failed; fix conflicts and then commit the result.


=> the conflict in opac-user.pl is trivial to solve (2 lines commented against no line at all. Just needed to remove those lines)

QUESTION : why are translations removed ?

QA comments:
* indentation is 2 spaces in many places, should be 4 spaces
* there is a "misc/release_notes/release_notes_3_4_1.txt that should not be here
* there are some things that are not related to hourly loans at all, like, in members/member.pl:
-#use warnings; FIXME - Bug 2505
+use warnings;
same in members/deletemem.pl members/guarantor_search.pl, I did not make an exhaustive list, but there are many many many !!! Please investigate & fix, i'll check after you've made a 1st fix)
* there is a lack of documentation. I see in a template "High demand item" => what's this ? some functionnal documentation would be usefull. I think the http://wiki.koha-community.org/wiki/Hourly_Loans_RFC is not what has been implemented. For example, the RFCs are speaking of fines in minuts, nothing about that in the branch/code. It's OK for me, but we must know what this patch does exactly ! A functionnal doc on the wiki, with screenshot would be perfect !

not a reason to failed QA, just for information:
* there are (needed) reindentings in the middle of usefull code, making the result hard to read. For example
-        my $branch_count_query = "SELECT COUNT(*) FROM issues 
+        my $branch_count_query = "SELECT COUNT(*) FROM issues
* unit tests are provided, ++, once dependancies are provided, they are successfull
* there are new dependancies, that are added in the related file, ++


that's enough for now.

This comment may seem very long, but I think the cleaning & documentation won't be too hard to do. Keep on the good work, it's an interesting feature !

PS: could you explain the history of this ? it's been started by nicole, chris seems to have made things, then colin from PTFS-Europe, then Eliott,
Comment 38 Paul Poulain 2012-01-20 16:52:19 UTC
added QA comment:
the "Fixed Bracketing Error" is in the libsysguy/hourly repo, i'll mark it as obsolete.
The Patch to correct calculation of large durations from Colin is not AFAIK
Comment 39 Chris Cormack 2012-03-15 20:43:47 UTC
The latest incarnation of this, which has undergone a lot of testing is available to pull from

git://git.koha-community.org/wip/koha-catalyst.git bug_5549
Comment 40 Elliott Davis 2012-03-15 20:51:36 UTC
Tested:

Fines Accrual
Setting Hourly Items in circ rules
Hourly Checkouts
Normal Checkouts
Comment 41 Elliott Davis 2012-03-15 23:46:15 UTC
An additional note, the attached patch is obsolete.  IT has already been integrated into the branch mentioned in Chris' comment.  You should still require the following libraries: libreadonly-perl, libdatetime-format-dateparse-perl, and libdatetime-set-perl.
You should also still apply the atomic update installer/data/mysql/atomicupdate/hourlyloans.sql
Comment 42 Nicole C. Engard 2012-03-16 14:10:34 UTC
Test plan:

* Set circ rules for daily and hourly loans side by side
* Checkout a daily item to a patron
* Checkout an hourly item to same patron
* Checkout another hourly item to same patron
* Return one hourly item on time
* Return another hourly item late and check that fines accrued
* Return daily item and make sure it works right still
* Repeat by checking in times later than 1 hour to make sure fine accrue every hour
* Repeat with a different patron category

All above tests passed.
Comment 43 Nicole C. Engard 2012-03-16 14:19:48 UTC
Comment on attachment 5436 [details] [review]
Patch to correct calculation of large durations

Obsoleting patch since it's a branch now.
Comment 44 Elliott Davis 2012-03-16 14:22:51 UTC Comment hidden (obsolete)
Comment 45 Ian Walls 2012-03-19 15:35:29 UTC
We're getting there!  Found 4 serious problems, 2 warnings and 1 scope issue.  There is also a followup issue that can be dealt with later or in a separate bug.  Marking as Failed QA until the issues below are resolved.  But we're almost there!


Problems:

1. t/db_dependent/rollingloans.t fails on test 1.  Looks like it's trying to use hard-coded barcodes for patrons and materials that don't necessarily exist.  This causes an invalid due date to be returned, blowing up the test.  Problem with the test, not the code.

2. On patron details page, "checked out on" is showing the due datetime, not the checkout datetime.  The Due column only shows date, not time.  This seems to be fine on the circulation screen, but not on members/moremember.pl

3. SetVersion removed from updatedatebase.pl version 3.05.00.004.  Also, DB revs from atomicupdate/issues_datetime.pl are repeated in 3.05.00.020.  Really, nothing should be in updatedatabase.pl until the RM adds it.

4.  Release notes for 3.4.1 have been inserted... these are wicked out of date now, and not really appropriate for master anyway.


Potential problems:

1. Seems like some of the work for 7001 is showing up as new to this branch... could be a rebase issue, but there is a borrower field, suburb, which is referenced but does not exist.

2. What's up with the .po files here?  Usually they don't get touched during the addition of a feature.  I think Git learned about these, and now needs to forget.


Scope issue:

This still has the High Holds development snuck into it.  This is outside the scope of hourly loans, and should be it's own enhancement.  This project is already large enough without other, unrelated functionality hitching a ride.  It needs to be extricated from this branch.



Followup:

RM:  automatically have updatedatabase.pl run the hourlyloans.sql and issue_datetimes.pl updates to properly set the DB.

Add new dependencies ReadOnly, ReadOnly::XS and DateTime::Format::DateParse to install_misc/debian.packages and install_misc/ubuntu.packages
Comment 46 Chris Cormack 2012-03-20 00:47:23 UTC Comment hidden (obsolete)
Comment 47 Chris Cormack 2012-03-20 01:34:33 UTC
The test won't work with the sample database, as it relies on there being items, and circulation rules defined for them.

I think at this stage we need to remove t/db_dependent/rollingloans.t 

Or add items and circulation rules to the sample data. 

Colin do you have sample data you could give us for that purpose?
Comment 48 Paul Poulain 2012-03-20 12:32:46 UTC
Testing this patch by adding it manually on sandbox6 (that has been switched to unimarc for the time of the hackfest in Marseille)

What I did:
git remote add catalyst git://git.koha-community.org/wip/koha-catalyst.git
git remote update catalyst
git checkout -b test
git merge catalyst/merge_5549

patching database:
mysql test6 <installer/data/mysql/atomicupdate/hourlyloans.sql
then 
perl data/mysql/atomicupdate/issuedate_times.pl

trying to reach a page, getting

Can't locate DateTime/Format/DateParse.pm in @INC (@INC contains: /home/koha/src /etc/perl /usr/local/lib/perl/5.10.1 /usr/local/share/perl/5.10.1 /usr/lib/perl5 /usr/share/perl5 /usr/lib/perl/5.10 /usr/share/perl/5.10 /usr/local/lib/site_perl .) at /home/koha/src/Koha/DateUtils.pm line 23.

installing new dependancy

Getting
Can't locate DateTime/Set.pm in @INC (@INC contains: /home/koha/src /etc/perl /usr/local/lib/perl/5.10.1 /usr/local/share/perl/5.10.1 /usr/lib/perl5 /usr/share/perl5 /usr/lib/perl/5.10 /usr/share/perl/5.10 /usr/local/lib/site_perl .) at /home/koha/src/Koha/Calendar.pm line 7.

installing new dependancy

Getting
Can't locate Readonly.pm in @INC (@INC contains: /home/koha/src /etc/perl /usr/local/lib/perl/5.10.1 /usr/local/share/perl/5.10.1 /usr/lib/perl5 /usr/share/perl5 /usr/lib/perl/5.10 /usr/share/perl/5.10 /usr/local/lib/site_perl .) at /home/koha/src/Koha/Calendar.pm line 7.

installing new dependancy

Getting login, testing now...
Comment 49 Corinne BULAC 2012-03-20 15:57:26 UTC
For Le Cannet Library, two new item types were created: « Prêt à domicile », « Consultation sur place »
The following circulation and fines rules were defined  (patron categorie = all) :
1 -> « Prêt à domicile » - 10 documents for 21 days, fine grace period = 0, suspension in day = 1, renewal allowed=1, holds allowed=5
2 -> « Consultation sur place – 10 documents for 1 hour, fine grace period = 0, suspension in day = 1, renewal allowed=1, holds allowed=5

Lacks ?

Circulation and fine rules ->No Fine Grace period in hours ? No hard due in « hour » ? No global « hour due » in pref system ?
Overdue notices -> no delay in hours ? It should be possible to notice the patrons who are late more than an hour (or more)
Renewal Period -> no hours ?

Tested : 
1. Check out a hourly item -> OK
2. Check out a daily item to the same patron -> Ok
3. Check out another hourly item -> OK
4. Renewal a hourly item -> not OK. The circulation preference RewalPeriodBase -> »the old due date of the checkout » is not used. Calculation is made from the current date/hour
Return an hourly item from tab «  check out »-> software error « cant’ locate object method « daysBetween » via package « Koha :Calendar » at home/koha/src/C4/Circulation.pm lin 1799. But…return is done
5. Hold on a hourly item -> Hold is possible but when the item is returned we’ve got a software error  « cant’ locate object method « daysBetween » via package « Koha :Calendar » at home/koha/src/C4/Circulation.pm lin 1799. But…hold is done
6. Return hourly item on late -> not OK. No suspension in day was applied
Comment 50 Elliott Davis 2012-03-20 16:40:01 UTC
I've gone back and tried to reproduce the software errors mentioned in comment 49 but can't seem to get them.  Have you applied all the database updates?
Comment 51 Chris Cormack 2012-03-20 17:04:54 UTC
You are running the latest branch eh Elliot?

Testers. You do need to have the fines cronjob running at a frequent interval. Say every 5 minutes
Comment 52 delaye 2012-03-20 17:16:03 UTC
We saw the following errors:

1 / When we want save the checkin of an item, an error message appears
Can not locate object method "daysBetween" via package "Koha :: Calendar" at / line home/koha/src/C4/Circulation.pm 1799.

2 / When we want save the checkin of an item, the patron  is not suspended while debarred is actived in the rules

3 / The renewal of a loan does not use all options of the preference system RenewalPeriodBase
It uses the current date only

The improvements requests:
1 / For the dates of the notification we have need to use the hour unit and not only the day unit

2 / in the rules, if the column 'unit' was selected hours, the column 'hard due date' should display a list of hours and not a list of date(calendar).
Comment 53 Ian Walls 2012-03-20 17:19:10 UTC
Pulled down the new code.  Comments as I find them:

high holds has been removed.

in C4::AddReturn, the call to GetItemIssue no longer passes the $dropbox variable.... since it doesn't seem to do this in master, I think this is yet another additional feature that's been separated out.

Removes bug 6335 content (hadn't noticed that was in there before!)

Reverts the 'use warnings' on many pages.

Removes passing 'issuescount' to build_issues_data in members/moremember.pl, which is good, since it's redundant to just getting array length.

Leaves release notes and .po files alone.


Remaining issues:


Fixes updatedatabase.pl problems that were noted, however the database update lines added are (mostly) redundant with atomicupdate/issues_datetime.pl   Up to the RM on how he wants to handle implementing the database changes here.

rollingloans.t still needs some love.

Issues as noted by Hackfesters.  Will address in my next comment.
Comment 54 Chris Cormack 2012-03-20 17:26:20 UTC
(In reply to comment #53)
> Pulled down the new code.  Comments as I find them:
> 
> high holds has been removed.

I put this on its on bug.
> 
> in C4::AddReturn, the call to GetItemIssue no longer passes the $dropbox
> variable.... since it doesn't seem to do this in master, I think this is yet
> another additional feature that's been separated out.
> 
> Removes bug 6335 content (hadn't noticed that was in there before!)
> 
> Reverts the 'use warnings' on many pages.
> 

I put these on bug 2505 where they belong.

> Removes passing 'issuescount' to build_issues_data in members/moremember.pl,
> which is good, since it's redundant to just getting array length.
> 
> Leaves release notes and .po files alone.
> 
> 
> Remaining issues:
> 
> 
> Fixes updatedatabase.pl problems that were noted, however the database
> update lines added are (mostly) redundant with
> atomicupdate/issues_datetime.pl   Up to the RM on how he wants to handle
> implementing the database changes here.
> 
> rollingloans.t still needs some love.
> 
This either needs sample data added for circ rules and items or to be left out until it does. I don't think it should block this though.
 
> Issues as noted by Hackfesters.  Will address in my next comment.
Comment 55 Ian Walls 2012-03-20 17:29:49 UTC
Chris,


Thanks for breaking those bugs out.  I agree rollingloans.t shouldn't block, but it would be great if we could either fix or squelch it so that the automated tests work.

I think the issues found by our hackfest testers are the next big things to solve.  I'll investigate shortly.
Comment 56 Chris Cormack 2012-03-20 17:34:09 UTC
(In reply to comment #55)
> Chris,
> 
> 
> Thanks for breaking those bugs out.  I agree rollingloans.t shouldn't block,
> but it would be great if we could either fix or squelch it so that the
> automated tests work.
> 

Yep if think for now remove the test. Unless Colin has sample data available.


> I think the issues found by our hackfest testers are the next big things to
> solve.  I'll investigate shortly.

Cool
Comment 57 Paul Poulain 2012-03-20 17:39:44 UTC
(In reply to comment #51)
> You are running the latest branch eh Elliot?
> 
> Testers. You do need to have the fines cronjob running at a frequent
> interval. Say every 5 minutes

Just for the record = that's not the case, the cronjob doesn't run every 5 minuts (maybe thats something that can be added in the documentation ), i'll improve sandbox behaviour tomorrow (time to leave now, hackfest day 2 finished in Marseille)
Comment 58 Ian Walls 2012-03-20 19:28:54 UTC
Looks like a typo in C4/Circulation.pm in 1799... Koha::Calendar subroutine is days_between(), not daysBetween().  Followup patch should be easy enough.

More shortly
Comment 59 Chris Cormack 2012-03-20 19:34:32 UTC
(In reply to comment #58)
> Looks like a typo in C4/Circulation.pm in 1799... Koha::Calendar subroutine
> is days_between(), not daysBetween().  Followup patch should be easy enough.
> 
Fixed and pushed to the branch
Comment 60 Ian Walls 2012-03-20 21:39:01 UTC
New error:  when attempting to check in an item from circ/circulation.pl issues table, I get:

Can't locate object method "truncate" via package "C4::Dates" at /home/sekjal/kohaclone/Koha/Calendar.pm line 171.
Comment 61 Chris Cormack 2012-03-20 21:52:01 UTC
(In reply to comment #60)
> New error:  when attempting to check in an item from circ/circulation.pl
> issues table, I get:
> 
> Can't locate object method "truncate" via package "C4::Dates" at
> /home/sekjal/kohaclone/Koha/Calendar.pm line 171.

Hmmm, working on it now
Comment 62 Chris Cormack 2012-03-20 22:25:10 UTC
Fix pushed.
Comment 63 Ian Walls 2012-03-20 22:26:21 UTC
(In reply to comment #52)
> We saw the following errors:
> 
> 1 / When we want save the checkin of an item, an error message appears
> Can not locate object method "daysBetween" via package "Koha :: Calendar" at
> / line home/koha/src/C4/Circulation.pm 1799.

Fixed!


> 
> 2 / When we want save the checkin of an item, the patron  is not suspended
> while debarred is actived in the rules

I see this...I checked in an hourly material several hours late, the next day, and was not debarred at all.

Looks like _FixFineDaysOnReturn still expects a C4::Dates object instead of a DateTime.


> 
> 3 / The renewal of a loan does not use all options of the preference system
> RenewalPeriodBase
> It uses the current date only

I cannot confirm this; RenewalPeriodBase == 'current due date' renews item 1 hr loan item for 1 more hour;  When I set it to 'right now', properly renews to now + 1 hr.

I'm renewing from circ/circulation.pl and from member/moremembers.pl, as well as by checking the material out to the same patron again.  Are you coming at it another way?


> 
> The improvements requests:
> 1 / For the dates of the notification we have need to use the hour unit and
> not only the day unit
> 
> 2 / in the rules, if the column 'unit' was selected hours, the column 'hard
> due date' should display a list of hours and not a list of date(calendar).

Both of these can be added as followup patches, as they're not required for the basic usage of this feature (though I absolutely agree they should be included).
Comment 64 Chris Cormack 2012-03-20 22:29:26 UTC
(In reply to comment #63)
> (In reply to comment #52)
> > We saw the following errors:
> > 
> > 1 / When we want save the checkin of an item, an error message appears
> > Can not locate object method "daysBetween" via package "Koha :: Calendar" at
> > / line home/koha/src/C4/Circulation.pm 1799.
> 
> Fixed!
> 
> 
> > 
> > 2 / When we want save the checkin of an item, the patron  is not suspended
> > while debarred is actived in the rules
> 
> I see this...I checked in an hourly material several hours late, the next
> day, and was not debarred at all.
> 
> Looks like _FixFineDaysOnReturn still expects a C4::Dates object instead of
> a DateTime.
> 
Fixed now
Comment 65 Ian Walls 2012-03-20 22:51:02 UTC
OK, so barring new information on the renewals issue, we're down to the following:

RM must decide how best to implement the necessary changes to the DB (databaseupdate.pl v. atomicupdate), and reconcile the three places where we have updates right now.

rollingloans.t should be pulled, since it's faulty, and will just break out automated testing.


That's all the issues I'm aware of that aren't followup improvements.  So, it is with great pleasure, that I mark this development as Passed QA.
Comment 66 Corinne BULAC 2012-03-21 09:40:02 UTC
about RenewalPeriodBase...two options are possible

"Option 1" ->The old due date of the checkout
"Option 2" ->The current date

For a daily loan

- « Option 1 » dones these things
A loan made the « 01/03/2012 », loan period = 14 days, the due date is « 15/03/2012 ». If you renew it the 02/03/2012 », the due date becomes « 29/03/2012 »

- « Option 2 » dones this things
A loan made the « 01/03/2012 », loan period = 14 days, due date is 15/03/2012 ». If you renew it the 02/03/2012 », the due date becomes « 16/03/2012 »

For a hourly loan these two options should done :

- « Option 1 »
A loan made the « 01/03/2012  at 15:00», loan period = 1 hour, due date is « 15/03/2012 at 16 :00 ». If you renew it the 01/03/2012 at 15:15, the due date becomes « 15/03/2012 at 17 :00 »

- « Option 2 »
A loan made the « 01/03/2012  at 15:00», loan period = 1 hour, due date is « 15/03/2012 at 16 :00 ». If you renew it the 01/03/2012 at 15:15, the due date becomes « 15/03/2012 at 16 :15 »

For an hourly loan, "option 1" doesnt't work. It is as if « option 2 » was systematically applied

New tests this morning : 

Check-in of a hourly loan gives a software error message “Can’t call method “inverse” without a package or object reference at /usr/lib/perl5/DateTime/Duration.pm line 237” and ckeckin is not done
Comment 67 Ian Walls 2012-03-21 12:09:42 UTC
Corrine,

> 
> For a hourly loan these two options should done :
> 
> - « Option 1 »
> A loan made the « 01/03/2012  at 15:00», loan period = 1 hour, due date is «
> 15/03/2012 at 16 :00 ». If you renew it the 01/03/2012 at 15:15, the due
> date becomes « 15/03/2012 at 17 :00 »
> 
> - « Option 2 »
> A loan made the « 01/03/2012  at 15:00», loan period = 1 hour, due date is «
> 15/03/2012 at 16 :00 ». If you renew it the 01/03/2012 at 15:15, the due
> date becomes « 15/03/2012 at 16 :15 »
> 
> For an hourly loan, "option 1" doesnt't work. It is as if « option 2 » was
> systematically applied
> 

My testing shows this to work just fine... can you confirm if your sandbox a) is running the latest code from the repo (we updated last night) and b) has applied both the atomicupdates/hourlyloans.sql AND atomicupdates/issues_datetime.pl database revisions?

Also, what mechanism(s) are you using to do the renewal?  It could be we're going about it different ways, and thus getting different results.


> New tests this morning : 
> 
> Check-in of a hourly loan gives a software error message “Can’t call method
> “inverse” without a package or object reference at
> /usr/lib/perl5/DateTime/Duration.pm line 237” and ckeckin is not done

Confirmed.... will investigate and patch.
Comment 68 Paul Poulain 2012-03-21 13:30:51 UTC
(In reply to comment #67)

> My testing shows this to work just fine... can you confirm if your sandbox
> a) is running the latest code from the repo (we updated last night) 
I confirm

> and b)
> has applied both the atomicupdates/hourlyloans.sql AND
> atomicupdates/issues_datetime.pl database revisions?
the sandbox has already been reseted so I can't check, but i'm sure yes, both where applied.

> Also, what mechanism(s) are you using to do the renewal?  It could be we're
> going about it different ways, and thus getting different results.
it's not me that has the answer to this question

About the sandbox used = it's the database available on sandboxes (UNIMARC, 20k records). I want to put it to git contrib repo, but can't be successfull pushing. We've asked galen, no feedback
Comment 69 Leila 2012-03-21 14:44:25 UTC
(In reply to comment #67)

> 
> Also, what mechanism(s) are you using to do the renewal?  It could be we're
> going about it different ways, and thus getting different results.
> 
Yesterday, we tried the renewal via the opac, and on the professional interface. Fail in the 2 cases.
Today, only on Home › Circulation › Checkouts › then, on the list of checkouts. Fail.
Comment 70 Ian Walls 2012-03-21 15:54:31 UTC
I think I've found the solution to the 'inverse' error... in the fines in days code, there was an attempt to subtract a number from a DateTime::Duration object.

Fix pass on a fix shortly (in conference sessions at the moment)
Comment 71 Ian Walls 2012-03-21 20:18:18 UTC
Sent Chris my patch to fix suspension in days and the resulting 'inverse' bug.
Comment 72 Chris Cormack 2012-03-21 20:38:41 UTC
Setting back to Passed QA, 

Am unable to recreate the renewal issue, after trying numerous times, but the other errors have been fixed.

Master has been merged in, so it should be a clean merge.
Comment 73 delaye 2012-03-22 10:59:28 UTC
Tests :

Return  of items : ok

Renews : it's not ok. 
koha change the minutes but not the hour. 
Examples : 
1/ preference system with current date
2/ The due hour of the item is  12:05
3/ I renew the item at 11:15
4/ Koha apply the due hour 12:15 and not 13:15


Examples : 
1/ preference system with the old due date of the checkout
2/ The due hour of the item is  12:05
3/ I renew the item at 11:15
4/ Koha apply the due hour 12:15 and not 13:05


Other cas to resolve : 
1/ In the rules the unit is 1 hour
2/ the library close at 18:00 (Information not indicated in koha : problem ?) 
3/ I checkin a item with hourly loan
4/ At 17H05 I want renew the item so the due hour it's 18h 05 but the libray close at 18H00 : in this cas what do koha ? nothing ! in this cas koha must forbidden the renew
So the following point is important :

In the rules, if the column 'unit' was selected hours, the column 'hard due date' should display a list of hours and not a list of date(calendar).
Comment 74 delaye 2012-03-22 12:57:18 UTC
I forgot  the debarred cas.
When a patron return an item with delay he is not debarred while the rules fix debarred for hourly loan.
Comment 75 delaye 2012-03-22 13:01:26 UTC
Erratum comment 73
You must understand :

Renews : it's not ok. 
koha change the minutes but not the hour. 
Examples : 
1/ preference system with current date
2/ The due hour of the item is  12:05
3/ I renew the item at 11:15
4/ Koha apply the due hour 11:15 and not 12:15


Examples : 
1/ preference system with the old due date of the checkout
2/ The due hour of the item is  12:05
3/ I renew the item at 11:15
4/ Koha apply the due hour 11:15 and not 12:05
Comment 76 Ian Walls 2012-03-22 14:09:37 UTC
Are you testing on a public sandbox?  We're just not seeing the renewals problem on other systems, so it's either got to be a settings or a configuration issue.  It *can* work... we just need to make sure it works under all normal circumstances

The hard duedates time is a followup enhancement, and should not hold up getting this feature into Koha.

Right now, suspension in days works like thus:

when a material is returned late, it's interval is calculated less the grace period (which is always measured in days).  If the duration is greater than 1, then the number of days is multiplied by the fine in days number, and the user is suspended until today + that many days.

So what I think needs to happen is that we modify it so that the grace period calculates in the same units as the loan does.  That is, 2 hours late with a 1 hour grace debars for 1 * (finedays) days.

I take it we don't need suspension in hours?  It would be easy enough to do... and since hours convert easily to days, it would work in all cases... but I'm not sure it's actually useful to anyone.
Comment 77 delaye 2012-03-22 14:33:35 UTC
we use a sandbox for the tests renew
it works under all normal circumstances
Comment 78 Paul Poulain 2012-03-22 14:52:47 UTC
(In reply to comment #76)
> So what I think needs to happen is that we modify it so that the grace
> period calculates in the same units as the loan does.  That is, 2 hours late
> with a 1 hour grace debars for 1 * (finedays) days.
> 
> I take it we don't need suspension in hours?  It would be easy enough to
> do... and since hours convert easily to days, it would work in all cases...
> but I'm not sure it's actually useful to anyone.

I thought I wrote that here, but maybe it was just on IRC: I spoke with librarians yesterday, or 2 days ago, and we couldn't figure a use case of having 1 hour debarment if you're 1 hour late.
If a library request that, that will be a future enhancement, we will quote it ;-)
Comment 79 Paul Poulain 2012-03-22 16:41:52 UTC
Patch pushed !!!

I spoke with Leila and Stéphane about the renewal problem, it seems not to be a blocker for pushing this wonderful feature, so, it's pushed.

I had a small conflict when pushing, in members/moremember.pl

<<<<<<<<
my $today       = POSIX::strftime("%Y-%m-%d", localtime);	# iso format
=======
my $today       = DateTime->now( time_zone => C4::Context->tz);
$today->truncate(to => 'days');
>>>>>>> catalyst/merged_5549

I've fixed it by keeping
my $today       = DateTime->now( time_zone => C4::Context->tz);
$today->truncate(to => 'days');

and removing
my $today       = POSIX::strftime("%Y-%m-%d", localtime);	# iso format

I couldn't see any side effect, but, Chris or Ian, pls confirm !

Will send a mail to koha-devel to call for more tests !
Comment 80 Chris Cormack 2012-03-22 19:44:09 UTC Comment hidden (obsolete)
Comment 81 Chris Cormack 2012-03-22 19:44:50 UTC
Fix for the failing test
Comment 82 Liz Rea 2012-03-22 20:27:48 UTC Comment hidden (obsolete)
Comment 83 Ian Walls 2012-03-22 21:36:15 UTC Comment hidden (obsolete)
Comment 84 Paul Poulain 2012-03-27 07:19:11 UTC
Comment on attachment 8543 [details] [review]
Bug 5549 : Fixing the test to only run if the test data exists

this patch has been pushed (Should please jenkins)
Comment 85 Katrin Fischer 2012-04-01 21:18:31 UTC
Is this still needs signoff? Seems the last patch has been pushed?
Comment 86 MJ Ray (software.coop) 2012-04-05 17:56:53 UTC Comment hidden (obsolete)
Comment 87 MJ Ray (software.coop) 2012-04-05 18:11:02 UTC Comment hidden (obsolete)
Comment 88 MJ Ray (software.coop) 2012-04-05 18:20:44 UTC Comment hidden (obsolete)
Comment 89 Katrin Fischer 2012-04-06 13:27:57 UTC
Changing severity to blocker - because missing change to kohastructure breaks new installations.
Comment 90 MJ Ray (software.coop) 2012-04-06 13:58:41 UTC Comment hidden (obsolete)
Comment 91 MJ Ray (software.coop) 2012-04-06 14:09:21 UTC Comment hidden (obsolete)
Comment 92 Katrin Fischer 2012-04-06 14:23:52 UTC Comment hidden (obsolete)
Comment 93 Ian Walls 2012-04-06 14:30:50 UTC
Signed off patch to fix kohastructure does exactly that, and adds a database update to make sure it's properly added no matter when you did your upgrade.

Passed QA.
Comment 94 Marcel de Rooy 2012-04-06 14:34:05 UTC
(In reply to comment #93)
> Signed off patch to fix kohastructure does exactly that, and adds a database
> update to make sure it's properly added no matter when you did your upgrade.

If I understand correctly, the column could be there already. Why not check that first before trying to add again?
Comment 95 Katrin Fischer 2012-04-06 14:38:14 UTC
Hi  Marcel, 
we discussed this on IRC - that's MJ deleted the column from the earlier update. This way, whereever you start from now, it will be ok.
Comment 96 Paul Poulain 2012-04-06 16:00:21 UTC
Comment on attachment 8959 [details] [review]
[SIGNED-OFF] bug 5549: change to kohastructure.sql missing from commit 8cea8e5cc05075dc391194248d31a54586389601

this patch has been pushed, well spotted cait!
Comment 97 Paul Poulain 2012-04-06 16:01:43 UTC
The BLO follow-up has been pushed. Unless i've misunderstood something, the Bug 5549 followup: fix suspension in days further still need to be signed-off.

Resetting the status here, and the importance to ENH
Comment 98 Chris Cormack 2012-04-09 02:51:55 UTC Comment hidden (obsolete)
Comment 99 Paul Poulain 2012-05-14 16:17:54 UTC
QA comment: tiny patch, passes QA
Comment 100 Chris Cormack 2012-05-14 21:12:56 UTC
Latest fix will be in 3.8.1