[Koha-bugs] [Bug 12446] Enable an adult to have a guarantor

[bugzilla-daemon at bugs.koha-community.org]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12446

--- Comment #61 from Katrin Fischer <katrin.fischer at bsz-bw.de> ---
Comment on attachment 72883
  --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=72883
Bug 12446 - Enable an adult to have a guarantor

Review of attachment 72883:
 --> (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=12446&attachment=72883)
-----------------------------------------------------------------

Hi Roch,

I've done some initial review, but I am not quite happy with this as is. The
biggest problem I see is that it does much more than the bug title advertises.
The flag on patron categories and the new preference to decide which patron
fields to copy should have been at least 2 bugs. There is also changes to the
display like displaying the account balance (which should probably use Price
TT?). Please consider splitting the bug when working on it again. 

1) QA script:
 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)

2) Spinter review:

::: installer/data/mysql/atomicupdate/bug_12446-EnableAdultGarantee.sql
@@ +6,5 @@
> +
> +-- ********* --
> +-- STRUCTURE --
> +-- ********* --
> +ALTER TABLE categories ADD COLUMN `canbeguarantee` tinyint(1) NOT NULL default '0';

Please use AFTER to declare where the column should be added to match with the
kohastructure statement. Because depending on when this is pushed, it might
lead to a different database structure between new and old installations
otherwise.

::: installer/data/mysql/kohastructure.sql
@@ +303,4 @@
>    `hidelostitems` tinyint(1) NOT NULL default '0', -- are lost items shown to this category (1 for yes, 0 for no)
>    `category_type` varchar(1) NOT NULL default 'A', -- type of Koha patron (Adult, Child, Professional, Organizational, Statistical, Staff)
>    `BlockExpiredPatronOpacActions` tinyint(1) NOT NULL default '-1', -- wheither or not a patron of this category can renew books or place holds once their card has expired. 0 means they can, 1 means they cannot, -1 means use syspref BlockExpiredPatronOpacActions
> +  `default_privacy` ENUM( 'default', 'never', 'forever' ) NOT NULL DEFAULT 'default', -- Default privacy setting for this patron category,

This change is not necessary, no comma needed after the comment.

::: installer/data/mysql/sysprefs.sql
@@ +6,5 @@
>  ('AcquisitionDetails', '1', '', 'Hide/Show acquisition details on the biblio detail page.', 'YesNo'),
>  ('AcqViewBaskets','user','user|branch|all','Define which baskets a user is allowed to view: his own only, any within his branch or all','Choice'),
>  ('AcqWarnOnDuplicateInvoice','0','','Warn librarians when they try to create a duplicate invoice','YesNo'),
> +('AdditionalGuarantorField','',NULL,'Additional fields name to be transfer from guarantor to guarantee.','free'),
> +('AddPatronLists','categorycode','categorycode|category_type','Allow user to choose what list to pick up from when adding patrons','Choice'),

Change to AddPatronLists seems misplaced here.

::: koha-tmpl/intranet-tmpl/prog/en/includes/member-main-address-style-us.inc
@@ +8,4 @@
>        <label for="streetnumber">
>        [% END %]
>        Street number: </label>
> +        <input type="text" id="streetnumber" name="streetnumber" size="5" value="[% streetnumber %]" />

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

::: koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt
@@ +154,3 @@
>                          [% IF logged_in_user.can_see_patron_infos( guarantee ) %]
> +                            <td><a href="/cgi-bin/koha/members/moremember.pl?borrowernumber=[% guarantee.borrowernumber %]">[% guarantee.firstname | html %] [% guarantee.surname | html %]</a></td>
> +                            <td style='text-align:right'>[% guarantee.account.balance %]</td>

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

::: members/moremember.pl
@@ +209,5 @@
> +    }
> +    $template->param( guarantees => @guarantees);
> +    $template->param( amounttot => sprintf("%.2f",$totalmount));
> +}
> +( $template->param( adultborrower => 1 ) ) if ( $category_type eq 'A' || $category_type eq 'I' );

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

-- 
You are receiving this mail because:
You are watching all bug changes.