Description
Maxime Pelletier
2012-08-07 15:50:51 UTC
Created attachment 11424 [details] [review] Add new field serial lifespan Patch sponsored by the CCSR ( http://www.ccsr.qc.ca ). The new field is in the Add a new subscription page of the serials, above the Library field. The patch was based on 3.8.3. Created attachment 11437 [details] [review] 8587 Add serial subscript lifespan Add lifespan to serial subscriptions Sponsored-by: CCSR ( http://www.ccsr.qc.ca ) Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> QA comments: Hi Maxime, 1/+ `seriallifespan` TINYINT(1) DEFAULT -1 You cannot store '10' into a TINYINT(1) 2/ 3 loops can be created to avoid a list of 10 lines: [% FOREACH i IN [ 2 .. 10 ] %] <option value="[% i %]" [% IF ( seriallifespan == [% i %] ) %]selected="selected"[% END %]>[% i %] years</option> [% END %] Furthermore you cannot write this line :-/ (see Bug 8407) And: [% IF seriallifespan == -1 %] <span>Immortal</span> [% ELSIF ( seriallifespan == 1 ) %] <span>1 year</span> [% ELSIF ( seriallifespan >= 10 ) %] [% FOREACH i IN [ 2 .. 10 ] %] <span>[% seriallifespan %] years</span> [% END %] [% END %] Something like that, I didn't test. 3/ Fonctionnaly what is the goal of this field? Indeed, you don't use this field, just store and display. Failed QA for 1 and 2 (In reply to comment #4) > QA comments: > > Hi Maxime, > > 1/+ `seriallifespan` TINYINT(1) DEFAULT -1 > You cannot store '10' into a TINYINT(1) > > 2/ 3 loops can be created to avoid a list of 10 lines: > [% FOREACH i IN [ 2 .. 10 ] %] > <option value="[% i %]" [% IF ( seriallifespan == [% i %] ) > %]selected="selected"[% END %]>[% i %] years</option> > [% END %] > > Furthermore you cannot write this line :-/ (see Bug 8407) > > And: > [% IF seriallifespan == -1 %] > <span>Immortal</span> > [% ELSIF ( seriallifespan == 1 ) %] > <span>1 year</span> > [% ELSIF ( seriallifespan >= 10 ) %] > [% FOREACH i IN [ 2 .. 10 ] %] > <span>[% seriallifespan %] years</span> > [% END %] > [% END %] > > Something like that, I didn't test. > > 3/ Fonctionnaly what is the goal of this field? Indeed, you don't use this > field, just store and display. > > Failed QA for 1 and 2 I think its just 2, tt inside a tag that is a problem, tinyint(1) is 0 - 255 unsigned and -128 to 127 signed. select seriallifespan from subscription; +----------------+ | seriallifespan | +----------------+ | 10 | +----------------+ (I tested that when signing off) (In reply to comment #5) > I think its just 2, tt inside a tag that is a problem, tinyint(1) is 0 - 255 > unsigned and -128 to 127 signed. Oups yes, you are right ! (In reply to comment #4) > QA comments: > > Hi Maxime, > > 2/ 3 loops can be created to avoid a list of 10 lines: > [% FOREACH i IN [ 2 .. 10 ] %] > <option value="[% i %]" [% IF ( seriallifespan == [% i %] ) > %]selected="selected"[% END %]>[% i %] years</option> > [% END %] > > Furthermore you cannot write this line :-/ (see Bug 8407) > > And: > [% IF seriallifespan == -1 %] > <span>Immortal</span> > [% ELSIF ( seriallifespan == 1 ) %] > <span>1 year</span> > [% ELSIF ( seriallifespan >= 10 ) %] > [% FOREACH i IN [ 2 .. 10 ] %] > <span>[% seriallifespan %] years</span> > [% END %] > [% END %] > > Something like that, I didn't test. I have to disagree on this. I don't think hardcoding the number and just translating the word year/years is the right way to go. I'm sure there's a language out there where the number is after or something like that. Id rather have 10 lines than an if/elseif/else. I don't understand bug 8407. Maybe translation for [IF] inside options is broken inside loops, but it's working for me in this patch. In french I have : <option value="-1" [% IF ( seriallifespan == -1 ) %]selected="selected"[% END %]>Immortel</option>. Anyway that doesn't mather because : > > 3/ Fonctionnaly what is the goal of this field? Indeed, you don't use this > field, just store and display. Good question. We use it with a saved sql report here attached. Anyway I wasn't really expecting this to be accepted. I think i'm fine leaving it here as Failed QA and to be an option if something needs it. Created attachment 11447 [details]
saved sql report we use
(In reply to comment #7) > I have to disagree on this. I don't think hardcoding the number and just > translating the word year/years is the right way to go. I'm sure there's a > language out there where the number is after or something like that. I think it is possible to do that when you translate your po file. > I don't understand bug 8407. Maybe translation for [IF] inside options is > broken inside loops, but it's working for me in this patch. Yes, it works :) But the problem is the script xt/tt_valid.t does not accept this syntax. Then the test fails. > > 3/ Fonctionnaly what is the goal of this field? Indeed, you don't use this > > field, just store and display. > > Good question. We use it with a saved sql report here attached. Anyway I > wasn't really expecting this to be accepted. I think i'm fine leaving it > here as Failed QA and to be an option if something needs it. humm... ok :) But if this development is useful for some people it is a good idea to include it into Koha. Chris, what do you think about this development ? Do you think we want it ? Created attachment 11513 [details] [review] Patch that passes tests Well in any case, here's a new patch that passes the template test. A suggestion: in English would not 'Permanent' be better terminology than 'Immortal'? Immortal intimates divinity, not a quality often associated with serials, (Infernal might be many peoples view!) (In reply to comment #11) > A suggestion: in English would not 'Permanent' be better terminology than > 'Immortal'? Immortal intimates divinity, not a quality often associated > with serials, (Infernal might be many peoples view!) I was looking for a better word when translating and "permanent" is perfect! In fact I think I will also use it in the original french feature. Thanks! Created attachment 17600 [details] [review] New patch that should apply in master. Created attachment 19415 [details] [review] Bug 8587 - Add a serial lifespan field to serial subscriptions http://bugs.koha-community.org/show_bug.cgi?id=8587 Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 19416 [details] [review] Bug 8587 - Add a serial lifespan to serial subscriptions - Followup - Change seriallifespan to serial_lifespan for readability Patch applied cleanly, go forth and signoff Patch applied cleanly, go forth and signoff The sandbox you've requested is not ready. Some problems occurred applying patches from bug 8587: <h1>Something went wrong !</h1>Applying: Bug 8587 - Add a serial lifespan field to serial subscriptions Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging C4/Serials.pm CONFLICT (content): Merge conflict in C4/Serials.pm Auto-merging installer/data/mysql/kohastructure.sql CONFLICT (content): Merge conflict in installer/data/mysql/kohastructure.sql Auto-merging installer/data/mysql/updatedatabase.pl CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/serials/subscription-add.tt CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/serials/subscription-add.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/serials/subscription-detail.tt Auto-merging serials/subscription-add.pl CONFLICT (content): Merge conflict in serials/subscription-add.pl Auto-merging serials/subscription-detail.pl Failed to merge in the changes. Patch failed at 0001 Bug 8587 - Add a serial lifespan field to serial subscriptions When you have resolved this problem run git bz apply --continue. If you would prefer to skip this patch, instead run git bz apply --skip. To restore the original branch and stop patching run git bz apply --abort. Bug 8587 - Add a serial lifespan to serial subscriptions 19415 - Bug 8587 - Add a serial lifespan field to serial subscriptions 19416 - Bug 8587 - Add a serial lifespan to serial subscriptions - Followup - Change seriallifespan to serial_lifespan for readability Apply? [(y)es, (n)o, (i)nteractive] Patch left in /tmp/Bug-8587---Add-a-serial-lifespan-field-to-serial-s-TRyUC2.patch . Created attachment 30636 [details] [review] Bug 8587 - Add a serial lifespan field to serial subscriptions http://bugs.koha-community.org/show_bug.cgi?id=8587 Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 30637 [details] [review] Bug 8587 - Add a serial lifespan to serial subscriptions - Followup - Change seriallifespan to serial_lifespan for readability I have fixed merge conflicts. Some notes and suggestions: 1) (blocker) There is a merge marker left in and the patch seems to add a bit too much code in serials/subscription-detail.tt - please remove and check. 2) (trivial) Please add the bug number to the database update. 3) Suggestion: Why limit to 10 years? I think translation would work with a variable: <span>[% serial_lifespan %] years</span>. That should give the translators something like: %s years where the sequence can be changed. 4) Suggestion: It think it would be nice to include a change to the help file or a patch to the manual (separately) where it's explained that this field is not tied to any functionality, but for use in reports. We already have a subscription end date and a subscription length, so this could cause a bit of confusion. Created attachment 30953 [details] [review] Bug 8587 - Follow-up to Comment 22 1) Removed all useless code. 2) Bug number added in updatadatabase.pl . 3) I've reworked the input form. There is now a select box with 'Permanent' and 'Years' as values. A number input field appears when 'Years' is selected. Range is from 1 to n years. 4) Help message added in the intranet help. modified: installer/data/mysql/updatedatabase.pl modified: koha-tmpl/intranet-tmpl/prog/en/modules/help/serials/subscription-add.tt modified: koha-tmpl/intranet-tmpl/prog/en/modules/serials/subscription-add.tt modified: koha-tmpl/intranet-tmpl/prog/en/modules/serials/subscription-detail.tt Created attachment 41217 [details] [review] Bug 8587 - Add a serial lifespan field to serial subscriptions Adds a serial lifespan field to serial subscriptions. Allows a serial subscription to be considered "out of date" after the chosen time period. This version should now be both cleanly applicable and properly working. Test plan: 1) Apply patch 2) Run ./installer/data/mysql/updatedatabase.pl 3) Create subscription using the field "Serial lifespan" CONFLICT (content): Merge conflict in serials/subscription-add.pl Hi, we have a new feature now that will allow to add new fields to subscriptions - I wonder if this development is still needed or could be covered by the new feature. I just tested it by defining a new field 'serial lifespan'. It shows on the subscription details and on the edit form. Maxime, could you check if the new feature for adding additional fields to subscriptions can provide the same functionality that you were looking for with your patches? Hi Cate, The sponsor is satisfied with the new functionality. We won't be pursuing this bug in the future. This can be closed (although I know it can't technically, so I'll just remove us from the cc) Thx for the feedback! closing this bug "RESOLVED MOVED" |