Bug 9499 - --itemscontent= option is undocumented in advanced-notice.pl cronjob + default date should be date_due, not issuedate
Summary: --itemscontent= option is undocumented in advanced-notice.pl cronjob + defaul...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Notices (show other bugs)
Version: master
Hardware: All All
: P5 - low trivial (vote)
Assignee: Liz Rea
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-01-27 20:27 UTC by Liz Rea
Modified: 2016-01-20 23:44 UTC (History)
2 users (show)

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


Attachments
Bug 9499 - --itemscontent= option is undocumented in advanced-notice.pl cronjob + default date should be date_due, not issuedate (7.31 KB, patch)
2013-01-27 21:47 UTC, Liz Rea
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 9499 - --itemscontent= option is undocumented in advanced-notice.pl cronjob + default date should be date_due, not issuedate (7.46 KB, patch)
2013-02-05 21:01 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 9499 - --itemscontent= option is undocumented in advanced-notice.pl cronjob + default date should be date_due, not issuedate (7.47 KB, patch)
2013-02-06 15:14 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 Liz Rea 2013-01-27 20:27:18 UTC
advance_notices.pl has an undocumented feature to allow --itemscontent=<fields> select what fields are put in itemscontent in notices. 

Additionally, these notices ship by default with the dateissued, instead of the date_due, which makes little sense to borrowers.

I propose we change the default date on advanced_notices to date_due (remember, you'll retain the option to set it to the dateissued if you want to), update the crontab.example to reflect the change as well, and improve the POD to reflect that the option is available for advanced/due notices.
Comment 1 Liz Rea 2013-01-27 21:47:28 UTC Comment hidden (obsolete)
Comment 2 Owen Leonard 2013-02-05 21:01:30 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2013-02-06 15:14:20 UTC
QA comment:

This patch updates the pod for the advance_notices.pl script using the Pod::Usage module.

Marked as Passed QA.
Comment 4 Jonathan Druart 2013-02-06 15:14:37 UTC
Created attachment 15105 [details] [review]
Bug 9499 - --itemscontent= option is undocumented in advanced-notice.pl cronjob + default date should be date_due, not issuedate

To Test:

Set up a borrower to receive due and/or predue notices.
Define your predue and/or due notice to use <<items.content>>
Give your borrower an issue that will trigger a notice to be sent
(Example: Henry Acevedo has checked out a book that will be coming due tomorrow,
he wants to receive predue notices 1 day in advance)

On the command line, run (your paths may vary, these are mine):
sudo env KOHA_CONF=/etc/koha/sites/devlibrary/koha-conf.xml PERL5LIB=/usr/share/koha/lib perl advance_notices.pl -c -n

Note that the date listed is the due date, not the issue date.

Then run:
sudo env KOHA_CONF=/etc/koha/sites/devlibrary/koha-conf.xml PERL5LIB=/usr/share/koha/lib perl advance_notices.pl -c -n --itemscontent=issuedate,title,author,barcode

Note that the date listed is the issue date, not the date due.

Also run
sudo env KOHA_CONF=/etc/koha/sites/devlibrary/koha-conf.xml PERL5LIB=/usr/share/koha/lib perl advance_notices.pl --help

Should show the help.

sudo env KOHA_CONF=/etc/koha/sites/devlibrary/koha-conf.xml PERL5LIB=/usr/share/koha/lib perl advance_notices.pl --man

Should show the man page version of the help.

sudo env KOHA_CONF=/etc/koha/sites/devlibrary/koha-conf.xml PERL5LIB=/usr/share/koha/lib perl advance_notices.pl

Should show the help. This script requires confirmation before running (-c or nothing is done).

Note that the documentation refers to the --itemscontent= option and now allows --man as well as --help. Also it is a proper POD.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Works as advertised according to the fine test plan.

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 5 Jared Camins-Esakov 2013-02-07 13:02:23 UTC
The one thing I'm not sure about in this patch is the change from issuedate to date_due. I agree it makes more sense, but before we change the default behavior I'd like to know if anyone has any idea why the default was issuedate before. Anyone?
Comment 6 Jonathan Druart 2013-02-07 16:09:31 UTC
(In reply to comment #5)
> The one thing I'm not sure about in this patch is the change from issuedate
> to date_due. I agree it makes more sense, but before we change the default
> behavior I'd like to know if anyone has any idea why the default was
> issuedate before. Anyone?

I don't know why. Regarding to the git log, it is an old commit.
But obviously, date_due is more relevant here.
Comment 7 Liz Rea 2013-02-07 20:21:47 UTC
When I was writing this patch, I went back and looked at when this was added - it has been this way since the beginning. 

No one has been much bothered to change it, apparently, or if they have, they used the heretofore undocumented --itemscontent flag.

My suspicion is that libraries don't look at the actual output of these notices very often - they get sent out and they never see what information is contained in items.content. That, or they see it but don't grok that it's issuedate instead of date-due. It's clear that if there were complaints, they were previously unvoiced, at least to bugzilla. However, the impetus for this patch came from a library who noticed the discrepancy - I can only assume others have noticed too.

That said, date_due makes a whole lot more sense for the purpose of the notice - we were sending out a notice telling patrons that an item was due, but that didn't tell patrons when the item was due, only when they checked it out. That doesn't seem helpful.

As noted, the behaviour can be reverted to "the old way" by using the --itemscontent flag on the cronjob and passing the issuedate instead of the due_date.

Also, if you had previously specified date_due using the items.content flag on the cron job, that behaviour won't change with this patch.
Comment 8 Jared Camins-Esakov 2013-02-08 00:45:13 UTC
This patch has been pushed to master.
Comment 9 Chris Cormack 2013-02-09 00:06:47 UTC
Pushed to 3.10.x and 3.8.x will be 3.10.3 and 3.8.10