Bug 6968 - Show items expired before today in check expiration of serials page
Summary: Show items expired before today in check expiration of serials page
Status: RESOLVED MOVED
Alias: None
Product: Koha
Classification: Unclassified
Component: Serials (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Maxime Beaulieu
QA Contact: Bugs List
URL:
Keywords:
Depends on:
Blocks: 23243
  Show dependency treegraph
 
Reported: 2011-10-04 17:53 UTC by Maxime Pelletier
Modified: 2019-07-01 11:21 UTC (History)
10 users (show)

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


Attachments
patch (3.08 KB, patch)
2011-10-04 17:53 UTC, Maxime Pelletier
Details | Diff | Splinter Review
screenshot of what it looks like with my patch (13.37 KB, image/png)
2011-10-04 17:55 UTC, Maxime Pelletier
Details
New patch to fix warnings and to make it work (3.16 KB, patch)
2012-01-04 16:59 UTC, Maxime Pelletier
Details | Diff | Splinter Review
signed off patch (3.22 KB, patch)
2012-02-03 10:28 UTC, Adrien SAURAT
Details | Diff | Splinter Review
check expiration in serials expired before today (2.67 KB, patch)
2014-08-11 15:07 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 6968 - Check expiration in serials expired before today (3.48 KB, patch)
2015-01-15 16:20 UTC, Charles Farmer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Maxime Pelletier 2011-10-04 17:53:57 UTC Comment hidden (obsolete)
Comment 1 Maxime Pelletier 2011-10-04 17:55:03 UTC
Created attachment 5703 [details]
screenshot of what it looks like with my patch
Comment 2 Maxime Pelletier 2011-10-04 17:57:51 UTC
Forgot to mention this was sponsored by our client http://www.ccsr.qc.ca/
Comment 3 Katrin Fischer 2011-11-02 20:26:36 UTC
There is a patch attached, I am changing the status to 'needs signoff'.
Comment 4 Katrin Fischer 2011-12-07 06:34:15 UTC
I tested this but had problems with the date search before and after applying the patch. 

I added a new subscription with expiration date 'today' and tried searching with expired before 8/12/2011.

My logs show lots of errors:

[Wed Dec 07 07:28:30 2011] [error] [client 127.0.0.1] [Wed Dec  7 07:28:30 2011] checkexpiration.pl: Use of uninitialized value $C4::Dates::debug in numeric gt (>) at /home/katrin/kohaclone/C4/Dates.pm line 170., referer: http://localhost:8080/cgi-bin/koha/serials/checkexpiration.pl
[Wed Dec 07 07:28:30 2011] [error] [client 127.0.0.1] [Wed Dec  7 07:28:30 2011] checkexpiration.pl: Use of uninitialized value $sqlwhere in concatenation (.) or string at /home/katrin/kohaclone/C4/Serials.pm line 606., referer: http://localhost:8080/cgi-bin/koha/serials/checkexpiration.pl
[Wed Dec 07 07:28:30 2011] [error] [client 127.0.0.1] [Wed Dec  7 07:28:30 2011] checkexpiration.pl: Use of uninitialized value in string eq at /home/katrin/kohaclone/C4/Serials.pm line 615., referer: http://localhost:8080/cgi-bin/koha/serials/checkexpiration.pl
[Wed Dec 07 07:28:30 2011] [error] [client 127.0.0.1] [Wed Dec  7 07:28:30 2011] checkexpiration.pl: Use of uninitialized value $expirationdate in pattern match (m//) at /home/katrin/kohaclone/serials/checkexpiration.pl line 81., referer: http://localhost:8080/cgi-bin/koha/serials/checkexpiration.pl
[Wed Dec 07 07:28:30 2011] [error] [client 127.0.0.1] [Wed Dec  7 07:28:30 2011] checkexpiration.pl: Use of uninitialized value $expirationdate in pattern match (m//) at /home/katrin/kohaclone/serials/checkexpiration.pl line 81., referer: http://localhost:8080/cgi-bin/koha/serials/checkexpiration.pl
[Wed Dec 07 07:28:30 2011] [error] [client 127.0.0.1] [Wed Dec  7 07:28:30 2011] checkexpiration.pl: Use of uninitialized value $C4::Dates::debug in numeric gt (>) at /home/katrin/kohaclone/C4/Dates.pm line 170., referer: http://localhost:8080/cgi-bin/koha/serials/checkexpiration.pl

I also tried different settings for the dateformat system preference with the same result.

Small suggestion for the patch: Perhaps change label to 'Expiring before today'. Currently it says 'Show items expired before today:' which is a bit long and 'items' might be a bit confusing.
Comment 5 Katrin Fischer 2011-12-07 06:45:53 UTC
I think something is weird with my test case, but an additional note about the patch:

You can't search only for "expired before today". You are always prompted to also enter a date. I am not sure how this is supposed to work. Shouldn't it be enough to enter tomorrow's date to find subscriptions that already have expired?
Comment 6 Maxime Pelletier 2012-01-04 16:59:02 UTC Comment hidden (obsolete)
Comment 7 Maxime Pelletier 2012-01-04 17:00:37 UTC
I think you misunderstood what this patch does. The Check Expiration pages shows serials subscription that are expired between today and the date you enter. The new option make it possible to also show what is expired before today, so you get everything expired before the chosen date.
Comment 8 Adrien SAURAT 2012-02-03 10:28:58 UTC Comment hidden (obsolete)
Comment 9 Ian Walls 2012-02-16 17:26:42 UTC
Where is "showexpiredtoday" getting the string value "on"?  There is no explicit value passed by the template.  Is "on" the value passed by a checkbox by default on all browsers?  I think having an explicit value in the template here would be more solid and stable.

Not quite ready to mark this as "Failed QA", but I would like some explanation of the safety of this before I mark it passed.
Comment 10 Paul Poulain 2012-02-21 14:29:18 UTC
(In reply to comment #9)
> Where is "showexpiredtoday" getting the string value "on"?  There is no
> explicit value passed by the template.  Is "on" the value passed by a checkbox
> by default on all browsers?  I think having an explicit value in the template
> here would be more solid and stable.
> 
> Not quite ready to mark this as "Failed QA", but I would like some explanation
> of the safety of this before I mark it passed.

Ian is right: you can't rely on the value "on", you should just test if there is something. Chrome returns 0/1 afaik while FF returns empty/on

However, i mark failed QA for another reason:
i've 2 subscriptions in my database.
One that has expired in 2010, one that will expire in march 2012.

If I set the date to 2012-12-12 I should see 1 or 2 subscriptions in my list, depending on the whether the checkbox has been checked or no.

If checked: I get 2, that's correct
If unchecked: I get none, that's not correct.
Comment 11 Charles Farmer 2014-08-11 15:07:35 UTC Comment hidden (obsolete)
Comment 12 Paola Rossi 2015-01-13 10:38:28 UTC
I've tried to apply the patch against master 3.19.00.005

But:
Patch format detection failed.

So I pass the patch to the "Patch doesn't' apply" status.
Comment 13 Charles Farmer 2015-01-15 16:20:52 UTC
Created attachment 35314 [details] [review]
Bug 6968 - Check expiration in serials expired before today

I'm resubmitting the last patch, which I could apply on master 3.19.00.006,  in a slightly different format.

Hoping this solves your issue.
Comment 14 Paola Rossi 2015-01-20 11:09:17 UTC
(In reply to Charles Farmer from comment #13)
> Created attachment 35314 [details] [review] [review]
> Bug 6968 - Check expiration in serials expired before today
>
> I'm resubmitting the last patch, which I could apply on master 3.19.00.006,
> in a slightly different format.
>
> Hoping this solves your issue.

Kind Charles,
I've applied the patch against master 3.19.00.006 head 13526.

Soon after having applied, when I:
git format-patch -s origin/master,

1-A) I got a file where "From" is:
charles <charles@inlibro.com>
instead of :
charles <charles.farmer@inlibro.com>

It could be an error.

1-B) In that file there was this:
Subject: [PATCH] Patch sponsored by our client http://www.ccsr.qc.ca/

instead of the usual commit message:
Bug 6968 - Show items expired before today in check expiration of serials page

identifying the patch and describing the content of the patch.
[The patches' commit messages will be seen in the master branch:
<http://git.koha-community.org/gitweb/?p=koha.git;a=summary>]

So I pass the patch to "Patch doesn't apply" again.

----------------------------------------------------------------------------
Anyway I've gone on and tested your patch, that is fixing a lasting bug.
I'm not a QA, I don't know precisely the QA guidelines. I write some considerations of mine you can evaluate.

The patch you wrote added a true difference between:
subscriptions "already" expired until today [included] (added to the 
selection by the new checkbox)
subscriptions that "will" expire (from tomorrow [included] to the input 
date [excluded], chosen by the calendar).
But IMO in master this difference was not so present: as you can see, the calendar is not limited from tomorrow on.
So I think that your patch could "match" this "error" not so well.
Perhaps :
A) extending the current selection to all the end-dates of the subscriptions, 
 and
B) adding a "From" and a "To" dates input fields (as an improvement),
could be a more "matching" solution. The default values of "From" and "To" 
could be the MIN and MAX end-date of the subscriptions in DB, whilst "today" 
could be set as the default in the 2 calendars.
C) moreover, the resulting table could be sortable, on end-date at least (as a 
further improvement).

Otherwise, if the solution provided by the patch has been already considered as
 being the best one, consider that some inaccuracies occur:
1-A) when I checked the checkbox and I chose an year ago as input date, the 
selection is not about "before today", but "before an year ago": the result is 
shown (IMO right).
1-B) if I didn't check the checkbox and I chose an year ago as input date, the 
selection is null, as you know [this result is wrong IMO].
If you limit the calendar from tomorrow on, the user will not be able to get a 
selection limited like 1-A).
If you don't limit the calendar from tomorrow on, B) is an available case of 
choice to the user, and it is unfriendly to the user (misleading).

Anyway, if the solution provided by the patch has been already considered as 
being the best one, in the form "before" won't be used in the same meaning: 
once "<", and the other time as " <=". This difference is unfriendly to the 
user: the resulting table could miss some particular subscriptions.
To solve, a better solution can choose one of the 2 cases:
hypothesis 2-1) ("<") "before" excluding the upper limit:
Adding "Show expired before tomorrow:" instead of adding "Show expired before 
today:" could be better.
hypothesis 2-2) ("<=") "before" including the upper limit (better, IMO):
The patch should keep "Show expired before today:". But the patch should also 
modify the "Expiring before: " behaviour, to include the chosen value too in 
the selection.

Anyway, if the solution provided by the patch has been already considered as 
being the best one, being the form a "Filters" one, its default should be 
better set to select all the subscriptions. So I think that:
3-A) a default could be set in the input fields: checked checkbox (!), usefull 
upper limit [=MAX end-date+included (2-2), or greater than MAX end-date].
As a further inprovement, the sort could be add to the table. If the expiring' 
time is so important to the user, the default sorting could be set to end-data 
DESC [in spite of the current subscription code, not related].

I hope these considerations can be of some help.
Comment 15 Martin Renvoize 2019-07-01 10:54:35 UTC
Bug 15171 actually hard codes to always display all expirations before the entered date and so resolved this issue..

However, I prefer the logic here which allowed the end user to select whether to display old expired subscriptions or not. (Also, I'm sure the description on bug 15171 is incorrect).

I'm going to resurrect this bug
Comment 16 Martin Renvoize 2019-07-01 11:21:13 UTC
Moved to bug 23243 which updates it to the current context.