Bug 12648

Summary: Link patrons to an order
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: AcquisitionsAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: amy, cbrannon, julian.maurice, katrin.fischer, nicole, paola.rossi, tomascohen
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 12833    
Bug Blocks: 12633, 13021, 14743, 21587, 24964    
Attachments: Bug 12648: Link patrons to an order - DB changes
Bug 12648: Refactoring to prepare user search for reuse
Bug 12648: Link patrons to an order
Bug 12648: Add unit tests for C4::Acquisition::*OrderUsers
Bug 12648: Link patrons to an order - DB changes
Bug 12648: Refactoring to prepare user search for reuse
Bug 12648: Link patrons to an order
Bug 12648: Add unit tests for C4::Acquisition::*OrderUsers
Bug 12648: The users added to basket should have a permission
Bug 12648: Fix conflict with bug 12833
Bug 12648: Hide the result search on load
Bug 12648: The patron's name should be the same as before
Bug 12648: Link patrons to an order - DB changes
Bug 12648: Refactoring to prepare user search for reuse
Bug 12648: Link patrons to an order
Bug 12648: Add unit tests for C4::Acquisition::*OrderUsers
Bug 12648: The users added to basket should have a permission
Bug 12648: Fix conflict with bug 12833
Bug 12648: Hide the result search on load
Bug 12648: The patron's name should be the same as before
Bug 12648: Link patrons to an order - DB changes
Bug 12648: Refactoring to prepare user search for reuse
Bug 12648: Link patrons to an order
Bug 12648: Add unit tests for C4::Acquisition::*OrderUsers
Bug 12648: The users added to basket should have a permission
Bug 12648: Fix conflict with bug 12833
Bug 12648: Hide the result search on load
Bug 12648: The patron's name should be the same as before
Bug 12648: Add the new 'Acquisition' entry in the letter module list
Bug 12648: Link patrons to an order - DB changes
Bug 12648: Refactoring to prepare user search for reuse
Bug 12648: Link patrons to an order
Bug 12648: Add unit tests for C4::Acquisition::*OrderUsers
Bug 12648: The users added to basket should have a permission
Bug 12648: Fix conflict with bug 12833
Bug 12648: Hide the result search on load
Bug 12648: The patron's name should be the same as before
Bug 12648: Add the new 'Acquisition' entry in the letter module list
[PASSED QA] Bug 12648: Link patrons to an order
[PASSED QA] Bug 12648: Link patrons to an order
[PASSED QA] Bug 12648: Link patrons to an order
[PASSED QA] Bug 12648: Add unit tests for C4::Acquisition::*OrderUsers
[PASSED QA] Bug 12648: The users added to basket should have a permission
[PASSED QA] Bug 12648: Fix conflict with bug 12833
[PASSED QA] Bug 12648: Hide the result search on load
[PASSED QA] Bug 12648: The patron's name should be the same as before
[PASSED QA] Bug 12648: Add the new 'Acquisition' entry in the letter module list
Bug 12648 - QA Followup
Bug 12648: Link patrons to an order - DB changes
Bug 12648: Refactoring to prepare user search for reuse
Bug 12648: Link patrons to an order
Bug 12648: Add unit tests for C4::Acquisition::*OrderUsers
Bug 12648: The users added to basket should have a permission
Bug 12648: Fix conflict with bug 12833
Bug 12648: Hide the result search on load
Bug 12648: The patron's name should be the same as before
Bug 12648: Add the new 'Acquisition' entry in the letter module list
Bug 12648 - QA Followup
Bug 12648: Rename aqorderusers to aqorder_users
Bug 12648: (RM followup) DBIx schema update
Bug 12648: Fix conflict with bug 8096
Bug 12648: Add some documentation on the kohastructure.sql file

Description Jonathan Druart 2014-07-24 11:29:20 UTC
A need is to notify 1+ patrons on receiving an order.
As baskets, we could link patrons to an order and send them an email when the order is completely received.
Comment 1 Jonathan Druart 2014-07-24 11:53:08 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2014-07-24 11:53:12 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2014-07-24 11:53:18 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2014-07-24 11:53:22 UTC Comment hidden (obsolete)
Comment 5 Paola Rossi 2014-08-14 14:52:58 UTC
I've applied the patches against master 3.17.00.013.

On adding/modifying users to a basket (acqui/basket.pl?basketno=XXX, "Managed by", "Add user"), the Search-for-patrons' Processing [opened in a new window] stops.
On the contrary, on Patron module the patrons' searching is ok.

On log I see: 
search: Template process failed: file error - .../koha-tmpl/intranet-tmpl/prog/en/modules/acqui/tables/members_results.tt: not found at .../C4/Templates.pm line 131

So I pass the patch to "Failed QA" status.

The first patch "Link patrons to an order - DB changes" needs to be rebased.
Comment 6 Jonathan Druart 2014-08-19 14:37:00 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2014-08-19 14:37:07 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2014-08-19 14:37:11 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2014-08-19 14:37:15 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2014-08-19 14:37:52 UTC
(In reply to Paola Rossi from comment #5)
> On log I see: 
> search: Template process failed: file error -
> .../koha-tmpl/intranet-tmpl/prog/en/modules/acqui/tables/members_results.tt:
> not found at .../C4/Templates.pm line 131

I forgot to add 1 file :-/
Should be fixed now.
Comment 11 Paola Rossi 2014-08-28 13:23:41 UTC
Thanks, Jonathan.

I've applied the patches against master 3.17.00.016.

[The first patch needs to be rebased]

On the patrons' search window I read: "Search for patron:
Only staff with superlibrarian or acquisitions permissions (or order_manage permission if granular permissions are enabled) are returned in the search results".

But some of the listed patrons are students, and so on.

I add a new patron, student category, with no permission.

On adding/modifying users to a basket, I can link this student to the basket.

So I pass the patch to "Failed QA" status.
Comment 12 Jonathan Druart 2014-08-29 09:14:06 UTC Comment hidden (obsolete)
Comment 13 Jonathan Druart 2014-08-29 09:17:12 UTC
(In reply to Paola Rossi from comment #11)
> [The first patch needs to be rebased]

It's a trivial rebase.


> On the patrons' search window I read: "Search for patron:
> Only staff with superlibrarian or acquisitions permissions (or order_manage
> permission if granular permissions are enabled) are returned in the search
> results".
> 
> But some of the listed patrons are students, and so on.

Yes, previous patches pretend to do the job but actually not at all.
This last patch does it, but it's not smartly.

Note for QA: I know this last patch is not the better way to do, but I don't have any other idea. Please let me know if you find another easy way to do the same job.
Comment 14 Jonathan Druart 2014-10-01 08:44:08 UTC
Bug 12833 breaks this enh.
Comment 15 Jonathan Druart 2014-10-01 09:19:23 UTC Comment hidden (obsolete)
Comment 16 Jonathan Druart 2014-10-01 09:19:29 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2014-10-11 13:56:46 UTC Comment hidden (obsolete)
Comment 18 Paola Rossi 2014-10-15 11:42:42 UTC
I've applied the 8 patches against master 3.17.00.027

[The first patch needs trivial rebase]

perl t/db_dependent/Acquisition/OrderUsers.t
1..3
not ok 1 - ModOrderUsers should link patrons to an order
#   Failed test 'ModOrderUsers should link patrons to an order'
#   at t/db_dependent/Acquisition/OrderUsers.t line 76.
#          got: '0'
#     expected: '1'
Use of uninitialized value in numeric gt (>) at /var/root-koha/bug-7162/C4/Acquisition.pm line 1511.
ok 2 - The letter has not been sent to message queue on receiving the order, the order is not entire received
Use of uninitialized value in numeric gt (>) at /var/root-koha/bug-7162/C4/Acquisition.pm line 1511.
not ok 3 - The letter has been sent to message queue on receiving the order
#   Failed test 'The letter has been sent to message queue on receiving the order'
#   at t/db_dependent/Acquisition/OrderUsers.t line 108.
#          got: '0'
#     expected: '1'
# Looks like you failed 2 tests of 3.

So I pass the patch to "Failed QA" status.
Comment 19 Jonathan Druart 2014-11-04 10:38:51 UTC Comment hidden (obsolete)
Comment 20 Jonathan Druart 2014-11-04 10:38:55 UTC Comment hidden (obsolete)
Comment 21 Jonathan Druart 2014-11-04 10:38:58 UTC Comment hidden (obsolete)
Comment 22 Jonathan Druart 2014-11-04 10:39:01 UTC Comment hidden (obsolete)
Comment 23 Jonathan Druart 2014-11-04 10:39:05 UTC Comment hidden (obsolete)
Comment 24 Jonathan Druart 2014-11-04 10:39:08 UTC Comment hidden (obsolete)
Comment 25 Jonathan Druart 2014-11-04 10:39:12 UTC Comment hidden (obsolete)
Comment 26 Jonathan Druart 2014-11-04 10:39:15 UTC Comment hidden (obsolete)
Comment 27 Jonathan Druart 2014-11-04 10:40:03 UTC
(In reply to Paola Rossi from comment #18)
> # Looks like you failed 2 tests of 3.

I don't get these failures, could you please retest with this new rebased patch set?
Comment 28 Paola Rossi 2014-12-09 12:26:17 UTC
I've tried to apply the patches agaist master 3.19.00.000

[The first patch needed a trivial rebase.]

At the six-th patch:

Applying: Bug 12648: Fix conflict with bug 12833
fatal: sha1 information is lacking or useless (svc/members/search).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 12648: Fix conflict with bug 12833

So I pass the patch to "Patch doesn't apply" status.
Comment 29 Jonathan Druart 2014-12-09 14:10:49 UTC Comment hidden (obsolete)
Comment 30 Jonathan Druart 2014-12-09 14:10:52 UTC Comment hidden (obsolete)
Comment 31 Jonathan Druart 2014-12-09 14:10:55 UTC Comment hidden (obsolete)
Comment 32 Jonathan Druart 2014-12-09 14:10:58 UTC Comment hidden (obsolete)
Comment 33 Jonathan Druart 2014-12-09 14:11:01 UTC Comment hidden (obsolete)
Comment 34 Jonathan Druart 2014-12-09 14:11:04 UTC Comment hidden (obsolete)
Comment 35 Jonathan Druart 2014-12-09 14:11:07 UTC Comment hidden (obsolete)
Comment 36 Jonathan Druart 2014-12-09 14:11:11 UTC Comment hidden (obsolete)
Comment 37 Jonathan Druart 2014-12-09 14:11:51 UTC
Patches rebased.
Comment 38 Paola Rossi 2014-12-16 14:28:07 UTC
I've applied the patches against master 3.19.00.000

About :
>Test plan:
>1/ Execute the updatedb entry and verify you have a new notification template in your table (tools/letter.pl).
>code: ACQ_NOTIF_ON_RECEIV, module: acquisition

I saw that the module is acquisition, OK

>2/ You can edit it if you want

I saw that the "Koha module" is "Catalog" ERROR, while "Acquisition" is not an available option in the drop down list.

On MASTER, in effect, the available modules are 8, and none of them is "Acquisition".

So I pass the patch to "Failed QA" status.
Comment 39 Jonathan Druart 2014-12-17 08:36:34 UTC Comment hidden (obsolete)
Comment 40 Jonathan Druart 2014-12-17 08:37:27 UTC
(In reply to Paola Rossi from comment #38)
> >2/ You can edit it if you want
> 
> I saw that the "Koha module" is "Catalog" ERROR, while "Acquisition" is not
> an available option in the drop down list.
> 
> On MASTER, in effect, the available modules are 8, and none of them is
> "Acquisition".
> 
> So I pass the patch to "Failed QA" status.

Sorry about that.
It should be fixed now.
Comment 41 Paola Rossi 2014-12-17 16:49:07 UTC Comment hidden (obsolete)
Comment 42 Paola Rossi 2014-12-17 16:49:35 UTC Comment hidden (obsolete)
Comment 43 Paola Rossi 2014-12-17 16:50:04 UTC Comment hidden (obsolete)
Comment 44 Paola Rossi 2014-12-17 16:50:32 UTC Comment hidden (obsolete)
Comment 45 Paola Rossi 2014-12-17 16:51:05 UTC Comment hidden (obsolete)
Comment 46 Paola Rossi 2014-12-17 16:51:32 UTC Comment hidden (obsolete)
Comment 47 Paola Rossi 2014-12-17 16:51:59 UTC Comment hidden (obsolete)
Comment 48 Paola Rossi 2014-12-17 16:52:33 UTC Comment hidden (obsolete)
Comment 49 Paola Rossi 2014-12-17 16:53:03 UTC Comment hidden (obsolete)
Comment 50 Kyle M Hall 2015-01-09 13:18:33 UTC Comment hidden (obsolete)
Comment 51 Kyle M Hall 2015-01-09 13:20:51 UTC Comment hidden (obsolete)
Comment 52 Kyle M Hall 2015-01-09 13:21:19 UTC Comment hidden (obsolete)
Comment 53 Kyle M Hall 2015-01-09 13:21:26 UTC Comment hidden (obsolete)
Comment 54 Kyle M Hall 2015-01-09 13:21:29 UTC Comment hidden (obsolete)
Comment 55 Kyle M Hall 2015-01-09 13:21:31 UTC Comment hidden (obsolete)
Comment 56 Kyle M Hall 2015-01-09 13:21:34 UTC Comment hidden (obsolete)
Comment 57 Kyle M Hall 2015-01-09 13:21:37 UTC Comment hidden (obsolete)
Comment 58 Kyle M Hall 2015-01-09 13:21:39 UTC Comment hidden (obsolete)
Comment 59 Kyle M Hall 2015-01-09 13:21:42 UTC Comment hidden (obsolete)
Comment 60 Katrin Fischer 2015-01-09 13:27:50 UTC
I like this! Should we add a default letter (separate bug)?
Comment 61 Jonathan Druart 2015-01-09 13:55:32 UTC
(In reply to Katrin Fischer from comment #60)
> I like this! Should we add a default letter (separate bug)?

The patch existed but it has been obsoleted (certainly a mistake).
I reintroduced it.
Comment 62 Tomás Cohen Arazi 2015-03-11 14:34:45 UTC
Jonathan, I like the patch set, but there are two minor things remaining:

- Missing the kohastructure.sql addition
- We should really stick with the adopted naming schema for tables. Even if it will not be consistent with the current table names for acquisitions (we should, at some point just rename them).

I would accept 'aqorder_users', but the ideal one would be 'aquisition_order_users', that would map to Koha::Acquisition::Order::Users.

I'm open to discussing the naming schema and I know this bug has been here before the guidelines have been accepted. It is just a minor refactoring that i will accept as PASSED-QA.

Thanks!
Comment 63 Jonathan Druart 2015-03-11 14:41:18 UTC
Created attachment 36795 [details] [review]
Bug 12648: Link patrons to an order - DB changes

This patch inserts the new notification template into the letters table.
It also add the insert statements into the language specific files.

Signed-off-by: Paola Rossi <paola.rossi@cineca.it>

Amended patch: Add the kohastructure.sql changes
Comment 64 Jonathan Druart 2015-03-11 14:41:23 UTC
Created attachment 36796 [details] [review]
Bug 12648: Refactoring to prepare user search for reuse

A previous enhancement allows to link basket with patrons.
Next patches will use the same way to link order with patrons.

In order to avoir c/p of code, this patch refactores this part of code.

Test plan:
1/ Verify there is no regression on adding/modifying users to a basket.
(acqui/basket.pl?basketno=XXX, "Managed by", "Add user").
2/ Note that you get a friendly message if the user is already present in the
list and when the user has correctly been added to the list.
3/ Note that the list uses the member search service (ie. DataTable +
serverside processing).

Signed-off-by: Paola Rossi <paola.rossi@cineca.it>
Comment 65 Jonathan Druart 2015-03-11 14:41:28 UTC
Created attachment 36797 [details] [review]
Bug 12648: Link patrons to an order

This patch is the main patch.

This feature adds the ability to link patrons to an order.
On that way, they will be notified when the order is completely
received.

Test plan:
1/ Execute the updatedb entry and verify you have a new notification template in your table (tools/letter.pl).
code: ACQ_NOTIF_ON_RECEIV, module: acquisition
2/ You can edit it if you want
3/ Create a basket and create an order with 1 or more items
4/ Link 1+ patrons to this order
5/ Close the basket and receive the order
6/ When you have received all items for this order, all patrons attached
will be notified. Check the message_queue table to check if the letters
have correctly been added to the queue.

Signed-off-by: Paola Rossi <paola.rossi@cineca.it>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 66 Jonathan Druart 2015-03-11 14:41:33 UTC
Created attachment 36798 [details] [review]
Bug 12648: Add unit tests for C4::Acquisition::*OrderUsers

Test plan:
Verify that unit tests pass with
    prove t/db_dependent/Acquisition/OrderUsers.t

Signed-off-by: Paola Rossi <paola.rossi@cineca.it>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 67 Jonathan Druart 2015-03-11 14:41:38 UTC
Created attachment 36799 [details] [review]
Bug 12648: The users added to basket should have a permission

Before this enh, the users to add to a basket should have the
acquisition.order_manage permission.
This patch reintroduces this behavior.

The code in acqui/add_user_search.pl was never used. The filter should
be done in the members/search service.

But it is not possible easily to filter using a sql query, so the filter
is done after. This means that we cannot use the DT pagination
(otherwise the results will become inconsistent).

Test plan:
1/ On adding patrons to a basket, verify that the search patron results contain
patron with the acquisition.order_manage permission.
2/ Verify that all patrons are return on the 'normal' patron search and
when adding patrons to an order.

Signed-off-by: Paola Rossi <paola.rossi@cineca.it>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 68 Jonathan Druart 2015-03-11 14:41:43 UTC
Created attachment 36800 [details] [review]
Bug 12648: Fix conflict with bug 12833

Signed-off-by: Paola Rossi <paola.rossi@cineca.it>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 69 Jonathan Druart 2015-03-11 14:41:47 UTC
Created attachment 36801 [details] [review]
Bug 12648: Hide the result search on load

When no search has been done yet, it's not necessary to display the
result list.

Signed-off-by: Paola Rossi <paola.rossi@cineca.it>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 70 Jonathan Druart 2015-03-11 14:41:53 UTC
Created attachment 36802 [details] [review]
Bug 12648: The patron's name should be the same as before

The displayed patron's name should be surname + firstname

Signed-off-by: Paola Rossi <paola.rossi@cineca.it>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 71 Jonathan Druart 2015-03-11 14:41:57 UTC
Created attachment 36803 [details] [review]
Bug 12648: Add the new 'Acquisition' entry in the letter module list

Signed-off-by: Paola Rossi <paola.rossi@cineca.it>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 72 Jonathan Druart 2015-03-11 14:42:02 UTC
Created attachment 36804 [details] [review]
Bug 12648 - QA Followup
Comment 73 Jonathan Druart 2015-03-11 14:42:07 UTC
Created attachment 36805 [details] [review]
Bug 12648: Rename aqorderusers to aqorder_users
Comment 74 Jonathan Druart 2015-03-11 14:43:10 UTC
(In reply to Tomás Cohen Arazi from comment #62)
> - Missing the kohastructure.sql addition

First patch amended.

> - We should really stick with the adopted naming schema for tables. Even if
> it will not be consistent with the current table names for acquisitions (we
> should, at some point just rename them).
> 
> I would accept 'aqorder_users', but the ideal one would be
> 'aquisition_order_users', that would map to Koha::Acquisition::Order::Users.

aqorderusers replaced with aqorders_users in the last patch.
Comment 75 Tomás Cohen Arazi 2015-03-11 14:53:36 UTC
Created attachment 36807 [details] [review]
Bug 12648: (RM followup) DBIx schema update

Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
Comment 76 Jonathan Druart 2015-03-11 15:35:44 UTC
Created attachment 36810 [details] [review]
Bug 12648: Fix conflict with bug 8096
Comment 77 Tomás Cohen Arazi 2015-03-11 17:04:41 UTC
Created attachment 36817 [details] [review]
Bug 12648: Add some documentation on the kohastructure.sql file

Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
Comment 78 Tomás Cohen Arazi 2015-03-11 17:39:31 UTC
Patches pushed to master.

Good job Jonathan!
Comment 79 Christopher Brannon 2016-05-26 21:21:04 UTC
I'm not finding any reference to this feature in the manual.  Is it there?
Comment 80 Jonathan Druart 2016-06-05 15:38:26 UTC
(In reply to Christopher Brannon from comment #79)
> I'm not finding any reference to this feature in the manual.  Is it there?

CCing Nicole.
Comment 81 Nicole C. Engard 2016-06-08 18:30:25 UTC
(In reply to Jonathan Druart from comment #80)
> (In reply to Christopher Brannon from comment #79)
> > I'm not finding any reference to this feature in the manual.  Is it there?
> 
> CCing Nicole.

It's in a screenshot but not in words. Adding it now.
Comment 82 Nicole C. Engard 2016-06-08 18:38:32 UTC
I lied - it was there:

"With any of the above ordering options you're presented with an option to notify patrons of the new item when it's received. The contents of that notification can be edited in the Notices & Slips tool and will have the code of ACQ_NOTIF_ON_RECEIV. In the 'Patrons' section you will see an option to 'Add user'. Click that button to add patrons who will be notified of the new issue. "
Comment 83 Christopher Brannon 2016-06-08 18:52:13 UTC
(In reply to Nicole C. Engard from comment #82)
> I lied - it was there:
> 
> "With any of the above ordering options you're presented with an option to
> notify patrons of the new item when it's received. The contents of that
> notification can be edited in the Notices & Slips tool and will have the
> code of ACQ_NOTIF_ON_RECEIV. In the 'Patrons' section you will see an option
> to 'Add user'. Click that button to add patrons who will be notified of the
> new issue. "

Ah!  It was way down there.  I assumed it would be under the first picture that shows it.  Thanks!

Christopher