https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30642 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #144457|1 |0 is obsolete| | --- Comment #7 from Martin Renvoize <martin.renvoize@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.