Description
Nicole C. Engard
2010-12-28 22:08:20 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 Have started breaking it into smaller pieces, db changes are on 6430 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 Colin Did you see the work I was doing on moving the Circulation code out into the Koha:: and into object oriented modules? 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 I was just testing origin/new/awaiting_qa/bug_5549 I don't see an updatedatabase.pl for adding "unit" to the issuingrules table. 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 When I was testing I also needed libreadonly-perl. My library (UT Tyler) is about to put this into production and would like to know what things we should look for to test. Also needed this dependency libdatetime-format-dateparse-perl 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 I have fixed the typo and updated new/awaiting_qa/bug_5549 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. Created attachment 5051 [details] [review] Fixed Bracketing Error Pushed the fix for the bracket to new/awaiting_qa/bug_5549 Fix for the strftime typo pushed 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. Areas we tested: - Hourly checkouts - checking out - returning - defining overdue rules - defining checkout rules - fines Areas we didn't test - messaging 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. 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 Created attachment 5436 [details] [review] Patch to correct calculation of large durations This patch should correct the cases where long durations are returned as 0 units in fine calculations, it uses the appropriate delta methods to return duration objects. Please give it a try Currently Testing. Looks good but I could really use a second signoff. 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. 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 merged with master https://github.com/libsysguy/koha-hourly 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. 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. .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. 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. 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. Found another little problem: changes from updatedatabase have to be added to kohastructure.sql Kohastructure.sql patched and pushed Hm, I think the recent changes perhaps need another sign-off. Changing the patch status to reflect that. 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 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 ! 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 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, 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 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 Tested: Fines Accrual Setting Hourly Items in circ rules Hourly Checkouts Normal Checkouts 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 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 on attachment 5436 [details] [review] Patch to correct calculation of large durations Obsoleting patch since it's a branch now. Created attachment 8228 [details]
Git Branch for hourly loans
This is the branch to pull from
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 Created attachment 8321 [details]
Branc to pull from
Have fixed:
The issue with the .po files
The High holds issue
Release notes are no longer touched
Fixed the suburb issue
Fixed db versioning
Fixed checked out on issue
Only thing left is to make the rollingloans.t work with the sample data, but I dont think think that is a blocker as it is a new test, that has the problem, I will work on that next.
I also rebased and added bug numbers to all the commits, its based on current master so should merge easily
git://git.koha-community.org/wip/koha-catalyst.git merged_5549
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? 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... 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 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? 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 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). 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. (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. 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. (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 (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) 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 (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 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. (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 Fix pushed. (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). (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 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. 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 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. (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 (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. 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) Sent Chris my patch to fix suspension in days and the resulting 'inverse' bug. 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. 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). 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. 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 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. we use a sandbox for the tests renew it works under all normal circumstances (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 ;-) 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 !
Created attachment 8541 [details] [review] Bug 5549 : Fixing the test to only run if the test data exists Fix for the failing test Created attachment 8543 [details] [review] Bug 5549 : Fixing the test to only run if the test data exists Signed-off-by: Liz Rea <wizzyrea@gmail.com> test passes after the patch. Win. Created attachment 8547 [details] [review] Bug 5549 followup: fix suspension in days further Grace periods are now measured in the same unit as the loan. Suspension in days works explicitly in days, independent of unit. Since the 'deltadays' duration is actually the duration, possibly in hours, between when the material was due and when it's returned, we need to take the finedays factor (how many days per unit overdue) and multiple it by 24 if the unit is hourly. Example: a 1 hour loan material with 1 hour grace period and finedays = 2, returned 2 hours late would give 4 days suspension To test: 1. create some circ rules: A) an hourly loan rule with suspension in days B) an hourly loan rule with suspension in days and a grace period C) a daily loan rule with suspension in days D) a daily loan rule with suspension in days and a grace period 2. Check out 2 materials under each rule. We'll call these A1-D1 and A2-D2 (to associate with the rule) 3. Return A1-D1 late, but within the grace period for B1 and D1 as appropriate 4. Return A2-D2 late, after the grace periods You should get: A1: suspended for (#hours late) * (#suspensionindays) days B1: not suspended C1: suspended for (#days late) * (#suspensionindays) days D1: not suspended A2: suspended for (#hours late) * (#suspensionindays) days B2: suspended for (#hours late) * (#suspensionindays) days C2: suspended for (#days late) * (#suspensionindays) days D2: suspended for (#days late) * (#suspensionindays) days 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) Is this still needs signoff? Seems the last patch has been pushed? Created attachment 8932 [details] [review] bug 5549: change to kohastructure.sql missing from commit 8cea8e5cc05075dc391194248d31a54586389601 Created attachment 8933 [details] [review] bug 5549: change to kohastructure.sql missing from commit 8cea8e5cc05075dc391194248d31a54586389601 Created attachment 8934 [details] [review] bug 5549: change to kohastructure.sql missing from commit 8cea8e5cc05075dc391194248d31a54586389601 Changing severity to blocker - because missing change to kohastructure breaks new installations. Created attachment 8957 [details] [review] bug 5549: change to kohastructure.sql missing from commit 8cea8e5cc05075dc391194248d31a54586389601 Also deletes earlier updatedatabase addition of lengthunit to avoid errors for people upgrading from table (thanks Katrin for the idea) Created attachment 8958 [details] [review] bug 5549: change to kohastructure.sql missing from commit 8cea8e5cc05075dc391194248d31a54586389601 Also deletes earlier updatedatabase addition of lengthunit to avoid errors for people upgrading from stable (thanks Katrin for the idea) Created attachment 8959 [details] [review] [SIGNED-OFF] bug 5549: change to kohastructure.sql missing from commit 8cea8e5cc05075dc391194248d31a54586389601 Also deletes earlier updatedatabase addition of lengthunit to avoid errors for people upgrading from stable (thanks Katrin for the idea) Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Checked creating a database from scratch and updating a database with missing lengthunit column. Both worked without any problem. If you update and have the column updatedatabase will report an error, but the database will be ok. 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. (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? 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 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! 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 Created attachment 9031 [details] Bug 5549 followup: fix suspension in days further Grace periods are now measured in the same unit as the loan. Suspension in days works explicitly in days, independent of unit. Since the 'deltadays' duration is actually the duration, possibly in hours, between when the material was due and when it's returned, we need to take the finedays factor (how many days per unit overdue) and multiple it by 24 if the unit is hourly. Example: a 1 hour loan material with 1 hour grace period and finedays = 2, returned 2 hours late would give 4 days suspension To test: 1. create some circ rules: A) an hourly loan rule with suspension in days B) an hourly loan rule with suspension in days and a grace period C) a daily loan rule with suspension in days D) a daily loan rule with suspension in days and a grace period 2. Check out 2 materials under each rule. We'll call these A1-D1 and A2-D2 (to associate with the rule) 3. Return A1-D1 late, but within the grace period for B1 and D1 as appropriate 4. Return A2-D2 late, after the grace periods You should get: A1: suspended for (#hours late) * (#suspensionindays) days B1: not suspended C1: suspended for (#days late) * (#suspensionindays) days D1: not suspended A2: suspended for (#hours late) * (#suspensionindays) days B2: suspended for (#hours late) * (#suspensionindays) days C2: suspended for (#days late) * (#suspensionindays) days D2: suspended for (#days late) * (#suspensionindays) days Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> QA comment: tiny patch, passes QA Latest fix will be in 3.8.1 |