Bug 21886 - Add option to send notices from owning library instead of issuing library
Summary: Add option to send notices from owning library instead of issuing library
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Notices (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Stefan Berndtsson
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-11-27 12:57 UTC by Stefan Berndtsson
Modified: 2020-11-23 08:54 UTC (History)
7 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 21886: Add option to send notices from owning library instead of issuing library (10.19 KB, patch)
2018-11-27 12:59 UTC, Stefan Berndtsson
Details | Diff | Splinter Review
Bug 21886: Add option to send notices from owning library instead of issuing library (10.32 KB, patch)
2019-03-12 12:12 UTC, Björn Nylén
Details | Diff | Splinter Review
Bug 21886: Add option to send notices from owning library instead of issuing library (10.81 KB, patch)
2020-10-14 02:51 UTC, Timothy Alexis Vass
Details | Diff | Splinter Review
Bug 21886: Add option to send notices from owning library instead of issuing library (12.97 KB, patch)
2020-10-14 03:08 UTC, Timothy Alexis Vass
Details | Diff | Splinter Review
Bug 21886: Add option to send notices from owning library instead of issuing library (13.56 KB, patch)
2020-10-15 06:48 UTC, Timothy Alexis Vass
Details | Diff | Splinter Review
Bug 21886: Add option to send notices from owning library instead of issuing library (11.31 KB, patch)
2020-10-15 11:54 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 21886: Add option to send notices from owning library instead of issuing library (11.39 KB, patch)
2020-10-16 07:08 UTC, Timothy Alexis Vass
Details | Diff | Splinter Review
Bug 21886: Add option to send notices from owning library instead of issuing library (11.59 KB, patch)
2020-10-16 13:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21886: (QA follow-up) Fix QA script errors (7.14 KB, patch)
2020-10-16 13:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21886: (QA follow-up) Update parameters (2.52 KB, patch)
2020-11-04 13:20 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Stefan Berndtsson 2018-11-27 12:57:47 UTC
We have several reasons to want our advance notices and overdue notices to be sent from the library owning the item rather than the library issuing the loan, and to be able to specify which libraries to send advance notices for (similar to how overdue notices has a library option).
Comment 1 Stefan Berndtsson 2018-11-27 12:59:24 UTC
Created attachment 82668 [details] [review]
Bug 21886: Add option to send notices from owning library instead of issuing library

The provided patch adds the following functionality:

* Add --owning to both advance_notice.pl and overdue_notice.pl
* Add --library to advance_notice.pl like the way overdue_notice.pl already works

When specifying "--owning" both scripts will use items.homebranch instead of
issues.branchcode to determine sending library.

For advance_notice.pl this affects non-digest DUE and PREDUE, but not digest notices.

To test:

1.  Have a patron who wants advance notices as email with 2 days in advance (not digest)
2.  Have the first overdue date set 3 days past due date
3.  Have issue for that patron where date_due is 2 days away where the item homebranch differs from the issuing branch
4.  Have issue for that patron where date_due is 3 days old
5.  Run advance_notices.pl without --owning
6.  Run overdue_notices.pl without --owning
7.  Confirm that two messages were created for that patron with the sender being the issuing branch
8.  Delete messages or create two more issues according to (3) and (4)
9.  Run advance_notices.pl with --owning
10. Run overdue_notices.pl with --owning
11. Confirm that the two messages created has the item homebranch as sender

Extra feature for advance_notices.pl is that it adds a "--library" the same way overdue_notices.pl
has. Adding variants of that flag in steps (5) and (9) above can confirm this option as well.

Sponsored-by: Gothenburg University Library
Comment 2 Katrin Fischer 2018-12-20 21:31:52 UTC
Hi Stefan, please don't forget to assign the bug to yourself when providing a patch!
Comment 3 Björn Nylén 2019-03-12 12:12:28 UTC
Created attachment 86513 [details] [review]
Bug 21886: Add option to send notices from owning library instead of issuing library

The provided patch adds the following functionality:

* Add --owning to both advance_notice.pl and overdue_notice.pl
* Add --library to advance_notice.pl like the way overdue_notice.pl already works

When specifying "--owning" both scripts will use items.homebranch instead of
issues.branchcode to determine sending library.

For advance_notice.pl this affects non-digest DUE and PREDUE, but not digest notices.

To test:

1.  Have a patron who wants advance notices as email with 2 days in advance (not digest)
2.  Have the first overdue date set 3 days past due date
3.  Have issue for that patron where date_due is 2 days away where the item homebranch differs from the issuing branch
4.  Have issue for that patron where date_due is 3 days old
5.  Run advance_notices.pl without --owning
6.  Run overdue_notices.pl without --owning
7.  Confirm that two messages were created for that patron with the sender being the issuing branch
8.  Delete messages or create two more issues according to (3) and (4)
9.  Run advance_notices.pl with --owning
10. Run overdue_notices.pl with --owning
11. Confirm that the two messages created has the item homebranch as sender

Extra feature for advance_notices.pl is that it adds a "--library" the same way overdue_notices.pl
has. Adding variants of that flag in steps (5) and (9) above can confirm this option as well.

Sponsored-by: Gothenburg University Library
Signed-off-by: Björn Nylén <bjorn.nylen@ub.lu.se>
Comment 4 Stefan Berndtsson 2019-05-06 13:38:49 UTC
There is a bug in this one where only one of two SQL queries in overdue_notices.pl are changed to handle the owning library parameter. I will try to push a fix soon, but in the meantime I've set this back to In Discussion.
Comment 5 Timothy Alexis Vass 2020-10-14 02:51:12 UTC
Created attachment 111581 [details] [review]
Bug 21886: Add option to send notices from owning library instead of issuing library

The provided patch adds the following functionality:

* Add --owning to both advance_notice.pl and overdue_notice.pl
* Add -library to advance_notice.pl like the way overdue_notice.pl already works

When specifying "--owning" both scripts will use items.homebranch instead of
issues.branchcode to determine sending library.

For advance_notice.pl this affects non-digest DUE and PREDUE, but not digest notices.

To test:

1.  Have a patron who wants advance notices as email with 2 days in advance (not digest)
2.  Have the first overdue date set 3 days past due date
3.  Have issue for that patron where date_due is 2 days away where the item homebranch differs from the issuing branch
4.  Have issue for that patron where date_due is 3 days old
5.  Run advance_notices.pl without --owning
6.  Run overdue_notices.pl without --owning
7.  Confirm that two messages were created for that patron with the sender being the issuing branch
8.  Delete messages or create two more issues according to (3) and (4)
9.  Run advance_notices.pl with --owning
10. Run overdue_notices.pl with --owning
11. Confirm that the two messages created has the item homebranch as sender

Extra feature for advance_notices.pl is that it adds "-library" the same way overdue_notices.pl has. Adding variants of that flag in steps (5) and (9) above can confirm this option as well.

Sponsored-by: Gothenburg University Library
Comment 6 Timothy Alexis Vass 2020-10-14 02:54:41 UTC
Attached a new patch correcting the merge conflict and:
--owning instead of -owning in the --man content.
-library instead of --library in the test description.
Comment 7 Timothy Alexis Vass 2020-10-14 03:08:38 UTC
Created attachment 111582 [details] [review]
Bug 21886: Add option to send notices from owning library instead of issuing library

The provided patch adds the following functionality:

* Add --owning to both advance_notice.pl and overdue_notice.pl
* Add -library to advance_notice.pl like the way overdue_notice.pl already works

When specifying "--owning" both scripts will use items.homebranch instead of
issues.branchcode to determine sending library.

For advance_notice.pl this affects non-digest DUE and PREDUE, but not digest notices.

To test:

1.  Have a patron who wants advance notices as email with 2 days in advance (not digest)
2.  Have the first overdue date set 3 days past due date
3.  Have issue for that patron where date_due is 2 days away where the item homebranch differs from the issuing branch
4.  Have issue for that patron where date_due is 3 days old
5.  Run advance_notices.pl without --owning
6.  Run overdue_notices.pl without --owning
7.  Confirm that two messages were created for that patron with the sender being the issuing branch
8.  Delete messages or create two more issues according to (3) and (4)
9.  Run advance_notices.pl with --owning
10. Run overdue_notices.pl with --owning
11. Confirm that the two messages created has the item homebranch as sender

Extra feature for advance_notices.pl is that it adds "-library" the same way overdue_notices.pl has. Adding variants of that flag in steps (5) and (9) above can confirm this option as well.

Sponsored-by: Gothenburg University Library
Comment 8 Timothy Alexis Vass 2020-10-14 03:09:30 UTC
This patch fixes the SQL statement in overdue_notice.pl
Comment 9 Timothy Alexis Vass 2020-10-15 06:48:23 UTC
Created attachment 111679 [details] [review]
Bug 21886: Add option to send notices from owning library instead of issuing library

The provided patch adds the following functionality:

* Add --owning to both advance_notice.pl and overdue_notice.pl
* Add -library to advance_notice.pl like the way overdue_notice.pl already works

When specifying "--owning" both scripts will use items.homebranch instead of
issues.branchcode to determine sending library.

For advance_notice.pl this affects non-digest DUE and PREDUE, but not digest notices.

To test:

1.  Have a patron who wants advance notices as email with 2 days in advance (not digest)
2.  Have the first overdue date set 3 days past due date
3.  Have issue for that patron where date_due is 2 days away where the item homebranch differs from the issuing branch
4.  Have issue for that patron where date_due is 3 days old
5.  Run advance_notices.pl without --owning
6.  Run overdue_notices.pl without --owning
7.  Confirm that two messages were created for that patron with the sender being the issuing branch
8.  Delete messages or create two more issues according to (3) and (4)
9.  Run advance_notices.pl with --owning
10. Run overdue_notices.pl with --owning
11. Confirm that the two messages created has the item homebranch as sender

Extra feature for advance_notices.pl is that it adds "-library" the same way overdue_notices.pl has. Adding variants of that flag in steps (5) and (9) above can confirm this option as well.

Sponsored-by: Gothenburg University Library
Comment 10 Timothy Alexis Vass 2020-10-15 06:51:22 UTC
This patch also fixes the SQL statements in Circulation.pm

Stefan has kindly informed that Gothenburg University Library has this working in production, which is great. We anticipate a proper patch from the proper source.
Comment 11 David Gustafsson 2020-10-15 11:54:31 UTC
Created attachment 111711 [details] [review]
Bug 21886: Add option to send notices from owning library instead of issuing library

The provided patch adds the following functionality:

* Add --owning to both advance_notice.pl and overdue_notice.pl
* Add -library to advance_notice.pl like the way overdue_notice.pl already works

When specifying "--owning" both scripts will use items.homebranch instead of
issues.branchcode to determine sending library.

For advance_notice.pl this affects non-digest DUE and PREDUE, but not digest notices.

To test:

1.  Have a patron who wants advance notices as email with 2 days in advance (not digest)
2.  Have the first overdue date set 3 days past due date
3.  Have issue for that patron where date_due is 2 days away where the item homebranch differs from the issuing branch
4.  Have issue for that patron where date_due is 3 days old
5.  Run advance_notices.pl without --owning
6.  Run overdue_notices.pl without --owning
7.  Confirm that two messages were created for that patron with the sender being the issuing branch
8.  Delete messages or create two more issues according to (3) and (4)
9.  Run advance_notices.pl with --owning
10. Run overdue_notices.pl with --owning
11. Confirm that the two messages created has the item homebranch as sender

Extra feature for advance_notices.pl is that it adds "-library" the same way overdue_notices.pl has. Adding variants of that flag in steps (5) and (9) above can confirm this option as well.

Sponsored-by: Gothenburg University Library
Comment 12 David Gustafsson 2020-10-15 11:55:36 UTC
I am not the author of this patch but have created a new patch rebased on latest Koha master with the code that is currently running in production.
Comment 13 Timothy Alexis Vass 2020-10-16 07:08:36 UTC
Created attachment 111828 [details] [review]
Bug 21886: Add option to send notices from owning library instead of issuing library

The provided patch adds the following functionality:

* Add --owning to both advance_notice.pl and overdue_notice.pl
* Add -library to advance_notice.pl like the way overdue_notice.pl already works

When specifying "--owning" both scripts will use items.homebranch instead of
issues.branchcode to determine sending library.

For advance_notice.pl this affects non-digest DUE and PREDUE, but not digest notices.

To test:

1.  Have a patron who wants advance notices as email with 2 days in advance (not digest)
2.  Have the first overdue date set 3 days past due date
3.  Have issue for that patron where date_due is 2 days away where the item homebranch differs from the issuing branch
4.  Have issue for that patron where date_due is 3 days old
5.  Run advance_notices.pl without --owning
6.  Run overdue_notices.pl without --owning
7.  Confirm that two messages were created for that patron with the sender being the issuing branch
8.  Delete messages or create two more issues according to (3) and (4)
9.  Run advance_notices.pl with --owning
10. Run overdue_notices.pl with --owning
11. Confirm that the two messages created has the item homebranch as sender

Extra feature for advance_notices.pl is that it adds "-library" the same way overdue_notices.pl has. Adding variants of that flag in steps (5) and (9) above can confirm this option as well.

Sponsored-by: Gothenburg University Library
Signed-off-by: Timothy Alexis Vass <timothy_alexis.vass@ub.lu.se>
Comment 14 Timothy Alexis Vass 2020-10-16 07:09:57 UTC
I corrected the --man where it the option was presented as -owning instead of --owning in the same commit as Sign-off.
Comment 15 Martin Renvoize 2020-10-16 13:29:09 UTC
Created attachment 111872 [details] [review]
Bug 21886: Add option to send notices from owning library instead of issuing library

The provided patch adds the following functionality:

* Add --owning to both advance_notice.pl and overdue_notice.pl
* Add --library to advance_notice.pl like the way overdue_notice.pl already works

When specifying "--owning" both scripts will use items.homebranch instead of
issues.branchcode to determine sending library.

For advance_notice.pl this affects non-digest DUE and PREDUE, but not digest notices.

To test:

1.  Have a patron who wants advance notices as email with 2 days in advance (not digest)
2.  Have the first overdue date set 3 days past due date
3.  Have issue for that patron where date_due is 2 days away where the item homebranch differs from the issuing branch
4.  Have issue for that patron where date_due is 3 days old
5.  Run advance_notices.pl without --owning
6.  Run overdue_notices.pl without --owning
7.  Confirm that two messages were created for that patron with the sender being the issuing branch
8.  Delete messages or create two more issues according to (3) and (4)
9.  Run advance_notices.pl with --owning
10. Run overdue_notices.pl with --owning
11. Confirm that the two messages created has the item homebranch as sender

Extra feature for advance_notices.pl is that it adds "--library" the same way overdue_notices.pl has.
Adding variants of that flag in steps (5) and (9) above can confirm this option as well.

Sponsored-by: Gothenburg University Library
Signed-off-by: Björn Nylén <bjorn.nylen@ub.lu.se>
Signed-off-by: Timothy Alexis Vass <timothy_alexis.vass@ub.lu.se>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 16 Martin Renvoize 2020-10-16 13:29:15 UTC
Created attachment 111873 [details] [review]
Bug 21886: (QA follow-up) Fix QA script errors

This patch fixes a perlcritic issue and also replaces tabs with four
spaces where appropriate.
Comment 17 Martin Renvoize 2020-10-16 13:31:02 UTC
This does the trick and now passes the QA scripts.

I would have liked to have seen the query in GetUpcomingDueIssues adapted for the --library case as opposed to pulling back all the results only to then filter them in perl.. but this would have also required unit tests.  One for a followup bug I think.

Passing QA
Comment 18 Jonathan Druart 2020-11-04 10:03:51 UTC
I am not sure about the term "owning", we usually use "homebranch". Maybe we should stick with this terminology as it is widely used in Koha.

Katrin, Nick, what do you think?
Comment 19 Martin Renvoize 2020-11-04 10:21:04 UTC
Good question..

WE could use --use-homebranch as the current implementation is a boolean.. the '-use-' would make that a little clearer..

OR

Perhaps we should change it to something more descriptive entirely.. '--branchfilter=issuebranch|homebranch' to make it clear that we're applying a filter on the lookup and it can take either 'issuebranch' or 'homebranch'.
Comment 20 Martin Renvoize 2020-11-04 10:30:32 UTC
(In reply to Martin Renvoize from comment #19)
> Good question..
> 
> WE could use --use-homebranch as the current implementation is a boolean..
> the '-use-' would make that a little clearer..
> 
> OR
> 
> Perhaps we should change it to something more descriptive entirely..
> '--branchfilter=issuebranch|homebranch' to make it clear that we're applying
> a filter on the lookup and it can take either 'issuebranch' or 'homebranch'.

That final one should more likely be `--frombranch=issuebranch|homebranch`
Comment 21 Katrin Fischer 2020-11-04 10:43:16 UTC
I just wanted to say that I think filter is wrong here. 

I think replacing owning is a good point. But I'd suggest different parameter names if we want to stick with terminology:

- checkout_library
- item_home_library

homebranch is a bit misleading as it could be the patron's or the item's.
Comment 22 Nick Clemens 2020-11-04 13:09:25 UTC
(In reply to Katrin Fischer from comment #21)
> I just wanted to say that I think filter is wrong here. 
> 
> I think replacing owning is a good point. But I'd suggest different
> parameter names if we want to stick with terminology:
> 
> - checkout_library
> - item_home_library
> 
> homebranch is a bit misleading as it could be the patron's or the item's.

+1
Comment 23 Martin Renvoize 2020-11-04 13:20:41 UTC
Created attachment 112990 [details] [review]
Bug 21886: (QA follow-up) Update parameters

As discussed, this patch updates the --owning parameter to more clearly
represent it's function.

--owning boolean => --frombranch=item-homebranch|item-issuebranch

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 24 Martin Renvoize 2020-11-04 13:21:23 UTC
Be great to get a final independant QA on my followup.. setting signed off.
Comment 25 Katrin Fischer 2020-11-04 14:40:30 UTC
got 2 votes for checkout and library now :) Maybe missed more discussion, why not using comment#22 suggestions?
Comment 26 Martin Renvoize 2020-11-04 16:08:49 UTC
(In reply to Katrin Fischer from comment #25)
> got 2 votes for checkout and library now :) Maybe missed more discussion,
> why not using comment#22 suggestions?

Why not using comment#20 ;).

I didn't get a reply to why that wasn't a good idea.. I feel my followup makes the syntax considerably clearer personally.  having two boolean options that are mutually excluding and don't clearly say what they actually do in the name is far less clear to me than having one parameter that's clearly named with two clear options available.
Comment 27 Timothy Alexis Vass 2020-11-23 08:54:24 UTC
Can we reach a decision on this?