Bug 12461

Summary: Add patron clubs feature
Product: Koha Reporter: Kyle M Hall <kyle>
Component: PatronsAssignee: Kyle M Hall <kyle>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: new feature    
Priority: P5 - low CC: abl, bgkriegel, bturnbull, chris, cmurdock, gmcharlt, gwilliams, jdemuth, jonathan.druart, josef.moravec, justinvos, katrin.fischer, kyle.m.hall, liz, luke.honiss, mtompset, nick, tredok.pierre, veron, ztajoli
Version: MainKeywords: Academy
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=813
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18510
Change sponsored?: --- Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
This features adds the ability to create clubs which patrons may be enrolled in. It is particularly useful for tracking summer reading programs, book clubs and other such clubs.
Version(s) released in:
Bug Depends on: 18171    
Bug Blocks: 18530, 18558, 35941, 35942, 14143, 18529, 18553, 18557, 18559, 18647, 18836, 18870, 19019, 19214, 19275, 33553    
Attachments: Bug 12461 - Add patron clubs feature
Bug 12461 - Add patron clubs feature
Bug 12461 - Add patron clubs feature
Bug 12461 - Add patron clubs feature
Bug 12461 - Add patron clubs feature
Bug 12461 - Add patron clubs feature
Bug 12461 - Add patron clubs feature
Bug 12461 - Add patron clubs feature
Bug 12461 - Add patron clubs feature
Bug 12461 - Add patron clubs feature
Bug 12461 - Add patron clubs feature
Bug 12461 - Add patron clubs feature
Problem with authorized values (in staff client)
Bug 12461 - Add patron clubs feature
Bug 12461 - Add patron clubs feature
Bug 12461 - Add patron clubs feature
Bug 12461 - Add patron clubs feature
Bug 12461 [QA Followup] - Changes based on suggestions
Bug 12461 [QA Followup] - Changes based on suggestions
Bug 12461 [QA Followup] - Changes based on suggestions
Bug 12461 [QA Followup] - Changes based on suggestions
Bug 12461 - Add patron clubs feature
Bug 12461 [QA Followup] - Changes based on suggestions
Bug 12461 - Add patron clubs feature
Bug 12461 [QA Followup] - Changes based on suggestions
Bug 12461 - Add patron clubs feature
Bug 12461 [QA Followup] - Changes based on suggestions
Bug 12461 [QA Followup] - Changes based on Koha standard interface pattern
Bug 12461 - Add patron clubs feature
Bug 12461 - Use snake case
Bug 12461 - Revert changes to circulation.pl
Bug 12461 - Template updates
Bug 12461 - Fix 'for=' and param issues
Bug 12461 - Return to clubs home after save
Bug 12461 - Remote collates from branchcode rows, add IGNORE for inserts
Bug 12461 - Add patron clubs feature
Bug 12461 - Add patron clubs feature
Bug 12461 - Add patron clubs feature
Bug 12461 - Add patron clubs feature
Bug 12461 [QA Followup]
Bug 12461 - Add patron clubs feature
Bug 12461 [QA Followup]
Bug 12461 - (QA Followup)

Description Kyle M Hall 2014-06-20 14:27:51 UTC
This features would add the ability to create clubs which patrons may be enrolled in. It would be particularly useful for tracking summer reading programs, book clubs and other such clubs.
Comment 1 Kyle M Hall 2014-06-20 15:20:42 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2014-08-01 14:46:41 UTC Comment hidden (obsolete)
Comment 3 Kyle M Hall 2014-09-12 18:01:52 UTC Comment hidden (obsolete)
Comment 4 Kyle M Hall 2015-01-12 12:09:06 UTC Comment hidden (obsolete)
Comment 5 Justin Vos 2015-01-14 20:45:53 UTC
In my test, I applied the patch and ran updatedatabase.pl. However I received this error message:

DBD::mysql::db do failed: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '' at line 13 at ./updatedatabase.pl line 9735.
DBD::mysql::db do failed: Can't create table 'koha_kohadev.#sql-a48_3a' (errno: 150) at ./updatedatabase.pl line 9776.
DBD::mysql::db do failed: Can't create table 'koha_kohadev.#sql-a48_3a' (errno: 150) at ./updatedatabase.pl line 9782.
DBD::mysql::db do failed: Table 'koha_kohadev.club_templates' doesn't exist at ./updatedatabase.pl line 9801.
DBD::mysql::db do failed: Can't create table 'koha_kohadev.#sql-a48_3a' (errno: 150) at ./updatedatabase.pl line 9806.
DBD::mysql::db do failed: Can't create table 'koha_kohadev.#sql-a48_3a' (errno: 150) at ./updatedatabase.pl line 9811.
Upgrade to 3.19.00.XXX done (Bug 12461 - Add patron clubs feature)
Comment 6 Kyle M Hall 2015-02-20 16:45:43 UTC Comment hidden (obsolete)
Comment 7 Kyle M Hall 2015-02-20 16:51:15 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2015-02-26 15:22:42 UTC Comment hidden (obsolete)
Comment 9 Pierre Angot 2015-03-03 09:20:12 UTC
We've tested it on sandbox.

The patch apply correctly, but we don't see the "Patron clubs" link.
Comment 10 Marc Véron 2015-03-03 17:32:58 UTC
I do not see see the "Patron clubs" link either.

It seems that changes to /prog/en/modules/tools/tools-home.tt are missing. They were e.g. in Attachment #30456 [details]

(Going to the page directly with /cgi-bin/koha/clubs/clubs.pl works)
Comment 11 Kyle M Hall 2015-05-01 14:33:53 UTC Comment hidden (obsolete)
Comment 12 Bernardo Gonzalez Kriegel 2015-05-11 20:03:09 UTC
Sorry, patch does not apply.
Small conflicts, and DB update is not on atomicupdate,
perhaps RM can make an exception
Comment 13 Kyle M Hall 2015-05-14 12:14:43 UTC Comment hidden (obsolete)
Comment 14 Bernardo Gonzalez Kriegel 2015-05-14 15:11:38 UTC
Almost all tested, looks good.
But reloading kohastructure with your patch applied, gives

mysql kohadev <  ~/kohaclone/installer/data/mysql/kohastructure.sql
ERROR 1005 (HY000) at line 3656: Can't create table 'kohadev.#sql-42d1_401' (errno: 150)

Can't understand or find the error
Comment 15 Mark Tompsett 2015-05-24 16:22:55 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #14)
> Almost all tested, looks good.
> But reloading kohastructure with your patch applied, gives
> 
> mysql kohadev <  ~/kohaclone/installer/data/mysql/kohastructure.sql
> ERROR 1005 (HY000) at line 3656: Can't create table 'kohadev.#sql-42d1_401'
> (errno: 150)
> 
> Can't understand or find the error

Not to mention, why add the constraints separately?!
If this was $sth = $dbh->prepare(), $sth->execute() method of doing this, then wouldn't likely get this issue either.
Comment 16 Mark Tompsett 2015-06-06 04:20:57 UTC
Additionally, this patch has a conflict with circ/circulation.pl now.
Comment 17 Kyle M Hall 2015-10-15 12:39:35 UTC Comment hidden (obsolete)
Comment 18 Kyle M Hall 2015-10-15 12:41:48 UTC
Updated patch merges constraints into table creation, fixes merge conflicts. Tested with fresh install and upgrade!
Comment 19 jdemuth@roseville.ca.us 2015-12-18 18:34:03 UTC
Hi, 

I tried testing this in two different sandboxes and I get this error.

Some problems occurred applying patches from bug 12461:
<h1>Something went wrong !</h1>Applying: Bug 12461 - Add patron clubs feature Using index info to reconstruct a base tree...
<stdin>:3143: new blank line at EOF.
+
warning: 1 line adds whitespace errors.
Falling back to patching base and 3-way merge...
Auto-merging opac/opac-user.pl
Auto-merging members/moremember.pl
Auto-merging koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-user.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt
Auto-merging installer/data/mysql/userpermissions.sql
Auto-merging installer/data/mysql/userflags.sql
Auto-merging installer/data/mysql/kohastructure.sql
CONFLICT (content): Merge conflict in installer/data/mysql/kohastructure.sql
Auto-merging circ/circulation.pl
Auto-merging Koha/Template/Plugin/Koha.pm Auto-merging Koha/Template/Plugin/Borrowers.pm CONFLICT (content): Merge conflict in Koha/Template/Plugin/Borrowers.pm Auto-merging Koha/Schema/Result/Borrower.pm CONFLICT (content): Merge conflict in Koha/Schema/Result/Borrower.pm Auto-merging C4/Members.pm CONFLICT (content): Merge conflict in C4/Members.pm Auto-merging C4/Auth.pm Failed to merge in the changes.
Patch failed at 0001 Bug 12461 - Add patron clubs feature

When you have resolved this problem run git bz apply --continue.
If you would prefer to skip this patch, instead run git bz apply --skip.
To restore the original branch and stop patching run git bz apply --abort.
Bug 12461 - Add patron clubs feature

43428 - Bug 12461 - Add patron clubs feature

Apply? [(y)es, (n)o, (i)nteractive] Patch left in /tmp/Bug-12461---Add-patron-clubs-feature-1dbTgL.patch .

Jen DeMuth
Comment 20 Bernardo Gonzalez Kriegel 2015-12-18 19:56:27 UTC
(In reply to Kyle M Hall from comment #18)
> Updated patch merges constraints into table creation, fixes merge conflicts.
> Tested with fresh install and upgrade!

Hi, testing again.
There are small conflicts. I fixed that and also have 2 follow-ups,
one for koha-qa and the other to fix atomicupdate (you missed
new permissions there)

But I'm stuck with the test plan.
Don't know how to do 7 and 9-13. I can't see any place to enroll or enable a club for OPAC.

Could you explain?
Comment 21 Kyle M Hall 2016-02-09 17:13:56 UTC Comment hidden (obsolete)
Comment 22 Kyle M Hall 2016-02-09 17:45:21 UTC Comment hidden (obsolete)
Comment 23 Kyle M Hall 2016-02-09 17:45:57 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #20)
> (In reply to Kyle M Hall from comment #18)
> > Updated patch merges constraints into table creation, fixes merge conflicts.
> > Tested with fresh install and upgrade!
> 
> Hi, testing again.
> There are small conflicts. I fixed that and also have 2 follow-ups,
> one for koha-qa and the other to fix atomicupdate (you missed
> new permissions there)
> 
> But I'm stuck with the test plan.
> Don't know how to do 7 and 9-13. I can't see any place to enroll or enable a
> club for OPAC.
> 
> Could you explain?

There was a bug that was preventing the Clubs tab from displaying on the opac side. Fixed!
Comment 24 Marc Véron 2016-02-09 18:29:38 UTC Comment hidden (obsolete)
Comment 25 Kyle M Hall 2016-02-10 14:36:01 UTC Comment hidden (obsolete)
Comment 26 Kyle M Hall 2016-02-10 14:36:26 UTC
(In reply to Marc Véron from comment #24)
> Created attachment 47825 [details]
> Problem with authorized values (in staff client)
> 
> With an authorized value defined as YES_NO, I get a strnge drop down when I
> want to enroll a patron in staff client. (Drop down list appears OK in OPAC).

Good catch! Should be fixed in this latest patch!
Comment 27 Marc Véron 2016-02-10 15:06:51 UTC
Went through test plan, works as expected.

However QA tools complain:

 FAIL	installer/data/mysql/kohastructure.sql
   FAIL	  charset_collate
		The table club_fields does not have the current charset collate (see bug 11944)
		The table club_template_enrollment_fields does not have the current charset collate (see bug 11944)
		The table club_enrollments does not have the current charset collate (see bug 11944)
		The table club_enrollment_fields does not have the current charset collate (see bug 11944)
		The table club_template_fields does not have the current charset collate (see bug 11944)
		The table club_templates does not have the current charset collate (see bug 11944)
		The table clubs does not have the current charset collate (see bug 11944)
Comment 28 Kyle M Hall 2016-03-14 15:59:51 UTC Comment hidden (obsolete)
Comment 29 Kyle M Hall 2016-03-14 16:00:49 UTC
All QA script issues should be fixed with this latest patch!
Comment 30 Marc Véron 2016-03-14 16:38:00 UTC Comment hidden (obsolete)
Comment 31 Katrin Fischer 2016-03-22 07:31:04 UTC
Comment on attachment 49121 [details] [review]
Bug 12461 - Add patron clubs feature

Review of attachment 49121 [details] [review]:
-----------------------------------------------------------------

Did a first code review - not a lot of findings and mostly minor. Haven't tested the code yet.

Patch doesn't apply atm - not sure why the modules appear in the general syspref.pm?

<<<<<<< HEAD
use C4::Log;
=======
use Koha::Clubs;
use Koha::Club::Enrollments;
>>>>>>> Bug 12461 - Add patron clubs feature

::: Koha/Template/Plugin/AuthorisedValues.pm
@@ +30,4 @@
>  
>  sub Get {
>      my ( $self, $category, $selected, $opac ) = @_;
> +    warn "GET( $self, $category, $selected, $opac )";

Unconditional warn - please remove.

::: koha-tmpl/intranet-tmpl/prog/en/modules/clubs/clubs-add-modify.tt
@@ +32,5 @@
> +<div class="yui-t7">
> +    <div class="yui-main">
> +        [% IF stored %]
> +            <div class="alert">
> +                <p>Your club was [% IF stored == 'updated' %] updated [% ELSE %] saved [% END %]</p>

Please avoid constructing sentences like that - it's hard to translate, because grammar and work order is different in other languages.

::: koha-tmpl/intranet-tmpl/prog/en/modules/clubs/clubs.tt
@@ +23,5 @@
> +        } ));
> +    });
> +
> +    function ConfirmDeleteTemplate( id, name, a ) {
> +        if ( confirm( _("Are you sure you want to delete the club template") + name + "?" ) ) {

It would be nicer for translations to use the new Javascript syntax with placeholders here.

@@ +41,5 @@
> +        }
> +    }
> +
> +    function ConfirmDeleteClub( id, name, a ) {
> +        if ( confirm( _("Are you sure you want to delete the club ") + name + "?" ) ) {

Same as above - placeholders preferrable!

::: koha-tmpl/intranet-tmpl/prog/en/modules/clubs/templates-add-modify.tt
@@ +9,5 @@
> +//<![CDATA[
> +
> +function CheckForm() {
> +  if ( !$("#club-template-name").val() ) {
> +    alert( _("Name is a required field!")  );

Would it be possible to use the standard markup for required fields insted here? See administration pages for examples.

@@ +30,5 @@
> +<div class="yui-t7">
> +    <div class="yui-main">
> +        [% IF stored %]
> +            <div class="alert">
> +                <p>Your club template was [% IF stored == 'updated' %] updated [% ELSE %] saved [% END %]</p>

Please don't construct sentences (see above).

::: koha-tmpl/opac-tmpl/bootstrap/en/modules/clubs/enroll.tt
@@ +27,5 @@
> +                    </li>
> +                [% END %]
> +
> +                <li>
> +                    <a href="#" class="btn" onclick="addEnrollment(); return false;"><i class="icon-plus"></i> Enroll</a>

Switch to FA icon - I saw most of them were already, maybe missed this one?
Comment 32 Katrin Fischer 2016-03-28 23:10:09 UTC
Some first tests:
1) New permissions need to be added to the permissions.inc file for translation.
2) Datatables on the new tools page are broken.
3) Misses tools sidebar navigation.

I feel like the templates look a bit odd - maybe it would be better to have the different parts in separate boxes? Once this is progressed a bit further, we can worry about prettying thing up :)
Comment 33 Kyle M Hall 2016-04-19 15:17:14 UTC
Created attachment 50422 [details] [review]
Bug 12461 - Add patron clubs feature

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

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Ensure your staff user has the new 'Patron clubs' permissions
4) Under the tools menu, click the "Patron clubs" link
5) Create a new club template
   * Here you can add fields that can be filled out at the time
     a new club is created based on the template, or a new enrollment
     is created for a given club based on the template.
6) Create a new club based on that template
7) Attempt to enroll a patron in that club
8) Create a club with email required set
9) Attempt to enroll a patron without an email address in that club
10) Create a club that is enrollable from the OPAC
11) Attempt to enroll a patron in that club
12) Attempt to cancel a club enrollment from the OPAC
13) Attempt to cancel a club enrollment from the staff interface

Followed test plan, works as expected.
Signed-off-by: Marc Véron <veron@veron.ch>
Comment 34 Kyle M Hall 2016-04-19 15:17:23 UTC Comment hidden (obsolete)
Comment 35 Kyle M Hall 2016-04-19 15:21:58 UTC Comment hidden (obsolete)
Comment 36 Kyle M Hall 2016-04-19 15:23:28 UTC
Created attachment 50427 [details] [review]
Bug 12461 [QA Followup] - Changes based on suggestions
Comment 37 Kyle M Hall 2016-04-19 15:26:16 UTC
(In reply to Katrin Fischer from comment #32)
> Some first tests:
> 1) New permissions need to be added to the permissions.inc file for
> translation.

Fixed!

> 2) Datatables on the new tools page are broken.

Fixed!

> 3) Misses tools sidebar navigation.

Fixed!

> I feel like the templates look a bit odd - maybe it would be better to have
> the different parts in separate boxes? Once this is progressed a bit
> further, we can worry about prettying thing up :)

I agree. The presentation of this feature requires layouts that have not been used in Koha before. However, this followup goes a very long way to putting elements into a more "Koha-like" display. I think you'll find all your concerns have been addressed!
Comment 38 Katrin Fischer 2016-07-15 08:45:38 UTC
I am sorry, there are too many conflicts for me to feel comfortable trying to fix:
CONFLICT (content): Merge conflict in opac/opac-user.pl
Auto-merging members/moremember.pl
Auto-merging koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-user.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/tools/tools-home.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.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
Auto-merging installer/data/mysql/kohastructure.sql
CONFLICT (content): Merge conflict in installer/data/mysql/kohastructure.sql
Auto-merging circ/circulation.pl
CONFLICT (content): Merge conflict in circ/circulation.pl
Auto-merging Koha/Schema/Result/Borrower.pm
Auto-merging Koha/Patron.pm
CONFLICT (content): Merge conflict in Koha/Patron.pm

Please take a look!
Comment 39 Chris Cormack 2017-01-10 19:21:07 UTC
Kyle, we would like to get the academy students testing this again, but the patches do not apply. If you have a chance to rebase before Monday please do, otherwise please remove the Academy tag
Comment 40 Kyle M Hall 2017-01-11 14:36:00 UTC
Created attachment 58818 [details] [review]
Bug 12461 [QA Followup] - Changes based on suggestions
Comment 41 Kyle M Hall 2017-01-11 14:36:25 UTC
Created attachment 58819 [details] [review]
Bug 12461 - Add patron clubs feature

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

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Ensure your staff user has the new 'Patron clubs' permissions
4) Under the tools menu, click the "Patron clubs" link
5) Create a new club template
   * Here you can add fields that can be filled out at the time
     a new club is created based on the template, or a new enrollment
     is created for a given club based on the template.
6) Create a new club based on that template
7) Attempt to enroll a patron in that club
8) Create a club with email required set
9) Attempt to enroll a patron without an email address in that club
10) Create a club that is enrollable from the OPAC
11) Attempt to enroll a patron in that club
12) Attempt to cancel a club enrollment from the OPAC
13) Attempt to cancel a club enrollment from the staff interface

Followed test plan, works as expected.
Signed-off-by: Marc Véron <veron@veron.ch>
Comment 42 Kyle M Hall 2017-01-11 14:36:34 UTC
Created attachment 58820 [details] [review]
Bug 12461 [QA Followup] - Changes based on suggestions
Comment 43 Kyle M Hall 2017-01-11 14:37:05 UTC
(In reply to Chris Cormack from comment #39)
> Kyle, we would like to get the academy students testing this again, but the
> patches do not apply. If you have a chance to rebase before Monday please
> do, otherwise please remove the Academy tag

Done!
Comment 44 Luke Honiss 2017-01-19 03:53:05 UTC
There are a large amount of conflicts when applying patch which makes it rather difficult to test. If these could be fixed that would help.

Cheers.
Comment 45 Kyle M Hall 2017-01-19 17:14:34 UTC
Created attachment 59272 [details] [review]
Bug 12461 - Add patron clubs feature

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

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Ensure your staff user has the new 'Patron clubs' permissions
4) Under the tools menu, click the "Patron clubs" link
5) Create a new club template
   * Here you can add fields that can be filled out at the time
     a new club is created based on the template, or a new enrollment
     is created for a given club based on the template.
6) Create a new club based on that template
7) Attempt to enroll a patron in that club
8) Create a club with email required set
9) Attempt to enroll a patron without an email address in that club
10) Create a club that is enrollable from the OPAC
11) Attempt to enroll a patron in that club
12) Attempt to cancel a club enrollment from the OPAC
13) Attempt to cancel a club enrollment from the staff interface

Followed test plan, works as expected.
Signed-off-by: Marc Véron <veron@veron.ch>
Comment 46 Kyle M Hall 2017-01-19 17:14:42 UTC
Created attachment 59273 [details] [review]
Bug 12461 [QA Followup] - Changes based on suggestions
Comment 47 Luke Honiss 2017-01-19 21:09:33 UTC
You have left in the '>>>>>>> BUG XXXXX' when resolving the conflicts in Koha/Patron.pm (line 644) and installer/data/mysql/kohastructure.sql (lines 4077 and 4078).

I cannot work out how to add a patron to a club however. If you could walk me through the process that would be awesome.

Also when adding another field in the creation of a template there is no colon after the field name unlike the default fields when editing the club. There is also an extra full stop in the tools homepage under the 'Patron clubs' link.

Looks like it will be a great feature when finished!
Comment 48 Josef Moravec 2017-01-24 20:42:20 UTC
Kyle, could you please provide followup due to bug 16239?

See https://wiki.koha-community.org/wiki/Interface_patterns
Comment 49 Kyle M Hall 2017-02-16 17:36:26 UTC
(In reply to Josef Moravec from comment #48)
> Kyle, could you please provide followup due to bug 16239?
> 
> See https://wiki.koha-community.org/wiki/Interface_patterns

Josef, can you please be more specific?

Thanks!
Comment 50 Josef Moravec 2017-02-20 06:32:57 UTC
(In reply to Kyle M Hall from comment #49)
> (In reply to Josef Moravec from comment #48)
> > Kyle, could you please provide followup due to bug 16239?
> > 
> > See https://wiki.koha-community.org/wiki/Interface_patterns
> 
> Josef, can you please be more specific?
> 
> Thanks!

Of course, you need to use new btn classes due to Bootstrap 3 - "btn" is not enough, you need to change it to  "btn btn-default" also btn size classes were renamed: 
  "btn-mini" -> "btn-xs" 
  "btn-small" -> "btn-sm"
Comment 51 Kyle M Hall 2017-02-21 14:14:38 UTC
Thanks! That helps a lot!

(In reply to Josef Moravec from comment #50)
> (In reply to Kyle M Hall from comment #49)
> > (In reply to Josef Moravec from comment #48)
> > > Kyle, could you please provide followup due to bug 16239?
> > > 
> > > See https://wiki.koha-community.org/wiki/Interface_patterns
> > 
> > Josef, can you please be more specific?
> > 
> > Thanks!
> 
> Of course, you need to use new btn classes due to Bootstrap 3 - "btn" is not
> enough, you need to change it to  "btn btn-default" also btn size classes
> were renamed: 
>   "btn-mini" -> "btn-xs" 
>   "btn-small" -> "btn-sm"
Comment 52 Kyle M Hall 2017-02-21 14:16:42 UTC
Created attachment 60519 [details] [review]
Bug 12461 - Add patron clubs feature

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

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Ensure your staff user has the new 'Patron clubs' permissions
4) Under the tools menu, click the "Patron clubs" link
5) Create a new club template
   * Here you can add fields that can be filled out at the time
     a new club is created based on the template, or a new enrollment
     is created for a given club based on the template.
6) Create a new club based on that template
7) Attempt to enroll a patron in that club
8) Create a club with email required set
9) Attempt to enroll a patron without an email address in that club
10) Create a club that is enrollable from the OPAC
11) Attempt to enroll a patron in that club
12) Attempt to cancel a club enrollment from the OPAC
13) Attempt to cancel a club enrollment from the staff interface

Followed test plan, works as expected.
Signed-off-by: Marc Véron <veron@veron.ch>
Comment 53 Kyle M Hall 2017-02-21 14:16:51 UTC
Created attachment 60520 [details] [review]
Bug 12461 [QA Followup] - Changes based on suggestions
Comment 54 Kyle M Hall 2017-02-21 14:16:56 UTC
Created attachment 60521 [details] [review]
Bug 12461 [QA Followup] - Changes based on Koha standard interface pattern
Comment 55 Jonathan Druart 2017-02-22 14:34:10 UTC
In my opinion, this patchset is not ready to be QAed. Lot of changes should be done to meet our coding guidelines.
Comment 56 Kyle M Hall 2017-02-22 14:55:33 UTC
(In reply to Jonathan Druart from comment #55)
> In my opinion, this patchset is not ready to be QAed. Lot of changes should
> be done to meet our coding guidelines.

Jonathan, can you be more specific? That sounds like QA to me ; )

I've been working on the feature since 2012. It's already gone through one full rewrite because Koha's coding guidelines had moved on so much since it was initially written.

If you want another signoff, that's fine. But you specifically wrote "Lot of changes should be done to meet our coding guidelines" which is not a task for a signer. That's a task for a QA'er and I'll really need some actionable comments if I am to make any changes!

If you want another sign-off, please set status to needs sign-off. Otherwise the status can go back to failed qa.
Comment 57 Jonathan Druart 2017-02-23 09:00:31 UTC
Kyle,

I did not write the different failures because most of them are obvious:

1. There are no tests
2. CamelCase instead of snake_case for methods of Koha::Clubs and Koha::Patron
3. GetClubEnrollmentsCount and GetEnrollableClubsCount are not needed, just call ->count
4. You should not need to add GetBranches to the Branches TT plugin. Take a look at options_for_libraries
5. Subroutine added to Koha TT plugin (UserEnv), but not used.
6. Changes to circ/circulation.pl are not needed. A variable 'patron' is already passed to the template.
7. All the labels of the form has the same value for the "for" attribute.
8. Tables on clubs/clubs.pl are ugly (missing datatables.css?)
9. On /clubs/clubs-add-modify.pl?club_template_id=1, there is an empty input at the end of the form.
8. CGI->param must not be called in scalar context
9. When I save a form, I get "Your club was created" with a link to "Return to patron clubs". That sounds weird. Especially because the box is yellow. I'd suggest to behave as everywhere else in Koha: Save and get the list.
I guess it's because there are too many small scripts, maybe it would have been easier to have a bigger one and have the different action handled in it (as everywhere else in admin scripts).
10. INSERT statements should have IGNORE in update db process
11. +  `branchcode` varchar(10) COLLATE utf8_unicode_ci NULL DEFAULT NULL,
you should not need to specify the COLLATE here.
12. QA tests do not pass
  FAIL   Koha/Patron.pm
  FAIL   forbidden patterns
         forbidden pattern: merge marker (>>>>>>>) (line 647)
Comment 58 Jonathan Druart 2017-02-23 09:11:27 UTC
When I hit /circ/circulation.pl?borrowernumber=42, I get a software error:
Template process failed: undef error - The method category_type is not covered by tests! at /home/vagrant/kohaclone/C4/Templates.pm line 121.


Yes I think another signoff is needed.
Comment 59 Kyle M Hall 2017-02-23 11:39:06 UTC
Thanks! That helps a lot!

(In reply to Jonathan Druart from comment #57)
> Kyle,
> 
> I did not write the different failures because most of them are obvious:
> 
> 1. There are no tests
> 2. CamelCase instead of snake_case for methods of Koha::Clubs and
> Koha::Patron
> 3. GetClubEnrollmentsCount and GetEnrollableClubsCount are not needed, just
> call ->count
> 4. You should not need to add GetBranches to the Branches TT plugin. Take a
> look at options_for_libraries
> 5. Subroutine added to Koha TT plugin (UserEnv), but not used.
> 6. Changes to circ/circulation.pl are not needed. A variable 'patron' is
> already passed to the template.
> 7. All the labels of the form has the same value for the "for" attribute.
> 8. Tables on clubs/clubs.pl are ugly (missing datatables.css?)
> 9. On /clubs/clubs-add-modify.pl?club_template_id=1, there is an empty input
> at the end of the form.
> 8. CGI->param must not be called in scalar context
> 9. When I save a form, I get "Your club was created" with a link to "Return
> to patron clubs". That sounds weird. Especially because the box is yellow.
> I'd suggest to behave as everywhere else in Koha: Save and get the list.
> I guess it's because there are too many small scripts, maybe it would have
> been easier to have a bigger one and have the different action handled in it
> (as everywhere else in admin scripts).
> 10. INSERT statements should have IGNORE in update db process
> 11. +  `branchcode` varchar(10) COLLATE utf8_unicode_ci NULL DEFAULT NULL,
> you should not need to specify the COLLATE here.
> 12. QA tests do not pass
>   FAIL   Koha/Patron.pm
>   FAIL   forbidden patterns
>          forbidden pattern: merge marker (>>>>>>>) (line 647)
Comment 60 Kyle M Hall 2017-02-23 14:54:28 UTC
Created attachment 60610 [details] [review]
Bug 12461 - Add patron clubs feature

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

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Ensure your staff user has the new 'Patron clubs' permissions
4) Under the tools menu, click the "Patron clubs" link
5) Create a new club template
   * Here you can add fields that can be filled out at the time
     a new club is created based on the template, or a new enrollment
     is created for a given club based on the template.
6) Create a new club based on that template
7) Attempt to enroll a patron in that club
8) Create a club with email required set
9) Attempt to enroll a patron without an email address in that club
10) Create a club that is enrollable from the OPAC
11) Attempt to enroll a patron in that club
12) Attempt to cancel a club enrollment from the OPAC
13) Attempt to cancel a club enrollment from the staff interface

Followed test plan, works as expected.
Signed-off-by: Marc Véron <veron@veron.ch>
Comment 61 Kyle M Hall 2017-02-23 14:54:38 UTC
Created attachment 60611 [details] [review]
Bug 12461 - Use snake case
Comment 62 Kyle M Hall 2017-02-23 14:54:42 UTC
Created attachment 60612 [details] [review]
Bug 12461 - Revert changes to circulation.pl
Comment 63 Kyle M Hall 2017-02-23 14:54:46 UTC
Created attachment 60613 [details] [review]
Bug 12461 - Template updates
Comment 64 Kyle M Hall 2017-02-23 14:54:51 UTC
Created attachment 60614 [details] [review]
Bug 12461 - Fix 'for=' and param issues
Comment 65 Kyle M Hall 2017-02-23 14:54:55 UTC
Created attachment 60615 [details] [review]
Bug 12461 - Return to clubs home after save
Comment 66 Kyle M Hall 2017-02-23 14:55:00 UTC
Created attachment 60616 [details] [review]
Bug 12461 - Remote collates from branchcode rows, add IGNORE for inserts
Comment 67 Kyle M Hall 2017-02-23 14:57:41 UTC
Created attachment 60617 [details] [review]
Bug 12461 - Add patron clubs feature
Comment 68 Kyle M Hall 2017-02-23 14:59:16 UTC
Still needs unit tests.
Comment 69 Kyle M Hall 2017-02-23 15:02:23 UTC
Created attachment 60618 [details] [review]
Bug 12461 - Add patron clubs feature

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

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Ensure your staff user has the new 'Patron clubs' permissions
4) Under the tools menu, click the "Patron clubs" link
5) Create a new club template
   * Here you can add fields that can be filled out at the time
     a new club is created based on the template, or a new enrollment
     is created for a given club based on the template.
6) Create a new club based on that template
7) Attempt to enroll a patron in that club
8) Create a club with email required set
9) Attempt to enroll a patron without an email address in that club
10) Create a club that is enrollable from the OPAC
11) Attempt to enroll a patron in that club
12) Attempt to cancel a club enrollment from the OPAC
13) Attempt to cancel a club enrollment from the staff interface

Followed test plan, works as expected.
Signed-off-by: Marc Véron <veron@veron.ch>
Comment 70 Kyle M Hall 2017-02-23 19:43:32 UTC
Created attachment 60624 [details] [review]
Bug 12461 - Add patron clubs feature

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

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Ensure your staff user has the new 'Patron clubs' permissions
4) Under the tools menu, click the "Patron clubs" link
5) Create a new club template
   * Here you can add fields that can be filled out at the time
     a new club is created based on the template, or a new enrollment
     is created for a given club based on the template.
6) Create a new club based on that template
7) Attempt to enroll a patron in that club
8) Create a club with email required set
9) Attempt to enroll a patron without an email address in that club
10) Create a club that is enrollable from the OPAC
11) Attempt to enroll a patron in that club
12) Attempt to cancel a club enrollment from the OPAC
13) Attempt to cancel a club enrollment from the staff interface

Followed test plan, works as expected.
Signed-off-by: Marc Véron <veron@veron.ch>
Comment 71 Liz Rea 2017-02-27 01:08:09 UTC
Did you mean to push this to master?
Comment 72 Liz Rea 2017-02-27 01:47:42 UTC
Changed to critical for the feature it blocks, but this is not a blocker anymore.
Comment 73 Liz Rea 2017-02-27 01:48:14 UTC
Doh wrong bug.
Comment 74 Jonathan Druart 2017-02-27 07:35:38 UTC
(In reply to Liz Rea from comment #71)
> Did you mean to push this to master?

It has been pushed and reverted...
Comment 75 Kyle M Hall 2017-02-27 11:22:51 UTC
(In reply to Jonathan Druart from comment #74)
> (In reply to Liz Rea from comment #71)
> > Did you mean to push this to master?
> 
> It has been pushed and reverted...

Yes, my bad. Accidentally applied a followup to my working branch instead of master so it went along for the ride! As much as I'd love for this to be in master it's not quite ready ; )
Comment 76 Nick Clemens 2017-04-25 19:02:34 UTC
a/clubs/clubs-add-modify.pl
47 my $stored = $cgi->param('name') ? $id ? 'updated' : 'stored' : undef;

*This is confusing

a/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-user.tt 
85 This item is on hold for another borrower.

*Should be patron

Koha/Template/Plugin/AuthorisedValues.pm
38 	my ( $self, $category, $selected, $opac ) = @_;
39 	return GetAuthorisedValues( $category, $selected, $opac );

*I think this is rebase error, no 'selected' param now


koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-user.tt:
[% IF borrower.get_club_enrollments_count.size || borrower.get_enrollable_clubs(1).size %]

*causes error - The method size is not covered by tests

*same as above in:
koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt


koha-tmpl/opac-tmpl/bootstrap/en/modules/clubs/clubs-tab.tt

* borrower.FirstValidEmailAddress is not covered by tests, I think you mean borrower.first_valid_email_address

When viewing tools/clubs.pl
1 - The 'New club' button has an empty dropdown until a template is defined, it should be disabled
2 - Once the first template is defined the Clubs table says: " No club templates defined. " - should say "No clubs defined"
3 - Dates for clubs should show in clubs table

When adding patrons to clubs - 
1 - If no fields are associated with a club you click 'Enroll' then have a new screen with no additional info and must click 'Enroll' again - should be clearer 'Finish enrollment' or something
2 - You should not be able to add a patron to club that is out of date, or they should be hidden by default

The feature adds valuable info for reporting, but I think it should have at least one default view, a link from the clubs table to "See patrons enrolled in this club" 
That link should be copied on the patrons club enrollement tab
Comment 77 Kyle M Hall 2017-04-27 14:26:11 UTC
Created attachment 62778 [details] [review]
Bug 12461 - Add patron clubs feature

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

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Ensure your staff user has the new 'Patron clubs' permissions
4) Under the tools menu, click the "Patron clubs" link
5) Create a new club template
   * Here you can add fields that can be filled out at the time
     a new club is created based on the template, or a new enrollment
     is created for a given club based on the template.
6) Create a new club based on that template
7) Attempt to enroll a patron in that club
8) Create a club with email required set
9) Attempt to enroll a patron without an email address in that club
10) Create a club that is enrollable from the OPAC
11) Attempt to enroll a patron in that club
12) Attempt to cancel a club enrollment from the OPAC
13) Attempt to cancel a club enrollment from the staff interface

Followed test plan, works as expected.
Signed-off-by: Marc Véron <veron@veron.ch>
Comment 78 Kyle M Hall 2017-04-27 14:26:22 UTC
Created attachment 62779 [details] [review]
Bug 12461 [QA Followup]
Comment 79 Nick Clemens 2017-04-28 10:48:02 UTC
Created attachment 62813 [details] [review]
Bug 12461 - Add patron clubs feature

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

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Ensure your staff user has the new 'Patron clubs' permissions
4) Under the tools menu, click the "Patron clubs" link
5) Create a new club template
   * Here you can add fields that can be filled out at the time
     a new club is created based on the template, or a new enrollment
     is created for a given club based on the template.
6) Create a new club based on that template
7) Attempt to enroll a patron in that club
8) Create a club with email required set
9) Attempt to enroll a patron without an email address in that club
10) Create a club that is enrollable from the OPAC
11) Attempt to enroll a patron in that club
12) Attempt to cancel a club enrollment from the OPAC
13) Attempt to cancel a club enrollment from the staff interface

Followed test plan, works as expected.
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 80 Nick Clemens 2017-04-28 10:48:08 UTC
Created attachment 62814 [details] [review]
Bug 12461 [QA Followup]

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 81 Nick Clemens 2017-04-28 10:48:14 UTC
Created attachment 62815 [details] [review]
Bug 12461 - (QA Followup)

    Club enrollments should only list active enrollments
    Confirms should warn of cascading delete
    Branch limited clubs should only be available to members of that
    branch
    Fix breadcrumbs
Comment 82 Kyle M Hall 2017-04-28 12:48:34 UTC
Pushed to master for 17.05, thanks Nick!
Comment 83 Jonathan Druart 2017-05-08 15:57:36 UTC
I do not understand the $return_scalar argument you use for get_club_enrollments and get_enrollable_clubs. Could you please explain?

It seems useless, I suggest to remove it.
Comment 84 Kyle M Hall 2017-05-08 16:10:56 UTC
(In reply to Jonathan Druart from comment #83)
> I do not understand the $return_scalar argument you use for
> get_club_enrollments and get_enrollable_clubs. Could you please explain?
> 
> It seems useless, I suggest to remove it.

It is a hack solution to an big issue with template toolkit. Basically, when calling a method from Template Toolkit that would return a Koha::Objects derived class, the result will always be returned in list context. I've tried using the scalar plugin for TT but had no luck.

Even worse, if the list has only 1 element, TT will convert it to just being that element with no list context. Then, if you try to call a TT list method ( such as 'size' ), it will try to call that method on the object instead and you'll get an error!
Comment 85 Katrin Fischer 2017-05-13 12:30:03 UTC
This won't get ported back to 16.11.x as it is a new feature.