Bug 7572

Summary: Add Clubs & Services Feature
Product: Koha Reporter: Kyle M Hall <kyle.m.hall>
Component: PatronsAssignee: Kyle M Hall <kyle.m.hall>
Status: CLOSED WONTFIX QA Contact: Ian Walls <koha.sekjal>
Severity: enhancement    
Priority: P5 - low CC: campbellreidtait, chris, dcook, frederic.durand, gmcharlt, katrin.fischer, kyle, magnus, matted-34813, nengard, veron, wizzyrea
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Clubs & Services
Clubs & Services
Bug 7572 - Add Clubs & Services Feature
Bug 7572 - Add Clubs & Services Feature
Bug 7572 - Add Clubs & Services Feature
Bug 7572 - Add Clubs & Services Feature
Bug 7572 - Add Clubs & Services Feature
Bug 7572 - Add Clubs & Services Feature
Strange display of menu at the left
Java Script Error related to Calendar
Bug 7572 - Add Clubs & Services Feature
Bug 7572 - Add Clubs & Services Feature
Bug 7572 - Add Clubs & Services Feature
Bug 7572 - Add Clubs & Services Feature
Bug 7572 - Add Clubs & Services Feature
Bug 7572 - Add Clubs & Services Feature
Bug 7572 - Add Clubs & Services Feature
Bug 7572 - Add Clubs & Services Feature
Bug 7572 - Add Clubs & Services Feature

Description Kyle M Hall 2012-02-21 19:25:37 UTC
Add the clubs and services feature to Koha. This feature has been in production at the Crawford County Federated Library system for a number of years now.

This features adds the ability to create clubs and services which patrons may be enrolled in. It is particularly useful for tracking summer reading programs, book clubs and other such clubs.

It includes 2 built-in service archetypes:

Bestsellers Club : Batch generate a holds list which shuffles the holds queue when new items matching a given title or author are catalogued.

New Items Weekly Mailing Lists : Create custom mailing lists to alert subscribers to new items in your library.
Comment 1 Kyle M Hall 2012-02-21 19:31:53 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2012-02-21 20:02:49 UTC Comment hidden (obsolete)
Comment 3 Nicole C. Engard 2012-02-26 20:37:11 UTC
Kyle,

Can you give us a testing plan or the start of a testing plan. How is it supposed to work, where to we look for changes to functionality etc.

Thanks a ton!
Nicole
Comment 4 Kyle M Hall 2012-02-27 15:38:24 UTC Comment hidden (obsolete)
Comment 5 Kyle M Hall 2012-03-08 21:03:15 UTC Comment hidden (obsolete)
Comment 6 Kyle M Hall 2012-03-08 21:05:59 UTC Comment hidden (obsolete)
Comment 7 Liz Rea 2012-03-08 22:14:59 UTC
how are the emails generated? In a cron?
Comment 8 Liz Rea 2012-03-08 22:26:50 UTC
It appears that when creating a club or service archetype, both the title and description are required, but are not labeled as such. Furthermore, if you try and create a service or club archetype and you don't enter a title or description, it lets you submit, but doesn't add the archetype and gives no error. It is the same condition for adding individual clubs and services.

So I suggest that we mark the title and description as required, and have some checking done that will alert users as to *why* their archetype isn't created if they persist in trying to submit the form without the required fields.

I'm up in the air as to whether this precludes inclusion or getting my signoff - it's an easy followup.
Comment 9 Kyle M Hall 2012-03-09 15:12:34 UTC Comment hidden (obsolete)
Comment 10 Kyle M Hall 2012-03-09 15:13:17 UTC
New patch adds form validation for title and description as per Liz Rea's comments.
Comment 11 Liz Rea 2012-03-09 15:24:13 UTC
I also feel like the files for this ought to be in the tools subdirectory, instead of at the root of the koha FS. That, also, could be a followup.
Comment 12 Liz Rea 2012-03-09 15:25:04 UTC
(also, awesome, will grab the new patch right now and check it out. :))
Comment 13 Kyle M Hall 2012-03-09 19:13:40 UTC
(In reply to comment #11)

I think it might be better to move the link out of the tools directory and into main section instead. It would certainly take less effort!

> I also feel like the files for this ought to be in the tools subdirectory,
> instead of at the root of the koha FS. That, also, could be a followup.
Comment 14 Liz Rea 2012-04-13 15:04:08 UTC
Falling back to patching base and 3-way merge...
Auto-merging C4/Auth.pm
Auto-merging cataloguing/addbiblio.pl
Auto-merging circ/circulation.pl
Auto-merging installer/data/mysql/kohastructure.sql
CONFLICT (content): Merge conflict in installer/data/mysql/kohastructure.sql
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/includes/circ-menu.inc
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/includes/circ-menu.inc
Auto-merging koha-tmpl/intranet-tmpl/prog/en/includes/members-menu.inc
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/tools/tools-home.tt

Most of those are trivial changes.
Comment 15 Kyle M Hall 2012-05-16 12:47:41 UTC Comment hidden (obsolete)
Comment 16 Marc Véron 2012-06-11 09:10:29 UTC
Wanted to sign off, but got "Does not apply" errors.
Marc
Comment 17 Kyle M Hall 2012-06-11 10:13:24 UTC Comment hidden (obsolete)
Comment 18 Marc Véron 2012-08-07 22:21:55 UTC
Patch did not apply. I tried to resolve it using git am -iu3 (...), got it to apply. However I got errors running updatedatabase.pl, removed Line 5250 (<<<..) that was alredy in the patch, droped all tables that were alreade created by the first run, and run updatedatabase.pl again. 

BTW, the SQL statements in updatedatabase.pl contain backticks (`).

Got JavaScript Eror while Editing Clubs & Services (Calendar not defined), not sure if any follwoing JavaScript then is broken (validations etc.) On a patrons checkout page the menu at the left is doubled, I suppose it is a conflict with the new layout.

Anyway, I could then follow the test plan and enroll a patron to a club (and cancel as well), both in the staff client and in the Opac. 

Then I was a little bit lost in testing.

It took a while until I found out that a self made Club Archetype *must* have the title "Bestsellers Club" to trigger holdings, see ClubsAndServices.pm line 1175:
$sth = $dbh->prepare("SELECT * FROM clubsAndServicesArchetypes WHERE title LIKE 'Bestsellers Club' ");

Adding items for biblios with matching titles somtimes placed a hold for the patron and sometimes not. Could not find out why. Maybe it is due to some misconfiguration related to my try to resolve the patch issue.

It seems to be a great and promising feature, but for now I am stuck. 

For a new test with a rebased patch I would really appreciate a more complete description of the overall concept and functionality (Wiki?) and a more detailed testing plan.
Comment 19 Marc Véron 2012-08-07 22:22:48 UTC Comment hidden (obsolete)
Comment 20 Marc Véron 2012-08-07 22:23:20 UTC Comment hidden (obsolete)
Comment 21 Kyle M Hall 2012-08-11 12:47:42 UTC
I'll see what I can do about documentation and a testing plan. Thanks for the followups! Also, backticks are allowed in updatedatabase.pl because it is a mysql specific file.
Comment 22 Katrin Fischer 2012-08-11 13:23:05 UTC
Hi Kyle, I think the hardcoded description "Bestsellers Club" sounds like a translation problem? Not sure as I have not tested the patch yet, only wondering while reading through this.
Comment 23 Marc Véron 2012-08-11 13:30:59 UTC
Katrin and Kyle,

Yes, I suppose it will be a translation problem.

To distinguish between the types of archetype I think it would be necessary to have an ID and then do the select on the ID instead of an English text.

Marc
Comment 24 Katrin Fischer 2012-09-18 20:48:06 UTC
Hi Kyle, 

I have read through the code and there are some issues with this patch:

1) Can you please add the new permisson for all languages? English is ok - easier to add a translation than figuring out a missing permission.

2) Some of the perl files have strict, but not warnings, can you add use Modern::Perl?

3) The Templates don't follow the capitalization rules. Examples:
Start Date
End Date
You Are Not Enrolled In Any Clubs Or Services

4) This will not work for other languages, as noted in some of the earlier comments:
$sth = $dbh->prepare("SELECT * FROM clubsAndServicesArchetypes WHERE title LIKE 'Bestsellers Club' ");

Also:
+# This script utilizes the 'New Items E-mail List' archetype
+# from the ClubsAndServices.pm module.
+# If you do not have this archtype, create an archetype of that name

5) If I read the code correctly, this is another translation problem, adding a hardcoded note to the hold:
+      my $notes = "Automatic Reserve for Bestsellers Club",

6) I wonder about the error messages - from reading the code I am not totally sure but I think they are also not translatable.
+              <div>Reason: <strong>[% failureMessage %]</strong></div>

7) Could you add documenation to the database tables? I was wondering about the line:
+  `casaId` int(11) NOT NULL default '0' COMMENT 'foreign key to clubsAndServicesArchetypes', - not sure if COMMENT is a mysqlism. 

We had some discussion about following the standard in one of the IRC meetings and agreed on using -- comment in the end. But I don't remember exactly why.

8) Another translation issue is the preset data you add in kohastructure.sql. Also adding data to the tables is normally done by separate sql files, while kohastructure is only creating the table structure without any data.

+-- Preset data for ClubsAndServicesArchetypes
+--
+
+INSERT INTO `clubsAndServicesArchetypes`...

9) There is a conflict marker in updatedatabase.pl and the version should be set to be XXX.
+<<<<<<< HEAD
 $DBversion = "3.09.00.005";

10) There is some untranslatable JavaScript too. Example:
+    alert("Title is a required field");
+    alert("Description is a required field");

11) There is a large block of code commented out. Maybe delete it, if it's not needed?
+<!-- These messages are uneccessary because you can see if it was deleted or created immediately...
Also some typos in there 'Archtype'.

12) It looks like the old calendar is being used? Not sure here.
+		<img src="/intranet-tmpl/prog/en/lib/calendar/cal.gif" alt="Show Calendar"  border="0" id="CalendarStartDate" style="cursor: pointer;" />

13) Ok, now I found something really big :(

The doc states:
HTML::Template file mailinglist.tmpl 

Also:
+use HTML::Template::Pro;

And then:
+++ b/misc/cronjobs/mailinglist/mailinglist.tmpl

This needs to be redone to use TT.
Comment 25 Kyle M Hall 2012-09-19 14:15:30 UTC Comment hidden (obsolete)
Comment 26 Kyle M Hall 2012-09-19 14:19:17 UTC Comment hidden (obsolete)
Comment 27 Kyle M Hall 2012-09-19 14:30:34 UTC Comment hidden (obsolete)
Comment 28 Kyle M Hall 2012-09-19 14:35:27 UTC
> 1) Can you please add the new permisson for all languages? English is ok -
> easier to add a translation than figuring out a missing permission.

Done
 
> 2) Some of the perl files have strict, but not warnings, can you add use
> Modern::Perl?

Done

> 3) The Templates don't follow the capitalization rules. Examples:
> Start Date
> End Date
> You Are Not Enrolled In Any Clubs Or Services

Fixed
 
> 4) This will not work for other languages, as noted in some of the earlier
> comments:
> $sth = $dbh->prepare("SELECT * FROM clubsAndServicesArchetypes WHERE title
> LIKE 'Bestsellers Club' ");

Fixed

> 5) If I read the code correctly, this is another translation problem, adding
> a hardcoded note to the hold:
> +      my $notes = "Automatic Reserve for Bestsellers Club",
> 

Fixed

> 6) I wonder about the error messages - from reading the code I am not
> totally sure but I think they are also not translatable.
> +              <div>Reason: <strong>[% failureMessage %]</strong></div>

Fixed

> 7) Could you add documenation to the database tables? I was wondering about
> the line:
> +  `casaId` int(11) NOT NULL default '0' COMMENT 'foreign key to
> clubsAndServicesArchetypes', - not sure if COMMENT is a mysqlism. 
> 
> We had some discussion about following the standard in one of the IRC
> meetings and agreed on using -- comment in the end. But I don't remember
> exactly why.

Fixed
 
> 8) Another translation issue is the preset data you add in
> kohastructure.sql. Also adding data to the tables is normally done by
> separate sql files, while kohastructure is only creating the table structure
> without any data.
> 
> +-- Preset data for ClubsAndServicesArchetypes
> +--
> +
> +INSERT INTO `clubsAndServicesArchetypes`...

Fixed

> 9) There is a conflict marker in updatedatabase.pl and the version should be
> set to be XXX.
> +<<<<<<< HEAD
>  $DBversion = "3.09.00.005";

Removed
 
> 10) There is some untranslatable JavaScript too. Example:
> +    alert("Title is a required field");
> +    alert("Description is a required field");

Fixed
 
> 11) There is a large block of code commented out. Maybe delete it, if it's
> not needed?
> +<!-- These messages are uneccessary because you can see if it was deleted
> or created immediately...
> Also some typos in there 'Archtype'.

Removed
 
> 12) It looks like the old calendar is being used? Not sure here.
> +		<img src="/intranet-tmpl/prog/en/lib/calendar/cal.gif" alt="Show
> Calendar"  border="0" id="CalendarStartDate" style="cursor: pointer;" />

Switched to the new calendar widget

> 13) Ok, now I found something really big :(
> 
> The doc states:
> HTML::Template file mailinglist.tmpl 
> 
> Also:
> +use HTML::Template::Pro;
> 
> And then:
> +++ b/misc/cronjobs/mailinglist/mailinglist.tmpl
> 
> This needs to be redone to use TT.

Done.

Thanks for your good eyes! The original version of this feature was written in 2007 and Koha has moved quite a bit since then! I think this updated patch addresses all your concerns, and is definitely a big improvement.
Comment 29 Katrin Fischer 2012-09-19 14:42:24 UTC
Hi Kyle, 

thx for reworking your patch! I took a quick look at the last of the attached patches and found some more "use stricts" - could you check? Thx.
Comment 30 Kyle M Hall 2012-09-19 14:44:41 UTC
Will do. Just a note, access to the Clubs & Services module is via the Tools module.
Comment 31 Kyle M Hall 2012-09-19 14:49:20 UTC Comment hidden (obsolete)
Comment 32 Kyle M Hall 2013-01-22 13:46:35 UTC Comment hidden (obsolete)
Comment 33 Frederic Durand 2013-03-21 10:49:35 UTC
Trying to test it on a sandbox, did not work, here the message :
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging C4/Auth.pm
CONFLICT (content): Merge conflict in C4/Auth.pm
Auto-merging cataloguing/addbiblio.pl
Auto-merging circ/circulation.pl
Auto-merging installer/data/mysql/de-DE/mandatory/userflags.sql
CONFLICT (content): Merge conflict in installer/data/mysql/de-DE/mandatory/userflags.sql
Auto-merging installer/data/mysql/de-DE/mandatory/userpermissions.sql
CONFLICT (content): Merge conflict in installer/data/mysql/de-DE/mandatory/userpermissions.sql
Auto-merging installer/data/mysql/en/mandatory/userflags.sql
CONFLICT (content): Merge conflict in installer/data/mysql/en/mandatory/userflags.sql
Auto-merging installer/data/mysql/en/mandatory/userpermissions.sql
CONFLICT (content): Merge conflict in installer/data/mysql/en/mandatory/userpermissions.sql
Auto-merging installer/data/mysql/es-ES/mandatory/userflags.sql
CONFLICT (content): Merge conflict in installer/data/mysql/es-ES/mandatory/userflags.sql
Auto-merging installer/data/mysql/es-ES/mandatory/userpermissions.sql
CONFLICT (content): Merge conflict in installer/data/mysql/es-ES/mandatory/userpermissions.sql
Auto-merging installer/data/mysql/fr-FR/1-Obligatoire/userflags.sql
CONFLICT (content): Merge conflict in installer/data/mysql/fr-FR/1-Obligatoire/userflags.sql
Auto-merging installer/data/mysql/fr-FR/1-Obligatoire/userpermissions.sql
CONFLICT (content): Merge conflict in installer/data/mysql/fr-FR/1-Obligatoire/userpermissions.sql
Auto-merging installer/data/mysql/it-IT/necessari/userflags.sql
CONFLICT (content): Merge conflict in installer/data/mysql/it-IT/necessari/userflags.sql
Auto-merging installer/data/mysql/it-IT/necessari/userpermissions.sql
CONFLICT (content): Merge conflict in installer/data/mysql/it-IT/necessari/userpermissions.sql
Auto-merging installer/data/mysql/kohastructure.sql
CONFLICT (content): Merge conflict in installer/data/mysql/kohastructure.sql
Auto-merging installer/data/mysql/nb-NO/1-Obligatorisk/userflags.sql
CONFLICT (content): Merge conflict in installer/data/mysql/nb-NO/1-Obligatorisk/userflags.sql
Auto-merging installer/data/mysql/nb-NO/1-Obligatorisk/userpermissions.sql
CONFLICT (content): Merge conflict in installer/data/mysql/nb-NO/1-Obligatorisk/userpermissions.sql
Auto-merging installer/data/mysql/pl-PL/mandatory/userflags.sql
CONFLICT (content): Merge conflict in installer/data/mysql/pl-PL/mandatory/userflags.sql
Auto-merging installer/data/mysql/pl-PL/mandatory/userpermissions.sql
CONFLICT (content): Merge conflict in installer/data/mysql/pl-PL/mandatory/userpermissions.sql
Auto-merging installer/data/mysql/ru-RU/mandatory/permissions_and_user_flags.sql
CONFLICT (content): Merge conflict in installer/data/mysql/ru-RU/mandatory/permissions_and_user_flags.sql
Auto-merging installer/data/mysql/uk-UA/mandatory/permissions_and_user_flags.sql
CONFLICT (content): Merge conflict in installer/data/mysql/uk-UA/mandatory/permissions_and_user_flags.sql
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in 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/tools/tools-home.tt
Failed to merge in the changes.
Comment 34 Kyle M Hall 2013-05-02 17:21:48 UTC Comment hidden (obsolete)
Comment 35 Kyle M Hall 2013-05-02 17:27:39 UTC Comment hidden (obsolete)
Comment 36 Campbell Reid-Tait 2013-08-29 05:01:41 UTC
Did not apply at first. Needed to solve some conflicts.

Could not seem to add any new archtypes. But that might just be me doing somthing wrong. 

Also on the Modify Professional patron page, on the sidebar for the Clubs and services label there is an extra "> at the start.
Comment 37 David Cook 2013-08-30 01:34:00 UTC
I created a "weekly mailing list" type feature as well recently. I haven't uploaded it yet (due to time constraints and possible intersection with this patch), but it seems to be a bit different.

I'm wondering...why would the mailing list be for new "items" rather than new "bibliographic records"? In my case, I opted for the latter, as I thought it made more sense to email people when a new record has been catalogued as it represented a completely new material that they might find interesting. 

If you send mailing lists based on items, couldn't you be receiving emails about items that already exist in the system? Or do you only send out emails for new items for a bib where there haven't been items before? I could see that being quite useful.

The downside of my feature is that you might add a record but not have any items attached, so while people would know about the new record, they wouldn't be able to really "use" it. 

Could you explain a bit more about how the mailing lists work, Kyle?
Comment 38 Kyle M Hall 2013-09-12 11:48:58 UTC
We could easily modify the new items list script to select records created within the last X days, and have at least one item.

(In reply to David Cook from comment #37)
> I created a "weekly mailing list" type feature as well recently. I haven't
> uploaded it yet (due to time constraints and possible intersection with this
> patch), but it seems to be a bit different.
> 
> I'm wondering...why would the mailing list be for new "items" rather than
> new "bibliographic records"? In my case, I opted for the latter, as I
> thought it made more sense to email people when a new record has been
> catalogued as it represented a completely new material that they might find
> interesting. 
> 
> If you send mailing lists based on items, couldn't you be receiving emails
> about items that already exist in the system? Or do you only send out emails
> for new items for a bib where there haven't been items before? I could see
> that being quite useful.
> 
> The downside of my feature is that you might add a record but not have any
> items attached, so while people would know about the new record, they
> wouldn't be able to really "use" it. 
> 
> Could you explain a bit more about how the mailing lists work, Kyle?
Comment 39 Kyle M Hall 2014-02-10 16:17:27 UTC Comment hidden (obsolete)
Comment 40 Kyle M Hall 2014-02-10 16:19:10 UTC
Created attachment 25177 [details] [review]
Bug 7572 - Add Clubs & Services Feature

This features adds the ability to create clubs and services which patrons may be enrolled in.
It is particularly useful for tracking summer reading programs, book clubs and other such clubs.

It includes 2 built-in service archetypes:
Bestsellers Club : Batch generate a holds list which shuffles the holds queue when new items matching a given title or author are catalogued.
New Items Weekly Mailing Lists : Create custom mailing lists to alert subscribers to new items in your library.

This code has been in production for a number of years at the Crawford County Federated Library System.

Test Plan:
1) Add clubs & services permissions for a librarian account.
2) Create a new archetype
3) Create a new club or service based on the archetype
4) Enroll a borrower in the club or service
5) Cancel the enrollment in the club or service
6) Verify the permissions work correctly by turning off each one
   in turn and attempt to perform the action that should be denied.
Comment 41 David Cook 2014-06-23 03:15:06 UTC
I'm curious why you've marked this one as resolved won't fix, Kyle. 

Is it because of no progress on the bug or other reasons?

I'd still be interested in this going forward at some stage.
Comment 42 Magnus Enger 2014-06-23 06:50:35 UTC
(In reply to David Cook from comment #41)
> I'm curious why you've marked this one as resolved won't fix, Kyle. 
> Is it because of no progress on the bug or other reasons?
> I'd still be interested in this going forward at some stage.

See "Bug 12461 - Add patron clubs feature", perhaps?