Created attachment 27517 [details] [review] Add two requirements when registering a new patron Attached patch adds two new requirements when registering a new patron: - A child patron must have a guarantor. - A guarantor cannot be a guarantee. Both of these stem from legal requirements in Finland - these might not be applicable for everywhere, so perhaps a syspref to control one, or both, might be in order?
(In reply to paxed from comment #0) > so perhaps a syspref to control one, or both, > might be in order? My library would definitely want a system preference to control the first option (child must have a guarantor). The second option seems like simple logic to me, but I've learned not to trust my sense of logic in these matters.
Created attachment 27584 [details] [review] Guarantor requirements when registering a patron Add two requirements when registering a new patron Attached patch adds two new requirements when registering a new patron: - A child patron must have a guarantor. This is controlled by a new syspref ChildNeedsGuarantor, which defaults to off. - A guarantor cannot be a guarantee. Both of these stem from legal requirements in Finland.
Please provide a test plan.
To test: 1) Add a new child patron. It will be successful without a guarantor. 2) Add a new child patron, with guarantor set to some other patron who already has a guarantor. Adding the child patron is successful. 3) Add a new non-child patron, with guarantor set to some other patron who already has a guarantor. Adding the patron is successful. 4) Install patch 5) Add a new child patron. It will be successful without a guarantor. 6) Add a new child patron, with guarantor set to some other patron who already has a guarantor. Adding the child patron should fail with error message. 7) Add a new non-child patron, with guarantor set to some other patron who already has a guarantor. Adding the patron should fail with error message. 8) Set syspref ChildNeedsGuarantor to ON 9) Try to add a new child patron without a guarantor. It should fail with error message. 10) Add a new non-child patron. It will be successful without a guarantor. 11) Set syspref ChildNeedsGuarantor to OFF 12) Repeat the steps 5-10, but instead of adding a patron, edit already existing patron.
Created attachment 33459 [details] [review] [SIGNED-OFF] Bug 12133: Guarantor requirements when registering a patron Add two requirements when registering a new patron: - A child patron must have a guarantor. This is controlled by a new syspref ChildNeedsGuarantor. - A guarantor cannot be a guarantee. Signed-off-by: ppp <ppp@inlibro.com>
On master, you can set a firstname+surname as guarantor. You are not forced to choose an existing patron. With you patch, pref enabled, it's not possible to do that. Worse, if you try to edit a patron without guarantorid, you are not allowed to save any changes. Marked as Failed QA.
Created attachment 91482 [details] [review] Bug 12133: Guarantor requirements when registering a patron Add two requirements when registering a new patron: - A child patron must have a guarantor. This is controlled by a new syspref ChildNeedsGuarantor. - A guarantor doesn't have to be a patron. This is controlled by a new syspref GuarantorHasToBePatron. - A guarantor cannot be a guarantee. Note that syspref "GuarantorHasToBePatron" depends on "ChildNeedsGuarantor" being ON. Test plan: - Add a child patron without a guarantor. Saving the patron is successful. - Add a child patron with a guarantor who has a guarantor. Saving is successful. - Apply this patch. - Turn syspref "ChildNeedsGuarantor" ON. - Add a child patron with a guarantor who has a guarantor. Error - Add a child patron without a guarantor and error "Child needs a guarantor" is raised. - Add the guarantor info (only surname is needed) and saving the patron should be succesful. - Turn syspref "GuarantorNeedsToBePatron" ON. - Add a child patron this time with guarantor info and error "Guarantor has to be a patron" is raised. - Add existing patron as guarantor and now saving the patron should be succesful. - Add a child patron with a guarantor who is also a guarantee. Error is raised. - Turn syspref "ChildNeedsGuarantor" OFF. - Add a child patron without a guarantor. Saving patron should be successful.
After install patch. When chose new patron child got this error. Error trace Could not compile /kohadevbox/koha/members/memberentry.pl: Global symbol "$guarantorid" requires explicit package name (did you forget to declare "my $guarantorid"?) at /kohadevbox/koha/members/memberentry.pl line 270. Global symbol "$guarantorid" requires explicit package name (did you forget to declare "my $guarantorid"?) at /kohadevbox/koha/members/memberentry.pl line 271. Global symbol "$guarantordata" requires explicit package name (did you forget to declare "my $guarantordata"?) at /kohadevbox/koha/members/memberentry.pl line 272. Global symbol "$guarantordata" requires explicit package name (did you forget to declare "my $guarantordata"?) at /kohadevbox/koha/members/memberentry.pl line 273. Global symbol "$guarantorinfo" requires explicit package name (did you forget to declare "my $guarantorinfo"?) at /kohadevbox/koha/members/memberentry.pl line 274. Global symbol "$guarantordata" requires explicit package name (did you forget to declare "my $guarantordata"?) at /kohadevbox/koha/members/memberentry.pl line 274. Global symbol "$guarantordata" requires explicit package name (did you forget to declare "my $guarantordata"?) at /kohadevbox/koha/members/memberentry.pl line 274. Global symbol "$guarantordata" requires explicit package name (did you forget to declare "my $guarantordata"?) at /kohadevbox/koha/members/memberentry.pl line 275. Global symbol "$guarantordata" requires explicit package name (did you forget to declare "my $guarantordata"?) at /kohadevbox/koha/members/memberentry.pl line 276. Global symbol "$guarantordata" requires explicit package name (did you forget to declare "my $guarantordata"?) at /kohadevbox/koha/members/memberentry.pl line 277. Global symbol "$guarantordata" requires explicit package name (did you forget to declare "my $guarantordata"?) at /kohadevbox/koha/members/memberentry.pl line 283. Global symbol "$guarantordata" requires explicit package name (did you forget to declare "my $guarantordata"?) at /kohadevbox/koha/members/memberentry.pl line 287. syntax error at /kohadevbox/koha/members/memberentry.pl line 292, near "HEAD my " (Might be a runaway multi-line '< string starting on line 287) /kohadevbox/koha/members/memberentry.pl has too many errors. at /usr/share/perl5/CGI/Compile.pm line 132 in CGI::Compile::compile at /usr/share/perl5/CGI/Compile.pm line 132 129: my $code = $self->_eval($eval); 130: my $exception = $@; 131: 132: die "Could not compile $script: $exception" if $exception; 133: 134: sub { 135: my @args = @_; Show function arguments in Plack::App::WrapCGI::prepare_app at /usr/share/perl5/Plack/App/WrapCGI.pm line 81 78: }; 79: $self->_app($app); 80: } else { 81: my $sub = CGI::Compile->compile($script); 82: my $app = CGI::Emulate::PSGI->handler($sub); 83: 84: $self->_app($app); Show function arguments in Plack::Component::to_app at /usr/share/perl5/Plack/Component.pm line 49 46: 47: sub to_app { 48: my $self = shift; 49: $self->prepare_app; 50: return sub { $self->call(@_) }; 51: } 52: Show function arguments in Plack::App::CGIBin::serve_path at /usr/share/perl5/Plack/App/CGIBin.pm line 47 44: 45: local @{$env}{qw(SCRIPT_NAME PATH_INFO)} = @{$env}{qw( plack.file.SCRIPT_NAME plack.file.PATH_INFO )}; 46: 47: my $app = $self->{_compiled}->{$file} ||= Plack::App::WrapCGI->new( 48: script => $file, execute => $self->would_exec($file), 49: )->to_app; 50: $app->($env); Show function arguments in Plack::App::File::call at /usr/share/perl5/Plack/App/File.pm line 34 31: $env->{'plack.file.PATH_INFO'} = ''; 32: } 33: 34: return $self->serve_path($env, $file); 35: } 36: 37: sub locate_file { Show function arguments in Plack::Component::__ANON__ at /usr/share/perl5/Plack/Component.pm line 50 47: sub to_app { 48: my $self = shift; 49: $self->prepare_app; 50: return sub { $self->call(@_) }; 51: } 52: 53: Show function arguments in Plack::App::URLMap::call at /usr/share/perl5/Plack/App/URLMap.pm line 71 68: return $self->response_cb($app->($env), sub { 69: $env->{PATH_INFO} = $orig_path_info; 70: $env->{SCRIPT_NAME} = $orig_script_name; 71: }); 72: } 73: 74: DEBUG && warn "All matching failed.\n"; Show function arguments in Plack::Component::__ANON__ at /usr/share/perl5/Plack/Component.pm line 50 47: sub to_app { 48: my $self = shift; 49: $self->prepare_app; 50: return sub { $self->call(@_) }; 51: } 52: 53: Show function arguments in Koha::Middleware::SetEnv::call at /kohadevbox/koha/Koha/Middleware/SetEnv.pm line 104 101: %setenvs 102: }; 103: 104: return $self->app->($env); 105: } 106: 107: =head1 AUTHOR Show function arguments in Plack::Component::__ANON__ at /usr/share/perl5/Plack/Component.pm line 50 47: sub to_app { 48: my $self = shift; 49: $self->prepare_app; 50: return sub { $self->call(@_) }; 51: } 52: 53: Show function arguments in Plack::Middleware::Static::call at /usr/share/perl5/Plack/Middleware/Static.pm line 18 15: return $res; 16: } 17: 18: return $self->app->($env); 19: } 20: 21: sub _handle_static { Show function arguments in Plack::Component::__ANON__ at /usr/share/perl5/Plack/Component.pm line 50 47: sub to_app { 48: my $self = shift; 49: $self->prepare_app; 50: return sub { $self->call(@_) }; 51: } 52: 53: Show function arguments in Plack::Middleware::ReverseProxy::call at /usr/share/perl5/Plack/Middleware/ReverseProxy.pm line 68 65: } 66: } 67: 68: $self->app->($env); 69: } 70: 71: 1; Show function arguments in Plack::Component::__ANON__ at /usr/share/perl5/Plack/Component.pm line 50 47: sub to_app { 48: my $self = shift; 49: $self->prepare_app; 50: return sub { $self->call(@_) }; 51: } 52: 53: Show function arguments in Plack::Middleware::Lint::call at /usr/share/perl5/Plack/Middleware/Lint.pm line 24 21: my $env = shift; 22: 23: $self->validate_env($env); 24: my $res = $self->app->($env); 25: return $self->validate_res($res); 26: } 27: Show function arguments in Plack::Middleware::StackTrace::try {...} at /usr/share/perl5/Plack/Component.pm line 50 47: sub to_app { 48: my $self = shift; 49: $self->prepare_app; 50: return sub { $self->call(@_) }; 51: } 52: 53: in (eval) at /usr/share/perl5/Try/Tiny.pm line 101 98: if ( $wantarray ) { 99: @ret = $try->(); 100: } elsif ( defined $wantarray ) { 101: $ret[0] = $try->(); 102: } else { 103: $try->(); 104: }; in Plack::Middleware::StackTrace::call at /usr/share/perl5/Try/Tiny.pm line 94 91: 92: # failed will be true if the eval dies, because 1 will not be returned 93: # from the eval body 94: my $failed = not eval { 95: $@ = $prev_error; 96: 97: # evaluate the try block in the correct context Show function arguments in Plack::Component::__ANON__ at /usr/share/perl5/Plack/Component.pm line 50 47: sub to_app { 48: my $self = shift; 49: $self->prepare_app; 50: return sub { $self->call(@_) }; 51: } 52: 53: Show function arguments in Plack::Middleware::AccessLog::call at /usr/share/perl5/Plack/Middleware/AccessLog.pm line 27 24: my $self = shift; 25: my($env) = @_; 26: 27: my $res = $self->app->($env); 28: 29: if ( ref($res) && ref($res) eq 'ARRAY' ) { 30: my $content_length = Plack::Util::content_length($res->[2]); Show function arguments in Plack::Component::__ANON__ at /usr/share/perl5/Plack/Component.pm line 50 47: sub to_app { 48: my $self = shift; 49: $self->prepare_app; 50: return sub { $self->call(@_) }; 51: } 52: 53: Show function arguments in (eval) at /usr/share/perl5/Plack/Util.pm line 145 142: sub run_app($$) { 143: my($app, $env) = @_; 144: 145: return eval { $app->($env) } || do { 146: my $body = "Internal Server Error"; 147: $env->{'psgi.errors'}->print($@); 148: [ 500, [ 'Content-Type' => 'text/plain', 'Content-Length' => length($body) ], [ $body ] ]; in Plack::Util::run_app at /usr/share/perl5/Plack/Util.pm line 145 142: sub run_app($$) { 143: my($app, $env) = @_; 144: 145: return eval { $app->($env) } || do { 146: my $body = "Internal Server Error"; 147: $env->{'psgi.errors'}->print($@); 148: [ 500, [ 'Content-Type' => 'text/plain', 'Content-Length' => length($body) ], [ $body ] ]; Show function arguments in Starman::Server::dispatch_request at /usr/share/perl5/Starman/Server.pm line 195 192: my ($self, $env) = @_; 193: 194: # Run PSGI apps 195: my $res = Plack::Util::run_app($self->{app}, $env); 196: 197: if (ref $res eq 'CODE') { 198: $res->(sub { $self->_finalize_response($env, $_[0]) }); Show function arguments in Starman::Server::process_request at /usr/share/perl5/Starman/Server.pm line 298 295: 296: $self->_prepare_env($env); 297: 298: $self->dispatch_request($env); 299: 300: DEBUG && warn "[$$] Request done\n"; 301: Show function arguments in Net::Server::run_client_connection at /usr/share/perl5/Net/Server.pm line 74 71: $self->post_accept_hook($c); # user customizable hook 72: 73: my $ok = $self->allow_deny($c) && $self->allow_deny_hook($c); # do allow/deny check on client info 74: if ($ok) { 75: $self->process_request($c); # This is where the core functionality of a Net::Server should be. 76: } else { 77: $self->request_denied_hook($c); # user customizable hook Show function arguments in (eval) at /usr/share/perl5/Net/Server/PreFork.pm line 229 226: $prop->{'connected'} = 1; 227: print $write "$$ processing\n"; 228: 229: my $ok = eval { $self->run_client_connection; 1 }; 230: if (! $ok) { 231: print $write "$$ exiting\n"; 232: die $@; in Net::Server::PreFork::run_child at /usr/share/perl5/Net/Server/PreFork.pm line 229 226: $prop->{'connected'} = 1; 227: print $write "$$ processing\n"; 228: 229: my $ok = eval { $self->run_client_connection; 1 }; 230: if (! $ok) { 231: print $write "$$ exiting\n"; 232: die $@; Show function arguments in Net::Server::PreFork::run_n_children at /usr/share/perl5/Net/Server/PreFork.pm line 187 184: if ($prop->{'child_communication'}) { 185: $prop->{'parent_sock'} = $childsock; 186: } 187: $self->run_child; 188: } 189: } 190: } Show function arguments in Net::Server::PreFork::loop at /usr/share/perl5/Net/Server/PreFork.pm line 111 108: 109: my $start = $prop->{'min_servers'}; 110: $self->log(3, "Beginning prefork ($start processes)"); 111: $self->run_n_children($start); 112: 113: $self->run_parent; 114: } Show function arguments in Net::Server::run at /usr/share/perl5/Net/Server.pm line 60 57: $self->post_bind; # allow for chrooting, becoming a different user and group 58: 59: $self->pre_loop_hook; # user customizable hook 60: $self->loop; # repeat accept/process cycle 61: 62: $self->server_close; # close the server and release the port 63: } Show function arguments in Starman::Server::run at /usr/share/perl5/Starman/Server.pm line 106 103: max_requests => $options->{max_requests} || 1000, 104: user => $options->{user} || $>, 105: group => $options->{group} || $), 106: listen => $options->{backlog} || 1024, 107: check_for_waiting => 1, 108: no_client_stdout => 1, 109: %extra Show function arguments in Plack::Handler::Starman::run at /usr/share/perl5/Plack/Handler/Starman.pm line 25 22: } 23: $self->{net_server_args} = \%nsa if %nsa; 24: 25: Starman::Server->new->run($app, {%$self}); 26: } 27: 28: 1; Show function arguments in Plack::Loader::Delayed::run at /usr/share/perl5/Plack/Loader/Delayed.pm line 20 17: }; 18: 19: $server->{psgi_app_builder} = $self->{builder}; 20: $server->run($app); 21: } 22: 23: 1; Show function arguments in Plack::Runner::run at /usr/share/perl5/Plack/Runner.pm line 277 274: $loader->preload_app($app); 275: 276: my $server = $self->load_server($loader); 277: $loader->run($server); 278: } 279: 280: 1; Show function arguments at /usr/bin/starman line 38 35: } 36: 37: $runner->set_options(argv => \@argv); 38: $runner->run; 39: 40: __END__ 41:
That's due changes made in bug 14570 which also makes changes proposed in this bugs. Marking this as resolved.
Spoke too soon, syspref to require a guarantor for a child is still needed :D
Created attachment 94371 [details] [review] Bug 12133: (QA Follow up) Guarantor requirements when registering a patron Add three requirements when registering a new patron: - A child patron must have a guarantor. This is controlled by a new syspref ChildNeedsGuarantor. - A guarantor has to be a patron. This is controlled by a new syspref GuarantorHasToBePatron. - A guarantor cannot be a guarantee. Test plan: -Add child patron without guarantor or child patron with guarantee as guarantor successfully. - Apply this patch. - Add a child patron with a guaranteer as guarantor. Error is raised. - Turn syspref "ChildNeedsGuarantor" ON. - Add a child patron without a guarantor and error "Child needs a guarantor" is raised. - Add guarantor. Guarantor can either be existing patron or added with "Contact" section. Save without errors. - Turn syspref "GuarantorNeedsToBePatron" ON. - Form fields for "Contact" section aren't displayed and only existing patron can be added as guarantor.
Created attachment 94538 [details] [review] Bug 12133: Add requirements for guarantor and guarantee Changed header of the patch.
Created attachment 94544 [details] [review] Bug 12133: Add requirements for guarantor and guarantee Add three requirements when registering a new patron: - A child patron must have a guarantor. This is controlled by a new syspref ChildNeedsGuarantor. - A guarantor has to be a patron. This is controlled by a new syspref GuarantorHasToBePatron. - A guarantor cannot be a guarantee. Test plan: -Add child patron without guarantor or child patron with guarantee as guarantor succesfully. - Apply this patch. - Add a child patron with a guarantee as guarantor. Error is raised. - Turn syspref "ChildNeedsGuarantor" ON. - Add a child patron without a guarantor and error "Child needs a guarantor" is raised. - Add guarantor. Guarantor can either be existing patron or added with "Contact" section. Save without errors. - Turn syspref "GuarantorNeedsToBePatron" ON. - Form fields for "Contact" section aren't displayed and only existing patron can be added as guarantor. Signed-off-by: Johanna Raisa <johanna.raisa@gmail.com>
I think this new restriction needs to be enforced at module level, when the patron is added/updated (Koha::Patron->store), with changes covered by tests.
Created attachment 99629 [details] [review] Bug 12133: Fix issue with modify Child patrons couldn't be modified when guarantor was patron due missing guarantor_id value. This patch fixes that. To test: 1. Add child patron with syspref ChildNeedsGuarantor and GuarantorNeedsToBePatron on. 2. Modify patron and save. 3. Error "Child needs guarantor" is raised even if guarantor exists. 4. Apply patch. 5. Edit again and save. No error should be raised. Sponsored-by: Koha-Suomi Oy
(In reply to Jonathan Druart from comment #14) > I think this new restriction needs to be enforced at module level, when the > patron is added/updated (Koha::Patron->store), with changes covered by test You mean "guarantor can't be guarantee"?
(In reply to Emmi Takkinen from comment #16) > (In reply to Jonathan Druart from comment #14) > > I think this new restriction needs to be enforced at module level, when the > > patron is added/updated (Koha::Patron->store), with changes covered by test > > You mean "guarantor can't be guarantee"? And "child patron must have a guarantor".
Created attachment 113099 [details] [review] Bug 12133: Fix issues with modifying child patron and adding guarantors Child patrons couldn't be modified when guarantor was patron due missing guarantor_id value. Also if multiple guarantors were added one could easily add child patron as a guarantor. This patch adds Instead of one guarantor id all new and existing guarantor ids are checked in case of child patron. To test: 1. Add child patron with syspref ChildNeedsGuarantor and GuarantorNeedsToBePatron on. 2. Add two guarantors for the patron, first one should be an adult patron and second a child. 3. Save patron. => Error "A guarantor cannot be a guarantee." should be raised but isn't. 4. Modify patron and save. => Error "Child needs guarantor" is raised even if (valid) guarantor exists. 5. Apply patch. 6. Repeat steps 1 to 4. => This time while saving a new patron error "A guarantor cannot be a guarantee." is raised but modifying is successful. Sponsored-by: Koha-Suomi Oy
Finally had time to look at this again and noticed that the latest patch didn't take into account possible multiple guarantor ids. It fixed issue with modifying but one could add child patron as guarantor. Since these stem up from same a problem, missing guarantor ids, I obsoleted the latest patch and provided a new one which fixes both of these issues.
Created attachment 113100 [details] [review] Bug 12133: Enforce guarantor restrictions on Koha::Patron->store() This patch adds checks for new guarantor requirements to Koha::Patron->store(). If requirements aren't met exception is raised. To test prove t/db_dependent/Koha/Patron.t Sponsored-by: Koha-Suomi Oy
Created attachment 134013 [details] [review] Bug 12133: Add requirements for guarantor and guarantee Add three requirements when registering a new patron: - A child patron must have a guarantor. This is controlled by a new syspref ChildNeedsGuarantor. - A guarantor has to be a patron. This is controlled by a new syspref GuarantorHasToBePatron. - A guarantor cannot be a guarantee. Test plan: -Add child patron without guarantor or child patron with guarantee as guarantor succesfully. - Apply this patch. - Add a child patron with a guarantee as guarantor. Error is raised. - Turn syspref "ChildNeedsGuarantor" ON. - Add a child patron without a guarantor and error "Child needs a guarantor" is raised. - Add guarantor. Guarantor can either be existing patron or added with "Contact" section. Save without errors. - Turn syspref "GuarantorNeedsToBePatron" ON. - Form fields for "Contact" section aren't displayed and only existing patron can be added as guarantor. Signed-off-by: Johanna Raisa <johanna.raisa@gmail.com>
Created attachment 134014 [details] [review] Bug 12133: Fix issues with modifying child patron and adding guarantors Child patrons couldn't be modified when guarantor was patron due missing guarantor_id value. Also if multiple guarantors were added one could easily add child patron as a guarantor. This patch adds Instead of one guarantor id all new and existing guarantor ids are checked in case of child patron. To test: 1. Add child patron with syspref ChildNeedsGuarantor and GuarantorNeedsToBePatron on. 2. Add two guarantors for the patron, first one should be an adult patron and second a child. 3. Save patron. => Error "A guarantor cannot be a guarantee." should be raised but isn't. 4. Modify patron and save. => Error "Child needs guarantor" is raised even if (valid) guarantor exists. 5. Apply patch. 6. Repeat steps 1 to 4. => This time while saving a new patron error "A guarantor cannot be a guarantee." is raised but modifying is successful. Sponsored-by: Koha-Suomi Oy
Created attachment 134015 [details] [review] Bug 12133: Enforce guarantor restrictions on Koha::Patron->store() This patch adds checks for new guarantor requirements to Koha::Patron->store(). If requirements aren't met exception is raised. To test prove t/db_dependent/Koha/Patron.t Sponsored-by: Koha-Suomi Oy
Bug 12133 - Guarantor requirements when registering a patron 134013 - Bug 12133: Add requirements for guarantor and guarantee 134014 - Bug 12133: Fix issues with modifying child patron and adding guarantors 134015 - Bug 12133: Enforce guarantor restrictions on Koha::Patron->store() Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 12133: Add requirements for guarantor and guarantee Using index info to reconstruct a base tree... M koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/patrons.pref M koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt M members/memberentry.pl Falling back to patching base and 3-way merge... Auto-merging members/memberentry.pl Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/patrons.pref CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/patrons.pref error: Failed to merge in the changes. Patch failed at 0001 Bug 12133: Add requirements for guarantor and guarantee hint: Use 'git am --show-current-patch=diff' to see the failed patch 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". Patch left in /tmp/Bug-12133-Add-requirements-for-guarantor-and-guara-ofLCAA.patch
Created attachment 146827 [details] [review] Bug 12133: Add requirements for guarantor and guarantee Add two requirements when registering a new patron: - A child patron must have a guarantor. This is controlled by a new syspref ChildNeedsGuarantor. - A guarantor cannot be a guarantee. Test plan: 1. Add a child patron without guarantor or child patron with guarantee as guarantor succesfully. 2. Apply this patch. 3. Add a child patron as a guarantor. => Error is raised. 4. Turn syspref "ChildNeedsGuarantor" ON. 5. Add a child patron without a guarantor and error "Child needs a guarantor" is raised. 6. Add guarantor. Guarantor can either be existing patron or added with "Contact" section. => Save without errors. Also prove t/db_dependent/Koha/Patron.t Sponsored-by: Koha-Suomi Oy
Combined and rebased patches into one patch. They became a nightmare to handle :D
Created attachment 147440 [details] [review] Bug 12133: Add requirements for guarantor and guarantee Add two requirements when registering a new patron: - A child patron must have a guarantor. This is controlled by a new syspref ChildNeedsGuarantor. - A guarantor cannot be a guarantee. Test plan: 1. Add a child patron without guarantor or child patron with guarantee as guarantor succesfully. 2. Apply this patch. 3. Add a child patron as a guarantor. => Error is raised. 4. Turn syspref "ChildNeedsGuarantor" ON. 5. Add a child patron without a guarantor and error "Child needs a guarantor" is raised. 6. Add guarantor. Guarantor can either be existing patron or added with "Contact" section. => Save without errors. Also prove t/db_dependent/Koha/Patron.t Sponsored-by: Koha-Suomi Oy
Created attachment 148058 [details] [review] Bug 12133: Add requirements for guarantor and guarantee Add two requirements when registering a new patron: - A child patron must have a guarantor. This is controlled by a new syspref ChildNeedsGuarantor. - A guarantor cannot be a guarantee. Test plan: 1. Add a child patron without guarantor or child patron with guarantee as guarantor succesfully. 2. Apply this patch. 3. Add a child patron as a guarantor. => Error is raised. 4. Turn syspref "ChildNeedsGuarantor" ON. 5. Add a child patron without a guarantor and error "Child needs a guarantor" is raised. 6. Add guarantor. Guarantor can either be existing patron or added with "Contact" section. => Save without errors. Also prove t/db_dependent/Koha/Patron.t Sponsored-by: Koha-Suomi Oy
Created attachment 148070 [details] [review] Bug 12133: Add requirements for guarantor and guarantee Add two requirements when registering a new patron: - A child patron must have a guarantor. This is controlled by a new syspref ChildNeedsGuarantor. - A guarantor cannot be a guarantee. Test plan: 1. Add a child patron without guarantor or child patron with guarantee as guarantor succesfully. 2. Apply this patch. 3. Add a child patron as a guarantor. => Error is raised. 4. Turn syspref "ChildNeedsGuarantor" ON. 5. Add a child patron without a guarantor and error "Child needs a guarantor" is raised. 6. Add guarantor. Guarantor can either be existing patron or added with "Contact" section. => Save without errors. Also prove t/db_dependent/Koha/Patron.t Sponsored-by: Koha-Suomi Oy
There's still some work to do with this. If one deletes patron information and tries to save patron, error "Child patron needs a guarantor" is lifted as it should be. But if patron modification is canceled after deleting the guarantor, all guarantor info is lost because there is no check if patron would end up without guarantor due delete.
Created attachment 152706 [details] [review] Bug 12133: Add requirements for guarantor and guarantee Add two requirements when registering a new patron: - A child patron must have a guarantor. This is controlled by a new syspref ChildNeedsGuarantor. - A guarantor cannot be a guarantee. Test plan: 1. Add a child patron without guarantor or child patron with guarantee as guarantor succesfully. 2. Apply this patch. 3. Add a child patron as a guarantor. => Error is raised. 4. Turn syspref "ChildNeedsGuarantor" ON. 5. Add a child patron without a guarantor and error "Child needs a guarantor" is raised. 6. Add guarantor. Guarantor can either be existing patron or added with "Contact" section. => Save without errors. Also prove t/db_dependent/Koha/Patron.t Sponsored-by: Koha-Suomi Oy
Everything in test plan worked fine for me, just failing t/db_dependent/Koha/Patron.t. # Subtest: guarantor requirements tests 1..6 ok 1 - Child patron can be stored without guarantor when ChildNeedsGuarantor is off. ok 2 - Exception thrown when guarantor is required but not provided. ok 3 - Exception thrown when child patron is added as guarantor. DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Incorrect datetime value: 'SCALAR(0xaaab1c4b5b58)' for column `koha_kohadev`.`borrowers`.`updated_on` at row 1 at /kohadevbox/koha/Koha/Object.pm line 170 # Looks like you planned 6 tests but ran 3. not ok 25 - guarantor requirements tests # Failed test 'guarantor requirements tests' # at t/db_dependent/Koha/Patron.t line 1588. Invalid value passed, borrowers.updated_on=SCALAR(0xaaab1c4b5b58) expected type is datetime# Looks like your test exited with 255 just after 25. Dubious, test returned 255 (wstat 65280, 0xff00) Failed 1/25 subtests
(In reply to Sam Lau from comment #32) > Everything in test plan worked fine for me, just failing > t/db_dependent/Koha/Patron.t. > > # Subtest: guarantor requirements tests > 1..6 > ok 1 - Child patron can be stored without guarantor when > ChildNeedsGuarantor is off. > ok 2 - Exception thrown when guarantor is required but not provided. > ok 3 - Exception thrown when child patron is added as guarantor. > DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st > execute failed: Incorrect datetime value: 'SCALAR(0xaaab1c4b5b58)' for > column `koha_kohadev`.`borrowers`.`updated_on` at row 1 at > /kohadevbox/koha/Koha/Object.pm line 170 > # Looks like you planned 6 tests but ran 3. > not ok 25 - guarantor requirements tests > > # Failed test 'guarantor requirements tests' > # at t/db_dependent/Koha/Patron.t line 1588. > Invalid value passed, borrowers.updated_on=SCALAR(0xaaab1c4b5b58) expected > type is datetime# Looks like your test exited with 255 just after 25. > Dubious, test returned 255 (wstat 65280, 0xff00) > Failed 1/25 subtests Hmm, I'm unable to repeat this. Did you test this with current master? However for some reason 'guarantor requirements tests' fail for me randomly with error "Child patron cannot be a guarantor." This probably happens because $child2 is first attempted to be stored with child as guarantor and then without it..? Adding $child2->discard_changes after first store seems to help.
Created attachment 153082 [details] [review] Bug 12133: Add requirements for guarantor and guarantee Add two requirements when registering a new patron: - A child patron must have a guarantor. This is controlled by a new syspref ChildNeedsGuarantor. - A guarantor cannot be a guarantee. Test plan: 1. Add a child patron without guarantor or child patron with guarantee as guarantor succesfully. 2. Apply this patch. 3. Add a child patron as a guarantor. => Error is raised. 4. Turn syspref "ChildNeedsGuarantor" ON. 5. Add a child patron without a guarantor and error "Child needs a guarantor" is raised. 6. Add guarantor. Guarantor can either be existing patron or added with "Contact" section. => Save without errors. Also prove t/db_dependent/Koha/Patron.t Sponsored-by: Koha-Suomi Oy
I'm setting status back to "Needs Signoff" since Im' unable to repeat Sams problem with tests. However if it recurs then I'll look again.
Emmi, When I apply this patch, run the database update, and restart services the tests fail the same as what Sam notes: # Subtest: guarantor requirements tests 1..6 ok 1 - Child patron can be stored without guarantor when ChildNeedsGuarantor is off. ok 2 - Exception thrown when guarantor is required but not provided. ok 3 - Exception thrown when child patron is added as guarantor. DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Incorrect datetime value: 'SCALAR(0x558a5143c3d8)' for column `koha_kohadev`.`borrowers`.`updated_on` at row 1 at /kohadevbox/koha/Koha/Object.pm line 170 # Looks like you planned 6 tests but ran 3. not ok 25 - guarantor requirements tests # Failed test 'guarantor requirements tests' # at t/db_dependent/Koha/Patron.t line 1589. Invalid value passed, borrowers.updated_on=SCALAR(0x558a5143c3d8) expected type is datetime# Looks like your test exited with 255 just after 25. Dubious, test returned 255 (wstat 65280, 0xff00) Failed 2/25 subtests
Okay, now I got the same error. I'll take a look ->
Okay, "Child cannot be guarantor" error happens because $patron_category needs "can_be_guarantee" value set as 0. "Invalid value passed, borrowers.updated_on=SCALAR(0x558a5143c3d8) expected type is datetime#" happens because calling store for $child2 a.k.a modding patron sends "updated_on" as a scalar. This can be solved with calling discard_changes() after first time calling $child2->store succesfully.
Created attachment 153154 [details] [review] Bug 12133: Add requirements for guarantor and guarantee Add two requirements when registering a new patron: - A child patron must have a guarantor. This is controlled by a new syspref ChildNeedsGuarantor. - A guarantor cannot be a guarantee. Test plan: 1. Add a child patron without guarantor or child patron with guarantee as guarantor succesfully. 2. Apply this patch. 3. Add a child patron as a guarantor. => Error is raised. 4. Turn syspref "ChildNeedsGuarantor" ON. 5. Add a child patron without a guarantor and error "Child needs a guarantor" is raised. 6. Add guarantor. Guarantor can either be existing patron or added with "Contact" section. => Save without errors. Also prove t/db_dependent/Koha/Patron.t Sponsored-by: Koha-Suomi Oy
I am still failing subtest 'safe_to_delete() tests'. The only thing that changed in that subtest was that you commented out this line: $patron->account->pay({ amount => 10, debits => [ $debit ] }); Uncommenting it made all tests successful for me
(In reply to Sam Lau from comment #40) > I am still failing subtest 'safe_to_delete() tests'. The only thing that > changed in that subtest was that you commented out this line: > > $patron->account->pay({ amount => 10, debits => [ $debit ] }); > > Uncommenting it made all tests successful for me Ouch, that comment was there to make tests pass for me and I forgot to remove it. They failed because of that line, but that wasn't in scope of this bug, maybe it's now fixed.
Created attachment 154300 [details] [review] Bug 12133: Add requirements for guarantor and guarantee Add two requirements when registering a new patron: - A child patron must have a guarantor. This is controlled by a new syspref ChildNeedsGuarantor. - A guarantor cannot be a guarantee. Test plan: 1. Add a child patron without guarantor or child patron with guarantee as guarantor succesfully. 2. Apply this patch. 3. Add a child patron as a guarantor. => Error is raised. 4. Turn syspref "ChildNeedsGuarantor" ON. 5. Add a child patron without a guarantor and error "Child needs a guarantor" is raised. 6. Add guarantor. Guarantor can either be existing patron or added with "Contact" section. => Save without errors. Also prove t/db_dependent/Koha/Patron.t Sponsored-by: Koha-Suomi Oy
I deleted the unnecessary comment, tests should now pass.
Created attachment 154316 [details] [review] Bug 12133: Add requirements for guarantor and guarantee Add two requirements when registering a new patron: - A child patron must have a guarantor. This is controlled by a new syspref ChildNeedsGuarantor. - A guarantor cannot be a guarantee. Test plan: 1. Add a child patron without guarantor or child patron with guarantee as guarantor succesfully. 2. Apply this patch. 3. Add a child patron as a guarantor. => Error is raised. 4. Turn syspref "ChildNeedsGuarantor" ON. 5. Add a child patron without a guarantor and error "Child needs a guarantor" is raised. 6. Add guarantor. Guarantor can either be existing patron or added with "Contact" section. => Save without errors. Also prove t/db_dependent/Koha/Patron.t Sponsored-by: Koha-Suomi Oy Signed-off-by: David Nind <david@davidnind.com>
Testing notes (using KTD): 1. After apply the patch, run updatedatabase
FAIL members/memberentry.pl FAIL critic # Variables::ProhibitConditionalDeclarations: Got 1 violation(s).
(In reply to Marcel de Rooy from comment #46) > FAIL members/memberentry.pl > FAIL critic > # Variables::ProhibitConditionalDeclarations: Got 1 > violation(s). Any ideas where that comes from?
(In reply to Emmi Takkinen from comment #47) > (In reply to Marcel de Rooy from comment #46) > > FAIL members/memberentry.pl > > FAIL critic > > # Variables::ProhibitConditionalDeclarations: Got 1 > > violation(s). > > Any ideas where that comes from? perlcritic. You should be able to recreate easily with `perlcritic members/memberentry.pl` it will give you more info.
Created attachment 156387 [details] [review] Bug 12133: Fix QA issue
Created attachment 156446 [details] [review] Bug 12133: Add requirements for guarantor and guarantee Add two requirements when registering a new patron: - A child patron must have a guarantor. This is controlled by a new syspref ChildNeedsGuarantor. - A guarantor cannot be a guarantee. Test plan: 1. Add a child patron without guarantor or child patron with guarantee as guarantor succesfully. 2. Apply this patch. 3. Add a child patron as a guarantor. => Error is raised. 4. Turn syspref "ChildNeedsGuarantor" ON. 5. Add a child patron without a guarantor and error "Child needs a guarantor" is raised. 6. Add guarantor. Guarantor can either be existing patron or added with "Contact" section. => Save without errors. Also prove t/db_dependent/Koha/Patron.t Sponsored-by: Koha-Suomi Oy Signed-off-by: David Nind <david@davidnind.com>
Created attachment 156447 [details] [review] Bug 12133: Fix QA issue Signed-off-by: David Nind <david@davidnind.com>
Created attachment 158344 [details] [review] Bug 12133: Fix QA issue Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 158345 [details] [review] Bug 12133: (QA follow-up) Fix number of tests Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 158346 [details] [review] Bug 12133: (QA follow-up) Tidy for qa script Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Thank you Kyle for Passed QA! But it seems you signed off only part of patches and obsoleted the main patch.
Restored the patch, but might be better if Kyle uploaded all again.
Pushed to master for 23.11. Nice work everyone, thanks!
Enhancement not pushed to 23.05.x
Tests are failing koha_1 | # Failed test 'No tests run for subtest "list() tests"' koha_1 | # at t/db_dependent/api/v1/erm_users.t line 92. koha_1 | Child patron cannot be a guarantor.# Looks like your test exited with 2 just after 1. koha_1 | [08:24:56] t/db_dependent/api/v1/erm_users.t koha_1 | # Failed test 'Test guarantor handling in SendQueuedMessages' koha_1 | # at t/db_dependent/Letters.t line 1163. koha_1 | Child patron cannot be a guarantor.# Looks like your test exited with 11 just after 95. koha_1 | [08:23:02] t/db_dependent/Letters.t Please fix ASAP!
(In reply to Jonathan Druart from comment #59) > Tests are failing > > koha_1 | # Failed test 'No tests run for subtest "list() tests"' > koha_1 | # at t/db_dependent/api/v1/erm_users.t line 92. > koha_1 | Child patron cannot be a guarantor.# Looks like your test > exited with 2 just after 1. > koha_1 | [08:24:56] t/db_dependent/api/v1/erm_users.t > > koha_1 | # Failed test 'Test guarantor handling in > SendQueuedMessages' > koha_1 | # at t/db_dependent/Letters.t line 1163. > koha_1 | Child patron cannot be a guarantor.# Looks like your test > exited with 11 just after 95. > koha_1 | [08:23:02] t/db_dependent/Letters.t > > Please fix ASAP! Fixed tests, but should I create new bug or add patch here?
Please attach here.
Created attachment 158919 [details] [review] Bug 12133: Fix failing tests Tests in erm_users.t and Letters.t failed because when we mock patrons they receive category where setting can_be_guarantee is not 0. This triggers error "Child patron cannot be a guarantor". We also need to mock syspref "ChildNeedsGuarantor" accordingly. To test prove t/db_dependent/api/v1/erm_users.t and t/db_dependent/Letters.t. Sponsored-by: Koha-Suomi Oy
(In reply to Emmi Takkinen from comment #62) > Created attachment 158919 [details] [review] [review] > Bug 12133: Fix failing tests > > Tests in erm_users.t and Letters.t failed because when > we mock patrons they receive category where setting > can_be_guarantee is not 0. This triggers error "Child > patron cannot be a guarantor". We also need to mock > syspref "ChildNeedsGuarantor" accordingly. > > To test prove t/db_dependent/api/v1/erm_users.t and > t/db_dependent/Letters.t. > > Sponsored-by: Koha-Suomi Oy Ha, just had written almost the same patch.
Follow-up pushed, thanks.
I don't understand why this has been designed in a so limited way. Why only children? This should have been a patron's category flag, or the syspref should accept a list of patron's categories. At some point only children were allowed to have guarantor, but since bug 12446 this limitation should not persist.
The test is still failing koha_1 | # Failed test 'No tests run for subtest "Test where parameter for SendQueuedMessages"' koha_1 | # at t/db_dependent/Letters.t line 1274. koha_1 | [Child patron needs a guarantor]# Looks like your test exited with 11 just after 97. koha_1 | [19:28:21] t/db_dependent/Letters.t .................................................... koha_1 | Dubious, test returned 11 (wstat 2816, 0xb00)
Created attachment 158979 [details] [review] Bug 12133: (follow-up) Fix failing tests Tests were still failing # Failed test 'No tests run for subtest "Test where parameter for SendQueuedMessages"' # at t/db_dependent/Letters.t line 1274. [Child patron needs a guarantor]# Looks like your test exited with 11 just after 97. Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
(In reply to Jonathan Druart from comment #67) > Created attachment 158979 [details] [review] [review] > Bug 12133: (follow-up) Fix failing tests > > Tests were still failing > > # Failed test 'No tests run for subtest "Test where parameter for > SendQueuedMessages"' > # at t/db_dependent/Letters.t line 1274. > [Child patron needs a guarantor]# Looks like your test exited with 11 just > after 97. > > Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Pushed to master.
(In reply to Jonathan Druart from comment #65) > I don't understand why this has been designed in a so limited way. > > Why only children? > > This should have been a patron's category flag, or the syspref should accept > a list of patron's categories. > > At some point only children were allowed to have guarantor, but since bug > 12446 this limitation should not persist. It does take into account if patrons category has setting "Can be guarantee" enabled. In Koha::Patron::store: if ( C4::Context->preference('ChildNeedsGuarantor') and ( $self->is_child or $self->category->can_be_guarantee ) and $self->contactname eq "" and !@$guarantors ) { Koha::Exceptions::Patron::Relationship::NoGuarantor->throw(); } But yeah, syspref and errors don't implicate that syspref has anything to do with other than child patrons.
I hope I understood this correctly. Basically, the system preference makes the guarantor mandatory when creating a patron whose category is 'can be guarantee'? I feel like the mention of children is a bit misleading... I created bug 37439 for that. Please reopen if I misunderstood.
> A guarantor cannot be a guarantee. We should add a warn on patron page when it happens. And a control in misc/maintenance/search_for_data_inconsistencies.pl