Bug 5872

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

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?

Depends on bug 5436 for subroutine 'SetMemberInfosInTemplate'

Depends of bug 5952 for subroutine 'GetMemberRelatives'

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

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?

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 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