Created attachment 5702 [details] [review] patch The page Check Expiration the in Serials section show items that are going to expire before the date selected, but doesn't show items that expired before today We needed to see what was already expired, so I added a checkbox to add it as an option.
Created attachment 5703 [details] screenshot of what it looks like with my patch
Forgot to mention this was sponsored by our client http://www.ccsr.qc.ca/
There is a patch attached, I am changing the status to 'needs signoff'.
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.
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?
Created attachment 7038 [details] [review] New patch to fix warnings and to make it work
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.
Created attachment 7435 [details] [review] signed off patch Added checkbox allows to see expired subscriptions. I sent by mistake an e-mail without the [SIGNED-OFF] tag in the subject, but it was tested ok.
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.
(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.
Created attachment 30703 [details] [review] check expiration in serials expired before today Patch sponsored by our client http://www.ccsr.qc.ca/ The page Check Expiration the in Serials section doesn't show items that expired before today. There is now a checkbox to add it as an option. Took the last patch and applied it to current master. The variable 'showexpiredtoday' is not tested against the string 'on' anymore. Also, I couldn't reproduce Paul Poulain's issue; when I make a subscription expire on December 1st and test to see if any subscriptions will expire before December 31st, I get the good number of subscriptions listed. If it happens again, I'll make a test file.
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.
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.
(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.
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
Moved to bug 23243 which updates it to the current context.