Bug 8587 - Add a serial lifespan to serial subscriptions
Summary: Add a serial lifespan to serial subscriptions
Status: RESOLVED MOVED
Alias: None
Product: Koha
Classification: Unclassified
Component: Serials (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Maxime Beaulieu
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-08-07 15:50 UTC by Maxime Pelletier
Modified: 2022-05-11 19:53 UTC (History)
10 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Add new field serial lifespan (16.26 KB, patch)
2012-08-07 15:52 UTC, Maxime Pelletier
Details | Diff | Splinter Review
8587 Add serial subscript lifespan (16.24 KB, patch)
2012-08-08 08:17 UTC, Chris Cormack
Details | Diff | Splinter Review
saved sql report we use (1.18 KB, text/plain)
2012-08-08 14:52 UTC, Maxime Pelletier
Details
Patch that passes tests (16.19 KB, patch)
2012-08-09 19:54 UTC, Maxime Pelletier
Details | Diff | Splinter Review
New patch that should apply in master. (17.65 KB, patch)
2013-04-22 20:11 UTC, Maxime Pelletier
Details | Diff | Splinter Review
Bug 8587 - Add a serial lifespan field to serial subscriptions (17.82 KB, patch)
2013-07-05 14:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8587 - Add a serial lifespan to serial subscriptions - Followup - Change seriallifespan to serial_lifespan for readability (17.11 KB, patch)
2013-07-05 14:36 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8587 - Add a serial lifespan field to serial subscriptions (25.74 KB, patch)
2014-08-08 17:44 UTC, Maxime Beaulieu
Details | Diff | Splinter Review
Bug 8587 - Add a serial lifespan to serial subscriptions - Followup - Change seriallifespan to serial_lifespan for readability (15.80 KB, patch)
2014-08-08 17:44 UTC, Maxime Beaulieu
Details | Diff | Splinter Review
Bug 8587 - Follow-up to Comment 22 (23.45 KB, patch)
2014-08-18 20:26 UTC, Maxime Beaulieu
Details | Diff | Splinter Review
Bug 8587 - Add a serial lifespan field to serial subscriptions (13.90 KB, patch)
2015-07-28 15:16 UTC, Rémi Mayrand-Provencher
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Maxime Pelletier 2012-08-07 15:50:51 UTC
This field is mostly useful when it comes to weeding. Sharing it here hoping it could be useful to anyone.
Comment 1 Maxime Pelletier 2012-08-07 15:52:37 UTC Comment hidden (obsolete)
Comment 2 Maxime Pelletier 2012-08-07 15:53:55 UTC
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.
Comment 3 Chris Cormack 2012-08-08 08:17:44 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2012-08-08 10:51:15 UTC
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
Comment 5 Chris Cormack 2012-08-08 11:04:40 UTC
(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)
Comment 6 Jonathan Druart 2012-08-08 11:31:24 UTC
(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 !
Comment 7 Maxime Pelletier 2012-08-08 14:51:21 UTC
(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.
Comment 8 Maxime Pelletier 2012-08-08 14:52:37 UTC
Created attachment 11447 [details]
saved sql report we use
Comment 9 Jonathan Druart 2012-08-09 07:41:59 UTC
(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 ?
Comment 10 Maxime Pelletier 2012-08-09 19:54:41 UTC Comment hidden (obsolete)
Comment 11 Colin Campbell 2012-08-13 15:28:36 UTC
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!)
Comment 12 Maxime Pelletier 2012-08-13 15:42:15 UTC
(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!
Comment 13 Maxime Pelletier 2013-04-22 20:11:07 UTC Comment hidden (obsolete)
Comment 14 Kyle M Hall 2013-07-05 14:33:58 UTC Comment hidden (obsolete)
Comment 15 Kyle M Hall 2013-07-05 14:36:39 UTC Comment hidden (obsolete)
Comment 16 I'm just a bot 2013-09-29 05:30:35 UTC
Patch applied cleanly, go forth and signoff
Comment 17 I'm just a bot 2013-10-24 19:36:21 UTC
Patch applied cleanly, go forth and signoff
Comment 18 Koha Team University Lyon 3 2014-06-18 12:53:41 UTC
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 .
Comment 19 Maxime Beaulieu 2014-08-08 17:44:38 UTC Comment hidden (obsolete)
Comment 20 Maxime Beaulieu 2014-08-08 17:44:51 UTC Comment hidden (obsolete)
Comment 21 Maxime Beaulieu 2014-08-08 17:46:00 UTC
I have fixed merge conflicts.
Comment 22 Katrin Fischer 2014-08-17 20:38:52 UTC
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.
Comment 23 Maxime Beaulieu 2014-08-18 20:26:21 UTC Comment hidden (obsolete)
Comment 24 Rémi Mayrand-Provencher 2015-07-28 15:16:27 UTC
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"
Comment 25 Jonathan Druart 2015-12-28 11:47:47 UTC
CONFLICT (content): Merge conflict in serials/subscription-add.pl
Comment 26 Katrin Fischer 2015-12-29 22:38:46 UTC
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.
Comment 27 Katrin Fischer 2016-02-28 22:37:52 UTC
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?
Comment 28 Blou 2016-02-29 13:27:35 UTC
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)
Comment 29 Katrin Fischer 2016-02-29 13:30:18 UTC
Thx for the feedback! closing this bug "RESOLVED MOVED"