Bug 7067

Summary: allow patron self registration via the opac
Product: Koha Reporter: Nicole C. Engard <nengard>
Component: PatronsAssignee: Kyle M Hall <kyle.m.hall>
Status: CLOSED FIXED QA Contact: Jonathan Druart <jonathan.druart>
Severity: new feature    
Priority: P5 - low CC: claire.hernandez, corinne.hayet, dpavlin, f.demians, fridolin.somers, gmcharlt, grant, jghiorzi, jonathan.druart, katrin.fischer, kyle.m.hall, kyle, magnus, paul.poulain, programming, smichaelxx, wizzyrea
Version: 3.12   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9384
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9393
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10293
Change sponsored?: Sponsored Patch complexity: Large patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 9930    
Attachments: Bug 7067 - OPAC Borrower Self Registration
Bug 7067 - OPAC Borrower Self Registration
Bug 7067 - OPAC Borrower Self Registration
Bug 7067 - OPAC Borrower Self Registration
Bug 7067 - OPAC Borrower Self Registration
Bug 7067 - OPAC Borrower Self Registration
Bug 7067 - OPAC Borrower Self Registration
Bug 7067 - OPAC Borrower Self Registration
Bug 7067 - OPAC Borrower Self Registration
Bug 7067 - OPAC Borrower Self Registration
Bug 7067 - OPAC Borrower Self Registration
Bug 7067 - OPAC Borrower Self Registration
Bug 7067 - OPAC Borrower Self Registration
Bug 7067 - OPAC Borrower Self Registration - Followup - Change redirect from mainpage.pl to members-update.pl
Bug 7067 - OPAC Borrower Self Registration - Followup
Bug 7067 - OPAC Borrower Self Registration - Followup
Bug 7067 - OPAC Borrower Self Registration - Followup
Bug 7067 - OPAC Borrower Self Registration - Followup
Bug 7067 - OPAC Borrower Self Registration - Followup - Rename PatronSelfRegistrationUseTemporaryStatus to PatronSelfRegistrationDefaultCategory
Bug 7067 - allow patron self registration via the opac - Followup - Hide register link unless PatronSelfRegistrationDefaultCategory is set.
Bug 7067 - allow patron self registration via the opac - Followup - Add invalid token page
Bug 7067 - OPAC Borrower Self Registration - Followup
Bug 7067 - OPAC Borrower Self Registration - Followup
Bug 7067 - OPAC Borrower Self Registration - Followup
Bug 7067 - OPAC Borrower Self Registration
Bug 7067 - OPAC Borrower Self Registration - Followup
Bug 7067 - OPAC Borrower Self Registration
Bug 7067 - OPAC Borrower Self Registration - Followup
Bug 7067 - OPAC Borrower Self Registration - Followup
[SIGNED-OFF] Bug 7067 - OPAC Borrower Self Registration
Whoops, duplicate attachment.
[SIGNED-OFF] Bug 7067 - OPAC Borrower Self Registration - Followup
Bug 7067 [Follow-up: templates] allow patron self registration via the opac
[SIGNED-OFF] Bug 7067 [Follow-up: templates] allow patron self registration via the opac
Bug 7067 - OPAC Borrower Self Registration
Bug 7067 - OPAC Borrower Self Registration - Followup
Bug 7067 [Follow-up: templates] allow patron self registration via the opac
Bug 7067: Followup FIX QA fails
Bug 7067: Followup FIX merge updatedatabase
[SIGNED-OFF] Bug 7067: Followup FIX QA fails
[SIGNED-OFF] Bug 7067: Followup FIX merge updatedatabase
Bug 7067 [1] OPAC Borrower Self Registration
Bug 7067 [2] OPAC Borrower Self Registration - Followup
Bug 7067 [3] [Follow-up: templates] allow patron self registration via the opac
Bug 7067 [4] Followup FIX QA fails
Bug 7067 [5] Followup FIX merge updatedatabase
Bug 7067 - OPAC Borrower Self Registration
Bug 7067 - OPAC Borrower Self Registration - Followup
Bug 7067 [Follow-up: templates] allow patron self registration via the opac
Bug 7067: Followup FIX QA fails
Bug 7067 [5] Followup FIX merge updatedatabase
Bug 7067 [6] - QA Followup
Bug 7067 [7] - QA Followup 2
Bug 7067 - OPAC Borrower Self Registration
Bug 7067 [Follow-up: templates] allow patron self registration via the opac
Bug 7067: Followup FIX QA fails
Bug 7067 Followup: FIX merge updatedatabase
Bug 7067 QA Followup
Bug 7067 QA Followup - Add descriptions to tests
Bug 7067 - QA Followup - Fix failing tests
Bug 7067 - Fix bad url in breadcrumbs
Bug 7067 - QA Followup - Fix error when confirming via email
no username
Bug 7067 - Followup - Change 'mobile phone' to 'other phone', run typed in captcha through uc before comparing.
Bug 7067 - Followup - Add note to circulation.pl if borrower has pending modifications
Bug 7067 - Followup - Add note to circulation.pl if borrower has pending modifications
Bug 7067 - Followup - Change 'mobile phone' to 'other phone', run typed in captcha through uc before comparing.
Bug 7067 - Followup - Add note to circulation.pl if borrower has pending modifications

Description Nicole C. Engard 2011-10-19 18:07:15 UTC
I thought there was a report for this, but I don't see it (feel free to mark as a duplicate).

It would be nice if the patron could register for a card via the OPAC.
Comment 1 Owen Leonard 2012-02-08 16:49:03 UTC
Moving this discussion from Bug 1153:

(In reply to comment #3)
> That sounds like an excellent idea. We could even have printable temporary
> library cards! One issue: my library system requires a piece of mail to verify
> a home address. What do other libraries require? Should it include the ability
> to submit scanned documents, or should just let you fill out your data, bring
> your documents in and let a librarian verify and complete the process.
> 
> (In reply to comment #2)
> > I think we have a great opportunity to integrate and update to this system with
> > a system for online patron registration: Both could potentially use the same
> > table to store information which would require the librarian to validate and
> > approve.

I think the idea merits a wider discussion but I would propose:

- Allow, or not, the user to submit a new registration online.

 - Allow, or not, the online registration to become fully active
   automatically. If not:

  - Allow, or not, that online registration to give the user "probationary"
    privileges in the OPAC and for the purposes of SIP authentication. In
    other words, the patron can place holds, but the library might require
    documentation upon pickup. Or the patron might be able to access
    electronic resources which are authenticated against Koha's patron table.

In the latter two cases Koha would need to email a response to the user so that they could activate their account (and so that we could validate the email). A cron job could be set up to delete accounts which hadn't been activated after a certain number of days.
Comment 2 Liz Rea 2012-02-08 17:22:41 UTC
(In reply to comment #1)
> Moving this discussion from Bug 1153:
> 
> (In reply to comment #3)
> > That sounds like an excellent idea. We could even have printable temporary
> > library cards! One issue: my library system requires a piece of mail to verify
> > a home address. What do other libraries require? Should it include the ability
> > to submit scanned documents, or should just let you fill out your data, bring
> > your documents in and let a librarian verify and complete the process.
> > 
> > (In reply to comment #2)
> > > I think we have a great opportunity to integrate and update to this system with
> > > a system for online patron registration: Both could potentially use the same
> > > table to store information which would require the librarian to validate and
> > > approve.
> 
> I think the idea merits a wider discussion but I would propose:
> 
> - Allow, or not, the user to submit a new registration online.
> 
>  - Allow, or not, the online registration to become fully active
>    automatically. If not:
> 
>   - Allow, or not, that online registration to give the user "probationary"
>     privileges in the OPAC and for the purposes of SIP authentication. In
>     other words, the patron can place holds, but the library might require
>     documentation upon pickup. Or the patron might be able to access
>     electronic resources which are authenticated against Koha's patron table.

Maybe a new hard-coded patron category type of "provisional" that you can then assign any patron category code?

Category codes in "provisional" have special rules that can be granularly defined:

* can place holds (x number of holds, or not at all)
* can authenticate via SIP (for x period, or not at all)
* can circulate (x times, or not at all)
* can only circulate items (not) of type x (say, you can't check out new dvd's as a provisional patron - how rude, but I can so see libraries doing it).
* Alert staff every visit to any patron info page *this is a provisional patron - pursue creating permanent patron!*

Additional functionality:
* Crons to expire and/or remove provisional patrons, or at least report them out so they can be manually pursued.

> 
> In the latter two cases Koha would need to email a response to the user so that
> they could activate their account (and so that we could validate the email). A
> cron job could be set up to delete accounts which hadn't been activated after a
> certain number of days.

I like the idea of at least requiring a valid email address and forcing activation of the account by clicking a link from the email. It would have to be optional, of course.

Practically speaking, I think this fits under Patron Categories administration.
Comment 3 Paul Poulain 2012-02-10 13:55:16 UTC
Progilone did something about that for BULAC = http://catalogue.bulac.fr/cgi-bin/koha/opac-memberentry.pl

We've sent a mail to them asking for opening the source.
Let's wait for their feedback. If something is proposed, i'm for including it and improving the behaviour with options like "pre-registering" or "full registering".
( I agree the minimum is a syspref to say "enable" or "do not enable" self-registration )

I remove the "in discussion status" and set to ASSIGNED until we've some feedback from progilone (i've added fridolyn to this entry)
Comment 4 Fridolin Somers 2012-02-10 14:48:00 UTC
Hie,
Sure it will be done soon.

Note that for this self-registration, a special branch code has been choosen to block the user's circulation untill he is really regritrated (by paying and showing his cards).
So the functionality is more a "pre"-registration online.
Comment 5 James Ghiorzi 2012-03-02 00:46:21 UTC
(In reply to comment #4)
> Hie,
> Sure it will be done soon.
> Note that for this self-registration, a special branch code has been choosen to
> block the user's circulation untill he is really regritrated (by paying and
> showing his cards).
> So the functionality is more a "pre"-registration online.

Hello sorry to bother you folks,

but how did you manage to create the "pre"-registration form.  did you edit the staff client page or did you create a separate perl page with hooks into the database/koha?

Thanks so much,

James Ghiorzi
Library Assistant II
San Benito County Free Library
470 Fifth St.
Hollister, CA 95023
(831) 636-4107
Comment 6 Fridolin Somers 2012-03-02 08:08:00 UTC
Hie,
We created a new Perl page in OPAC based on Intranet patron creation page.
It allows account creation for all OPAC signed-in functions (lists, history) but not circulation (checkouts, holds).
For security, we added a Captcha system and server-side controls.

Whe also added a page for lost password but it is doesn't reset the password.
Comment 7 Magnus Enger 2012-03-28 13:06:20 UTC
Is anyone thinking/working on getting this into Koha?
Comment 8 Paul Poulain 2012-03-28 13:22:22 UTC
(In reply to comment #7)
> Is anyone thinking/working on getting this into Koha?

I know for sure that BULAC has this feature. They have progilone as support company, and Corinne Hayet (that I just added in cc: of this bug) was at the hackfest, and told us she was responsible of managing contributing all what has been made for official Koha.

so, corinne, if you want to jump in the discussion, you're more than welcomed, you're hoped ;-)
Comment 9 koala 2012-03-29 13:13:44 UTC
(In reply to comment #7)
> Is anyone thinking/working on getting this into Koha?

yes, we are. We do not allow patrons to check out books, so the self-registration would be great.
Comment 10 Owen Leonard 2012-07-25 11:48:31 UTC
*** Bug 8502 has been marked as a duplicate of this bug. ***
Comment 11 koala 2012-08-01 12:59:38 UTC
I have simply created a html and php page that speaks with the db. 
It would have been better if it was in perl and in the koha package, but I need to find some time to get more used with the koha implementation.
Regardless, if you'd like, I could share the info here, i.e. the html and php pages.
Cheers
Comment 12 Owen Leonard 2012-08-03 13:22:34 UTC
This is sponsored now? Is there an RFC? Has there been communication with the library which has already implemented this feature?
Comment 13 Kyle M Hall 2012-08-03 13:53:36 UTC
(In reply to comment #12)
> This is sponsored now? Is there an RFC? Has there been communication with
> the library which has already implemented this feature?

Yes, http://wiki.koha-community.org/wiki/Online_Borrower_Registration_RFC , and no.

Hope this helps.
Comment 14 Paul Poulain 2012-08-03 13:56:54 UTC
(In reply to comment #12)
> This is sponsored now? Is there an RFC? Has there been communication with
> the library which has already implemented this feature?

I had some discussion with BULAC some weeks ago. They still plan to share the code of what they did, but not before the end of the year or something like that. Progilone does not the job without BULAC explicit request, and Corinne is in vacation until end of august
Comment 15 Kyle M Hall 2012-09-04 14:58:06 UTC Comment hidden (obsolete)
Comment 16 Kyle M Hall 2012-09-04 15:04:53 UTC
Test Plan:
1) Enable PatronSelfRegistration
2) Set PatronSelfRegistrationExpireTemporaryAccountsDelay to a number of days
3) Create a self-registered borrower category
4) Set PatronSelfRegistrationUseTemporaryStatus
5) Set PatronSelfRegistrationVerifyByEmail to "Don't require"
6) Go to OPAC, log out if logged in.
7) You should see the "Register here" link below the login box
8) Attempt to register yourself
9) Verify you can log in with your temporary password.
10) Set PatronSelfRegistrationVerifyByEmail to "Require"
11) Attempt another self-registration
12) Check the messages table, you should see a new message with a verification link.
13) Copy and paste the link into a web browser to verify the registration
14) Log in with the given credentials to verify the account was created.
Comment 17 Kyle M Hall 2012-09-04 15:09:16 UTC Comment hidden (obsolete)
Comment 18 Kyle M Hall 2012-09-04 15:14:09 UTC
Test Plan - Part 2 - Borrower Modifications

1) Log in to OPAC, go to "my personal details" tab.
2) Make some modifications to your details.
3) Repeat steps 1 and 2 for two more borrowers.
4) Log in to Koha intranet with a user that can modify borrowers.
5) At the bottom of mainpage.pl, you should see:
  Patrons requesting modifications: 3
6) Click the link
7) Approve one change, deny a different one, and ignore the third, then submit.
8) Check the records, you should see the changes take affect on the approved one, and no changes to the other two. You should also see "Patrons requesting modifications: 1" at the bottom of mainpage.pl now.
Comment 19 Kyle M Hall 2012-09-04 15:21:23 UTC Comment hidden (obsolete)
Comment 20 Kyle M Hall 2012-09-04 15:49:21 UTC
I forgot to add, the system pref OPACBaseURL will need to be filled out if you are using the email verification.
Comment 21 Kyle M Hall 2012-09-04 16:36:04 UTC Comment hidden (obsolete)
Comment 22 Owen Leonard 2012-09-04 16:36:51 UTC
I'm still thinking it through, but I think maybe it would be good to have a separate preference for defining what fields are required for self-registration.
Comment 23 Kyle M Hall 2012-09-04 16:40:41 UTC Comment hidden (obsolete)
Comment 24 Kyle M Hall 2012-09-04 16:43:35 UTC Comment hidden (obsolete)
Comment 25 Kyle M Hall 2012-09-04 16:52:53 UTC Comment hidden (obsolete)
Comment 26 Kyle M Hall 2012-09-04 18:11:41 UTC Comment hidden (obsolete)
Comment 27 Kyle M Hall 2012-09-04 18:21:19 UTC
You are right. I was planning for that, then I forgot to add the prefs!

Kyle

(In reply to comment #22)
> I'm still thinking it through, but I think maybe it would be good to have a
> separate preference for defining what fields are required for
> self-registration.
Comment 28 Jonathan Druart 2012-09-05 10:00:31 UTC
Hy Kyle,
Here some remarks after a little test:
- A new dependency (String::Random) is required and has to be added to the C4/Installer/PerlDependencies.pm file

- The page opac-user.pl does not have a register link.

- There is no verification on the date of birth entry and could be caused a warning

- After sending the form, I get an error in my log DBD::mysql::st execute failed: Column 'user' cannot be null". But I don't know where does it come.

- I can't continue after the step "13) Copy and paste the link into a web browser to verify the registration". Indeed, there are a lot of error in the page opac/opac-registration-verify.pl (variables not defined, etc.)

+ The qa tools say me a lot of failed qa points exist:

* misc/cronjobs/delete_expired_opac_registrations.pl                       FAIL
	critic                      FAIL
		"require" statement with library name as string at line 28, column 12. Use a bareword instead.
		Code before strictures are enabled at line 23, column 1. See page 429 of PBP.

* misc/cronjobs/delete_unverified_opac_registrations.pl                    FAIL
	critic                      FAIL
		"require" statement with library name as string at line 28, column 12. Use a bareword instead.
		Code before strictures are enabled at line 23, column 1. See page 429 of PBP.

* opac/opac-registration-verify.pl                                         FAIL
	valid                       FAIL
		Global symbol "$borrowernumber" requires explicit package name at opac/opac-registration-verify.pl line 58.
		Global symbol "$cookie" requires explicit package name at opac/opac-registration-verify.pl line 35.
		Global symbol "%borrower" requires explicit package name at opac/opac-registration-verify.pl line 52.
		Global symbol "$borrowernumber" requires explicit package name at opac/opac-registration-verify.pl line 35.
		Global symbol "$template" requires explicit package name at opac/opac-registration-verify.pl line 59.
		Global symbol "$cookie" requires explicit package name at opac/opac-registration-verify.pl line 69.
		opac/opac-registration-verify.pl had compilation errors.
		Global symbol "$template" requires explicit package name at opac/opac-registration-verify.pl line 44.
		Bareword "token" not allowed while "strict subs" in use at opac/opac-registration-verify.pl line 47.
		Global symbol "$template" requires explicit package name at opac/opac-registration-verify.pl line 69.
		Global symbol "$template" requires explicit package name at opac/opac-registration-verify.pl line 56.
		Global symbol "$template" requires explicit package name at opac/opac-registration-verify.pl line 35.
		Global symbol "$borrowernumber" requires explicit package name at opac/opac-registration-verify.pl line 52.
		Global symbol "$template" requires explicit package name at opac/opac-registration-verify.pl line 57.
Comment 29 Kyle M Hall 2012-09-05 11:50:44 UTC Comment hidden (obsolete)
Comment 30 Jonathan Druart 2012-09-05 12:21:36 UTC
Step 9 fails (Verify you can log in with your temporary password.)

I got:
Username:
Password: sHGrskYvtx 

with an error in my log:
 DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_coderun`.`borrowers`, CONSTRAINT `borrowers_ibfk_1` FOREIGN KEY (`categorycode`) REFERENCES `categories` (`categorycode`)) at /home/koha/C4/SQLHelper.pm line 184., referer: http://opac.koha.localhost/cgi-bin/koha/opac-memberentry.pl
[Wed Sep 05 14:17:55 2012] [error] [client 127.0.0.1] [Wed Sep  5 14:17:55 2012] opac-memberentry.pl: DBD::mysql::st execute failed: Column 'user' cannot be null at /home/koha/C4/Log.pm line 79., referer: http://opac.koha.localhost/cgi-bin/koha/opac-memberentry.pl

I filled all fields into the form.


And - The page opac-user.pl does not have a register link.
Comment 31 Kyle M Hall 2012-09-05 13:43:13 UTC Comment hidden (obsolete)
Comment 32 Kyle M Hall 2012-09-05 13:46:27 UTC
> - A new dependency (String::Random) is required and has to be added to the
> C4/Installer/PerlDependencies.pm file

Fixed.

> - The page opac-user.pl does not have a register link.

Fixed

> - There is no verification on the date of birth entry and could be caused a
> warning

Good catch. I told myself I would take care of it later and then forgot ; )

> 
> * misc/cronjobs/delete_expired_opac_registrations.pl                      
> FAIL
> 	critic                      FAIL
> 		"require" statement with library name as string at line 28, column 12. Use
> a bareword instead.
> 		Code before strictures are enabled at line 23, column 1. See page 429 of
> PBP.
> 
> * misc/cronjobs/delete_unverified_opac_registrations.pl                   
> FAIL
> 	critic                      FAIL
> 		"require" statement with library name as string at line 28, column 12. Use
> a bareword instead.
> 		Code before strictures are enabled at line 23, column 1. See page 429 of
> PBP.

These are not valid. I use 'use Modern::Perl' which enables both warnings and strictures. The rest of the errors you saw should be fixed.
Comment 33 Jonathan Druart 2012-09-05 14:55:52 UTC
Thanks for your work !

Some remarks again :

1/ I still have the sql error on submitting form. It seems caused by the action_log constraint:
DBD::mysql::st execute failed: Column 'user' cannot be null at /home/koha/C4/Log.pm line 79., referer: http://opac.koha.localhost/cgi-bin/koha/opac-memberentry.pl

2/ > > - There is no verification on the date of birth entry and could be caused a
> > warning
> 
> Good catch. I told myself I would take care of it later and then forgot ; )
I don't know if it is configurable (in the jquery plugin) but now it's not possible to select a year < 2002 with just one click :-/

3/ on opac-user.pl:
"If you don't have a library card, stop by your local library to sign up or register here.. "
There is 2 dots on the end of the line.

4/ Strange behaviour:
modify your personnal details to replace a value with an other.
Resubmit a change but replacing a value with an empty string.
At intranet, you see an empty table (just with headers) in the 'changes' column.


> These are not valid. I use 'use Modern::Perl' which enables both warnings
> and strictures. The rest of the errors you saw should be fixed.
Your first patch did not have any module use ;)


This patch does not allow a librarian to validate or reject a self registration isn't it ?
Comment 34 Jonathan Druart 2012-09-05 14:59:42 UTC
Arg, please let me edit my posts! :) (I am sorry about my grammar...)
Comment 35 Jonathan Druart 2012-09-05 15:07:38 UTC
(In reply to comment #33)
> This patch does not allow a librarian to validate or reject a self
> registration isn't it ?

Auto answer: In fact yes. In the issuing rules, we can restrict issues for the corresponding patron category.
Like that we can simulate a pre-registration.
A user is fully registrated when he has a "real" patron category.
Comment 36 Kyle M Hall 2012-09-06 11:30:57 UTC
(In reply to comment #35)
> (In reply to comment #33)
> > This patch does not allow a librarian to validate or reject a self
> > registration isn't it ?
> 
> Auto answer: In fact yes. In the issuing rules, we can restrict issues for
> the corresponding patron category.
> Like that we can simulate a pre-registration.
> A user is fully registrated when he has a "real" patron category.

That is exactly what I was going for. They idea is to create a TEMP patron category and when the patron comes in to the library, the patron can show whatever proof they need to and a library can change his or her patron category accordingly.
Comment 37 Kyle M Hall 2012-09-06 12:34:10 UTC Comment hidden (obsolete)
Comment 38 Kyle M Hall 2012-09-06 12:40:50 UTC
> 1/ I still have the sql error on submitting form. It seems caused by the
> action_log constraint:
> DBD::mysql::st execute failed: Column 'user' cannot be null at
> /home/koha/C4/Log.pm line 79., referer:
> http://opac.koha.localhost/cgi-bin/koha/opac-memberentry.pl

Fixed. It was a minor semantic error in Log.pm. It would get a borrowernumber from env if env was a hash, but wasn't bothering to check if there was actually a borrowernumber stored in env!
 
> 2/ > > 
> I don't know if it is configurable (in the jquery plugin) but now it's not
> possible to select a year < 2002 with just one click :-/

Another good catch! I've fixed it so it displays the last 120 years. That should far enough back for 99.999% of people.

> 3/ on opac-user.pl:
> "If you don't have a library card, stop by your local library to sign up or
> register here.. "
> There is 2 dots on the end of the line.

Fixed.

> 4/ Strange behaviour:
> modify your personnal details to replace a value with an other.
> Resubmit a change but replacing a value with an empty string.
> At intranet, you see an empty table (just with headers) in the 'changes'
> column.

Also fixed.

> 
> > These are not valid. I use 'use Modern::Perl' which enables both warnings
> > and strictures. The rest of the errors you saw should be fixed.
> Your first patch did not have any module use ;)

Did not realize that! Better late than never!

> 
> This patch does not allow a librarian to validate or reject a self
> registration isn't it ?

No it doesn't, that is meant to be 'interactive' where the patron comes in to the library with whatever documentation is necessary for membership.
Comment 39 Kyle M Hall 2012-09-06 12:41:04 UTC Comment hidden (obsolete)
Comment 40 Jonathan Druart 2012-09-06 14:33:09 UTC Comment hidden (obsolete)
Comment 41 Owen Leonard 2012-09-06 14:34:09 UTC
A point for discussion: I think the registration confirmation messages should come from the email address of the library the user chose as their home library. The counter-argument is that the user may not have chosen the correct library, but I don't see that as much of a problem since either way the reply would be coming from an email address which wasn't their library's.

The big issue I'm still seeing: New records are saved with NULL expiration date and NULL registration date. Will the delete_expired_opac_registrations cron job work with a NULL dateenrolled?

Some small issues:

- I think a note about pending patron modifications should appear on 
  the patrons page (members/members-home.pl) in addition to the staff
  client home page, just as pending tag/comment/suggestions also show
  as pending on the Tools page.

- Bug 8593 added unique IDs to the other "pending" notes. This patch
  should include one for the line it adds.

- members/members-update.pl needs a descriptive page title and page
  header. If there are no pending operations it should display a message
  saying so rather than an empty table.

- The table markup on members/members-update.pl is missing a </tr>,
  causing validation errors.

- I think the random-character verification field should be styled like 
  other required fields

- Since the datepicker widget doesn't make it easy to pick a date far in
  the past (as are birthdays for some of us), I don't think the field 
  should be read-only.

- I think opac-memberentry-update-submitted.tt should include the user
  sidebar menu.

- I see an error in the log when submitting the registration form in 
  the OPAC: 

substr outside of string at /home/oleonard/kohaclone/opac/opac-memberentry.pl line 239

If PatronSelfRegistrationExpireTemporaryAccountsDelay is set to "0" does that mean never? If so the description should say so.
Comment 42 Jonathan Druart 2012-09-06 14:37:55 UTC
Ho, collision :) So I don't switch to signed off
Comment 43 Katrin Fischer 2012-09-06 14:41:01 UTC
Hm, only a note, but isn't PatronSelfRegistrationExpireTemporaryAccountsDelay a bit montrous for a syspref name?
Comment 44 Kyle M Hall 2012-09-06 15:43:12 UTC
(In reply to comment #43)
> Hm, only a note, but isn't
> PatronSelfRegistrationExpireTemporaryAccountsDelay a bit montrous for a
> syspref name?

Yes, yes it is ; )

But it was the shortest version that would describe it correctly.
Comment 45 Kyle M Hall 2012-09-06 15:48:35 UTC
(In reply to comment #41)
> A point for discussion: I think the registration confirmation messages
> should come from the email address of the library the user chose as their
> home library. The counter-argument is that the user may not have chosen the
> correct library, but I don't see that as much of a problem since either way
> the reply would be coming from an email address which wasn't their library's.

I suppose a followup for that would not be too much trouble.

> The big issue I'm still seeing: New records are saved with NULL expiration
> date and NULL registration date. Will the delete_expired_opac_registrations
> cron job work with a NULL dateenrolled?

This should be fixed in the current version of the patch.

> Some small issues:
> 
> - I think a note about pending patron modifications should appear on 
>   the patrons page (members/members-home.pl) in addition to the staff
>   client home page, just as pending tag/comment/suggestions also show
>   as pending on the Tools page.

I'll take a look and add a followup for that.

> 
> - Bug 8593 added unique IDs to the other "pending" notes. This patch
>   should include one for the line it adds.

I'll have to look into that.

> - members/members-update.pl needs a descriptive page title and page
>   header. If there are no pending operations it should display a message
>   saying so rather than an empty table.

I suppose we can do that, but if there are no pending operations, the link for the page disappears, so it is assumed no one will visit it when there are no pending operations.

> - The table markup on members/members-update.pl is missing a </tr>,
>   causing validation errors.

I'll look at it.

> - I think the random-character verification field should be styled like 
>   other required fields

Good idea.

> - Since the datepicker widget doesn't make it easy to pick a date far in
>   the past (as are birthdays for some of us), I don't think the field 
>   should be read-only.

I suppose we can do that. I'm a bit iffy about that one though.

> - I think opac-memberentry-update-submitted.tt should include the user
>   sidebar menu.

I'll see what I can do about that.

> - I see an error in the log when submitting the registration form in 
>   the OPAC: 
> 
> substr outside of string at
> /home/oleonard/kohaclone/opac/opac-memberentry.pl line 239

I'll take a look at that too.
 
> If PatronSelfRegistrationExpireTemporaryAccountsDelay is set to "0" does
> that mean never? If so the description should say so.

That is correct. I'll have to add it to the description.
Comment 46 Owen Leonard 2012-09-06 16:00:28 UTC
(In reply to comment #45)
> > The big issue I'm still seeing: New records are saved with NULL expiration
> > date and NULL registration date. Will the delete_expired_opac_registrations
> > cron job work with a NULL dateenrolled?
> 
> This should be fixed in the current version of the patch.

I don't find that to be the case.

Another issue, which came up on IRC but should be noted here as well: The registration confirmation messages are sent to the message queue, but the message queue might not be processed in a timely manner. Others on IRC said theirs were set to process as infrequently as once an hour. I think a delay as long as one hour is contrary to the expectations many users will have based on other web applications. As a result I think repeated self-registrations are likely as people assume theirs didn't work.

I think this is a pretty serious concern.
Comment 47 Owen Leonard 2012-09-06 18:06:32 UTC
Another issue: The note about Patrons requesting modifications on the staff client home page does not show up if you are not a superlibrarian. The permissions check needs to be added to the conditional which controls the display of all pending operations (line 101 of intranet-main.tt).
Comment 48 Owen Leonard 2012-09-06 18:10:48 UTC
If the "IndependantBranches" preference is turned on it should not be possible for a librarian from Library A to approve changes to the account of a patron with branchcode = Library B.
Comment 49 Owen Leonard 2012-09-06 18:22:08 UTC
(In reply to comment #45)
> > If there are no pending operations it should display a message
> > saying so rather than an empty table.
> 
> I suppose we can do that, but if there are no pending operations, the link
> for the page disappears, so it is assumed no one will visit it when there
> are no pending operations.

Actually as I work with it more and test leaving some patron modifications to "ignore," it makes sense to me to have this page redirect back to itself rather than the home page. If some lines had been set to "ignore" then they re-display. If all are complete it shows a message that the queue is empty.
Comment 50 Kyle M Hall 2012-09-10 11:51:31 UTC
That makes a lot of sense. I'll add a followup for that.

Kyle

(In reply to comment #49)
> (In reply to comment #45)
> > > If there are no pending operations it should display a message
> > > saying so rather than an empty table.
> > 
> > I suppose we can do that, but if there are no pending operations, the link
> > for the page disappears, so it is assumed no one will visit it when there
> > are no pending operations.
> 
> Actually as I work with it more and test leaving some patron modifications
> to "ignore," it makes sense to me to have this page redirect back to itself
> rather than the home page. If some lines had been set to "ignore" then they
> re-display. If all are complete it shows a message that the queue is empty.
Comment 51 Kyle M Hall 2012-09-10 12:06:53 UTC Comment hidden (obsolete)
Comment 52 Owen Leonard 2012-09-10 13:15:37 UTC
Another major bug: If I have PatronSelfRegistration turned off, clicking the "my personal details" page redirects to the OPAC home page. There needs to be several working combinations:

- PatronSelfRegistration is ON, OPACPatronDetails is ON:

   New patrons can sign up on the OPAC, existing patrons can edit their
   details via the entry form.

- PatronSelfRegistration is ON, OPACPatronDetails is OFF:

   New patrons can sign up on the OPAC but existing patrons can't
   edit their details via the entry form. [This option may not make
   much logical sense but since we can't link two preferences...]

- PatronSelfRegistration is OFF, OPACPatronDetails is ON:

   New patrons can't sign up on the OPAC but existing patrons can
   edit their details via the entry form.

- PatronSelfRegistration is OFF, OPACPatronDetails is OFF:

   Patrons can only view their details on the OPAC.

Jonathan I hope you don't mind I'm removing the "signed off" status. I think there are too many issues with this for it to move forward just yet.
Comment 53 Jonathan Druart 2012-09-10 13:26:27 UTC
(En réponse au commentaire 52)
> Jonathan I hope you don't mind I'm removing the "signed off" status. I think
> there are too many issues with this for it to move forward just yet.

No problem Owen :)
If there are problems with this patch, it is normal to passed it as failed qa.
Comment 54 Kyle M Hall 2012-09-10 15:46:27 UTC Comment hidden (obsolete)
Comment 55 Kyle M Hall 2012-09-10 16:27:47 UTC Comment hidden (obsolete)
Comment 56 Kyle M Hall 2012-09-10 16:28:09 UTC
This new followup should address all the issues that Owen has brought up.
Comment 57 Owen Leonard 2012-09-10 17:04:51 UTC
After the latest patch I find that if PatronSelfRegistration is ON and OPACPatronDetails is OFF I can submit a new registration. The check for OPACPatronDetails on the entry form disables the form.

Turning OPACPatronDetails ON, I find that submitting changes to an existing patron record and approving them in the staff client results in NO changes to the patron record.

Now when I submit a new patron registration it appears to work but the username field isn't pre-filled in the login form. In the log I see errors:

- opac-memberentry.pl: Use of uninitialized value in pattern match (m//) at /home/oleonard/kohaclone/C4/SQLHelper.pm line 418., referer: http://localhost:88

- opac-memberentry.pl: DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha`.`borrowers`, CONSTRAINT `borrowers_ibfk_1` FOREIGN KEY (`categorycode`) REFERENCES `categories` (`categorycode`)) at /home/oleonard/kohaclone/C4/SQLHelper.pm line 168., referer: http://localhost:88/cgi-bin/koha/opac-memberentry.pl


Other issues which still need to be addressed:

> - The table markup on members/members-update.pl is missing a </tr>,
>   causing validation errors.
 
> - I think the random-character verification field should be styled like 
>   other required fields
 
> - Since the datepicker widget doesn't make it easy to pick a date far in
>   the past (as are birthdays for some of us), I don't think the field 
>   should be read-only.

> - I see an error in the log when submitting the registration form in 
>   the OPAC: 
> 
> substr outside of string at
> /home/oleonard/kohaclone/opac/opac-memberentry.pl line 239
 
Also: Any thoughts on the message queue issue?
Comment 58 Kyle M Hall 2012-09-10 18:07:35 UTC Comment hidden (obsolete)
Comment 59 Kyle M Hall 2012-09-10 18:11:21 UTC
(In reply to comment #57)
> After the latest patch I find that if PatronSelfRegistration is ON and
> OPACPatronDetails is OFF I can submit a new registration. The check for
> OPACPatronDetails on the entry form disables the form.

Doh! Fixed.

> Turning OPACPatronDetails ON, I find that submitting changes to an existing
> patron record and approving them in the staff client results in NO changes
> to the patron record.

Caused by a bug in master. Fixed.

> Now when I submit a new patron registration it appears to work but the
> username field isn't pre-filled in the login form. In the log I see errors:
> 
> - opac-memberentry.pl: Use of uninitialized value in pattern match (m//) at
> /home/oleonard/kohaclone/C4/SQLHelper.pm line 418., referer:
> http://localhost:88
> 
> - opac-memberentry.pl: DBD::mysql::st execute failed: Cannot add or update a
> child row: a foreign key constraint fails (`koha`.`borrowers`, CONSTRAINT
> `borrowers_ibfk_1` FOREIGN KEY (`categorycode`) REFERENCES `categories`
> (`categorycode`)) at /home/oleonard/kohaclone/C4/SQLHelper.pm line 168.,
> referer: http://localhost:88/cgi-bin/koha/opac-memberentry.pl

I'm not getting this error. Please retest with the new followup. Either I fixed it incidentally, or it's a different in configuration perhaps?

> 
> Other issues which still need to be addressed:
> 

I know I had fixed these, the followup must have gone awol!

> > - The table markup on members/members-update.pl is missing a </tr>,
> >   causing validation errors.

Fixed

> > - I think the random-character verification field should be styled like 
> >   other required fields

Fixed

> > - Since the datepicker widget doesn't make it easy to pick a date far in
> >   the past (as are birthdays for some of us), I don't think the field 
> >   should be read-only.

Fixed
 
> > - I see an error in the log when submitting the registration form in 
> >   the OPAC: 
> > 
> > substr outside of string at
> > /home/oleonard/kohaclone/opac/opac-memberentry.pl line 239

Fixed
  
> Also: Any thoughts on the message queue issue?

Is there any good reason the process_message_queue cron can't be run every minute? We need a "send it now" function, but we cannot until the email configuration is pulled out of the cron script parameters and put into Koha system preferences.
Comment 60 Galen Charlton 2012-09-10 18:29:40 UTC
(In reply to comment #59)
> Is there any good reason the process_message_queue cron can't be run every
> minute? We need a "send it now" function, but we cannot until the email
> configuration is pulled out of the cron script parameters and put into Koha
> system preferences.

There's no reason not to have the cronjob run once a minute, but if we start recommending that configuration, it will need to be patched to use a lockfile so that it doesn't allow simultaneous jobs to run if it takes longer than a minute to clear the message queue.
Comment 61 Owen Leonard 2012-09-10 18:41:12 UTC
(In reply to comment #59)
> > - opac-memberentry.pl: Use of uninitialized value in pattern match (m//) at
> > /home/oleonard/kohaclone/C4/SQLHelper.pm line 418., referer:
> > http://localhost:88
> > 
> > - opac-memberentry.pl: DBD::mysql::st execute failed: Cannot add or update a
> > child row: a foreign key constraint fails (`koha`.`borrowers`, CONSTRAINT
> > `borrowers_ibfk_1` FOREIGN KEY (`categorycode`) REFERENCES `categories`
> > (`categorycode`)) at /home/oleonard/kohaclone/C4/SQLHelper.pm line 168.,
> > referer: http://localhost:88/cgi-bin/koha/opac-memberentry.pl


I figured it out: I cleared the PatronSelfRegistration preferences for testing and didn't put in a patron category for PatronSelfRegistrationUseTemporaryStatus. How might we deal with this possible omission?
Comment 62 Owen Leonard 2012-09-10 18:48:02 UTC
New issue: Submitting a change to an existing patron's home library works in that approving the change will correctly update the record, but the line doesn't show up in the list of fields which will be updated on members/members-update.pl.
Comment 63 Kyle M Hall 2012-09-11 11:57:23 UTC
What we really need is a 'system preference validator' in Koha. I recall there are other preference dependencies, so it would not only be useful for this.

We could just add a warning to mainpage.pl for these things instead. So if PatronSelfRegistration is enabled, but PatronSelfRegistrationUseTemporaryStatus is empty, a warning will be written out on mainpage.pl.

At the moment, these issues are only covered by documentation.

Kyle

(In reply to comment #61)
> (In reply to comment #59)
> > > - opac-memberentry.pl: Use of uninitialized value in pattern match (m//) at
> > > /home/oleonard/kohaclone/C4/SQLHelper.pm line 418., referer:
> > > http://localhost:88
> > > 
> > > - opac-memberentry.pl: DBD::mysql::st execute failed: Cannot add or update a
> > > child row: a foreign key constraint fails (`koha`.`borrowers`, CONSTRAINT
> > > `borrowers_ibfk_1` FOREIGN KEY (`categorycode`) REFERENCES `categories`
> > > (`categorycode`)) at /home/oleonard/kohaclone/C4/SQLHelper.pm line 168.,
> > > referer: http://localhost:88/cgi-bin/koha/opac-memberentry.pl
> 
> 
> I figured it out: I cleared the PatronSelfRegistration preferences for
> testing and didn't put in a patron category for
> PatronSelfRegistrationUseTemporaryStatus. How might we deal with this
> possible omission?
Comment 64 Kyle M Hall 2012-09-11 12:13:10 UTC Comment hidden (obsolete)
Comment 65 Kyle M Hall 2012-09-11 12:25:07 UTC
This should be fixed in my new followup.

(In reply to comment #62)
> New issue: Submitting a change to an existing patron's home library works in
> that approving the change will correctly update the record, but the line
> doesn't show up in the list of fields which will be updated on
> members/members-update.pl.
Comment 66 Owen Leonard 2012-09-11 13:30:43 UTC
(In reply to comment #63)

> We could just add a warning to mainpage.pl for these things instead. So if
> PatronSelfRegistration is enabled, but
> PatronSelfRegistrationUseTemporaryStatus is empty, a warning will be written
> out on mainpage.pl.

Maybe we could make display of the "register" link in the OPAC dependent on PatronSelfRegistrationUseTemporaryStatus being defined?

And now that I think about it, shouldn't this preference be "PatronSelfRegistrationDefaultCategory" ? It's required, so it's not as if we're saying "use this temporary category if you want to."
Comment 67 Kyle M Hall 2012-09-11 14:28:54 UTC
(In reply to comment #66)
> (In reply to comment #63)
> 
> > We could just add a warning to mainpage.pl for these things instead. So if
> > PatronSelfRegistration is enabled, but
> > PatronSelfRegistrationUseTemporaryStatus is empty, a warning will be written
> > out on mainpage.pl.
> 
> Maybe we could make display of the "register" link in the OPAC dependent on
> PatronSelfRegistrationUseTemporaryStatus being defined?
> 
> And now that I think about it, shouldn't this preference be
> "PatronSelfRegistrationDefaultCategory" ? It's required, so it's not as if
> we're saying "use this temporary category if you want to."

That is a good idea, and a good rename as well.
Comment 68 Kyle M Hall 2012-09-11 14:43:10 UTC Comment hidden (obsolete)
Comment 69 Kyle M Hall 2012-09-11 14:51:40 UTC Comment hidden (obsolete)
Comment 70 Owen Leonard 2012-09-11 14:56:47 UTC
If you submit a new registration via the OPAC and then reload the confirmation page (whether you're asked for email confirmation or not), you get duplicate patron records. The patron who required email confirmation gets duplicate fully-valid activation links. The patron who didn't require email confirmation gets newly-generated passwords on each reload but it doesn't appear that reloaded ones are valid.
Comment 71 Kyle M Hall 2012-09-11 16:28:11 UTC Comment hidden (obsolete)
Comment 72 Kyle M Hall 2012-09-11 16:28:56 UTC
(In reply to comment #70)
> If you submit a new registration via the OPAC and then reload the
> confirmation page (whether you're asked for email confirmation or not), you
> get duplicate patron records. The patron who required email confirmation
> gets duplicate fully-valid activation links. The patron who didn't require
> email confirmation gets newly-generated passwords on each reload but it
> doesn't appear that reloaded ones are valid.

With email registration on, I'm not getting duplicate patrons. I just get the "Registration Complete" page with an empty username and password if I reload the page. That is safe, but it would be better to redirect to an 'invalid token' page instead.

When not requiring email verification, I *do* get duplicate borrowers. I'm not sure if this situation has a solution. It's no different than the user hitting back and submitting the same data again. We could redirect to a new page, rather than loading the Registration Complete page directly from opac-memberentry.pl, but that just shifts the problem. Instead of reloading creating a duplicate patron, hitting back will.

If anyone has any suggestions, please let me know. But as far as I can tell this is more of a HTTP problem than a Koha problem.
Comment 73 Kyle M Hall 2012-09-11 19:37:04 UTC Comment hidden (obsolete)
Comment 74 Kyle M Hall 2012-09-11 19:38:08 UTC Comment hidden (obsolete)
Comment 75 Owen Leonard 2012-09-12 14:22:25 UTC
(In reply to comment #74)
>  * Add invalid token page

This works great, thanks,

As for the potential duplicate patrons when not requiring email notification, I guess the upside is that delete_unverified_opac_registrations.pl job will deal with them. However, that assumes that the patron is coming in to the library to confirm their identity (or whatever procedure changes them from a temp category to a permanent one), and that the library is running that cron job. It remains a potential problem for libraries which allow patrons to register for full-fledged accounts without confirmation (however unlikely that scenario seems to me).
Comment 76 Kyle M Hall 2012-09-12 14:29:12 UTC
(In reply to comment #75)
> (In reply to comment #74)
> >  * Add invalid token page
> 
> This works great, thanks,
> 
> As for the potential duplicate patrons when not requiring email
> notification, I guess the upside is that
> delete_unverified_opac_registrations.pl job will deal with them. However,
> that assumes that the patron is coming in to the library to confirm their
> identity (or whatever procedure changes them from a temp category to a
> permanent one), and that the library is running that cron job. It remains a
> potential problem for libraries which allow patrons to register for
> full-fledged accounts without confirmation (however unlikely that scenario
> seems to me).

Agreed. Perhaps just adding a caveat in the manual should suffice.
Comment 77 Owen Leonard 2012-09-12 14:31:15 UTC
Hm, but delete_unverified_opac_registrations.pl doesn't do that does it? It deletes from borrower_modifications. But that's just for pending updates to existing patrons isn't it? Is that how it's supposed to work?

Either way, that script needs help added to it, and should not run without setting an explicit flag to proceed with deletions.
Comment 78 Kyle M Hall 2012-09-12 15:01:27 UTC
(In reply to comment #77)
> Hm, but delete_unverified_opac_registrations.pl doesn't do that does it? It
> deletes from borrower_modifications. But that's just for pending updates to
> existing patrons isn't it? Is that how it's supposed to work?
> 
> Either way, that script needs help added to it, and should not run without
> setting an explicit flag to proceed with deletions.

It doesn't touch modifications to existing borrowers. If someone submits a registration from the opac, but never clicks the verification link, that row will just sit in the borrower_modifications table forever. This script deletes rows that have been sitting in the table for more than 24 hours.

I'll add some help to it, and while I'm at it, I can make the hours part configurable.
Comment 79 Kyle M Hall 2012-09-12 15:16:11 UTC
(In reply to comment #77)
> Hm, but delete_unverified_opac_registrations.pl doesn't do that does it? It
> deletes from borrower_modifications. But that's just for pending updates to
> existing patrons isn't it? Is that how it's supposed to work?
> 
> Either way, that script needs help added to it, and should not run without
> setting an explicit flag to proceed with deletions.

I think the script you are thinking of is misc/cronjobs/delete_expired_opac_registrations.pl

That script will delete patrons with the category set in PatronSelfRegistrationDefaultCategory after the specified delay.
Comment 80 Kyle M Hall 2012-09-12 15:20:49 UTC Comment hidden (obsolete)
Comment 81 Kyle M Hall 2012-09-12 15:23:18 UTC
I guess I should note that no cron script deletes modification to existing borrowers. Those changes will sit in the borrower_modifications table until a librarian either approves or denies the changes, no matter how long that will take.
Comment 82 Owen Leonard 2012-09-12 15:44:46 UTC
Thanks for the clarifications Kyle. Unfortunately I have to ask you to rebase this again since we got a new database revision and changes to SQLHelper.pm
Comment 83 Kyle M Hall 2012-09-13 14:28:39 UTC Comment hidden (obsolete)
Comment 84 Kyle M Hall 2012-09-13 14:28:57 UTC Comment hidden (obsolete)
Comment 85 Owen Leonard 2012-09-13 15:56:13 UTC
Sorry:

Applying: Bug 7067 - OPAC Borrower Self Registration
error: patch failed: installer/data/mysql/sysprefs.sql:372
error: installer/data/mysql/sysprefs.sql: patch does not apply
Patch failed at 0001 Bug 7067 - OPAC Borrower Self Registration
Comment 86 Kyle M Hall 2012-09-13 16:19:56 UTC Comment hidden (obsolete)
Comment 87 Kyle M Hall 2012-09-13 16:20:12 UTC Comment hidden (obsolete)
Comment 88 Owen Leonard 2012-09-13 18:13:06 UTC
I'm ready to sign off on this, but I'm wondering if this issue is something which can be left to a follow-up or if it will fail QA without it:

(In reply to comment #60)
> There's no reason not to have the cronjob run once a minute, but if we start
> recommending that configuration, it will need to be patched to use a
> lockfile so that it doesn't allow simultaneous jobs to run if it takes
> longer than a minute to clear the message queue.

I have some template-related fixups but I'd like to get these patches signed off first.
Comment 89 Owen Leonard 2012-09-13 18:35:47 UTC
Oops, new small glitch: If PatronSelfRegistrationVerifyByEmail is set to "require," the Primary email field in the patron record update form in the OPAC becomes mandatory even if it's not listed in PatronSelfRegistrationBorrowerMandatoryField. If it's not listed in PatronSelfRegistrationBorrowerMandatoryField and the patron is doing an update, not a new registration, the field should not be required.
Comment 90 Kyle M Hall 2012-09-14 15:22:08 UTC Comment hidden (obsolete)
Comment 91 Owen Leonard 2012-09-14 15:45:27 UTC Comment hidden (obsolete)
Comment 92 Owen Leonard 2012-09-14 15:47:20 UTC Comment hidden (obsolete)
Comment 93 Owen Leonard 2012-09-14 15:50:32 UTC Comment hidden (obsolete)
Comment 94 Owen Leonard 2012-09-17 13:45:40 UTC Comment hidden (obsolete)
Comment 95 Kyle M Hall 2012-09-17 14:39:37 UTC Comment hidden (obsolete)
Comment 96 Jared Camins-Esakov 2012-10-06 04:15:57 UTC
Changing target version so this bug report will show up in the list of 3.12-targeted features and doesn't get lost in the shuffle during feature freeze.
Comment 97 Jonathan Druart 2012-10-25 14:29:38 UTC Comment hidden (obsolete)
Comment 98 Jonathan Druart 2012-10-25 14:29:48 UTC Comment hidden (obsolete)
Comment 99 Jonathan Druart 2012-10-25 14:29:58 UTC Comment hidden (obsolete)
Comment 100 Jonathan Druart 2012-10-25 14:47:56 UTC Comment hidden (obsolete)
Comment 101 Jonathan Druart 2012-10-25 15:14:57 UTC Comment hidden (obsolete)
Comment 102 Jonathan Druart 2012-10-25 15:17:07 UTC
Well done :) Works as expected so it passes QA !
Comment 103 Kyle M Hall 2012-10-25 15:31:17 UTC
(In reply to comment #102)
> Well done :) Works as expected so it passes QA !

Excellent! Thanks for the followups Janathan!
Comment 104 Jared Camins-Esakov 2012-11-05 19:36:55 UTC
Could someone please sign off on the last two patches? Once they've been signed off, this can be set back to "Passed QA" and I will review it.
Comment 105 Owen Leonard 2012-11-05 20:13:20 UTC Comment hidden (obsolete)
Comment 106 Owen Leonard 2012-11-05 20:19:59 UTC Comment hidden (obsolete)
Comment 107 Jonathan Druart 2012-11-07 14:41:42 UTC
(In reply to comment #104)
> Could someone please sign off on the last two patches? Once they've been
> signed off, this can be set back to "Passed QA" and I will review it.

Jared,
Sorry about that. I thought I could pass QA directly without requesting a signed off for my patch, given that it was simple modifications (merge marker, pod adjustement and tabulations characters). It was in order to avoid another SO and QA (by someone else looking at it my own patch).
I will be more respecful toward the procedure next time.
Comment 108 Jared Camins-Esakov 2012-11-07 14:48:54 UTC
(In reply to comment #107)
> (In reply to comment #104)
> > Could someone please sign off on the last two patches? Once they've been
> > signed off, this can be set back to "Passed QA" and I will review it.
> 
> Jared,
> Sorry about that. I thought I could pass QA directly without requesting a
> signed off for my patch, given that it was simple modifications (merge
> marker, pod adjustement and tabulations characters). It was in order to
> avoid another SO and QA (by someone else looking at it my own patch).
> I will be more respecful toward the procedure next time.

No problem. Since the patches meet the requirements set out by QA, I am marking this passed QA now and will review the patch.
Comment 109 Jared Camins-Esakov 2012-11-25 23:59:52 UTC
Due to a conflict on the first patch, the third patch refuses to apply at all (sha1 information lacking or useless). Owen, since you have a branch with these patches could you please rebase the branch on latest master and re-upload?
Comment 110 Owen Leonard 2012-11-28 20:24:55 UTC Comment hidden (obsolete)
Comment 111 Owen Leonard 2012-11-28 20:25:36 UTC Comment hidden (obsolete)
Comment 112 Owen Leonard 2012-11-28 20:26:26 UTC Comment hidden (obsolete)
Comment 113 Owen Leonard 2012-11-28 20:27:15 UTC Comment hidden (obsolete)
Comment 114 Owen Leonard 2012-11-28 20:27:52 UTC Comment hidden (obsolete)
Comment 115 Owen Leonard 2012-11-28 20:29:47 UTC
All patches rebased on current master.
Comment 116 Jared Camins-Esakov 2012-11-29 00:16:09 UTC
Owen, do you think you could push your branch to gitorious (or somewhere)? I still can't seem to apply this, so I'll just cherry-pick directly off your branch.
Comment 117 Jonathan Druart 2012-11-29 09:14:14 UTC
(In reply to comment #116)
> Owen, do you think you could push your branch to gitorious (or somewhere)? I
> still can't seem to apply this, so I'll just cherry-pick directly off your
> branch.

Hi Jared,
It's my fault, I proposed a dirty followup to avoid to resubmit an already signed-off patch. So the last patch just moves the new database entry to the end of the file.

I pushed the branch with a new last patch in our git repository:
http://git.biblibre.com/?p=kohac;a=shortlog;h=refs/heads/ft/Bug7067
Comment 118 Jared Camins-Esakov 2012-11-29 14:38:17 UTC Comment hidden (obsolete)
Comment 119 Jared Camins-Esakov 2012-11-29 14:38:32 UTC Comment hidden (obsolete)
Comment 120 Jared Camins-Esakov 2012-11-29 14:38:41 UTC Comment hidden (obsolete)
Comment 121 Jared Camins-Esakov 2012-11-29 14:38:49 UTC Comment hidden (obsolete)
Comment 122 Jared Camins-Esakov 2012-11-29 14:39:00 UTC Comment hidden (obsolete)
Comment 123 Jared Camins-Esakov 2012-11-29 15:20:09 UTC
There are no unit tests for Koha::Borrower::Modifications, nor for the AddMember_Opac routine (any other routines added should also have unit tests, of course). Starting with the 3.12 release cycle, unit tests are required for code added to the Koha:: and C4:: namespaces.

Also, please use hashrefs rather than hashes as arguments in the Koha:: namespace. I could have sworn you did a follow-up changing all the hash arguments to hashrefs but I don't see it anywhere, even among the obsolete patches.

Other notes:
* Do not access the database in BEGIN {} blocks, especially not in the Koha:: namespace.
* Use of C4::SQLHelper from the Koha:: namespace. Calling into the C4:: namespace from Koha:: is not supposed to be done. If that was the only objection, I would probably push it anyway, at least this time, but arguably I probably shouldn't.
* Package-level my variables are verboten, since they break persistence, and replacing "my" with "our" should be done only under extreme duress, and never in new code (note: you can use our when it's called for by the code, just not as a workaround for my not working under Plack).

Thank you for including a test plan on the bug. I copied it (and the original RFC) into the commit message for the first patch. I did not review it for accuracy yet, since I discovered the lack of unit tests before I got that far, so you may want to do that.

Also, I noticed a few other issues that would not prevent me pushing this but you might want to keep in mind for the future:
* The standard for help in command-line scripts is to use pod2usage.
* Object-oriented classes do not export routines and therefore should not use Exporter. Even procedural classes that do not export any routines should not use Exporter.
* When creating ->new() subroutines, the following idiom may be useful:
    return bless( { 'verification_token' => $args{'verification_token'}, ... }, $class );

Or even:
    return bless( $args, $class );

I find those two idioms make code easier to read, and certainly save typing, but they are by no means required.
Comment 124 Kyle M Hall 2012-12-10 20:46:03 UTC Comment hidden (obsolete)
Comment 125 Kyle M Hall 2012-12-10 20:49:35 UTC
(In reply to comment #123)
> There are no unit tests for Koha::Borrower::Modifications, nor for the
> AddMember_Opac routine (any other routines added should also have unit
> tests, of course). Starting with the 3.12 release cycle, unit tests are
> required for code added to the Koha:: and C4:: namespaces.

Add a Unit Test.

> Also, please use hashrefs rather than hashes as arguments in the Koha::
> namespace. I could have sworn you did a follow-up changing all the hash
> arguments to hashrefs but I don't see it anywhere, even among the obsolete
> patches.

Fixed

> Other notes:
> * Do not access the database in BEGIN {} blocks, especially not in the
> Koha:: namespace.

Fixed

> * Use of C4::SQLHelper from the Koha:: namespace. Calling into the C4::
> namespace from Koha:: is not supposed to be done. If that was the only
> objection, I would probably push it anyway, at least this time, but arguably
> I probably shouldn't.

This is the only one I haven't changed. Once we have DBIx::Class support, I pledge to switch this module from C4::SQLHelper to that.

> * Package-level my variables are verboten, since they break persistence, and
> replacing "my" with "our" should be done only under extreme duress, and
> never in new code (note: you can use our when it's called for by the code,
> just not as a workaround for my not working under Plack).

Fixed
 
> * Object-oriented classes do not export routines and therefore should not
> use Exporter. Even procedural classes that do not export any routines should
> not use Exporter.

Fixed

> * When creating ->new() subroutines, the following idiom may be useful:
>     return bless( { 'verification_token' => $args{'verification_token'}, ...
> }, $class );
> 
> Or even:
>     return bless( $args, $class );

Fixed
Comment 126 Kyle M Hall 2012-12-10 20:50:42 UTC
Setting back to 'Signed Off' as this is a QA Followup only.
Comment 127 Kyle M Hall 2012-12-11 15:32:30 UTC Comment hidden (obsolete)
Comment 128 Jared Camins-Esakov 2012-12-13 16:52:56 UTC
We are getting very close, I think.

There are four remaining problems:
1) Two of the unit tests fail:
[...]
not ok 7
#   Failed test at t/db_dependent/Koha_borrower_modifications.t line 55.
not ok 8
#   Failed test at t/db_dependent/Koha_borrower_modifications.t line 56.
[...]

2) The unit tests do not have labels that would indicate what they are testing.

3) The breadcrumb on /cgi-bin/koha/members/members-update.pl has an incorrect link for the "Patrons" section (/cgi-bin/koha/members-home.pl instead of /cgi-bin/koha/members/members-home.pl).

4) When I try to confirm a patron via e-mail, I get the following in my logs and the patron is not created:
 INSERT INTO borrowers SET   borrowers.dateenrolled = ? ,  borrowers.userid = ? ,  borrowers.categorycode = ? ,  borrowers.dateexpiry = ? ,  borrowers.password = ? ,  borrowers.cardnumber = ? 2012-12-13,,PT,2021-03-13,4eTMRTxoZ2tV15HgDg4fXw,23529001223641 at /home/jcamins/kohaclone/C4/SQLHelper.pm line 182.
DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha`.`borrowers`, CONSTRAINT `borrowers_ibfk_2` FOREIGN KEY (`branchcode`) REFERENCES `branches` (`branchcode`)) at /home/jcamins/kohaclone/C4/SQLHelper.pm line 184.DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha`.`borrowers`, CONSTRAINT `borrowers_ibfk_2` FOREIGN KEY (`branchcode`) REFERENCES `branches` (`branchcode`)) at /home/jcamins/kohaclone/C4/SQLHelper.pm line 184.


Not blocking this patch, but just a comment:
I am slightly dubious about the wisdom of allowing the public to register users on the OPAC without giving staff the ability to review those registrations. However, if this meets the needs of the library that requested the feature, I have no objection to including it in Koha as-is. Those libraries concerned with security can simply disable the feature.
Comment 129 Jared Camins-Esakov 2012-12-13 17:19:19 UTC Comment hidden (obsolete)
Comment 130 Jared Camins-Esakov 2012-12-13 17:19:33 UTC Comment hidden (obsolete)
Comment 131 Jared Camins-Esakov 2012-12-13 17:19:48 UTC Comment hidden (obsolete)
Comment 132 Jared Camins-Esakov 2012-12-13 17:20:04 UTC Comment hidden (obsolete)
Comment 133 Jared Camins-Esakov 2012-12-13 17:20:17 UTC Comment hidden (obsolete)
Comment 134 Kyle M Hall 2012-12-13 17:28:51 UTC Comment hidden (obsolete)
Comment 135 Kyle M Hall 2012-12-13 18:11:49 UTC Comment hidden (obsolete)
Comment 136 Kyle M Hall 2012-12-13 18:13:41 UTC Comment hidden (obsolete)
Comment 137 Jared Camins-Esakov 2012-12-13 22:59:26 UTC
At this point the only thing blocking this patch is:

4) When I try to confirm a patron via e-mail, I get the following in my logs and the patron is not created (when I click on the link in the e-mail):
 INSERT INTO borrowers SET   borrowers.dateenrolled = ? ,  borrowers.userid = ? ,  borrowers.categorycode = ? ,  borrowers.dateexpiry = ? ,  borrowers.password = ? ,  borrowers.cardnumber = ? 2012-12-13,,PT,2021-03-13,4eTMRTxoZ2tV15HgDg4fXw,23529001223641 at /home/jcamins/kohaclone/C4/SQLHelper.pm line 182.
DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha`.`borrowers`, CONSTRAINT `borrowers_ibfk_2` FOREIGN KEY (`branchcode`) REFERENCES `branches` (`branchcode`)) at /home/jcamins/kohaclone/C4/SQLHelper.pm line 184.DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha`.`borrowers`, CONSTRAINT `borrowers_ibfk_2` FOREIGN KEY (`branchcode`) REFERENCES `branches` (`branchcode`)) at /home/jcamins/kohaclone/C4/SQLHelper.pm line 184.
Comment 138 Kyle M Hall 2012-12-14 12:59:31 UTC Comment hidden (obsolete)
Comment 139 Michal 2012-12-15 19:09:23 UTC
Gets this error

Not enough arguments for C4::Members::fixup_cardnumber at /usr/share/koha/lib/C4/Members.pm line 2361, near "()"
Compilation failed in require at /usr/share/koha/lib/C4/Auth.pm line 149.
Comment 140 Jared Camins-Esakov 2012-12-15 19:11:40 UTC
(In reply to comment #139)
> Gets this error
> 
> Not enough arguments for C4::Members::fixup_cardnumber at
> /usr/share/koha/lib/C4/Members.pm line 2361, near "()"
> Compilation failed in require at /usr/share/koha/lib/C4/Auth.pm line 149.

What does?
Comment 141 Michal 2012-12-15 19:47:58 UTC
(In reply to comment #140)
> (In reply to comment #139)
> > Gets this error
> > 
> > Not enough arguments for C4::Members::fixup_cardnumber at
> > /usr/share/koha/lib/C4/Members.pm line 2361, near "()"
> > Compilation failed in require at /usr/share/koha/lib/C4/Auth.pm line 149.
> 
> What does?

Trying to make this in version 3.8.07 but gets this message on opac and intranet. In log is
[Sat Dec 15 21:34:41 2012] [error] [client 127.0.0.1] (2)No such file or directory: exec of '/usr/share/koha/intranet/cgi-bin/mainpage.pl' failed
[Sat Dec 15 21:34:41 2012] [error] [client 127.0.0.1] Premature end of script headers: mainpage.pl
[Sat Dec 15 21:34:42 2012] [error] [client 127.0.0.1] [Sat Dec 15 21:34:42 2012] 500.pl: Not enough arguments for C4::Members::fixup_cardnumber at /usr/share/koha/lib/C4/Members.pm line 2361, near "()"
[Sat Dec 15 21:34:42 2012] [error] [client 127.0.0.1] [Sat Dec 15 21:34:42 2012] 500.pl: Compilation failed in require at /usr/share/koha/lib/C4/Auth.pm line 149.
Comment 142 Jared Camins-Esakov 2012-12-15 19:58:04 UTC
(In reply to comment #141)
> Trying to make this in version 3.8.07 but gets this message on opac and
> intranet. In log is

These patches are for 3.12+ only. They were not intended for backporting to 3.8 (and, as you can see, rebasing causes syntax errors).

Resetting the status to "Pushed to Master." If you have further questions about backporting, I suggest you take them to the koha-devel mailing list, as bugzilla is not really the place for that sort of discussion and many people who might be able to offer advice would not read your messages.
Comment 143 Nicole C. Engard 2012-12-18 15:18:06 UTC
One problem with this. opac-memberentry.pl  says 'mobile phone' for the third phone - it should be 'other phone' like in the staff client cause the primary phone might be your mobile phone.
Comment 144 Nicole C. Engard 2012-12-18 15:20:21 UTC
Also the captcha is case sensitive which is unusual.  Usually I just type the letters and case doesn't matter - this might confuse people.

Nicole
Comment 145 Nicole C. Engard 2012-12-18 15:21:33 UTC Comment hidden (obsolete)
Comment 146 Kyle M Hall 2012-12-18 15:56:15 UTC Comment hidden (obsolete)
Comment 147 Kyle M Hall 2012-12-18 16:00:09 UTC
Nicole, I cannot recreate this issue. AFAIK, there are no system preferences related to the automatic generation of user names, are there?

(In reply to comment #145)
> Created attachment 14190 [details]
> no username
> 
> Sorry for all the updates to this - I was about to document it and when I
> registered via the OPAC I got the attached. It shows me a password but no
> username ... the username wasn't generated.
> 
> Nicole
Comment 148 Nicole C. Engard 2012-12-18 16:12:58 UTC
Hmm,

There is no preference and when I create a patron in the staff client the username is generated fine: http://screencast.com/t/OPRjsR80EAi
Comment 149 Jonathan Druart 2012-12-18 16:44:40 UTC
Hi Nicole,
Did you fill the syspref PatronSelfRegistrationDefaultCategory with an existing patron category?
There are errors in your log file?
Comment 150 Nicole C. Engard 2012-12-18 16:51:46 UTC
(In reply to comment #149)
> Hi Nicole,
> Did you fill the syspref PatronSelfRegistrationDefaultCategory with an
> existing patron category?
> There are errors in your log file?

BINGO! I had an invalid default category :)

Thanks Jonathan
Comment 151 Kyle M Hall 2012-12-18 19:18:51 UTC Comment hidden (obsolete)
Comment 152 Kyle M Hall 2012-12-18 19:21:33 UTC Comment hidden (obsolete)
Comment 153 Jonathan Druart 2012-12-27 08:51:50 UTC Comment hidden (obsolete)
Comment 154 Jonathan Druart 2012-12-27 08:52:06 UTC Comment hidden (obsolete)
Comment 155 Jared Camins-Esakov 2013-01-15 04:37:31 UTC
If these two patches are intended for inclusion in 3.12, a new bug needs to reported for them, to which they need to be attached with their subject lines changed to reflect their new bug number. If the follow-ups are unrelated, two separate bugs would make sense.
Comment 156 Jared Camins-Esakov 2013-01-15 12:30:05 UTC
*** Bug 4848 has been marked as a duplicate of this bug. ***
Comment 157 Kyle M Hall 2013-01-15 12:34:27 UTC
Patches moved to http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9393

(In reply to comment #155)
> If these two patches are intended for inclusion in 3.12, a new bug needs to
> reported for them, to which they need to be attached with their subject
> lines changed to reflect their new bug number. If the follow-ups are
> unrelated, two separate bugs would make sense.