Circulation rules management : everything (issues, holds, fines) now at granular level All scripts = there are some (a few) reindenting changes. * C4/IssuintRules.pm script created : contains all circ rules related functions * C4/Circulation.pm - rewrite issuing rules according to new behaviour : default values applies as default, not as total. There is also an inheritance, meaning you don't have to define all rules - debarment management : debarred patrons now can have a limit date of debarment (for fine in days, but also for a limited debarrment) - lot of lines removes (now in IssuinRules.pm) - modifys the offline circ : works in conjunction with the Firefox plugin. All offline circ code is in another patch * C4/Overdues.pm - use IssuingRules.pm functions - deal with debarment up to a givendate * C4/Reserves.pm - deal with HoldRules at granular level. The library can now define rules for hold at ccode(itype) / branch / patron category level - added a pickupDelay * admin/smart-rules.pl - script rewritten - added ajax (smart-rules-service) to modify a cell directly * admin/branch_transfer_limits.pl - some variable renamed for more readability * circ/branchtransfers.pl - ModReserveAffect now deals with branches to check hold rules * circ/circulation.pl and circ/returns.pl - deals with debardate where applicable - deals with possible holds on a given item adapted to fit new hold rules - display borrower relatives issues (on circulation.pl only) * reserve/modrequest.pl, placerequest.pl, renewscript.pl and request.pl - deals with new granular-level hold rules * unit tests for Circulation.pm, GetMemberDetails.pm and Overdues.pm - deals with IssuingRules.pm package & API changes * opac-reserve.pl and opac-modrequest: - deals with holds at granular level
Created attachment 3310 [details] [review] Proposed Patch
Created attachment 3329 [details] [review] Proposed patch
Patch does not apply. -- Applying: Bug 5872 : Circulation management enhancements /home/julian/koha/versions/koha-community-git/.git/rebase-apply/patch:75: trailing whitespace. /home/julian/koha/versions/koha-community-git/.git/rebase-apply/patch:83: trailing whitespace. ); /home/julian/koha/versions/koha-community-git/.git/rebase-apply/patch:97: trailing whitespace. my $itype = (C4::Context->preference('item-level_itypes')) /home/julian/koha/versions/koha-community-git/.git/rebase-apply/patch:212: trailing whitespace. } else { /home/julian/koha/versions/koha-community-git/.git/rebase-apply/patch:213: trailing whitespace. $count_query .= " JOIN biblioitems USING (biblionumber) error: patch failed: members/member.pl:99 error: members/member.pl: patch does not apply Patch failed at 0001 Bug 5872 : Circulation management enhancements
Did you use the patch attached to this bug? git checkout kc/master wget http://bugs.koha-community.org/bugzilla3/attachment.cgi\?id\=3329 git am -iu3 attachment.cgi\?id=3329 Commit Body is: -------------------------- Bug 5872 : Circulation management enhancements See http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=5872 for details -------------------------- Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all y Applying: Bug 5872 : Circulation management enhancements Using index info to reconstruct a base tree... <stdin>:75: trailing whitespace. <stdin>:83: trailing whitespace. ); <stdin>:97: trailing whitespace. my $itype = (C4::Context->preference('item-level_itypes')) <stdin>:212: trailing whitespace. } else { <stdin>:213: trailing whitespace. $count_query .= " JOIN biblioitems USING (biblionumber) warning: squelched 77 whitespace errors warning: 82 lines add whitespace errors. Falling back to patching base and 3-way merge... Auto-merging C4/Reserves.pm rorohiko:[git/95a8e26...-]:~/git/catalyst-koha% Seems to work ok for me
It seems that I omitted the -3 option. Sorry.
This patch use C4/IssuingRules.pm module but this file does not exist in master and I can't find any bug about it. Must I file a new bug in order to add this file?
Created attachment 3394 [details] [review] Follow-up patch This patch adds the missing C4/IssuingRules.pm
Depends on bug 5436 for subroutine 'SetMemberInfosInTemplate'
Depends of bug 5952 for subroutine 'GetMemberRelatives'
Created attachment 3432 [details] [review] Follow-up patch Add missing columns in issuingrules table. Some code which was causing errors was removed/modified.
christophe@christophe-desktop:~/Workspace/koha3.2$ git am -3 Patch/0001-Bug-5872-Circulation-management-enhancements.patch Applying: Bug 5872 : Circulation management enhancements Using index info to reconstruct a base tree... <stdin>:75: trailing whitespace. <stdin>:83: trailing whitespace. ); <stdin>:97: trailing whitespace. my $itype = (C4::Context->preference('item-level_itypes')) <stdin>:212: trailing whitespace. } else { <stdin>:213: trailing whitespace. $count_query .= " JOIN biblioitems USING (biblionumber) warning: squelched 77 whitespace errors warning: 82 lines add whitespace errors. Falling back to patching base and 3-way merge... Auto-merging C4/Circulation.pm CONFLICT (content): Merge conflict in C4/Circulation.pm Auto-merging C4/Overdues.pm Auto-merging C4/Reserves.pm Auto-merging admin/branch_transfer_limits.pl Auto-merging admin/smart-rules.pl CONFLICT (content): Merge conflict in admin/smart-rules.pl Auto-merging circ/branchtransfers.pl Auto-merging circ/circulation.pl CONFLICT (content): Merge conflict in circ/circulation.pl Auto-merging circ/returns.pl CONFLICT (content): Merge conflict in circ/returns.pl Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tmpl CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tmpl Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tmpl CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tmpl Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tmpl CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tmpl Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tmpl Auto-merging members/member.pl Auto-merging reserve/request.pl Failed to merge in the changes. Patch failed at 0001 Bug 5872 : Circulation management enhancements
All those patches have been reprocessed in order to get them rebased on current master and respecting the HardCodedDuedates. see git://git.koha-community.org/koha-biblibre.git branch wip/bug5872 This is the new proposed branch. This is working with jeditable, added as a submodule. One would have to add launch git submodule update --init in order to download that.
We probably need to get jeditable in without using submodule, so it can be merged into master. Also I would need to make sure we can use packaged versions jquery still with it. Has this been tested with the debian packaged version of jquery?
(In reply to comment #14) > We probably need to get jeditable in without using submodule, so it can be > merged into master. Why not using submodules ? I mean, there is a main repo for this feature. If we want to keep synch, the usage of submodules seems to me the solution. And definitely, Koha Code should not copy paste other projetcs as it has done in the past. Since when the code is edited, then it is not synched back into the original project. And it is quite easy to get the code. The command to get the code is really simple. And the fact that there are independent repository guaranties the independency of Code. If there is true technical reason to get that code in, then explain me which. I can see none. > Also I would need to make sure we can use packaged versions jquery still with > it. > > Has this been tested with the debian packaged version of jquery? No, hadn't had the time to do so.
How do you propose we put the code in a release? If we dont either include it in the tarball, or use a version that comes packaged for the OS?
(In reply to comment #16) > How do you propose we put the code in a release? If we dont either include it > in the tarball, or use a version that comes packaged for the OS? If ppl need the tarball and all the files, then you add locally the files with git submodule update --init just before making the MANIFEST and the tarball and you're set. Or in the install process add a git submodule update --init which will fetch the code. Then when all the required jquery plugins will be packaged in debian, just add them to dependencies (or let's package that).
Most people install from releases, not from git, so adding a submodule step in the install is not tenable, it would have to be included in the tarball, or be able to be installed another way easily. It is possible to do the update before the building the release, I will then have to of course check the files havent changed in a way that breaks our usage of them, unless we pin to a specific version. Can you do that, pin the update to only grab a certain version, that way it could be added before the release without as much danger. Otherwise we are effectively including untested files in the release. (We use version numbers for the perl modules we depend on, using the same for the js would make sense to me).
(In reply to comment #18) > Most people install from releases, not from git, so adding a submodule step in > the install is not tenable, it would have to be included in the tarball, or be > able to be installed another way easily. I didn't want ppl to do that. But I meant some clever developer could add that command in the command line script (Makefile.PL or into the make process)(before copying koha directory into the blib directory could have been a nice way). Of course, if one does not have internet access, git submodule update --init would fail. But if one doesnot have internet access, then it is nearly impossible to install koha. > > It is possible to do the update before the building the release, I will then > have to of course check the files havent changed in a way that breaks our usage > of them, unless we pin to a specific version. > > Can you do that, pin the update to only grab a certain version, that way it > could be added before the release without as much danger. Otherwise we are > effectively including untested files in the release. (We use version numbers > for the perl modules we depend on, using the same for the js would make sense > to me). My experience of git-submodule is that when you commit your submodule addition, it stores the commit id of the project in your directory, so that when ppl do a git submodule update --init then it uses the commit id stored to synch with it. And not the latest version. See here what is described as a flaw would act in fact as a feature in our context : http://www.openembedded.org/index.php/MultipleRepositoryMethods When we want to upgrade for instance jquery version (for tests or for good) and are ready to do that, we could do a git submodule synch -- jquery And we could locally test the side effects or their absence. then git commit the directory when tested. In our workflow, that means no direct changes in those libs, at least at the moment, and hopefully for a long time, I think it is easier than the get the tarball unzip the tarball commit all the files... It is just a new command.
something in those patches is missing : the C4::Members::DebarMember() sub now deals with a date The signature on biblibre/master is : my $success = DebarMember( $borrowernumber, $todate ); the signature on official/master is: my $success = DebarMember( $borrowernumber );
(In reply to comment #20) > something in those patches is missing : the C4::Members::DebarMember() sub now > deals with a date > The signature on biblibre/master is : > my $success = DebarMember( $borrowernumber, $todate ); > the signature on official/master is: > my $success = DebarMember( $borrowernumber ); This one is fixed through 6328, that must be applied before this one
spent all the day : * rebasing * moving to T::T * testing & fixing a few forgotten things I hope i'll be able to submit a unified patch tomorrow
Created attachment 4741 [details] [review] proposed patch (updated), squashed & cleaned What this -big- patch does and how to test it * git am -3 the patch * retrieve a new jquery submodule by entering : git submodule update --init You must do that only once. If you build a tarball, you'll have everything correctly included * test What to test: * koha > admin > circulation rules. What has changed : - are splitted in tabs, all / circulation / fines / reserves. Those tabs are like quick filters, you can enter values in a tab or in "all" - rules now have inheritance. If you define a rule for a given itemtype, for example, then a rule for a given itemtype/patron category, if you set just the duration, the number of issues will be inherited from the more generic rule. Example : default / default / default = 10 items for 21 days default / book / default = enter 5. You'll see that the 21 is automatically displayed, in grey. It means you've a rule saying "OK, for books, it's only 5, -but it's still 21 days-" - Hold rules are also at granular level : you can select, for each patron category/itemtype how many holds you enable, how many days the patron has to pickup the item, and if you allow on shelf holds or holds only for this library patrons. - there is now a renewal duration. If you enter nothing, default checkout duration will be used. If you enter a value, it will be used. Example : You can then have 1st checkout for 3 weeks, 2 renewals, each for 1 week. - rules can be edited directly in the cell : you want to change 10 items to 12 ? Just click on 10, you can set 12, click OK, you're done. Note that inheritance values are not updated, so there may be a display glitch sometimes. Not really a problem, you know what you do, right ? * circulation : once you've defined your circulation rules, just place hold, circulate, check that the rules you've set are followed. It's very usefull to check all hold related systempreferences Database changes : * there is now a reservenumber in reserves (and old_reserves) table, that is the primary key. It was a big lack for years ! * new fields are added into issuingrules to deal with holds : holdpickupdelay, renewalperiod, allowonshelfholds, holdrestricted, Systempreferences : * AllowOnShelfHolds is no more needed, as it's at branch/categorycode/itemtype level * ReservesControlBranch has been added to define wether you use item or patron branch for holds developer note : issuingrules have been moved from C4/Circulation.pm to C4/IssuingRules.pm
removing dependancies that are now irrelevent, i've tested this patch alone
This is trivial, but a bit confusing -- it looks like when creating a new rule, the initial setting for "hard due date compare" cannot be set to '0', it defaults to -1. You can, however, edit the rule to reflect (a possibly more correct) default value of 0. I recognize that unless you are using hard due date this doesn't affect much, but I thought I would point it out. It's possible that I don't understand the implications of the default value properly. I would welcome enlightenment if that's the case!
It appears that editing am existing holdrestricted by typing "yes" or "no" resets the rule to 0, which means nothing to me. The "holds restricted to library" should be drop down selection instead of click/type to edit, like "allow on shelf holds" is.
here is a short, silent video illustrating the last comment. It also illustrates how the display changes when you edit the 'allow on shelf holds.' The display shows the numeric value for the setting instead of the previously tidy 'yes' or 'no.' http://screencast.com/t/4duT8KV6VjQ
Some first comments from playing with the new interface: kf 1) Filter is a bit confusing to use. I didn't know what to put in there at first, but Liz figured it out. You can enter itemtype or category descriptions and it will limit the display to those. It's a cool feature, but I think we may need a hint about it in an updated help file and perhaps a better label for the field. kf 2) There is something wrong with systempreference ReservesControlBranch. There is an insert for this systempreference in the patch (+$DBversion = '3.05.00.005';) and it's already in the pref file in master. But I can't find it in the systempreference sql files? It also already present in updatedatabase.pl: 2741 $DBversion = '3.01.00.066'; kf 3) Fine amount is not formatted nicely, displayed like saved in the database: 1.00000 kf 4) 'Reserves' tab should be named 'holds' tab. kf 5) 'Holds restricted to library' is a checkbox. Allow on shelf holds is a yes/now pull down. Why use different elements for boolean values? Question: In the old circ matrix it was possible to define that an itemtype can not be placed on hold at all (below circ matrix). How is that done now?
(In reply to comment #26) > It appears that editing am existing holdrestricted by typing "yes" or "no" > resets the rule to 0, which means nothing to me. > > The "holds restricted to library" should be drop down selection instead of > click/type to edit, like "allow on shelf holds" is. Patch from Aug, 1st fixes the behaviour : the "Hold restricted to library" is now a drop down selection I can't find how to deal correctly with the display of the result : the value displayed is returned by the webservice. If it returns "Yes" or "No", then that won't be translatable. There is no solution to this problem. (In reply to comment #28) > Some first comments from playing with the new interface: > > kf 1) Filter is a bit confusing to use. I didn't know what to put in there at > first, but Liz figured it out. You can enter itemtype or category descriptions > and it will limit the display to those. It's a cool feature, but I think we may > need a hint about it in an updated help file and perhaps a better label for the > field. Comment added to the field in patch from Aug, 1st > kf 2) There is something wrong with systempreference ReservesControlBranch. > There is an insert for this systempreference in the patch (+$DBversion = > '3.05.00.005';) and it's already in the pref file in master. But I can't find > it in the systempreference sql files? It also already present in > updatedatabase.pl: > 2741 $DBversion = '3.01.00.066'; fixed in Patch from Aug, 1st > kf 3) Fine amount is not formatted nicely, displayed like saved in the > database: 1.00000 => fixed by using the same num_formatter as in acq module, patch from Aug 1st > kf 4) 'Reserves' tab should be named 'holds' tab. fixed in Patch from Aug, 1st > kf 5) 'Holds restricted to library' is a checkbox. > Allow on shelf holds is a yes/now pull down. > Why use different elements for boolean values? => fixed (see answer to liz comment) > Question: In the old circ matrix it was possible to define that an itemtype can > not be placed on hold at all (below circ matrix). How is that done now?
Created attachment 4795 [details] [review] proposed patch (updated solving comments 28 and 29) This patch squash previous patch and liz & Katrin comments
Note that yes/no fields are not a checkbox because they have 3 states : Yes / No, and empty, that means "inherit"
> Question: In the old circ matrix it was possible to define that an itemtype can > not be placed on hold at all (below circ matrix). How is that done now? I forgot to answer this question from Katrin = just enter 0 as number of holds
re the yes/no fields that are not a checkbox (Allow on shelf holds): it would be nice if the 3rd state were explicit so the field would have 'yes, no, inherit' as options. the behavior could be the same (it can show the inherited value) I just think that when you're picking, you would need to know what the blank option was
*** Bug 4354 has been marked as a duplicate of this bug. ***
Applying: Bug 5872 : Circulation management enhancements Using index info to reconstruct a base tree... <stdin>:99: trailing whitespace. <stdin>:113: trailing whitespace. my $itype = (C4::Context->preference('item-level_itypes')) <stdin>:228: trailing whitespace. } else { <stdin>:229: trailing whitespace. $count_query .= " JOIN biblioitems USING (biblionumber) <stdin>:319: space before tab in indent. $issuingimpossible{DEBARRED} = format_date($dateenddebarred); warning: squelched 85 whitespace errors warning: 90 lines add whitespace errors. Falling back to patching base and 3-way merge... Auto-merging installer/data/mysql/de-DE/mandatory/sysprefs.sql CONFLICT (content): Merge conflict in installer/data/mysql/de-DE/mandatory/sysprefs.sql Auto-merging installer/data/mysql/fr-FR/1-Obligatoire/unimarc_standard_systemprefs.sql CONFLICT (content): Merge conflict in installer/data/mysql/fr-FR/1-Obligatoire/unimarc_standard_systemprefs.sql Auto-merging installer/data/mysql/it-IT/necessari/sysprefs.sql Auto-merging installer/data/mysql/pl-PL/mandatory/sysprefs.sql CONFLICT (content): Merge conflict in installer/data/mysql/pl-PL/mandatory/sysprefs.sql Auto-merging installer/data/mysql/ru-RU/mandatory/system_preferences_full_optimal_for_install_only.sql CONFLICT (content): Merge conflict in installer/data/mysql/ru-RU/mandatory/system_preferences_full_optimal_for_install_only.sql Auto-merging installer/data/mysql/uk-UA/mandatory/system_preferences_full_optimal_for_install_only.sql Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tt Failed to merge in the changes. Patch failed at 0001 Bug 5872 : Circulation management enhancements
Created attachment 4811 [details] [review] proposed patch (updated solving comment 35) a blank line had been added in many sysprefs files, conflicting with the one in this patch
Created attachment 4854 [details] [review] proposed patch (rebased aug, 6th) patch just rebased from previous version
The patch applies (using git am, but not git bz apply). After applying I find that the circulation screen no longer shows checkboxes in the renew column for checked-out items which are eligible for renewal.
These paths in the smart-rules.tt template are broken: <link href="/content/ui-lightness/jquery-ui-1.8.1.custom.css" rel="stylesheet" /> <script src="/scripts/jquery-ui-1.8.1.custom.min.js" type="text/javascript"></script> ...the datepicker seems to function without it. Are these needed for something else? The datepicker doesn't honor the dateformat system preference.
This patch would be MUCH easier to test if it were broken down into more granular pieces. Perhaps a patch to lay whatever groundwork is necessary, then patches for making each of the new rules manageable at the Circ Rules level (including both the rules administration page, and the actual usage of the rule in the system). Breaking this down would not only help us identify where any potential conflicts lay, but would also make it easier to get parts of this development into place even if the total patch has an issue somewhere.
Updating Version : This ENH will be for Koha 3.8
Bug versionned for master. entries will be made against rel_3_8 once the patch has been applied (see thread about that on koha-devel yesterday)
patch has diverged too much from Koha 3.6 Some ideas very usefull, but patch must be completly rewritten