[Koha-bugs] [Bug 6968] Show items expired before today in check expiration of serials page

[bugzilla-daemon at bugs.koha-community.org]

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=6968

Paola Rossi <paola.rossi at cineca.it> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|Needs Signoff               |Patch doesn't apply

--- Comment #14 from Paola Rossi <paola.rossi at cineca.it> ---
(In reply to Charles Farmer from comment #13)
> 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.

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 at inlibro.com>
instead of :
charles <charles.farmer at 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.

-- 
You are receiving this mail because:
You are watching all bug changes.
You are the QA Contact for the bug.