[Koha-bugs] [Bug 12461] Add patron clubs feature

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

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

--- Comment #31 from Katrin Fischer <katrin.fischer at bsz-bw.de> ---
Comment on attachment 49121
  --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=49121
Bug 12461 - Add patron clubs feature

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

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

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

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

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

Unconditional warn - please remove.

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

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

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

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

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

Same as above - placeholders preferrable!

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

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

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

Please don't construct sentences (see above).

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

Switch to FA icon - I saw most of them were already, maybe missed this one?

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