Bug 12446 - Enable an adult to have a guarantor
Summary: Enable an adult to have a guarantor
Status: Passed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P5 - low new feature (vote)
Assignee: Maryse Simard
QA Contact: Kyle M Hall
URL:
Keywords:
: 6922 9305 (view as bug list)
Depends on: 17168 14570
Blocks:
  Show dependency treegraph
 
Reported: 2014-06-18 15:01 UTC by simith.doliveira
Modified: 2020-11-12 21:45 UTC (History)
30 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 12446 - Enable an adult to have a guarantor (19.91 KB, patch)
2014-06-26 13:38 UTC, simith.doliveira
Details | Diff | Splinter Review
Bug 12446 - Enable an adult to have a guarantor (19.89 KB, patch)
2015-02-26 14:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
bug 12446 - Enable adult patrons to have a guarantor (28.14 KB, patch)
2015-06-01 13:44 UTC, Maxime Beaulieu
Details | Diff | Splinter Review
bug 12446 - Enable adult patrons to have a guarantor (28.44 KB, patch)
2015-09-09 19:50 UTC, Genevieve Plantin
Details | Diff | Splinter Review
Bug 12446 - Clean code according to Comment 12 (4.55 KB, patch)
2015-09-09 19:51 UTC, Genevieve Plantin
Details | Diff | Splinter Review
Bug 12446 - Displayed button Add guarantee in Details section (1.06 KB, patch)
2015-09-14 19:17 UTC, Genevieve Plantin
Details | Diff | Splinter Review
bug 12446 - Enable adult patrons to have a guarantor (65.78 KB, patch)
2016-01-05 20:12 UTC, Rémi Mayrand-Provencher
Details | Diff | Splinter Review
bug 12446 - Enable adult patrons to have a guarantor (65.57 KB, patch)
2016-01-05 20:17 UTC, Rémi Mayrand-Provencher
Details | Diff | Splinter Review
bug 12446 - Enable adult patrons to have a guarantor (26.02 KB, patch)
2016-01-06 13:53 UTC, Rémi Mayrand-Provencher
Details | Diff | Splinter Review
bug 12446 - Enable adult patrons to have a guarantor (27.40 KB, patch)
2016-01-06 14:27 UTC, Rémi Mayrand-Provencher
Details | Diff | Splinter Review
bug 12446 - Enable adult patrons to have a guarantor (26.42 KB, patch)
2016-01-06 18:28 UTC, Rémi Mayrand-Provencher
Details | Diff | Splinter Review
bug 12446 - Atomic update now sets canbeguarantee to 1 for categories of type C and P (1.14 KB, patch)
2016-01-11 14:38 UTC, Rémi Mayrand-Provencher
Details | Diff | Splinter Review
Bug 12446 - Enable adult patrons to have a guarantor (27.51 KB, patch)
2016-04-01 14:32 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 12446 - Enable adult patrons to have a guarantor (26.83 KB, patch)
2016-04-28 20:41 UTC, Rémi Mayrand-Provencher
Details | Diff | Splinter Review
Bug 12446 - Enable an adult to have a guarantor (34.40 KB, patch)
2016-07-20 19:03 UTC, Mehdi Hamidi
Details | Diff | Splinter Review
Bug 12446 - Enable an adult to have a guarantor (34.40 KB, patch)
2016-07-20 19:06 UTC, Mehdi Hamidi
Details | Diff | Splinter Review
Bug 12446 - Enable an adult to have a guarantor (33.52 KB, patch)
2016-07-20 19:51 UTC, Mehdi Hamidi
Details | Diff | Splinter Review
Bug 12446 - QA Follow-up (7.61 KB, patch)
2016-08-02 14:41 UTC, Mehdi Hamidi
Details | Diff | Splinter Review
Bug 12446 - Enable an adult to have a guarantor (31.30 KB, patch)
2017-03-08 19:00 UTC, Bouzid Fergani
Details | Diff | Splinter Review
Bug 12446 - Enable an adult to have a guarantor (31.30 KB, patch)
2017-03-08 19:01 UTC, Bouzid Fergani
Details | Diff | Splinter Review
Bug 12446 - Enable an adult to have a guarantor (31.23 KB, patch)
2017-03-08 19:17 UTC, Bouzid Fergani
Details | Diff | Splinter Review
Remove wrong importation in DataTables/Members.pm (2.59 KB, patch)
2017-05-04 19:16 UTC, Philippe Audet-Fortin
Details | Diff | Splinter Review
Remove wrong importation in DataTables/Members.pm (2.64 KB, patch)
2017-05-04 19:32 UTC, Philippe Audet-Fortin
Details | Diff | Splinter Review
ID change for jQuery listener (3.79 KB, patch)
2017-05-05 20:42 UTC, Philippe Audet-Fortin
Details | Diff | Splinter Review
Cam 722: Permet a n'importe quelle categorie d'avoir un garant (bz12446) (26.24 KB, patch)
2017-07-04 21:04 UTC, Philippe Audet-Fortin
Details | Diff | Splinter Review
Bug 12446 - Enable an adult to have a guarantor (30.39 KB, patch)
2017-07-04 21:18 UTC, Philippe Audet-Fortin
Details | Diff | Splinter Review
Bug 12446 - Added a missing comma in kohastructure.sql (1.50 KB, patch)
2017-07-05 19:54 UTC, Philippe Audet-Fortin
Details | Diff | Splinter Review
Bug 12446 - Enable an adult to have a guarantor (33.77 KB, patch)
2018-02-28 23:19 UTC, Roch D'Amour
Details | Diff | Splinter Review
Bug 12446 - Enable an adult to have a guarantor (33.82 KB, patch)
2018-03-14 15:30 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 12446 - Enable an adult to have a guarantor (34.72 KB, patch)
2018-08-13 20:40 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 12446: Enable an adult to have a guarantor (34.72 KB, patch)
2018-08-13 20:41 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 12446: (QA follow-up) Cleaning, and fixing, old code carried accidentally (5.77 KB, patch)
2018-08-13 20:41 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 12446: (QA follow-up) Cleaning, and fixing, old code carried accidentally (7.74 KB, patch)
2018-09-24 20:06 UTC, Pierre-Marc Thibault
Details | Diff | Splinter Review
Bug 12446 - Enable an adult to have a guarantor (35.03 KB, patch)
2018-09-24 20:11 UTC, Pierre-Marc Thibault
Details | Diff | Splinter Review
Bug 12446: (QA follow-up) Cleaning, and fixing, old code carried accidentally (7.74 KB, patch)
2018-09-24 20:12 UTC, Pierre-Marc Thibault
Details | Diff | Splinter Review
Bug 12446 - Enable an adult to have a guarantor (35.03 KB, patch)
2018-09-24 20:27 UTC, Pierre-Marc Thibault
Details | Diff | Splinter Review
Bug 12446: (QA follow-up) Cleaning, and fixing, old code carried accidentally (7.74 KB, patch)
2018-09-24 20:27 UTC, Pierre-Marc Thibault
Details | Diff | Splinter Review
Bug 12446: (follow-up) Show all guarantees on the guarantor detail page (878 bytes, patch)
2019-02-13 23:04 UTC, Maryse Simard
Details | Diff | Splinter Review
Bug 12446: Enable an adult to have a guarantor (35.11 KB, patch)
2019-04-19 18:03 UTC, Pierre-Marc Thibault
Details | Diff | Splinter Review
Bug 12446: (QA follow-up) Cleaning, and fixing, old code carried accidentally (7.93 KB, patch)
2019-04-19 18:03 UTC, Pierre-Marc Thibault
Details | Diff | Splinter Review
Bug 12446: (follow-up) Show all guarantees on the guarantor detail page (878 bytes, patch)
2019-04-19 18:03 UTC, Pierre-Marc Thibault
Details | Diff | Splinter Review
Bug 12446: (QA follow-up) QA tests fixing, cleaning code (2.94 KB, patch)
2019-04-19 18:03 UTC, Pierre-Marc Thibault
Details | Diff | Splinter Review
Bug 12446: Enable an adult to have a guarantor (37.16 KB, patch)
2019-06-06 19:00 UTC, Nadine Pierre
Details | Diff | Splinter Review
Bug 12446: (QA follow-up) Cleaning, and fixing, old code carried accidentally (7.92 KB, patch)
2019-06-06 19:00 UTC, Nadine Pierre
Details | Diff | Splinter Review
Bug 12446: (follow-up) Show all guarantees on the guarantor detail page (885 bytes, patch)
2019-06-06 19:00 UTC, Nadine Pierre
Details | Diff | Splinter Review
Bug 12446: (QA follow-up) QA tests fixing, cleaning code (2.94 KB, patch)
2019-06-06 19:01 UTC, Nadine Pierre
Details | Diff | Splinter Review
Bug 12446: Enable an adult to have a guarantor (37.32 KB, patch)
2019-06-11 03:33 UTC, Hayley Mapley
Details | Diff | Splinter Review
Bug 12446: (QA follow-up) Cleaning, and fixing, old code carried accidentally (8.03 KB, patch)
2019-06-11 03:33 UTC, Hayley Mapley
Details | Diff | Splinter Review
Bug 12446: (follow-up) Show all guarantees on the guarantor detail page (946 bytes, patch)
2019-06-11 03:34 UTC, Hayley Mapley
Details | Diff | Splinter Review
Bug 12446: (QA follow-up) QA tests fixing, cleaning code (3.02 KB, patch)
2019-06-11 03:34 UTC, Hayley Mapley
Details | Diff | Splinter Review
Bug 12446: Enable an adult to have a guarantor (25.97 KB, patch)
2019-10-01 20:02 UTC, Maryse Simard
Details | Diff | Splinter Review
Bug 12446: (QA follow-up) Cleaning, and fixing, old code carried accidentally (3.14 KB, patch)
2019-10-01 20:03 UTC, Maryse Simard
Details | Diff | Splinter Review
Bug 12446: (follow-up) Show all guarantees on the guarantor detail page (939 bytes, patch)
2019-10-01 20:03 UTC, Maryse Simard
Details | Diff | Splinter Review
Bug 12446: (QA follow-up) QA tests fixing, cleaning code (3.00 KB, patch)
2019-10-01 20:03 UTC, Maryse Simard
Details | Diff | Splinter Review
Bug 12446: Schema update Category.pm (961 bytes, patch)
2019-10-01 20:03 UTC, Maryse Simard
Details | Diff | Splinter Review
Bug 12446: (follow-up) Remove mandatory transfered values (8.07 KB, patch)
2019-10-01 20:03 UTC, Maryse Simard
Details | Diff | Splinter Review
Bug 12446: (follow-up) Code cleaning (10.47 KB, patch)
2019-10-01 20:03 UTC, Maryse Simard
Details | Diff | Splinter Review
Bug 12446: (follow-up) Update for Bug 17168 (3.59 KB, patch)
2019-11-13 21:11 UTC, Maryse Simard
Details | Diff | Splinter Review
Bug 12446: Enable an adult to have a guarantor (26.16 KB, patch)
2019-11-27 16:18 UTC, Maryse Simard
Details | Diff | Splinter Review
Bug 12446: (QA follow-up) Cleaning, and fixing, old code carried accidentally (3.14 KB, patch)
2019-11-27 16:18 UTC, Maryse Simard
Details | Diff | Splinter Review
Bug 12446: (follow-up) Show all guarantees on the guarantor detail page (939 bytes, patch)
2019-11-27 16:19 UTC, Maryse Simard
Details | Diff | Splinter Review
Bug 12446: (QA follow-up) QA tests fixing, cleaning code (3.00 KB, patch)
2019-11-27 16:19 UTC, Maryse Simard
Details | Diff | Splinter Review
Bug 12446: Schema update Category.pm (961 bytes, patch)
2019-11-27 16:19 UTC, Maryse Simard
Details | Diff | Splinter Review
Bug 12446: (follow-up) Remove mandatory transfered values (8.08 KB, patch)
2019-11-27 16:19 UTC, Maryse Simard
Details | Diff | Splinter Review
Bug 12446: (follow-up) Code cleaning (10.47 KB, patch)
2019-11-27 16:19 UTC, Maryse Simard
Details | Diff | Splinter Review
Bug 12446: (follow-up) Update for Bug 17168 (3.59 KB, patch)
2019-11-27 16:19 UTC, Maryse Simard
Details | Diff | Splinter Review
Bug 12446: Enable an adult to have a guarantor (25.75 KB, patch)
2020-07-09 18:17 UTC, Maryse Simard
Details | Diff | Splinter Review
Bug 12446: (follow-up) Remove mandatory transfered values (10.56 KB, patch)
2020-07-09 18:17 UTC, Maryse Simard
Details | Diff | Splinter Review
Bug 12446: (follow-up) Code cleaning (8.02 KB, patch)
2020-07-09 18:17 UTC, Maryse Simard
Details | Diff | Splinter Review
Bug 12446: Schema update Category.pm (1.27 KB, patch)
2020-07-09 18:17 UTC, Maryse Simard
Details | Diff | Splinter Review
Bug 12446: (follow-up) Update for Bug 17168 (3.60 KB, patch)
2020-07-09 18:17 UTC, Maryse Simard
Details | Diff | Splinter Review
Unintentionally blue screenshot of the new fines display for guarantees (21.16 KB, image/png)
2020-07-30 21:43 UTC, Katrin Fischer
Details
Bug 12446: Ability to allow guarantor relationship for all patron category types (13.01 KB, patch)
2020-10-02 04:19 UTC, Maryse Simard
Details | Diff | Splinter Review
Bug 12446: Schema update Category.pm (1.20 KB, patch)
2020-10-02 04:19 UTC, Maryse Simard
Details | Diff | Splinter Review
Bug 12446: Ability to allow guarantor relationship for all patron category types (13.12 KB, patch)
2020-10-06 18:53 UTC, David Nind
Details | Diff | Splinter Review
Bug 12446: Schema update Category.pm (1.25 KB, patch)
2020-10-06 18:53 UTC, David Nind
Details | Diff | Splinter Review
Bug 12446: Limit available categories when using "Add guarantee" button (1.41 KB, patch)
2020-10-21 14:25 UTC, Maryse Simard
Details | Diff | Splinter Review
Bug 26751: Fix fatal exception if only one repo defined (2.24 KB, patch)
2020-10-22 10:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12446: Ability to allow guarantor relationship for all patron category types (13.13 KB, patch)
2020-10-22 10:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12446: Schema update Category.pm (1.32 KB, patch)
2020-10-22 10:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12446: Limit available categories when using "Add guarantee" button (1.48 KB, patch)
2020-10-22 10:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12446: Add canbeguarantee value to patron categories sample data (24.55 KB, patch)
2020-11-12 21:44 UTC, Maryse Simard
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description simith.doliveira 2014-06-18 15:01:41 UTC
Our sponsor require an adjustment to allow an adult to have a guarantor and some other modifications concerning. This Fix:

- Enable an adult to have a guarantor;
- Shows guarantees fines in the guarantor's page;
- Transfer some guarantor's information to the guarantee while creating;
- Transfer guarantor's alternate contact to the guarantee while creating.  

Sponsor by: ville de victoriaville
Comment 1 simith.doliveira 2014-06-26 13:38:58 UTC Comment hidden (obsolete)
Comment 2 Marc Véron 2015-02-10 16:50:55 UTC
Patch does not apply:

CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/patrons.pref
Comment 3 Jonathan Druart 2015-02-26 14:37:40 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2015-02-26 14:37:57 UTC
Easy conflict, fixed.
Comment 5 Nicole C. Engard 2015-03-02 14:44:16 UTC
While the code works, this is a problem for many libraries. I think this should be an on/off switch on the patron category administration page. That way some categories can have it and others won't.  If it's there for all patrons it will be requested that we hide it for all but children and that can't be done easily.

Nicole
Comment 6 Kyle M Hall 2015-03-04 15:12:28 UTC
I completely agree. This should be an optional feature implemented at the patron category level. Otherwise this makes an irreversible change to Koha. Many librarians complain that the patron form is too long already. We cannot hide this for adults without also hiding it for children, thus it must be implemented as an optional feature. The most sensible way to do this would be to add a switch at the patron category level that enables the ability to set a guarantor for a given patron type.

(In reply to Nicole C. Engard from comment #5)
> While the code works, this is a problem for many libraries. I think this
> should be an on/off switch on the patron category administration page. That
> way some categories can have it and others won't.  If it's there for all
> patrons it will be requested that we hide it for all but children and that
> can't be done easily.
> 
> Nicole
Comment 7 Francois Charbonnier 2015-03-04 15:25:28 UTC
Hi Nicole, Hi Kyle,

I do not agree since it fixes an issue in Koha.

Koha already allows librarian to linked adult cards together. But once the cards are linked together, if the guarantee is an adult, the patron edit page doesn't display the "Guarantor information". So you can't edit the link. If you want to, you have to change the patron category to a child type, edit it and then put back the correct patron category.

To me, it's a feature that needed to be fixed.

I agree there is a need to be able to hide parts of the patron edit page but I think it should be part of a different feature. If we add the ability to control the display of the edit patron page, we should be able to do it for every part of the page.

What do you think?
Comment 8 Nicole C. Engard 2015-03-04 15:59:26 UTC
Discussion from IRC: http://irc.koha-community.org/koha/2015-03-04#i_1645311
Comment 9 Francois Charbonnier 2015-03-04 16:16:41 UTC
Thanks nicole!
We just talked about this with Nicole and Kyle.
We agreeded on this :
* On the patron category page, there will be a new button : "display the edit garantor information box" : yes | no
* by default, we will have "yes" for child and professional categories
* if yes, we will display the edit guarantor box on the patron edit page
Comment 10 Maxime Beaulieu 2015-06-01 13:44:36 UTC Comment hidden (obsolete)
Comment 11 Marc Véron 2015-09-03 12:47:06 UTC
Wanted to test but patch does not apply.
Comment 12 Mark Tompsett 2015-09-04 05:52:51 UTC
Comment on attachment 39744 [details] [review]
bug 12446 - Enable adult patrons to have a guarantor

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

I just skimmed the patches. The linking guarantor to a specific category code is bad. I like the notion of a canbeguaranteed code field. Some code changes aren't obvious to me.

::: C4/Utils/DataTables/Members.pm
@@ +38,5 @@
> +        @columns = grep { ! $seen{ $_ }++ } ( @columns, @guarantor_attributes );
> +    };
> +    $_ = "borrowers.".$dbh->quote_identifier($_) for (@columns);
> +
> +    my $select = "SELECT ${ \join(',', @columns) },

This code is horrible to read!

::: installer/data/mysql/atomicupdate/bug_12446-EnableAdultGarantee.sql
@@ +9,5 @@
> +-- ********* --
> +ALTER TABLE categories ADD COLUMN `canbeguarantee` tinyint(1) NOT NULL default '0';
> +
> +-- Set defaults for new column
> +UPDATE categories set canbeguarantee = 1 WHERE category_type IN ('C', 'P');
\ No newline at end of file

Default behaviour should NOT activate new functionality.

::: installer/data/mysql/sysprefs.sql
@@ +481,4 @@
>  ('XSLTDetailsDisplay','default','','Enable XSL stylesheet control over details page display on intranet','Free'),
>  ('XSLTResultsDisplay','default','','Enable XSL stylesheet control over results page display on intranet','Free'),
>  ('z3950AuthorAuthFields','701,702,700',NULL,'Define the MARC biblio fields for Personal Name Authorities to fill biblio.author','free'),
> +('z3950NormalizeAuthor','0','','If ON, Personal Name Authorities will replace authors in biblio.author','YesNo'),

Why was , added at the end?

::: koha-tmpl/intranet-tmpl/prog/en/includes/members-toolbar.inc
@@ +148,4 @@
>  
>      [% IF ( CAN_user_borrowers ) %]
>          [% IF ( adultborrower AND activeBorrowerRelationship ) %]
> +            <a id="addchild" class="btn btn-small" href="/cgi-bin/koha/members/memberentry.pl?op=add&amp;guarantorid=[% borrowernumber %]&amp;category_type=C"><i class="icon-plus"></i> Add guarantiee</a>

Is that spelt correctly?

::: koha-tmpl/intranet-tmpl/prog/en/modules/admin/categorie.tt
@@ +25,5 @@
>          $("#branches option:first").attr("selected", "selected");
>      }
> +    $("#category_type").change(function(){
> +        var selected = $(this).val();
> +        $("#canbeguarantee").val( (selected == 'C' || selected == 'P') ? "1" : "0" );

Hardcoded category codes is a bad thing.
Is there a better way to do this?

::: members/memberentry.pl
@@ +251,4 @@
>  }
>  
>    #recover all data from guarantor address phone ,fax... 
> +if ( $guarantorid and ( $category_type eq 'C' || $category_type eq 'P')) {

Why remove the space?

@@ -259,5 @@
>          if ( $op eq 'add' ) {
>  	        foreach (qw(streetnumber address streettype address2
> -                        zipcode country city state phone phonepro mobile fax email emailpro branchcode
> -                        B_streetnumber B_streettype B_address B_address2
> -                        B_city B_state B_zipcode B_country B_email B_phone)) {

Why were these removed?

::: members/moremember.pl
@@ +204,2 @@
>  	if ($category_type eq 'C'){
>  		$template->param('C' => 1);

Is this used anymore in the template file? I saw you removed one C condition.
Comment 13 Genevieve Plantin 2015-09-09 19:50:48 UTC Comment hidden (obsolete)
Comment 14 Genevieve Plantin 2015-09-09 19:51:01 UTC Comment hidden (obsolete)
Comment 15 Genevieve Plantin 2015-09-14 19:17:05 UTC Comment hidden (obsolete)
Comment 16 Aleisha Amohia 2015-12-29 01:46:49 UTC
Applying: bug 12446 - Enable adult patrons to have a guarantor
/home/vagrant/kohaclone/.git/rebase-apply/patch:456: trailing whitespace.
#recover all data from guarantor address phone ,fax... 
/home/vagrant/kohaclone/.git/rebase-apply/patch:468: trailing whitespace.
                        B_city B_state B_zipcode B_country B_email B_phone)) {		        
fatal: sha1 information is lacking or useless (C4/Utils/DataTables/Members.pm).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 bug 12446 - Enable adult patrons to have a guarantor
Comment 17 Rémi Mayrand-Provencher 2016-01-05 20:12:03 UTC Comment hidden (obsolete)
Comment 18 Rémi Mayrand-Provencher 2016-01-05 20:17:51 UTC Comment hidden (obsolete)
Comment 19 Jonathan Druart 2016-01-06 12:33:37 UTC
Bug 15407 is going to remove C4::Category, you should use Koha::Patron::Categories instead.
Comment 20 Rémi Mayrand-Provencher 2016-01-06 13:53:14 UTC Comment hidden (obsolete)
Comment 21 Rémi Mayrand-Provencher 2016-01-06 13:59:16 UTC Comment hidden (obsolete)
Comment 22 Rémi Mayrand-Provencher 2016-01-06 14:27:17 UTC Comment hidden (obsolete)
Comment 23 Marc Véron 2016-01-06 15:56:33 UTC
One small issue: I have Euro set as active currency, but the table with the Guarantee's fines show the $ sign.
BTW there is a bug pending regarding the display of curreny signs and formats (Bug 4078) .
Comment 24 Rémi Mayrand-Provencher 2016-01-06 18:28:11 UTC Comment hidden (obsolete)
Comment 25 Liz Rea 2016-01-07 02:59:11 UTC
Hi,
I don't see that the atomic update is setting the field for existing categories, with this patch on and the atomic update run, all of the categories (including C and P) are all set to 0 in the database after the update.

Cheers,
Liz
Comment 26 Rémi Mayrand-Provencher 2016-01-11 14:38:40 UTC Comment hidden (obsolete)
Comment 27 Katrin Fischer 2016-03-30 09:08:57 UTC
Lots of work done on this - would be nice to see a sign off!

But a little note from me: While I don't want to complicate things here even more, please consider splitting patches up a bit more in the future. From the initial description this could be 3-4 separate features:
- Enable an adult to have a guarantor;
- Shows guarantees fines in the guarantor's page;
- Transfer some guarantor's information to the guarantee while creating;
- Transfer guarantor's alternate contact to the guarantee while creating. 

This would make the features more visible in bugzilla, but also in the release notes. As right now - the bug title doesn't really describe all that this patch tries to achieve!
Comment 28 Katrin Fischer 2016-03-30 09:09:22 UTC
... also small feature patches tend to move quicker!
Comment 29 Marc Véron 2016-03-30 09:20:18 UTC
Sorry, I get many conflicts while applying the patches.
Comment 30 Charles Farmer 2016-04-01 14:32:04 UTC Comment hidden (obsolete)
Comment 31 Marc Véron 2016-04-02 06:47:18 UTC
Sorry, I get:
Undefined subroutine &main::GetGuarantees called at /usr/share/kohaclone/members/moremember.pl line 180.

I think it is related to Bug 15656 Bug 15656 - Move the guarantor/guarantees code to Koha::Patron (Pushed to aster)
Comment 32 Rémi Mayrand-Provencher 2016-04-28 20:41:05 UTC Comment hidden (obsolete)
Comment 33 Owen Leonard 2016-06-17 12:28:32 UTC
Sorry, this doesn't apply again.

Any patch which touches memberentrygen.tt is really hard to keep up to date!
Comment 34 Mehdi Hamidi 2016-07-20 19:03:22 UTC Comment hidden (obsolete)
Comment 35 Mehdi Hamidi 2016-07-20 19:06:07 UTC Comment hidden (obsolete)
Comment 36 Mehdi Hamidi 2016-07-20 19:51:04 UTC Comment hidden (obsolete)
Comment 37 Mark Tompsett 2016-07-27 18:43:51 UTC Comment hidden (obsolete)
Comment 38 Mehdi Hamidi 2016-08-02 14:41:46 UTC Comment hidden (obsolete)
Comment 39 Owen Leonard 2016-08-12 16:06:32 UTC
Does this feature make it inaccurate to label the "Relatives' checkouts" tab as such? "Guarantees' checkouts" perhaps?
Comment 40 Blou 2016-10-07 21:06:39 UTC
That's an easy change, but is this a requirement?  I think it would be more precise, but maybe some would not like this new "unpersonnal phrasing" ?

An easy fix, just let us know if required to reach Signed Off :)
Comment 41 Marc Véron 2017-02-22 08:37:35 UTC
Wanted to test but patch does no longer apply.

Maybe rebasing is a good occasion to address Comment #39 as well ( "Guarantees' checkouts" instead of "Relatives' checkouts" tab).
Comment 42 Kyle M Hall 2017-02-23 11:17:41 UTC
(In reply to Owen Leonard from comment #39)
> Does this feature make it inaccurate to label the "Relatives' checkouts" tab
> as such? "Guarantees' checkouts" perhaps?

I think we us the term colloquially but I've not problem switching our language to guarantees to be technically correct :)

I think we should do that on a separate bug since it affects other parts of Koha not in the purview of this bug patch.
Comment 43 Bouzid Fergani 2017-03-08 19:00:16 UTC Comment hidden (obsolete)
Comment 44 Bouzid Fergani 2017-03-08 19:01:56 UTC Comment hidden (obsolete)
Comment 45 Bouzid Fergani 2017-03-08 19:17:20 UTC Comment hidden (obsolete)
Comment 46 Josef Moravec 2017-05-02 10:14:49 UTC
QA test tool complains: 

 FAIL	C4/Members/Attributes.pm
   FAIL	  pod
		 
		 in file C4/Members/Attributes.pm
		
		Apparent command =cut not preceded by blank line
		*** ERROR: 

 FAIL	C4/Utils/DataTables/Members.pm
   FAIL	  forbidden patterns
		forbidden pattern: tab char (line 58)
		forbidden pattern: tab char (line 59)
   FAIL	  valid
		Can't continue after import errors 
		"GetMemberIssuesAndFines" is not exported by the C4::Members module

 FAIL	koha-tmpl/intranet-tmpl/prog/en/modules/members/tables/guarantor_search.tt
   FAIL	  forbidden patterns
		forbidden pattern: Contains old bootstrap style for buttons (see bug 16239) (line 20)

 FAIL	members/moremember.pl
   FAIL	  forbidden patterns
		forbidden pattern: tab char (line 221)

also the patch does not apply cleanly
Comment 47 Philippe Audet-Fortin 2017-05-04 19:16:44 UTC Comment hidden (obsolete)
Comment 48 Philippe Audet-Fortin 2017-05-04 19:32:49 UTC Comment hidden (obsolete)
Comment 49 Philippe Audet-Fortin 2017-05-05 20:42:51 UTC Comment hidden (obsolete)
Comment 50 Jonathan Druart 2017-05-08 15:05:08 UTC
Comment on attachment 63214 [details] [review]
ID change for jQuery listener

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

Philippe,

Please do not add unnecessary changes to your patches:
blank lines, indentation changes, trailing spaces should only be fixed when you are modifying these lines.

::: koha-tmpl/intranet-tmpl/prog/en/modules/common/patron_search.tt
@@ +112,4 @@
>      $("body").on("click",".select_user",function(e){
>          e.preventDefault();
>          var borrowernumber = $(this).data("borrowernumber");
> +        var borrower_data = $("#guarantor_data"+borrowernumber).val();

You should not modify existing code without thinking about side-effects.
Assume that existing code is useful and used by other scripts :)

For instance with this change you break admin/add_user_search.pl when called to set the owner of a fund (from admin/aqbudgets.tt).
Comment 51 Jonathan Druart 2017-05-08 15:06:13 UTC
Aware of bug 14570?
Comment 52 Philippe Audet-Fortin 2017-07-04 21:04:13 UTC Comment hidden (obsolete)
Comment 53 Philippe Audet-Fortin 2017-07-04 21:18:18 UTC Comment hidden (obsolete)
Comment 54 Philippe Audet-Fortin 2017-07-05 11:54:13 UTC
I corrected my old mistake (hope it doesn't affect the rest of the code :)) and rebase it. Now the patch apply.
Comment 55 Philippe Audet-Fortin 2017-07-05 11:55:24 UTC Comment hidden (obsolete)
Comment 56 Philippe Audet-Fortin 2017-07-05 19:54:01 UTC Comment hidden (obsolete)
Comment 57 Katrin Fischer 2017-10-07 20:42:03 UTC Comment hidden (obsolete)
Comment 58 Roch D'Amour 2018-02-28 23:19:47 UTC Comment hidden (obsolete)
Comment 59 sandboxes@biblibre.com 2018-03-14 15:30:12 UTC
Patch tested with a sandbox, by delaye <stephane.delaye@biblibre.com>
Comment 60 sandboxes@biblibre.com 2018-03-14 15:30:41 UTC Comment hidden (obsolete)
Comment 61 Katrin Fischer 2018-04-16 06:22:59 UTC Comment hidden (obsolete)
Comment 62 Katrin Fischer 2018-05-16 23:35:39 UTC
*** Bug 9305 has been marked as a duplicate of this bug. ***
Comment 63 Charles Farmer 2018-08-13 20:40:24 UTC Comment hidden (obsolete)
Comment 64 Charles Farmer 2018-08-13 20:41:01 UTC Comment hidden (obsolete)
Comment 65 Charles Farmer 2018-08-13 20:41:09 UTC Comment hidden (obsolete)
Comment 66 Charles Farmer 2018-08-13 20:57:24 UTC
New patch to fix, in part, some problems raised by QA.

> Why only changes to the ...style-us and not the other templates?

If I look at it chronologically, this patch was first submitted at a time when no {de,fr} templates existed. Commits for these templates came in 2015 and 2017, and I couldn't say when they were added to the master. It's no excuse for not reworking the patch, if that's what QA needs, but at least there's a rationale: it was rebased over and over without extending what had been done.

> What has this to do with making it possible for adults to be guarantors? ;)

That looks like a prime candidate for an optional patch, I concede. But some of the 'extra' developments in this patch were naturally asked by our clients when we offered linking adults to adults. I do think there are merits to keeping these functionalities together (though perhaps not as far as keeping the example you quoted).

> If we set it on the patron category, why check here for I and A?

Indeed, it's information easily retrievable from the $patron object already sent to the template. Worst of all, it's not even used by any template linked to moremember.pl :/

I feel like that echoes my previous comment about it being an old cookie: it has been rebased many times to quickly offer something to test, but at the cost of not being rewritten everytime to leverage new functions/variables/developments.
Comment 67 Pierre-Marc Thibault 2018-09-24 20:06:00 UTC Comment hidden (obsolete)
Comment 68 Pierre-Marc Thibault 2018-09-24 20:11:57 UTC Comment hidden (obsolete)
Comment 69 Pierre-Marc Thibault 2018-09-24 20:12:22 UTC Comment hidden (obsolete)
Comment 70 Pierre-Marc Thibault 2018-09-24 20:27:47 UTC Comment hidden (obsolete)
Comment 71 Pierre-Marc Thibault 2018-09-24 20:27:58 UTC Comment hidden (obsolete)
Comment 72 Katrin Fischer 2018-09-26 05:28:45 UTC
> > Why only changes to the ...style-us and not the other templates?
> 
> If I look at it chronologically, this patch was first submitted at a time
> when no {de,fr} templates existed. Commits for these templates came in 2015
> and 2017, and I couldn't say when they were added to the master. It's no
> excuse for not reworking the patch, if that's what QA needs, but at least
> there's a rationale: it was rebased over and over without extending what had
> been done.

I see the follow-up doesn't contain changes to the other address format files still, could you please check?

> > What has this to do with making it possible for adults to be guarantors? ;)
> 
> That looks like a prime candidate for an optional patch, I concede. But some
> of the 'extra' developments in this patch were naturally asked by our
> clients when we offered linking adults to adults. I do think there are
> merits to keeping these functionalities together (though perhaps not as far
> as keeping the example you quoted).

I understand that it happens, my main point is documentation. By slipping in features into bugs without documenting them at least in the commit message, they often get lost and end up undocumented, especially when they don't fit the bugs description which is often all people look at in release notes.

I hope to see this signed off soon!
Comment 73 Martin Renvoize 2018-10-29 12:12:37 UTC
I took a quick look at the revised code and I'm afraid it still looks like some additional cleanup is required.

1) C4::Utils::Datatables::Members::search seems to duplicate code which has been modified in the interim.
2) Not sure how 'AddPatronLists' syspref comes into this?
3) I also agree this would be significantly simpler to SO and QA if it were split into two bugs.
Comment 74 Maryse Simard 2019-02-13 23:04:57 UTC Comment hidden (obsolete)
Comment 75 Pierre-Marc Thibault 2019-04-19 18:03:02 UTC Comment hidden (obsolete)
Comment 76 Pierre-Marc Thibault 2019-04-19 18:03:07 UTC Comment hidden (obsolete)
Comment 77 Pierre-Marc Thibault 2019-04-19 18:03:12 UTC Comment hidden (obsolete)
Comment 78 Pierre-Marc Thibault 2019-04-19 18:03:17 UTC Comment hidden (obsolete)
Comment 79 Pierre-Marc Thibault 2019-04-19 18:09:58 UTC
(In reply to Martin Renvoize from comment #73)
> I took a quick look at the revised code and I'm afraid it still looks like
> some additional cleanup is required.
> 
> 1) C4::Utils::Datatables::Members::search seems to duplicate code which has
> been modified in the interim.
> 2) Not sure how 'AddPatronLists' syspref comes into this?
> 3) I also agree this would be significantly simpler to SO and QA if it were
> split into two bugs.

1) I added a new patch which removes the duplicated code.
2) AddPatronLists doesn't come into this. If it has been added, it has been removed in the next patch.
Comment 80 Katrin Fischer 2019-04-28 23:09:04 UTC
*** Bug 6922 has been marked as a duplicate of this bug. ***
Comment 81 Nadine Pierre 2019-06-06 19:00:37 UTC Comment hidden (obsolete)
Comment 82 Nadine Pierre 2019-06-06 19:00:46 UTC Comment hidden (obsolete)
Comment 83 Nadine Pierre 2019-06-06 19:00:52 UTC Comment hidden (obsolete)
Comment 84 Nadine Pierre 2019-06-06 19:01:00 UTC Comment hidden (obsolete)
Comment 85 Nadine Pierre 2019-06-06 19:03:01 UTC
I rebased the patches against master.
Ready to be tested.
Comment 86 Hayley Mapley 2019-06-11 03:33:53 UTC Comment hidden (obsolete)
Comment 87 Hayley Mapley 2019-06-11 03:33:59 UTC Comment hidden (obsolete)
Comment 88 Hayley Mapley 2019-06-11 03:34:06 UTC Comment hidden (obsolete)
Comment 89 Hayley Mapley 2019-06-11 03:34:12 UTC Comment hidden (obsolete)
Comment 90 Katrin Fischer 2019-09-02 06:12:10 UTC
I am sorry, but there are a lot of conflicts now with this patch.
With bug 14570 pushed, this might need to be reworked.
Comment 91 Maryse Simard 2019-10-01 20:02:23 UTC Comment hidden (obsolete)
Comment 92 Maryse Simard 2019-10-01 20:03:07 UTC Comment hidden (obsolete)
Comment 93 Maryse Simard 2019-10-01 20:03:13 UTC Comment hidden (obsolete)
Comment 94 Maryse Simard 2019-10-01 20:03:19 UTC Comment hidden (obsolete)
Comment 95 Maryse Simard 2019-10-01 20:03:24 UTC Comment hidden (obsolete)
Comment 96 Maryse Simard 2019-10-01 20:03:30 UTC Comment hidden (obsolete)
Comment 97 Maryse Simard 2019-10-01 20:03:35 UTC Comment hidden (obsolete)
Comment 98 Maryse Simard 2019-10-01 20:07:35 UTC
I rebased the patches. The code has changed a lot with bug 14570, please retest.

I've also made some fixes and changed the new system preference AdditionalGuarantorField to GuarantorFields. This way, the system can be set so no information is transfered to the guarantee while adding a guarantor. The test plan for this change is in the patch's commit message.
Comment 99 Martin Renvoize 2019-10-25 16:25:19 UTC
This will need to update the cleanup routine introduced in bug 17168.
Comment 100 Maryse Simard 2019-11-13 21:11:03 UTC Comment hidden (obsolete)
Comment 101 Maryse Simard 2019-11-27 16:18:54 UTC
Created attachment 95853 [details] [review]
Bug 12446: Enable an adult to have a guarantor

Conflicts have been fixed. Also attributes in system preference : AdditionalGuarantorField are now displayed correctly in all scenarios.

I have rebased every patch.
This includes all the features from the previous patches.

Removed the dollar signs and stopped using C4::Category

On the patron category page, there is a new field to specify whether patrons can be guarantee or not.
The atomic update SQL script adds this new field and sets it to true for categories of type "C" and "P".

1) Apply the patch
2) Run updatedatabase.pl to add AdditionalGuarantorField to preferences and canbeguarantee to categories.
3) Go to patron category administration, edit a category and change the value of Can Be Guarantee to yes (for Child and Professional it should have been set by default)
4) See below to test various scenarios

- Enable an adult to have a guarantor:
0.0) Create or edit a patron category of type 'A'.
0.1) Set the "Can be guarantee" field to "Yes" and save.
1) Select an adult patron -> details tab;
2) Click edit button;
3) validate "guarantor information" fieldset;

- Shows guarantees' fines in the guarantor's page:
4) Click Set to patron button and select a guarantor;
5) Generate a fine;
6) Select guarantor patron -> details tab;
7) Validate guarantee's fine information.

- Transfer some guarantor's information to the guarantee while adding a guarantee:
0) Select an adult patron;
1) Insert all information;
2) Click Add guarantee bouton;
3) Validate Garantor information/Main address/Contact fieldsets filled;

- Transfer some guarantor's information to the guarantee while creating a new patron
0) Select an adult patron;
1) Insert all information;
2) Add a new adult patron;
3) Click "Set to patron" button;
4) Select the adult patron from 0);
5) Validate Garantor information/Main address/Contact fieldsets filled;

- Transfer guarantor's alternate address/contact to the guarantee while creating a new patron:
0) Search AdditionalGuarantorField preference;
1) Insert some additional database columns from alternate address/contact to be transferred from guarantor;
2) Select an adult patron;
3) Insert all information;
4) Add a new adult patron;
5) Click "Set to patron" button;
6) Select the adult patron from 0);
7) Validate Garantor information/Main address/Contact fieldsets filled;
8) Validate Garantor additional alternate address/contact filled;

- Transfer guarantor's alternate address/contact to the guarantee while adding a guarantee:
0) Search AdditionalGuarantorField preference;
1) Insert some additional database columns from alternate address/contact to be transferred from guarantor;
3) Select an adult patron;
4) Insert all information;
5) Click Add guarantee bouton;
6) Validate Garantor information/Main address/Contact fieldsets filled;
7) Validate Garantor additional alternate address/contact filled;

Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>
Comment 102 Maryse Simard 2019-11-27 16:18:59 UTC
Created attachment 95854 [details] [review]
Bug 12446: (QA follow-up) Cleaning, and fixing, old code carried accidentally

Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>
Comment 103 Maryse Simard 2019-11-27 16:19:05 UTC
Created attachment 95855 [details] [review]
Bug 12446: (follow-up) Show all guarantees on the guarantor detail page

Only one guarantee is shown on the guarantor detail page. The
guarantees list should be passed to the template as a ref.

Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>
Comment 104 Maryse Simard 2019-11-27 16:19:10 UTC
Created attachment 95856 [details] [review]
Bug 12446: (QA follow-up) QA tests fixing, cleaning code

This patch does 3 things :
- Remove old code in C4/Utils/DataTables/Members.pm.
- Fix the AdditionalGuarantorField dead link for the borrowers table schema.
- Remove unused variables so QA tests pass.

Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>
Comment 105 Maryse Simard 2019-11-27 16:19:15 UTC
Created attachment 95857 [details] [review]
Bug 12446: Schema update Category.pm
Comment 106 Maryse Simard 2019-11-27 16:19:20 UTC
Created attachment 95858 [details] [review]
Bug 12446: (follow-up) Remove mandatory transfered values

With the new system preference AdditionalGuarantorField, one can add
data to be transfered to the guarantee, but cannot remove default ones.

This patch renames the syspref to GuarantorFields and set hardcoded
default values as the preference's default.

To test:
1. After database update, check that the default value for the syspref
GuarantorFields contains these columns:
    streetnumber|address|address2|city|state|zipcode|country|
    branchcode|phonepro|mobile|email|emailpro|fax
2. Set the value as empty
3. Try adding a guarantor to a patron (from the guarantor's page or the
guarantee's): no values should be imported.
4. Check that it still works with different values for the preference.
Comment 107 Maryse Simard 2019-11-27 16:19:25 UTC
Created attachment 95859 [details] [review]
Bug 12446: (follow-up) Code cleaning

Some variable name change,
put database update in a perl file,
remove unused code
and other small fixes.
Comment 108 Maryse Simard 2019-11-27 16:19:31 UTC
Created attachment 95860 [details] [review]
Bug 12446: (follow-up) Update for Bug 17168
Comment 109 Maryse Simard 2019-11-27 16:20:37 UTC
Rebased the patches against current master.
Comment 110 Katrin Fischer 2020-02-19 21:56:42 UTC
Hi Maryse, small failure in the QA script:


1) QA script
 FAIL	installer/data/mysql/atomicupdate/bug_12446-EnableAdultGarantee.sql
   FAIL	  git manipulation
		The file has been added and deleted in the same patchset

 FAIL	installer/data/mysql/kohastructure.sql
   OK	  boolean_vs_tinyint
   OK	  charset_collate
   OK	  git manipulation
   FAIL	  tinyint_has_boolean_flag
		The new column categories.canbeguarantee is using TINYINT(1) as type but is not defined as boolean in the schema file (Koha/Schema/Result/Category.pm), see the SQL12 coding guideline

The first could be fixed by squashing the patches in question, but should only be done if they got the same author/tester lines.

2) Quick code review

Looks good so far, will continue testing with the follow-ups (switch back to signed-off)

Note: I feel that GuarantorFields and "Shows guarantees' fines in the guarantor's page" should have been separate features/bugs - they add even more code to check and work through and make this this harder to test and harder to advertise/document. The bug title/description is actually not what this patch does, but is what will show up in release notes. - Too late now I guess, but I think it's part of the reason this has not been moving as fast as hoped for. :(
Comment 111 Martin Renvoize 2020-04-15 15:36:23 UTC
Too late for 20.05 now.. it would be great to see some concerted effort on this near the beginning of the 20.11 cycle.. I agree with Katrin, this bug would have been much clearer as a set of dependant bugs.. easier to test and qa and better for advertising in the release notes.
Comment 112 Martha Fuerst 2020-06-29 18:53:32 UTC
Huntsville-Madison County is also interested in this bug. Some of it we got in 19.11 (fines) but the info transfer at creation and the ability for Adult patrons to be guarantees would be very helpful.

Thanks,

Marti Fuerst
Comment 113 Maryse Simard 2020-07-09 18:17:29 UTC
Created attachment 106753 [details] [review]
Bug 12446: Enable an adult to have a guarantor

Conflicts have been fixed. Also attributes in system preference : AdditionalGuarantorField are now displayed correctly in all scenarios.

I have rebased every patch.
This includes all the features from the previous patches.

Removed the dollar signs and stopped using C4::Category

On the patron category page, there is a new field to specify whether patrons can be guarantee or not.
The atomic update script adds this new field and sets it to true for categories of type "C" and "P".

1) Apply the patch
2) Run updatedatabase.pl to add AdditionalGuarantorField to preferences and canbeguarantee to categories.
3) Go to patron category administration, edit a category and change the value of Can Be Guarantee to yes (for Child and Professional it should have been set by default)
4) See below to test various scenarios

- Enable an adult to have a guarantor:
0.0) Create or edit a patron category of type 'A'.
0.1) Set the "Can be guarantee" field to "Yes" and save.
1) Select an adult patron -> details tab;
2) Click edit button;
3) validate "guarantor information" fieldset;

- Shows guarantees' fines in the guarantor's page:
4) Click Set to patron button and select a guarantor;
5) Generate a fine;
6) Select guarantor patron -> details tab;
7) Validate guarantee's fine information.

- Transfer some guarantor's information to the guarantee while adding a guarantee:
0) Select an adult patron;
1) Insert all information;
2) Click Add guarantee bouton;
3) Validate Garantor information/Main address/Contact fieldsets filled;

- Transfer some guarantor's information to the guarantee while creating a new patron
0) Select an adult patron;
1) Insert all information;
2) Add a new adult patron;
3) Click "Set to patron" button;
4) Select the adult patron from 0);
5) Validate Garantor information/Main address/Contact fieldsets filled;

- Transfer guarantor's alternate address/contact to the guarantee while creating a new patron:
0) Search AdditionalGuarantorField preference;
1) Insert some additional database columns from alternate address/contact to be transferred from guarantor;
2) Select an adult patron;
3) Insert all information;
4) Add a new adult patron;
5) Click "Set to patron" button;
6) Select the adult patron from 0);
7) Validate Garantor information/Main address/Contact fieldsets filled;
8) Validate Garantor additional alternate address/contact filled;

- Transfer guarantor's alternate address/contact to the guarantee while adding a guarantee:
0) Search AdditionalGuarantorField preference;
1) Insert some additional database columns from alternate address/contact to be transferred from guarantor;
3) Select an adult patron;
4) Insert all information;
5) Click Add guarantee bouton;
6) Validate Garantor information/Main address/Contact fieldsets filled;
7) Validate Garantor additional alternate address/contact filled;

Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz>
Comment 114 Maryse Simard 2020-07-09 18:17:36 UTC
Created attachment 106754 [details] [review]
Bug 12446: (follow-up) Remove mandatory transfered values

With the new system preference AdditionalGuarantorField, one can add
data to be transfered to the guarantee, but cannot remove default ones.

This patch renames the syspref to GuarantorFields and set hardcoded
default values as the preference's default.

To test:
1. After database update, check that the default value for the syspref
GuarantorFields contains these columns:
    streetnumber|address|address2|city|state|zipcode|country|
    branchcode|phonepro|mobile|email|emailpro|fax
2. Set the value as empty
3. Try adding a guarantor to a patron (from the guarantor's page or the
guarantee's): no values should be imported.
4. Check that it still works with different values for the preference.
Comment 115 Maryse Simard 2020-07-09 18:17:41 UTC
Created attachment 106755 [details] [review]
Bug 12446: (follow-up) Code cleaning

Some variable name change,
remove unused code
and other small fixes.
Comment 116 Maryse Simard 2020-07-09 18:17:47 UTC
Created attachment 106756 [details] [review]
Bug 12446: Schema update Category.pm
Comment 117 Maryse Simard 2020-07-09 18:17:55 UTC
Created attachment 106757 [details] [review]
Bug 12446: (follow-up) Update for Bug 17168
Comment 118 Maryse Simard 2020-07-09 18:20:04 UTC
I rebased the patches and merged the ones which were signed-off. By adding the atomic update changes to it, it took care of the first qa script error. For the second error, I directly altered the schema update patch to correct it.

The status has been set back to signed off, as requested.
Comment 119 Katrin Fischer 2020-07-30 21:42:33 UTC
Hi Maryse,

I am no expert on the guarantor features, mostly because we work with academic libraries that have no children and the limitation on the relationships at the moment. So I am interested in this one, but still learning how it all fits together.

1) Conflict of system preferences?

Current Koha has the PrefillGuaranteeField system preference that appears to achieve the same as GuarantorFields added by this patch set.

Can you please check and explain if there is a difference in behaviour?

Please adjust the commit message with the test plan (either remove or update the pref name)

2) Not only adults

I believe this patch goes even further than the initial description: it will enable patron categories from all category types to be able to act as guarantees (not only adults). So it will allow arbitrary relationships? We should adjust the  bug title, if I am right about this.

Maybe "Ability to allow guarantor relationship for all patron category types" ?

3) Children can be guarantors?

This seems to be the case before your patch set: Searching for a guarantor doesn't seem to have any limits. You can use child patrons as guarantor. 
No change required - just noting as this might be worth discussing separately.

4) Guarantees fines display

I am not quite happy with the table used for display, I think it doesn't work very well with the rest of the page. We also often get asked to hide anything fine related for libraries not using fines. 
It would be nice if the information only showed up when there are actually fines. We should at least add an id and maybe some classes. Attaching a screenshot later.

1) is the blocker here - please check!
Comment 120 Katrin Fischer 2020-07-30 21:43:17 UTC
Created attachment 107611 [details]
Unintentionally blue screenshot of the new fines display for guarantees
Comment 121 Katrin Fischer 2020-07-30 21:48:00 UTC
Hm another question: can you explain the changes to the various patron/member search related files?
Comment 122 Maryse Simard 2020-10-02 04:19:18 UTC
Created attachment 111052 [details] [review]
Bug 12446: Ability to allow guarantor relationship for all patron category types

This adds a new field "Can be guarantee" to patron categories so it
becomes possible for any category type to have a guarantor.

To test:
1) Have a patron category of type 'Adult' and one of type 'Child'
2) Confirm, by searching for the "Patron guarantor" fieldset in the
edit/create form, that:
    => a patron of the first category can't have a guarantor
    => a patron from the second category can
3) Apply patch and run updatedatabase.pl
4) Edit the categories and note the new "Can be guarantee" field
5) It should have been set to "yes" for the "Child" and to "no" for
the "Adult"
5) Repeat step 2. It should behave in the same way.
6) Edit the "Can be guarantee" for any of the category and check
that the fieldset only appears when "Can be guarantee" is set to "yes"
7) prove t/db_dependent/Patrons.t
    => tests should still pass
Comment 123 Maryse Simard 2020-10-02 04:19:39 UTC
Created attachment 111053 [details] [review]
Bug 12446: Schema update Category.pm
Comment 124 Maryse Simard 2020-10-02 04:20:47 UTC
It has been said a few times before that this would have been better splitted into different bugs. I agree with that and feel that now is a good time to do so, considering the changes which have to be done anyway because of the new PrefillGuaranteeField preference. Therefore this patch has been splitted into 3 different bugs. This one has been renamed to "Ability to allow guarantor relationship for all patron category types", as proposed, to better reflect what it does.

1) I wasn't aware of the new PrefillGuaranteeField preference, but it does indeed replace GuarantorFields. However, this patch also introduced a new use of the preference to transfer informations from guarantor to guarantee not only when creating a new guarantee, but also when adding a new guarantor relationship to an existing patron. This part of the patch has been moved to a new bug (Bug 26597).

> Hm another question: can you explain the changes to the various
> patron/member search related files?

The changes to search files are for this part of the patch.

4) I've decided to make a few changes to the way the fines are displayed, following the advice mentionned here. However, since I was splitting this already, I've moved these corrections to a new bug (Bug 26598).

Finally, since this is a big change in the content of the bug, I merged all patches and set it back to need signoff.
Comment 125 Katrin Fischer 2020-10-04 18:26:18 UTC
Hi Maryse, thx a lot for reworking and splitting these patches. I hope they will hit the QA queue soon.
Comment 126 David Nind 2020-10-06 18:53:05 UTC
Created attachment 111317 [details] [review]
Bug 12446: Ability to allow guarantor relationship for all patron category types

This adds a new field "Can be guarantee" to patron categories so it
becomes possible for any category type to have a guarantor.

To test:
1) Have a patron category of type 'Adult' and one of type 'Child'
2) Confirm, by searching for the "Patron guarantor" fieldset in the
edit/create form, that:
    => a patron of the first category can't have a guarantor
    => a patron from the second category can
3) Apply patch and run updatedatabase.pl
4) Edit the categories and note the new "Can be guarantee" field
5) It should have been set to "yes" for the "Child" and to "no" for
the "Adult"
5) Repeat step 2. It should behave in the same way.
6) Edit the "Can be guarantee" for any of the category and check
that the fieldset only appears when "Can be guarantee" is set to "yes"
7) prove t/db_dependent/Patrons.t
    => tests should still pass

Signed-off-by: David Nind <david@davidnind.com>
Comment 127 David Nind 2020-10-06 18:53:20 UTC
Created attachment 111318 [details] [review]
Bug 12446: Schema update Category.pm

Signed-off-by: David Nind <david@davidnind.com>
Comment 128 Kyle M Hall 2020-10-16 12:55:47 UTC
If I use the "Add guarantee" button, I can create a guarantee of any patron type, regardless of "can be guarantee" setting for that patron category. It makes sense to filter the patron category pulldown to remove categories that cannot be guarantees. Can you add that?
Comment 129 Maryse Simard 2020-10-21 14:25:27 UTC
Created attachment 112102 [details] [review]
Bug 12446: Limit available categories when using "Add guarantee" button

Test plan:
1) Have some patron categories that can and cannot be guarantee
2) Visit a patron's account and click the "Add guarantee" button
3) In the "category" dropdown, note that all categories are available
4) Apply this patch
5) Repeat step 2 and 3; the dropdown now only contains the categories
for which "can be guarantee" is set to "Yes".
Comment 130 Kyle M Hall 2020-10-22 10:58:04 UTC
Created attachment 112160 [details] [review]
Bug 26751: Fix fatal exception if only one repo defined

This patch fixes this nasty problem with how XML is parsed: if more than
one entry is found, repo => [] is generated. If only one, repo => {}.

To test:
1. Have more than one entries in the plugins_repo entry:

<plugin_repos>
    <repo>
        <name>ByWater Solutions</name>
        <org_name>bywatersolutions</org_name>
        <service>github</service>
    </repo>
    <repo>
        <name>Theke Solutions</name>
        <org_name>thekesolutions</org_name>
        <service>github</service>
    </repo>
    <repo>
        <name>PTFS Europe</name>
        <org_name>ptfs-europe</org_name>
        <service>github</service>
    </repo>
 </plugin_repos>

2. Search for plugins
=> SUCCESS: Searching works!
3. Leave only one entry
4. Restart all to refresh things
5. Try searching for plugins
=> FAIL: Fatal error
6. Apply this patch
7. Repeat 5
=> SUCCESS: It doesn't explode anymore!
8. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 131 Kyle M Hall 2020-10-22 10:58:26 UTC
Created attachment 112161 [details] [review]
Bug 12446: Ability to allow guarantor relationship for all patron category types

This adds a new field "Can be guarantee" to patron categories so it
becomes possible for any category type to have a guarantor.

To test:
1) Have a patron category of type 'Adult' and one of type 'Child'
2) Confirm, by searching for the "Patron guarantor" fieldset in the
edit/create form, that:
    => a patron of the first category can't have a guarantor
    => a patron from the second category can
3) Apply patch and run updatedatabase.pl
4) Edit the categories and note the new "Can be guarantee" field
5) It should have been set to "yes" for the "Child" and to "no" for
the "Adult"
5) Repeat step 2. It should behave in the same way.
6) Edit the "Can be guarantee" for any of the category and check
that the fieldset only appears when "Can be guarantee" is set to "yes"
7) prove t/db_dependent/Patrons.t
    => tests should still pass

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 132 Kyle M Hall 2020-10-22 10:58:34 UTC
Created attachment 112162 [details] [review]
Bug 12446: Schema update Category.pm

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 133 Kyle M Hall 2020-10-22 10:58:40 UTC
Created attachment 112163 [details] [review]
Bug 12446: Limit available categories when using "Add guarantee" button

Test plan:
1) Have some patron categories that can and cannot be guarantee
2) Visit a patron's account and click the "Add guarantee" button
3) In the "category" dropdown, note that all categories are available
4) Apply this patch
5) Repeat step 2 and 3; the dropdown now only contains the categories
for which "can be guarantee" is set to "Yes".

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 134 Jonathan Druart 2020-11-10 08:23:47 UTC
Please retest, there is something wrong here.

"Add guarantee" button and I get:
"""
Cannot add patron

There are no patron categories defined. Please add a patron category.
"""
Comment 135 Jonathan Druart 2020-11-10 08:27:27 UTC
It's coming from the default value in DB, why did you default to 0? I think it should default to 1.
Comment 136 Jonathan Druart 2020-11-10 08:30:32 UTC
Also, Maryse, a quick note: the DBIC schema should be in a separate patch, but if you have specific change to add to the file (like here the is_boolean flag), it should be on another patch as well. I will take care of it for this time.
Comment 137 David Cook 2020-11-11 01:21:47 UTC
(In reply to Jonathan Druart from comment #135)
> It's coming from the default value in DB, why did you default to 0? I think
> it should default to 1.

I think defaulting to 1 would be logical, but that would also be a change in system behaviour, since previously only patron categories C and P could be guaranteed it seems. 

Maybe it's a case where the default should be 1, but we set it to 0 for non-C and non-P patrons when doing a database update...
Comment 138 Katrin Fischer 2020-11-11 21:11:02 UTC
(In reply to David Cook from comment #137)
> (In reply to Jonathan Druart from comment #135)
> > It's coming from the default value in DB, why did you default to 0? I think
> > it should default to 1.
> 
> I think defaulting to 1 would be logical, but that would also be a change in
> system behaviour, since previously only patron categories C and P could be
> guaranteed it seems. 
> 
> Maybe it's a case where the default should be 1, but we set it to 0 for
> non-C and non-P patrons when doing a database update...

But that's exactly what this patch does:
$dbh->do("UPDATE categories SET canbeguarantee = 1 WHERE category_type = 'P' OR category_type = 'C'");

Jonathan, do you have neither P nor C categories maybe? Maybe we should hide the option then intead.

Note: with the normal sample data that should be working.
Comment 139 David Cook 2020-11-11 22:10:57 UTC
(In reply to Katrin Fischer from comment #138)
> (In reply to David Cook from comment #137)
> > (In reply to Jonathan Druart from comment #135)
> > > It's coming from the default value in DB, why did you default to 0? I think
> > > it should default to 1.
> > 
> > I think defaulting to 1 would be logical, but that would also be a change in
> > system behaviour, since previously only patron categories C and P could be
> > guaranteed it seems. 
> > 
> > Maybe it's a case where the default should be 1, but we set it to 0 for
> > non-C and non-P patrons when doing a database update...
> 
> But that's exactly what this patch does:
> $dbh->do("UPDATE categories SET canbeguarantee = 1 WHERE category_type = 'P'
> OR category_type = 'C'");
> 

No, that does the opposite of what I describe. I'm saying that the schema default should be 1 but then there should be a update patch to do the following:

 $dbh->do("UPDATE categories SET canbeguarantee = 0 WHERE category_type NOT IN ('P','C');

It's a subtle but significant difference.
Comment 140 Katrin Fischer 2020-11-11 22:35:46 UTC
Hm ok, other way aorund. But I think the more common case is that you'd want 0 not 1 (no guarantors as default) - so I am not sure why we should change the default on the table?
Comment 141 David Cook 2020-11-11 22:46:42 UTC
(In reply to Katrin Fischer from comment #140)
> Hm ok, other way aorund. But I think the more common case is that you'd want
> 0 not 1 (no guarantors as default) - so I am not sure why we should change
> the default on the table?

Ah that's an interesting perspective. I hadn't thought of that. I was thinking how for new installs we would want to allow guarantors by default.

That said, we actually only have 1 out of 100 libraries who have adults guaranteeing other adults. 

Yeah I withdraw my earlier remarks. I think it's probably fine the way it is now then.
Comment 142 Jonathan Druart 2020-11-12 08:46:22 UTC
(In reply to Katrin Fischer from comment #138)
> (In reply to David Cook from comment #137)
> > (In reply to Jonathan Druart from comment #135)
> > > It's coming from the default value in DB, why did you default to 0? I think
> > > it should default to 1.
> > 
> > I think defaulting to 1 would be logical, but that would also be a change in
> > system behaviour, since previously only patron categories C and P could be
> > guaranteed it seems. 
> > 
> > Maybe it's a case where the default should be 1, but we set it to 0 for
> > non-C and non-P patrons when doing a database update...
> 
> But that's exactly what this patch does:
> $dbh->do("UPDATE categories SET canbeguarantee = 1 WHERE category_type = 'P'
> OR category_type = 'C'");
> 
> Jonathan, do you have neither P nor C categories maybe? Maybe we should hide
> the option then intead.
> 
> Note: with the normal sample data that should be working.

On a new install (reset_all so sample data):
/cgi-bin/koha/members/moremember.pl?borrowernumber=5

With the patch:
Click "Add guarantee" and get 
"Cannot add patron
There are no patron categories defined. Please add a patron category."

Without the patch, on master:
Click "Add guarantee" and get the "add patron form"
/members/memberentry.pl?op=add&guarantor_id=5&category_type=C
Comment 143 Maryse Simard 2020-11-12 21:44:59 UTC
Created attachment 113583 [details] [review]
Bug 12446: Add canbeguarantee value to patron categories sample data

This patch changes sample data so patron categories of type 'C' or
'P' can be guarantees by default.
Comment 144 Maryse Simard 2020-11-12 21:45:20 UTC
I decided to keep the default value in the database as 0 because I feel this would be the most common case. Instead, what I did was add "canbeguarantee" values in the sample data used for fresh installs. This, plus the database update, should make sure patron categories of type 'C' and 'P' can be guarantees by default for both new and updated installs.

As for the "Add guarantee" button displaying the "There are no patron categories defined." error, this is due to the last patch wich limit categories to the ones which can be guarantees. It seems normal to me that adding a guarantee would fail if no patron categories can be used. If this is not ok, what we could do would be to either hide the button if there is no categories available or use a more specific error message in that particular case.