https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30642
Martin Renvoize <martin.renvo...@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #144457|1 |0 is obsolete| | --- Comment #7 from Martin Renvoize <martin.renvo...@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. _______________________________________________ Koha-bugs mailing list Koha-bugs@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/