[Koha-bugs] [Bug 30642] We should record the renewal type (Automatic/Manual)

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

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

Martin Renvoize <martin.renvoize at ptfs-europe.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #144457|1                           |0
        is obsolete|                            |

--- Comment #7 from Martin Renvoize <martin.renvoize at ptfs-europe.com> ---
Comment on attachment 144457
  --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=144457
Bug 30642: Record renewal type

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

This is a great submission Matt.  I agree with Lucas's comment, we'll need to
use a regex to catch the file as the install process can put the file into
different locations depending on the install type... but otherwise the code is
working well.

A few minor code review comments added too.

::: installer/data/mysql/atomicupdate/bug_30642-add_renewal_type.pl
@@ +1,4 @@
> +use Modern::Perl;
> +
> +return {
> +    bug_number => "BUG_30642",

You only need the number here Matt ;P

@@ +8,5 @@
> +        my ($dbh, $out) = @$args{qw(dbh out)};
> +
> +        if( !column_exists( 'checkout_renewals', 'renewal_type' ) ) { 
> +          $dbh->do(q{
> +              ALTER TABLE checkout_renewals ADD COLUMN `renewal_type` varchar(9) NOT NULL AFTER `timestamp`

Thinking about it, this could be an ENUM rather than a varchar(9) perhaps?

Also, as we're adding a 'NOT NULL' we probably need to deal with existing
rows.. Especially as we've not added a DEFAULT..  perhaps 'DEFAULT 'MANUAL' and
ENUM('AUTO','MANUAL').

::: koha-tmpl/intranet-tmpl/prog/js/checkout_renewals_modal.js
@@ +20,4 @@
>          });
>      });
>      function createLi(renewal) {
> +        return '<li><span style="font-weight:bold">' + $datetime(renewal.timestamp) + '</span> ' + renewed + ' <span style="font-weight:bold">' + $patron_to_html(renewal.renewer) + '</span>' + renewed_type + ' <span style="font-weight:bold">' + renewal.renewal_type + '</span></li>';

This is interesting, I hadn't really considered it before today.

I imagine 'auto' renewals don't have a renewal.renewer associated..  worth a
little manual testing to see what happens there.

Finally.. translations.. we'll need to think a little about this.  With the
ENUM change suggested above we could template out the descriptive string and
use __('Manual') to expose the display strings to the translation system.

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