Bug 7728 - Fixing subscription endddate inconsistency: should be empty when the subscription is running
Summary: Fixing subscription endddate inconsistency: should be empty when the subscrip...
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Serials (show other bugs)
Version: master
Hardware: All All
: P4 normal (vote)
Assignee: Paul Poulain
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-16 18:06 UTC by Koha Team Lyon 3
Modified: 2017-05-03 03:57 UTC (History)
13 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
histenddate box legend (6.33 KB, image/png)
2012-03-16 18:06 UTC, Koha Team Lyon 3
Details
Bug 7728: fix subs history end date consistency (1.36 KB, patch)
2015-12-09 13:31 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 7728: fix subs history end date consistency (1.41 KB, patch)
2015-12-17 14:48 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
BZ 7728 adding unit test (3.71 KB, patch)
2016-01-26 14:09 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 7728: fix subs history end date consistency (1.40 KB, patch)
2017-01-11 14:12 UTC, Julian Maurice
Details | Diff | Splinter Review
BZ 7728 adding unit test (3.71 KB, patch)
2017-01-11 14:12 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 7728: QA fixes (1.81 KB, patch)
2017-01-11 14:12 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 7728: Fix subs history end date consistency (1.50 KB, patch)
2017-03-17 12:40 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 7728: Adding unit test ReNewSubscription.t (3.82 KB, patch)
2017-03-17 12:40 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 7728: QA fixes (1.90 KB, patch)
2017-03-17 12:40 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 7728: [QA Follow-up] Fix POD whitespace (1.16 KB, patch)
2017-03-17 12:40 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Koha Team Lyon 3 2012-03-16 18:06:49 UTC
Created attachment 8240 [details]
histenddate box legend

May be I missed something but even if the subscription history displays the following legend below the histenddate box:
(if empty, subscription is still active)
the new subscription enddate is systematically copied as histenddate during the renew process as we can see in the ReNewSubscription function (line 1332 from Serial.pm) :
    $query = qq|
        UPDATE subscriptionhistory
        SET    histenddate=?
        WHERE  subscriptionid=?
    |;
    $sth = $dbh->prepare($query);
    $sth->execute( $enddate, $subscriptionid );
Comment 1 mathieu saby 2013-11-11 21:57:14 UTC
Hello Lyon3
We will soon renew subscriptions for next year. Can you tell me if it is still an issue?

Mathieu
Comment 2 Koha Team Lyon 3 2013-11-12 15:43:28 UTC
(In reply to mathieu saby from comment #1)
> Hello Lyon3
> We will soon renew subscriptions for next year. Can you tell me if it is
> still an issue?
> 
> Mathieu

yes it is on version 3.10.07 ,no changes.

Olivier
Comment 3 Jonathan Druart 2014-12-11 14:54:02 UTC
Olivier, what do you suggest?
Comment 4 Paul Poulain 2015-12-09 09:46:22 UTC
The Renewal of a subscription fills the subscriptionhistory.enddate field. This field is not filled when you create a subscription. It is supposed to keep track of the subscription end for ended subscription. The renewal should follow the same workflow as the creation.
Comment 5 Paul Poulain 2015-12-09 13:31:47 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2015-12-09 17:59:14 UTC
Please provide tests to highlight the changes ;)
Comment 7 beroud 2015-12-17 09:26:59 UTC
 I created a subscription, and in 'subscription-detail.pl' page, tab "Summary". No History end date was set
 I renewed the subscription => a date had been set
 I applied the patch
 I created another subscription
 I renewed it => no history end date had been set
I signed-off
Comment 8 sandboxes@biblibre.com 2015-12-17 14:48:14 UTC
Patch tested with a sandbox, by beroud <isabelle.beroud@univ-lyon3.fr>
Comment 9 sandboxes@biblibre.com 2015-12-17 14:48:35 UTC
Created attachment 45776 [details] [review]
Bug 7728: fix subs history end date consistency

Without this patch, there is an inconsistency in subscriptions:
 * a new subscription has no history end date set (as expected)
 * if you renew it, a date is set.

This patch removes the setting of the history end date during renewal

Test plan:
 * create a subscription, go to subscription-detail.pl page, tab "Summary". No History end date is set
 * renew the subscription => a date has been set
 * apply the patch
 * create another subscription
 * renew it => no history end date has been set
 * sign-off

Signed-off-by: beroud <isabelle.beroud@univ-lyon3.fr>
Comment 10 Paul Poulain 2015-12-17 15:00:19 UTC
Failing QA this patch, I have to write unit tests before it can be QAed.
Once I've added unit tests, I'll switch back to "signed-off"

(if someone else want to write UT, feel free !)
Comment 11 Paul Poulain 2016-01-26 14:09:19 UTC
Created attachment 47303 [details] [review]
BZ 7728 adding unit test

test plan:
 * run without the patch, the test will fail
 * run with the patch, the test will pass
Comment 12 Jonathan Druart 2016-02-02 11:09:03 UTC
Comment on attachment 47303 [details] [review]
BZ 7728 adding unit test

Review of attachment 47303 [details] [review]:
-----------------------------------------------------------------

1/ Should not we update the histenddate if it has been set?
2/ Could we imagine fixing existing DB entries?

::: t/db_dependent/Serials/ReNewSubscription.t
@@ +40,5 @@
> +
> +#my $mContext = new Test::MockModule('C4::Context');
> +#$mContext->mock( 'userenv', sub {
> +#    return { branch => $library->{branchcode} };
> +#});

Useless commented lines, please remove.

@@ +94,5 @@
> +# Actual testing starts here!
> +
> +# Renew the subscription and check that enddate has not been set
> +ReNewSubscription($subscription->{subscriptionid},'',"2016-01-01",'','',12,'');
> +my @history = Koha::Subscription::Histories->search( {subscriptionid => $subscription->{subscriptionid} } );

use
  Koha::Subscription::Histories->find( $subscription->{subscriptionid} )
when you already have the primary key.
Comment 13 Julian Maurice 2017-01-11 14:12:19 UTC
Created attachment 58813 [details] [review]
Bug 7728: fix subs history end date consistency

Without this patch, there is an inconsistency in subscriptions:
 * a new subscription has no history end date set (as expected)
 * if you renew it, a date is set.

This patch removes the setting of the history end date during renewal

Test plan:
 * create a subscription, go to subscription-detail.pl page, tab "Summary". No History end date is set
 * renew the subscription => a date has been set
 * apply the patch
 * create another subscription
 * renew it => no history end date has been set
 * sign-off

Signed-off-by: beroud <isabelle.beroud@univ-lyon3.fr>
Comment 14 Julian Maurice 2017-01-11 14:12:23 UTC
Created attachment 58814 [details] [review]
BZ 7728 adding unit test

test plan:
 * run without the patch, the test will fail
 * run with the patch, the test will pass
Comment 15 Julian Maurice 2017-01-11 14:12:27 UTC
Created attachment 58815 [details] [review]
Bug 7728: QA fixes

- Remove useless comments
- Use Koha::Objects::find instead of Koha::Objects::search
Comment 16 Julian Maurice 2017-01-11 14:26:54 UTC
(In reply to Jonathan Druart from comment #12)
> 1/ Should not we update the histenddate if it has been set?
I don't know what the expected behaviour is here. Any serials
expert around ?

> 2/ Could we imagine fixing existing DB entries?
We could imagine, but is it possible to know if an histenddate has been
set by a renewal or by hand ?
Comment 17 Marcel de Rooy 2017-03-17 12:40:34 UTC
Created attachment 61212 [details] [review]
Bug 7728: Fix subs history end date consistency

Without this patch, there is an inconsistency in subscriptions:
 * a new subscription has no history end date set (as expected)
 * if you renew it, a date is set.

This patch removes the setting of the history end date during renewal

Test plan:
 * create a subscription, go to subscription-detail.pl page, tab "Summary". No History end date is set
 * renew the subscription => a date has been set
 * apply the patch
 * create another subscription
 * renew it => no history end date has been set
 * sign-off

Signed-off-by: beroud <isabelle.beroud@univ-lyon3.fr>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 18 Marcel de Rooy 2017-03-17 12:40:40 UTC
Created attachment 61213 [details] [review]
Bug 7728: Adding unit test ReNewSubscription.t

Test plan:
 * run without the patch, the test will fail
 * run with the patch, the test will pass

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 19 Marcel de Rooy 2017-03-17 12:40:45 UTC
Created attachment 61214 [details] [review]
Bug 7728: QA fixes

- Remove useless comments
- Use Koha::Objects::find instead of Koha::Objects::search

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 20 Marcel de Rooy 2017-03-17 12:40:49 UTC
Created attachment 61215 [details] [review]
Bug 7728: [QA Follow-up] Fix POD whitespace

Resolves complaints from qa tools.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 21 Marcel de Rooy 2017-03-17 12:41:14 UTC
Two patches from Paul again; come, write a few more :)
Comment 22 Kyle M Hall 2017-03-31 13:31:52 UTC
Pushed to master for 17.05, thanks Paul, Marcel, Julian!
Comment 23 Katrin Fischer 2017-04-02 17:15:30 UTC
These patches have been pushed to 16.11.x and will be in 16.11.07.
.
Comment 24 Julian Maurice 2017-04-21 10:15:25 UTC
Pushed to 3.22.x for 3.22.20
Comment 25 Mason James 2017-05-03 03:57:21 UTC
Pushed to 16.05.x, for 16.05.12 release