Bug 17865 - If a subscription has no history end date, it shows as expired today in OPAC
Summary: If a subscription has no history end date, it shows as expired today in OPAC
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Serials (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Luke Honiss
QA Contact: Testopia
URL:
Whiteboard:
Keywords: Academy
Depends on:
Blocks:
 
Reported: 2017-01-09 09:26 UTC by Katrin Fischer
Modified: 2017-03-15 11:14 UTC (History)
7 users (show)

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


Attachments
Bug 17865 'If a subscription has no history end date, it shows as expired today in OPAC' fixed (1.12 KB, patch)
2017-01-17 02:37 UTC, Luke Honiss
Details | Diff | Splinter Review
Bug 17865 'If a subscription has no history end date, it shows as expired today in OPAC' fixed (1.12 KB, patch)
2017-01-17 02:38 UTC, Luke Honiss
Details | Diff | Splinter Review
Bug 17865 'If a subscription has no history end date, it shows as expired today in OPAC' fixed (2.99 KB, patch)
2017-01-18 20:58 UTC, Luke Honiss
Details | Diff | Splinter Review
Bug 17865 'If a subscription has no history end date, it shows as expired today in OPAC' (1.97 KB, patch)
2017-01-19 01:22 UTC, Luke Honiss
Details | Diff | Splinter Review
[SIGNED OFF] Bug 17865 'If a subscription has no history end date, it shows as expired today in OPAC' (2.05 KB, patch)
2017-02-17 15:44 UTC, Baptiste
Details | Diff | Splinter Review
Bug 17865 'If a subscription has no history end date, it shows as expired today in OPAC' (2.11 KB, patch)
2017-02-20 09:30 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2017-01-09 09:26:58 UTC
If there is no subscription histenddate (NULL) in subscriptionhistory, the subscription will show as expired today in the OPAC.

I think this happens becasue of this bit in GetSubscriptionsFromBiblionumber:


 451         if ( $subs->{enddate} eq '0000-00-00' ) {
 452             $subs->{enddate} = '';
 453         } else {
 454             $subs->{enddate} = output_pref( { dt => dt_from_string( $subs->{enddate}), dateonly => 1 } );
 455         }

I am not sure why this appears to return the current date, but it seems a good candidate to start the search.
Comment 1 Jonathan Druart 2017-01-09 10:23:35 UTC
Where is it displayed?
On opac-detail I see "Subscription from: 01/01/2017 to: now (current)"
Comment 2 Jonathan Druart 2017-01-09 10:31:13 UTC
Ok click on "More details" and arrived on /opac-serial-issues.pl

Since 3.00.00.106 we should not have 0000-00-00 anymore in subscription.enddate, so 
 451         if ( $subs->{enddate} eq '0000-00-00' ) {
should be
 451         if ( not defined $subs->{enddate} ) {
Comment 3 Luke Honiss 2017-01-17 02:37:11 UTC
Created attachment 59049 [details] [review]
Bug 17865 'If a subscription has no history end date, it shows as expired today in OPAC' fixed

--TEST PLAN--
1. Find a record in the OPAC with a serial subscription
2. Check more details under subscription tab
3. The expiration date should be incorrect
4. Apply patch
5. Reload and now the expiration date should now be correct
Comment 4 Luke Honiss 2017-01-17 02:38:04 UTC Comment hidden (obsolete)
Comment 5 Katrin Fischer 2017-01-18 02:41:06 UTC
I am not sure why, but Jonathan's suggested change doesn't seem to do the trick: 
For the subscription without history end date, it still shows the current date in the OPAC for me. 
For a subscription with an end date in the past, the display is correct. 
For a subscription end date in the future, the note shows as well.

I have restarted Plack to make sure that's not the problem.
Comment 6 Jonathan Druart 2017-01-18 08:36:09 UTC
Actually it's histenddate that is displayed, not enddate.

Katrin, what's the expected behaviour?
Comment 7 Katrin Fischer 2017-01-18 10:45:07 UTC
You are right, I was not clear in my description, we are always talking about the history end date here.

If it's empty, it should not display the subscription as expired - that still happened to my surprised when I applied the patch. I was wondering if we should also check for a subscription history end date in the future, but that seems outside of scope here.
Comment 8 Luke Honiss 2017-01-18 20:58:54 UTC Comment hidden (obsolete)
Comment 9 Katrin Fischer 2017-01-18 22:34:13 UTC
This is much better, I think the date checking now works correctly.

I have a bit worried that patrons won't know what "The subscription has no history end date" means. If the date is empty, it means that the subsription is possibly still active - I think it would be better not to display something in this case. What do you think?
Comment 10 Luke Honiss 2017-01-18 23:57:24 UTC
I thought it may be a bit confusing. In the staff checkout it shows a similar field as being blank, but having the title for that field is "History end date" which is why I put the message as that.
Comment 11 Katrin Fischer 2017-01-19 00:02:41 UTC
I think for the librarians it probably makes sense - more or less. Serials is one of the more complicated modules of Koha. I think for the patrons it might be confusing, that's why I think maybe only show the expired message, but not the other message. 

I was also pondering to rephrase it to something like "The library still subscribes to this serial", but I am not sure if the history end date is always set if a subscription is not renewed - so that might not be right in all cases.
Comment 12 Luke Honiss 2017-01-19 01:07:37 UTC
I'll just remove the line. There should hopefully be no confusion that way
Comment 13 Luke Honiss 2017-01-19 01:22:12 UTC
Created attachment 59199 [details] [review]
Bug 17865 'If a subscription has no history end date, it shows as expired today in OPAC'

--TEST PLAN--
1) View a subscription with no history end date
2) Search for an item with a subcription in OPAC
3) Under subscription tab click more details
4) The end date will be the current date
5) Apply patch and refresh
6) The end date will not be shown
Comment 14 M. Tompsett 2017-02-13 02:34:59 UTC
Comment on attachment 59199 [details] [review]
Bug 17865 'If a subscription has no history end date, it shows as expired today in OPAC'

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

My concern with the conditions modified is the poor date handling of MySQL. Sometimes unset dates were 0000-00-00, sometimes they were null. When and how aren't the issue, but for those still using MySQL 5.5 in less than strict mode, defined might not be sufficient. In MySQL 5.7 or strict mode, defined is perfectly okay.
Comment 15 Katrin Fischer 2017-02-13 06:30:12 UTC
The installation I have seen this in was using Debian, so I think the less strict MySQL and all the dates were empty, not invalid ones. I think it should be ok.
Comment 16 Baptiste 2017-02-17 15:44:44 UTC
Created attachment 60430 [details] [review]
[SIGNED OFF] Bug 17865 'If a subscription has no history end date, it shows as expired today in OPAC'

--TEST PLAN--
1) View a subscription with no history end date
2) Search for an item with a subcription in OPAC
3) Under subscription tab click more details
4) The end date will be the current date
5) Apply patch and refresh
6) The end date will not be shown

Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>
Comment 17 Jonathan Druart 2017-02-20 09:30:17 UTC
Created attachment 60453 [details] [review]
Bug 17865 'If a subscription has no history end date, it shows as expired today in OPAC'

--TEST PLAN--
1) View a subscription with no history end date
2) Search for an item with a subcription in OPAC
3) Under subscription tab click more details
4) The end date will be the current date
5) Apply patch and refresh
6) The end date will not be shown

Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 18 Jonathan Druart 2017-02-20 09:30:44 UTC
Note that this patch also fixes the same problem on /serials/serial-issues.pl
Comment 19 Kyle M Hall 2017-02-21 19:55:21 UTC
Pushed to master for 17.05, thanks Luke!
Comment 20 Katrin Fischer 2017-02-21 21:03:53 UTC
This patch has been pushed to 16.11.x and will be in 16.11.04.

Thx Luke!
Comment 21 Mason James 2017-02-23 20:37:58 UTC
Pushed to 16.05.x, for 16.05.10 release
Comment 22 Julian Maurice 2017-03-15 11:14:24 UTC
Pushed to 3.22.x, will be in 3.22.18