Bug 23971

Summary: Add logging for basket related actions
Product: Koha Reporter: Andrew Isherwood <bugzilla>
Component: AcquisitionsAssignee: Bugs List <koha-bugs>
Status: CLOSED FIXED QA Contact: Tomás Cohen Arazi <tomascohen>
Severity: enhancement    
Priority: P5 - low CC: black23, fridolin.somers, hc, jonathan.druart, katrin.fischer, marjorie.barry-vila, martin.renvoize, Niamh.WalkerHeadon, severine.queune, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26582
Change sponsored?: Sponsored Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
This enhancement adds back the `AcquisitionsLog` preference and when enabled it adds logging for the following actions: * Adding new baskets * Re-opening closed baskets * Modifying baskets * Modifying basket headers * Modifying basket users * Closing baskets * Approving baskets
Version(s) released in:
21.05.00
Bug Depends on: 26582    
Bug Blocks: 22794, 25916, 24190, 28872    
Attachments: Bug 23971: Add Acq action logging
Bug 23971: Add Acq action logging
Bug 23971: Add Acq action logging
Bug 23971 - Add logging for basket related actions
Bug 23961: (follow-up) Remove data duplication
Bug 23971: (follow-up) Add additional basket logging
Bug 23971 - Add logging for basket related actions
Bug 23971: (follow-up) Remove data duplication
Bug 23971: (follow-up) Add additional basket logging
Bug 23971: Add logging for basket related actions
Bug 23971: (follow-up) Remove data duplication
Bug 23971: (follow-up) Add additional basket logging
Bug 23971: (follow-up) Add additional basket logging
Bug 23971: (QA follow-up) Fix punctuation, doubled up return and syspref order
Bug 23971: Add logging for basket related actions
Bug 23971: (follow-up) Remove data duplication
Bug 23971: (follow-up) Add additional basket logging
Bug 23971: (QA follow-up) Fix punctuation, doubled up return and syspref order
Bug 23971: (follow-up) Add unit tests
Bug 23971: Add logging for basket related actions
Bug 23971: (follow-up) Remove data duplication
Bug 23971: (follow-up) Add additional basket logging
Bug 23971: (QA follow-up) Fix punctuation, doubled up return and syspref order
Bug 23971: (follow-up) Add unit tests
Bug 23971: (follow-up) Respond to feedback
Bug 23971: Add logging for basket related actions
Bug 23971: (follow-up) Remove data duplication
Bug 23971: (follow-up) Add additional basket logging
Bug 23971: (QA follow-up) Fix punctuation, doubled up return and syspref order
Bug 23971: (follow-up) Add unit tests
Bug 23971: (follow-up) Respond to feedback
Bug 23971: (follow-up) Fix tests
Bug 23971: Add logging for basket related actions
Bug 23971: (follow-up) Remove data duplication
Bug 23971: (follow-up) Add additional basket logging
Bug 23971: (QA follow-up) Fix punctuation, doubled up return and syspref order
Bug 23971: (follow-up) Add unit tests
Bug 23971: (follow-up) Respond to feedback
Bug 23971: (follow-up) Fix tests
Bug 23971: (follow-up) Make changes for 26582
Bug 23971: Add logging for basket related actions
Bug 23971: (follow-up) Remove data duplication
Bug 23971: (follow-up) Add additional basket logging
Bug 23971: (QA follow-up) Fix punctuation, doubled up return and syspref order
Bug 23971: (follow-up) Add unit tests
Bug 23971: (follow-up) Respond to feedback
Bug 23971: (follow-up) Fix tests
Bug 23971: (follow-up) Make changes for 26582
Bug 23971: (QA follow-up) New DBrev syntax
Bug 23971: (QA follow-up) New DBrev syntax
Bug 23971: Add logging for basket related actions
Bug 23971: (follow-up) Remove data duplication
Bug 23971: (follow-up) Add additional basket logging
Bug 23971: (QA follow-up) Fix punctuation, doubled up return and syspref order
Bug 23971: (follow-up) Add unit tests
Bug 23971: (follow-up) Respond to feedback
Bug 23971: (follow-up) Fix tests
Bug 23971: (follow-up) Make changes for 26582
Bug 23971: (QA follow-up) New DBrev syntax
Bug 23971: (follow-up) Log entire objects
Bug 23971: Add logging for basket related actions
Bug 23971: (follow-up) Remove data duplication
Bug 23971: (follow-up) Add additional basket logging
Bug 23971: (QA follow-up) Fix punctuation, doubled up return and syspref order
Bug 23971: (follow-up) Add unit tests
Bug 23971: (follow-up) Respond to feedback
Bug 23971: (follow-up) Fix tests
Bug 23971: (follow-up) Make changes for 26582
Bug 23971: (QA follow-up) New DBrev syntax
Bug 23971: (follow-up) Log entire objects
Bug 23971: (follow-up) Remove stale param
Bug 23971: Add logging for basket related actions
Bug 23971: (follow-up) Remove data duplication
Bug 23971: (follow-up) Add additional basket logging
Bug 23971: (QA follow-up) Fix punctuation, doubled up return and syspref order
Bug 23971: (follow-up) Add unit tests
Bug 23971: (follow-up) Respond to feedback
Bug 23971: (follow-up) Fix tests
Bug 23971: (follow-up) Make changes for 26582
Bug 23971: (QA follow-up) New DBrev syntax
Bug 23971: (follow-up) Log entire objects
Bug 23971: (follow-up) Remove stale param
Bug 23971: (follow-up) Fix template typo
Bug 23971: Add logging for basket related actions
Bug 23971: (follow-up) Remove data duplication
Bug 23971: (follow-up) Add additional basket logging
Bug 23971: (QA follow-up) Fix punctuation, doubled up return and syspref order
Bug 23971: (follow-up) Add unit tests
Bug 23971: (follow-up) Respond to feedback
Bug 23971: (follow-up) Fix tests
Bug 23971: (follow-up) Make changes for 26582
Bug 23971: (QA follow-up) New DBrev syntax
Bug 23971: (follow-up) Log entire objects
Bug 23971: (follow-up) Remove stale param
Bug 23971: Add logging for basket related actions
Bug 23971: Add logging for basket related actions
Bug 23971: (follow-up) Remove data duplication
Bug 23971: (follow-up) Add additional basket logging
Bug 23971: (QA follow-up) Fix punctuation, doubled up return and syspref order
Bug 23971: (follow-up) Add unit tests
Bug 23971: (follow-up) Respond to feedback
Bug 23971: (follow-up) Fix tests
Bug 23971: (follow-up) Make changes for 26582
Bug 23971: (QA follow-up) New DBrev syntax
Bug 23971: (follow-up) Log entire objects
Bug 23971: (follow-up) Remove stale param
Bug 23971: Rename pref with AcquisitionLog
Bug 23971: Do not always flag AcquisitionLog as 'not enabled'
Bug 23971: Fix UsageStats.t

Description Andrew Isherwood 2019-11-05 10:57:34 UTC
Acquisitions currently creates no action_logs. Some of our customers are requesting the addition of logs. The first stage of this work is to add the following logging:

1. Who created a basket and when
Basket number, Basket name, Creation date, Vendor name, Creator name

2. Who edited a basket and when
Basket number, Basket name, Edited date, Vendor name, Editor name

3. Who approved a basket and when
Basket number, Basket name, Approval date, Vendor name, Who approved

4. Who closed a basket and when
Basket number, Basket name, Closed date, Vendor name, Who closed

(all should include EDI order information)

This bug is related to Bug 13325, which requests logging of fund related actions
Comment 1 Andrew Isherwood 2019-11-12 08:48:20 UTC
Created attachment 95285 [details] [review]
Bug 23971: Add Acq action logging

This patch adds logging for the following Acq actions:

- Basket creation
- Basket editing
- Basket approval (via EDI)
- Basket closure
Comment 2 Andrew Isherwood 2019-11-12 09:10:29 UTC
Created attachment 95286 [details] [review]
Bug 23971: Add Acq action logging

This patch adds logging for the following Acq actions:

- Basket creation
- Basket editing
- Basket approval (via EDI)
- Basket closure
Comment 3 Andrew Isherwood 2019-11-12 09:12:31 UTC
Test plan:

- Apply the patch
- Create a basket
=> TEST: Observe that an Acquisitions action log has been created containing:
  => Basket number
  => Basket name
  => Creation date
  => Vendor name
  => Creator name
- Modify a basket
=> TEST: Observe that an Acquisitions action log has been created containing:
  => Basket number
  => Basket name
  => Edited date
  => Vendor name
  => Editor name
- Approve a basket via EDI
=> TEST: Observe that an Acquisitions action log has been created containing:
  => Basket number
  => Basket name
  => Approval date
  => Vendor name
  => Approver name
- Close a basket
=> TEST: Observe that an Acquisitions action log has been created containing:
  => Basket number
  => Basket name
  => Closed date
  => Vendor name
  => Closer name
Comment 4 Séverine Queune 2019-11-19 15:13:19 UTC
I did not manage to test the EDI part, it's a feature that I know who to use.
Except that point, the 3 others work as described, even if I'm always surprised by the random order of the informations in actions_logs, but it's certainly out of scope of this patch.

I think I could be good if libraries could disable this log, like it's possible for the other modules using the "Logging" sysprefs.

I would probably add the reopening of a basket in the log, for, in my library, this is THE action out of workflow and I'm always quite annoy not knowing who did it.
Comment 5 Katrin Fischer 2019-11-24 14:37:19 UTC
I think Severine is right - logging the reopening of a basket would be really helpful. Andrew, could this be added to this patch set?
Comment 6 Katrin Fischer 2019-11-24 14:38:32 UTC
I think the missing pref could be considered a "Failed QA" as we should stick with existing patterns and it makes sense here.
Comment 7 Andrew Isherwood 2019-11-29 15:18:16 UTC
Created attachment 95885 [details] [review]
Bug 23971: Add Acq action logging

This patch adds logging for the following Acq actions:

- Basket creation
- Basket editing
- Basket approval (via EDI)
- Basket closure
Comment 8 Andrew Isherwood 2019-11-29 15:26:31 UTC
The nature of the logging has now changed according to customer requirements. I am logging less, and it is necessary now to use table joins in reports in order to gain access to the more detailed information, this will make the logging done here much more usable and flexible.

Katrin and Séverine - I agree that basket reopening would be a useful thing to log, but it is outside of the scope of the requirements for this bug. There are many Acq actions that could be logged, I'm hoping these will be added in time.

Katrin - I have put the logging behind an AcqLog syspref, thanks for highlighting this.

Revised test plan:

- Apply the patch
=> TEST: Observe there is a new "AcqLog" syspref
- Enable the AcqLog syspref
- Create a basket
=> TEST: Observe that an Acquisitions action log has been created containing:
  => Basket number
- Modify a basket
=> TEST: Observe that an Acquisitions action log has been created containing:
  => Basket number
  => Borrowernumber of the user who modified the basket, this is stored in "info" as a left padded 10 digit number which can be joined on to the borrowers table
- Approve a basket via EDI
=> TEST: Observe that an Acquisitions action log has been created containing:
  => Basket number
  => Borrowernumber of the user who approved the basket, this is stored in "info" as a left padded 10 digit number which can be joined on to the borrowers table
- Close a basket
=> TEST: Observe that an Acquisitions action log has been created containing:
  => Basket number
  => Borrowernumber of the user who closed the basket, this is stored in "info" as a left padded 10 digit number which can be joined on to the borrowers table
Comment 9 Andrew Isherwood 2019-11-29 15:41:49 UTC
Created attachment 95888 [details] [review]
Bug 23971 - Add logging for basket related actions

This patch adds logging for the following Acq actions:

- Basket creation
- Basket editing
- Basket approval (via EDI)
- Basket closure
Comment 10 Séverine Queune 2019-12-04 11:14:30 UTC
(In reply to Andrew Isherwood from comment #8)
> Katrin and Séverine - I agree that basket reopening would be a useful thing
> to log, but it is outside of the scope of the requirements for this bug.
> There are many Acq actions that could be logged, I'm hoping these will be
> added in time.
I will open a new bug to add this action.


> Revised test plan:
> 
> - Modify a basket
> => TEST: Observe that an Acquisitions action log has been created containing:
>   => Basket number
>   => Borrowernumber of the user who modified the basket, this is stored in
> "info" as a left padded 10 digit number which can be joined on to the
> borrowers table
3 things about this MODIFY_BASKET log :
  - as the borrowernumber is saved in the 'user' field, why duplicating it in the 'info' field ?
  - in this 'info' field, we usually have the details of the action made by the user. To me, the information displays for now in not really usefull.
  - I only get log when I modify the "Managing library" but not when I update the managers of the basket, not when I edit the basket. Is that the expected behaviour ?

> - Approve a basket via EDI
> => TEST: Observe that an Acquisitions action log has been created containing:
>   => Basket number
>   => Borrowernumber of the user who approved the basket, this is stored in
> "info" as a left padded 10 digit number which can be joined on to the
> borrowers table
I tried several EDI settings but it still doesn't work so one mode ime, I didn't test this part (would be happy if someone could help !).
Comment 11 Katrin Fischer 2019-12-05 06:58:37 UTC
> Katrin and Séverine - I agree that basket reopening would be a useful thing
> to log, but it is outside of the scope of the requirements for this bug.
> There are many Acq actions that could be logged, I'm hoping these will be
> added in time.

I am sorry... but I'd like to at least try to explain my point of view a bit more. This bug states to add logging for basket related actions and of all the basket related actions one is missing. I don't feel this is out of scope at all. I've thought about why this is bothering me so much. I think it comes down to this: One of the biggest challenges we face is to develop Koha to be consistent and intuitive, to work as one piece of software, for everyone using it. When I have a feeling that something will cause question marks, and have me say: 'I know that doesn't make sense' to a library later, I feel I have to point it out.

> Katrin - I have put the logging behind an AcqLog syspref, thanks for
> highlighting this.

Thank you. I am wondering if we should make the pref description a bit more specific to not lead to false expectations for now. Easy to change the text when coverage is extended beyond baskets.

Switching to FQA because of Severine's comment#10
Comment 12 Andrew Isherwood 2019-12-06 10:31:21 UTC
Created attachment 96063 [details] [review]
Bug 23961: (follow-up) Remove data duplication

In response to Séverine observations in comment #10, this patch removes
the duplicate logging of the borrowernumber
Comment 13 Andrew Isherwood 2019-12-06 10:32:43 UTC
(In reply to Séverine Queune from comment #10)
> 3 things about this MODIFY_BASKET log :
>   - as the borrowernumber is saved in the 'user' field, why duplicating it
> in the 'info' field ?

Thanks for point this out, I have attached a patch that removes this duplication.

>   - in this 'info' field, we usually have the details of the action made by
> the user. To me, the information displays for now in not really usefull.

The idea is that we're logging the minimal amount of information in the action logs but, through a report, it would be possible to obtain additional information about the related objects as required, such as basket, borrower etc. This logging is very much a starting point, I'll be elaborating on this point in response to Katrin's message.

>   - I only get log when I modify the "Managing library" but not when I
> update the managers of the basket, not when I edit the basket. Is that the
> expected behaviour ?

Again, this logging is a starting point

> 
> > - Approve a basket via EDI
> > => TEST: Observe that an Acquisitions action log has been created containing:
> >   => Basket number
> >   => Borrowernumber of the user who approved the basket, this is stored in
> > "info" as a left padded 10 digit number which can be joined on to the
> > borrowers table
> I tried several EDI settings but it still doesn't work so one mode ime, I
> didn't test this part (would be happy if someone could help !).

Thanks, I'll do some testing and fix any problems.
Comment 14 Andrew Isherwood 2019-12-06 10:57:11 UTC
(In reply to Katrin Fischer from comment #11)

> I am sorry... but I'd like to at least try to explain my point of view a bit
> more. This bug states to add logging for basket related actions and of all
> the basket related actions one is missing. I don't feel this is out of scope
> at all. I've thought about why this is bothering me so much. I think it
> comes down to this: One of the biggest challenges we face is to develop Koha
> to be consistent and intuitive, to work as one piece of software, for
> everyone using it. When I have a feeling that something will cause question
> marks, and have me say: 'I know that doesn't make sense' to a library later,
> I feel I have to point it out.

Hi Katrin

Thanks for your comments :) Firstly, I *completely* understand what you're saying and agree with everything. The challenge I face is that we have very specific requirements from the customer who is sponsoring this work and the patches that I have created address those requirements. So my comment about additional logging being out of scope meant that it was out of scope of those requirements. You're right, it is not out of scope of basket logging.

There are many actions that can be performed on a basket, Séverine highlighted this by pointing out that logging did not occur when a basket manager is changed. Our requirement was to add logging of basket modification in the context of modifying the basket contents. Ideally, this bug would log every possible action on a basket, but that would significantly increase development time. I feel that this work is a foundation which will facilitate the addition of further logging.

That said, I am about to create a patch to add basket reopening and basket header modification logging since those were specifically mentioned by yourself and Séverine.

It's also worth me pointing out that I am about to embark on a further Acq logging development that is being sponsored by another customer, this will add substantial logging to other areas of Acq and will hopefully partly address some of your concerns.
Comment 15 Katrin Fischer 2019-12-06 11:17:02 UTC
(In reply to Andrew Isherwood from comment #14)
> (In reply to Katrin Fischer from comment #11)
> 
> > I am sorry... but I'd like to at least try to explain my point of view a bit
> > more. This bug states to add logging for basket related actions and of all
> > the basket related actions one is missing. I don't feel this is out of scope
> > at all. I've thought about why this is bothering me so much. I think it
> > comes down to this: One of the biggest challenges we face is to develop Koha
> > to be consistent and intuitive, to work as one piece of software, for
> > everyone using it. When I have a feeling that something will cause question
> > marks, and have me say: 'I know that doesn't make sense' to a library later,
> > I feel I have to point it out.
> 
> Hi Katrin
> 
> Thanks for your comments :) Firstly, I *completely* understand what you're
> saying and agree with everything. The challenge I face is that we have very
> specific requirements from the customer who is sponsoring this work and the
> patches that I have created address those requirements. So my comment about
> additional logging being out of scope meant that it was out of scope of
> those requirements. You're right, it is not out of scope of basket logging.
> 
> There are many actions that can be performed on a basket, Séverine
> highlighted this by pointing out that logging did not occur when a basket
> manager is changed. Our requirement was to add logging of basket
> modification in the context of modifying the basket contents. Ideally, this
> bug would log every possible action on a basket, but that would
> significantly increase development time. I feel that this work is a
> foundation which will facilitate the addition of further logging.
> 
> That said, I am about to create a patch to add basket reopening and basket
> header modification logging since those were specifically mentioned by
> yourself and Séverine.
> 
> It's also worth me pointing out that I am about to embark on a further Acq
> logging development that is being sponsored by another customer, this will
> add substantial logging to other areas of Acq and will hopefully partly
> address some of your concerns.

Thx Andrew for not getting this wrong most of all :)

Also for considering an additon - you are right, Severine made a good point about the basket users. I am not going to block this and secretly hope the path of logging might lead you in direction of funds and budgetes - I know this was often wished for.
Comment 16 Andrew Isherwood 2019-12-06 11:22:18 UTC
(In reply to Katrin Fischer from comment #15)

> [..] I am not going to block this and secretly hope the
> path of logging might lead you in direction of funds and budgetes - I know
> this was often wished for.

Well, you will hopefully be pleased. These are the requirements for the next work I am doing:

1. Order line cancellations
This would show order line details including who made the cancellation and when the cancellation was made


2. Order line creations
This would show order line details including who created the order line, when it was created, the fund, and amount


3. Invoice adjustment additions
This would show the adjustment details, who added the adjustment, when it was added, the fund, and amount


4. Invoice adjustment amendments
This would show the adjustment details, who amended the adjustment, when it was amended, the fund, and amount


5. Invoice adjustment deletions
This would show the adjustment details, who deleted the adjustment, when it was deleted, the fund, and amount


6. Order line receipts against an invoice
This would show the order line details, who received the order line, when it was received, the fund, and amount


7. Budget Adjustments
This would show amounts added or removed from a budget (excluding spent), who, when, and amount

8. Fund adjustments
This is essentially a bank statement. It would show additions and subtractions to a fund (excluding spent), who, when, amount, previous total, new total

9. Order release date (1)
This would show individual order lines from an EDI message, when they were sent, who sent it, fund, and amount

10. Order release date (2)
This would show individual order lines from a basket, when it was closed, who closed it, fund, and amount
Comment 17 Andrew Isherwood 2019-12-06 11:36:50 UTC
Created attachment 96064 [details] [review]
Bug 23971: (follow-up) Add additional basket logging

This patch adds additional basket logging as discussed in comment #14

- Basket reopening
- Basket users (manager) modification
- Basket header modification
Comment 18 Andrew Isherwood 2019-12-06 11:54:00 UTC
I can't find any problems with the EDI approval basket closure. I'm going to set this back to "Needs Signoff" as I think I've addressed the additional requirements.
Comment 19 Andrew Isherwood 2020-03-06 10:28:38 UTC
Created attachment 100239 [details] [review]
Bug 23971 - Add logging for basket related actions

This patch adds logging for the following Acq actions:

- Basket creation
- Basket editing
- Basket approval (via EDI)
- Basket closure
Comment 20 Andrew Isherwood 2020-03-06 10:28:41 UTC
Created attachment 100240 [details] [review]
Bug 23971: (follow-up) Remove data duplication

In response to Séverine observations in comment #10, this patch removes
the duplicate logging of the borrowernumber
Comment 21 Andrew Isherwood 2020-03-06 10:28:45 UTC
Created attachment 100241 [details] [review]
Bug 23971: (follow-up) Add additional basket logging

This patch adds additional basket logging as discussed in comment #14

- Basket reopening
- Basket users (manager) modification
- Basket header modification
Comment 22 Holly 2020-04-08 10:23:31 UTC
I tested this patch but the following information is not appearing in the Log Viewer.

- Basket reopening
- Basket users (manager) modification
- Basket header modification

Other actions are appearing in the Acquisitions log.
Comment 23 Martin Renvoize 2020-04-29 10:29:51 UTC
Created attachment 103911 [details] [review]
Bug 23971: Add logging for basket related actions

This patch adds logging for the following Acq actions:

- Basket creation
- Basket editing
- Basket approval (via EDI)
- Basket closure
Comment 24 Martin Renvoize 2020-04-29 10:29:55 UTC
Created attachment 103912 [details] [review]
Bug 23971: (follow-up) Remove data duplication

In response to Séverine observations in comment #10, this patch removes
the duplicate logging of the borrowernumber
Comment 25 Martin Renvoize 2020-04-29 10:29:58 UTC
Created attachment 103913 [details] [review]
Bug 23971: (follow-up) Add additional basket logging

This patch adds additional basket logging as discussed in comment #14

- Basket reopening
- Basket users (manager) modification
- Basket header modification
Comment 26 Andrew Isherwood 2020-04-29 11:18:00 UTC
Seems there was a problem with applying this bug, which the latest patches attached by Martin should fix. Hopefully this will now pass QA.
Comment 27 ByWater Sandboxes 2020-04-29 14:06:59 UTC
Created attachment 103945 [details] [review]
Bug 23971: (follow-up) Add additional basket logging

This patch adds additional basket logging as discussed in comment #14

- Basket reopening
- Basket users (manager) modification
- Basket header modification

Signed-off-by: Holly <hc@interleaf.ie>
Comment 28 Jonathan Druart 2020-08-04 14:26:00 UTC
Some comments from bug 24190 comment 14 (&15) apply here.

Additionally I don't think it's a good idea to pass the flag to CloseBasket.

 577             # Close the basket, passing a flag indicating that this action
 578             # originated from an approval
 579             CloseBasket($b, 1);

Cannot we imagine that Koha::EDI deals with this specific situation? (UPDATE the action_log with the desire action)
Comment 29 Katrin Fischer 2020-08-23 21:22:05 UTC
2 things to note, but I think not blocker:

- When a basket is created, a "MODIFY_BASKET_HEADER" is logged at the same time
- Deleting a basket is not logged yet
- The info column is not logged here, so the information known is the basket affected and the users involved, but no information about the change itself. I think this is worth knowing, but could be easily extended later.

I see that the question from Jonathan in comment#28 remained unanswered - can you please check, so we can move this forward? Testing looks good so far.
Comment 30 Katrin Fischer 2020-08-23 21:25:12 UTC
Created attachment 108962 [details] [review]
Bug 23971: (QA follow-up) Fix punctuation, doubled up return and syspref order
Comment 31 Andrew Isherwood 2020-09-24 14:00:52 UTC
(In reply to Jonathan Druart from comment #28)
Hi Jonathan

> Some comments from bug 24190 comment 14 (&15) apply here.

I will review those comments when I get to that bug and respond there, any decisions made there I will also apply here where appropriate.


> Cannot we imagine that Koha::EDI deals with this specific situation? (UPDATE the action_log with the desire action)

Sorry, I'm not 100% understanding. Are you suggesting that I drop the flag in the call to CloseBasket from Koha/EDI.pm (and consequently also remove it from the CloseBasket definition). And, instead, cause EDI::process_quote to update the action_log entry that was created in CloseBasket?
Comment 32 Andrew Isherwood 2020-10-06 12:43:46 UTC
Created attachment 111300 [details] [review]
Bug 23971: Add logging for basket related actions

This patch adds logging for the following Acq actions:

- Basket creation
- Basket editing
- Basket approval (via EDI)
- Basket closure
Comment 33 Andrew Isherwood 2020-10-06 12:43:51 UTC
Created attachment 111301 [details] [review]
Bug 23971: (follow-up) Remove data duplication

In response to Séverine observations in comment #10, this patch removes
the duplicate logging of the borrowernumber
Comment 34 Andrew Isherwood 2020-10-06 12:43:58 UTC
Created attachment 111302 [details] [review]
Bug 23971: (follow-up) Add additional basket logging

This patch adds additional basket logging as discussed in comment #14

- Basket reopening
- Basket users (manager) modification
- Basket header modification

Signed-off-by: Holly <hc@interleaf.ie>
Comment 35 Andrew Isherwood 2020-10-06 12:44:03 UTC
Created attachment 111303 [details] [review]
Bug 23971: (QA follow-up) Fix punctuation, doubled up return and syspref order
Comment 36 Andrew Isherwood 2020-10-06 12:44:08 UTC
Created attachment 111304 [details] [review]
Bug 23971: (follow-up) Add unit tests

This commit adds unit tests for the logging done in Acquisitions.pm
Comment 37 Andrew Isherwood 2020-10-06 12:46:57 UTC
(In reply to Jonathan Druart from comment #28)
> Hi Jonathan
> 
> > Some comments from bug 24190 comment 14 (&15) apply here.
> 
> I will review those comments when I get to that bug and respond there, any
> decisions made there I will also apply here where appropriate.
> 

Hi Jonathan - I've now added unit tests for the logging, as per your comments in bug 24190
Comment 38 Katrin Fischer 2020-10-14 21:32:53 UTC
(In reply to Jonathan Druart from comment #28)
> Some comments from bug 24190 comment 14 (&15) apply here.
> 
> Additionally I don't think it's a good idea to pass the flag to CloseBasket.
> 
>  577             # Close the basket, passing a flag indicating that this
> action
>  578             # originated from an approval
>  579             CloseBasket($b, 1);
> 
> Cannot we imagine that Koha::EDI deals with this specific situation? (UPDATE
> the action_log with the desire action)

Hi Andrew, could you answer the question above about EDI please?

Then should this be reset to Signed off or NSO?
Comment 39 Andrew Isherwood 2020-10-15 07:18:18 UTC
(In reply to Katrin Fischer from comment #38)
> Hi Andrew, could you answer the question above about EDI please?

Hi Katrin - I can't answer it until Jonathan clarifies what he means, as I requested in comment #31

> Then should this be reset to Signed off or NSO?

Well, it's signed off, so I guess "Signed off"
Comment 40 Jonathan Druart 2020-10-15 08:59:05 UTC
(In reply to Andrew Isherwood from comment #31)
> > Cannot we imagine that Koha::EDI deals with this specific situation? (UPDATE the action_log with the desire action)
> 
> Sorry, I'm not 100% understanding. Are you suggesting that I drop the flag
> in the call to CloseBasket from Koha/EDI.pm (and consequently also remove it
> from the CloseBasket definition). And, instead, cause EDI::process_quote to
> update the action_log entry that was created in CloseBasket?

Yes, it's what I meant!
Comment 41 Andrew Isherwood 2020-10-19 09:56:48 UTC
Hi Jonathan

OK, I've now made the change you suggested. We no longer pass anything to CloseBasket indicating whether the closure is tied to an EDI approval. I now log the closure and the approval separately, since they are actually distinct things.

Revised test plan:

- Apply the patch
=> TEST: Observe there is a new "AcqLog" syspref
- Enable the AcqLog syspref
- Create a basket
=> TEST: Observe that an Acquisitions "Create an acquisitions basket" action log has been created containing:
  => Basket number in "Object" column
  => Borrowernumber of staff member in "Librarian" column
=> TEST: Observe that an Acquisitions "Modify an acquisitions basket header" action log has been created containing:
  => Basket number in "Object" column
  => Borrowernumber of staff member in "Librarian" column
- Modify a basket
=> TEST: Observe that an Acquisitions "Modify an acquisitions basket header" action log has been created containing:
  => Basket number in "Object" column
  => Borrowernumber of staff member in "Librarian" column
- Approve a basket via EDI
=> TEST: Observe that an Acquisitions "Close an acquisitions basket" action log has been created containing:
  => Basket number in "Object" column
  => Borrowernumber of staff member in "Librarian" column
=> TEST: Observe that an Acquisitions "Approve an acquisitions basket" action log has been created containing:
  => Basket number in "Object" column
  => Borrowernumber of staff member in "Librarian" column
- Close a basket
=> TEST: Observe that an Acquisitions "Close an acquisitions basket" action log has been created containing:
  => Basket number in "Object" column
  => Borrowernumber of staff member in "Librarian" column
Comment 42 Andrew Isherwood 2020-10-19 09:57:38 UTC
Created attachment 111955 [details] [review]
Bug 23971: Add logging for basket related actions

This patch adds logging for the following Acq actions:

- Basket creation
- Basket editing
- Basket approval (via EDI)
- Basket closure
Comment 43 Andrew Isherwood 2020-10-19 09:57:44 UTC
Created attachment 111956 [details] [review]
Bug 23971: (follow-up) Remove data duplication

In response to Séverine observations in comment #10, this patch removes
the duplicate logging of the borrowernumber
Comment 44 Andrew Isherwood 2020-10-19 09:57:51 UTC
Created attachment 111957 [details] [review]
Bug 23971: (follow-up) Add additional basket logging

This patch adds additional basket logging as discussed in comment #14

- Basket reopening
- Basket users (manager) modification
- Basket header modification

Signed-off-by: Holly <hc@interleaf.ie>
Comment 45 Andrew Isherwood 2020-10-19 09:57:59 UTC
Created attachment 111958 [details] [review]
Bug 23971: (QA follow-up) Fix punctuation, doubled up return and syspref order
Comment 46 Andrew Isherwood 2020-10-19 09:58:04 UTC
Created attachment 111959 [details] [review]
Bug 23971: (follow-up) Add unit tests

This commit adds unit tests for the logging done in Acquisitions.pm
Comment 47 Andrew Isherwood 2020-10-19 09:58:11 UTC
Created attachment 111960 [details] [review]
Bug 23971: (follow-up) Respond to feedback

As requested by Jonathan in commit #28:

- We now no longer pass a flag to CloseBasket indicating if the closure
was due to an EDI basket approval, we log the closure and approval as
separate events.
Comment 48 Tomás Cohen Arazi 2020-10-22 15:03:10 UTC
Hi, it all looks ok.

Things to look at:
- Bug 26582 is PQA, should we rebase this one on top of it? I can help (it means moving the tests to the relevant file, etc). This is mainly a question for the RM.
- I don't see tests for Koha::EDI related to the added APPROVE_BASKET action. They should be in t/db_dependent/Koha/EDI.t

There's a minor rebase issue (tests count).
Comment 49 Andrew Isherwood 2020-10-23 11:57:11 UTC
(In reply to Tomás Cohen Arazi from comment #48)
> - Bug 26582 is PQA, should we rebase this one on top of it? I can help (it
> means moving the tests to the relevant file, etc). This is mainly a question
> for the RM.
I think there's more to it than that. Looking at my code, currently the logging is embedded within C4::Acquisision::CloseBasket. So I'd need to move that to Koha::Acquisition::Basket::close. This isn't a problem, I just wanted to highlight it in case it would be problematic. Having logging as a side-effect in this way feels slightly weird, but it seems like it's less potentially error prone than having to explicitly log at all the places where Koha::Acquisition::Basket::close is called.

So, I think I'd have to move the logging as described above and modify the unit tests accordingly.

> - I don't see tests for Koha::EDI related to the added APPROVE_BASKET
> action. They should be in t/db_dependent/Koha/EDI.t
> 
As discussed on Slack, to be able to test this very, very small bit of functionality (is the approval of a basket logged) we'd have to implement a large amount of EDI testing infrastructure that doesn't currently exist, this seems out of scope to me.

I'm not going to do anything more on this until we're all happy with how to move forward :-)
Comment 50 Andrew Isherwood 2020-10-23 14:30:10 UTC
Created attachment 112277 [details] [review]
Bug 23971: Add logging for basket related actions

This patch adds logging for the following Acq actions:

- Basket creation
- Basket editing
- Basket approval (via EDI)
- Basket closure
Comment 51 Andrew Isherwood 2020-10-23 14:30:19 UTC
Created attachment 112278 [details] [review]
Bug 23971: (follow-up) Remove data duplication

In response to Séverine observations in comment #10, this patch removes
the duplicate logging of the borrowernumber
Comment 52 Andrew Isherwood 2020-10-23 14:30:25 UTC
Created attachment 112279 [details] [review]
Bug 23971: (follow-up) Add additional basket logging

This patch adds additional basket logging as discussed in comment #14

- Basket reopening
- Basket users (manager) modification
- Basket header modification

Signed-off-by: Holly <hc@interleaf.ie>
Comment 53 Andrew Isherwood 2020-10-23 14:30:31 UTC
Created attachment 112280 [details] [review]
Bug 23971: (QA follow-up) Fix punctuation, doubled up return and syspref order
Comment 54 Andrew Isherwood 2020-10-23 14:30:37 UTC
Created attachment 112281 [details] [review]
Bug 23971: (follow-up) Add unit tests

This commit adds unit tests for the logging done in Acquisitions.pm
Comment 55 Andrew Isherwood 2020-10-23 14:30:43 UTC
Created attachment 112282 [details] [review]
Bug 23971: (follow-up) Respond to feedback

As requested by Jonathan in commit #28:

- We now no longer pass a flag to CloseBasket indicating if the closure
was due to an EDI basket approval, we log the closure and approval as
separate events.
Comment 56 Andrew Isherwood 2020-10-23 14:30:48 UTC
Created attachment 112283 [details] [review]
Bug 23971: (follow-up) Fix tests

Following a discussion with Tomás on Slack, this commit implements the
following suggestions:

- Switch from ->find to ->search in tests. ->find is only ever going to return 0
or 1 rows, which doesn't help us if there is breakage which results in
more than 1 row being returned, the test would fail due to ->find not
expecting more than 1 row, but we should be testing for it. So switching
to ->search returns all matching rows.
- Moved tests into their own subtest, they had the potential to pollute
surrounding tests
- Remove all action logs before each test, this ensures we're only
testing the result of the current test
Comment 57 Andrew Isherwood 2020-10-23 14:47:16 UTC
Created attachment 112284 [details] [review]
Bug 23971: Add logging for basket related actions

This patch adds logging for the following Acq actions:

- Basket creation
- Basket editing
- Basket approval (via EDI)
- Basket closure
Comment 58 Andrew Isherwood 2020-10-23 14:47:22 UTC
Created attachment 112285 [details] [review]
Bug 23971: (follow-up) Remove data duplication

In response to Séverine observations in comment #10, this patch removes
the duplicate logging of the borrowernumber
Comment 59 Andrew Isherwood 2020-10-23 14:47:27 UTC
Created attachment 112286 [details] [review]
Bug 23971: (follow-up) Add additional basket logging

This patch adds additional basket logging as discussed in comment #14

- Basket reopening
- Basket users (manager) modification
- Basket header modification

Signed-off-by: Holly <hc@interleaf.ie>
Comment 60 Andrew Isherwood 2020-10-23 14:47:32 UTC
Created attachment 112287 [details] [review]
Bug 23971: (QA follow-up) Fix punctuation, doubled up return and syspref order
Comment 61 Andrew Isherwood 2020-10-23 14:47:37 UTC
Created attachment 112288 [details] [review]
Bug 23971: (follow-up) Add unit tests

This commit adds unit tests for the logging done in Acquisitions.pm
Comment 62 Andrew Isherwood 2020-10-23 14:47:42 UTC
Created attachment 112289 [details] [review]
Bug 23971: (follow-up) Respond to feedback

As requested by Jonathan in commit #28:

- We now no longer pass a flag to CloseBasket indicating if the closure
was due to an EDI basket approval, we log the closure and approval as
separate events.
Comment 63 Andrew Isherwood 2020-10-23 14:47:48 UTC
Created attachment 112290 [details] [review]
Bug 23971: (follow-up) Fix tests

Following a discussion with Tomás on Slack, this commit implements the
following suggestions:

- Switch from ->find to ->search in tests. ->find is only ever going to return 0
or 1 rows, which doesn't help us if there is breakage which results in
more than 1 row being returned, the test would fail due to ->find not
expecting more than 1 row, but we should be testing for it. So switching
to ->search returns all matching rows.
- Moved tests into their own subtest, they had the potential to pollute
surrounding tests
- Remove all action logs before each test, this ensures we're only
testing the result of the current test
Comment 64 Andrew Isherwood 2020-10-23 14:47:53 UTC
Created attachment 112291 [details] [review]
Bug 23971: (follow-up) Make changes for 26582

This commit makes changes necessary to allow this bug to be dependent on
Bug 23562:

- Move basket closure logging from C4::Acquisision::CloseBasket to Koha::Acquisition::Basket::close
- Move basket closure unit test from t/db_dependent/Acquisition.t to
t/db_dependent/Koha/Acquisition/Basket.t
Comment 65 Andrew Isherwood 2020-10-23 14:48:50 UTC
Hi Tomás

I've now made the changes we discussed on Slack. Should be ready for you again now.
Comment 66 Tomás Cohen Arazi 2020-10-23 18:34:41 UTC
Created attachment 112298 [details] [review]
Bug 23971: Add logging for basket related actions

This patch adds logging for the following Acq actions:

- Basket creation
- Basket editing
- Basket approval (via EDI)
- Basket closure

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 67 Tomás Cohen Arazi 2020-10-23 18:34:54 UTC
Created attachment 112299 [details] [review]
Bug 23971: (follow-up) Remove data duplication

In response to Séverine observations in comment #10, this patch removes
the duplicate logging of the borrowernumber

https://bugs.koha-community.org/show_bug.cgi?id=23971
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 68 Tomás Cohen Arazi 2020-10-23 18:35:08 UTC
Created attachment 112300 [details] [review]
Bug 23971: (follow-up) Add additional basket logging

This patch adds additional basket logging as discussed in comment #14

- Basket reopening
- Basket users (manager) modification
- Basket header modification

Signed-off-by: Holly <hc@interleaf.ie>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 69 Tomás Cohen Arazi 2020-10-23 18:35:20 UTC
Created attachment 112301 [details] [review]
Bug 23971: (QA follow-up) Fix punctuation, doubled up return and syspref order

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 70 Tomás Cohen Arazi 2020-10-23 18:35:33 UTC
Created attachment 112302 [details] [review]
Bug 23971: (follow-up) Add unit tests

This commit adds unit tests for the logging done in Acquisitions.pm

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 71 Tomás Cohen Arazi 2020-10-23 18:35:44 UTC
Created attachment 112303 [details] [review]
Bug 23971: (follow-up) Respond to feedback

As requested by Jonathan in commit #28:

- We now no longer pass a flag to CloseBasket indicating if the closure
was due to an EDI basket approval, we log the closure and approval as
separate events.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 72 Tomás Cohen Arazi 2020-10-23 18:36:00 UTC
Created attachment 112304 [details] [review]
Bug 23971: (follow-up) Fix tests

Following a discussion with Tomás on Slack, this commit implements the
following suggestions:

- Switch from ->find to ->search in tests. ->find is only ever going to return 0
or 1 rows, which doesn't help us if there is breakage which results in
more than 1 row being returned, the test would fail due to ->find not
expecting more than 1 row, but we should be testing for it. So switching
to ->search returns all matching rows.
- Moved tests into their own subtest, they had the potential to pollute
surrounding tests
- Remove all action logs before each test, this ensures we're only
testing the result of the current test

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 73 Tomás Cohen Arazi 2020-10-23 18:36:33 UTC
Created attachment 112305 [details] [review]
Bug 23971: (follow-up) Make changes for 26582

This commit makes changes necessary to allow this bug to be dependent on
Bug 23562:

- Move basket closure logging from C4::Acquisision::CloseBasket to Koha::Acquisition::Basket::close
- Move basket closure unit test from t/db_dependent/Acquisition.t to
t/db_dependent/Koha/Acquisition/Basket.t

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 74 Tomás Cohen Arazi 2020-10-23 18:37:51 UTC
Created attachment 112306 [details] [review]
Bug 23971: (QA follow-up) New DBrev syntax

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 75 Tomás Cohen Arazi 2020-10-23 18:39:08 UTC
Created attachment 112307 [details] [review]
Bug 23971: (QA follow-up) New DBrev syntax

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 76 Tomás Cohen Arazi 2020-10-29 15:58:16 UTC
I still feel the logs should include the objects, following what's been done so far with the rest of the logs.
Comment 77 Jonathan Druart 2020-11-09 11:06:38 UTC
This is conflicting with bug 26584

CONFLICT (content): Merge conflict in acqui/basket.pl
CONFLICT (content): Merge conflict in Koha/EDI.pm
CONFLICT (content): Merge conflict in C4/Acquisition.pm

Please rebase.

(In reply to Tomás Cohen Arazi from comment #76)
> I still feel the logs should include the objects, following what's been done
> so far with the rest of the logs.

I am with Tomas, we will know who modified the basket, but not the changes that have been made.
Comment 78 Andrew Isherwood 2020-11-10 12:21:37 UTC
Created attachment 113384 [details] [review]
Bug 23971: Add logging for basket related actions

This patch adds logging for the following Acq actions:

- Basket creation
- Basket editing
- Basket approval (via EDI)
- Basket closure

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 79 Andrew Isherwood 2020-11-10 12:21:41 UTC
Created attachment 113385 [details] [review]
Bug 23971: (follow-up) Remove data duplication

In response to Séverine observations in comment #10, this patch removes
the duplicate logging of the borrowernumber

https://bugs.koha-community.org/show_bug.cgi?id=23971
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 80 Andrew Isherwood 2020-11-10 12:21:46 UTC
Created attachment 113386 [details] [review]
Bug 23971: (follow-up) Add additional basket logging

This patch adds additional basket logging as discussed in comment #14

- Basket reopening
- Basket users (manager) modification
- Basket header modification

Signed-off-by: Holly <hc@interleaf.ie>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 81 Andrew Isherwood 2020-11-10 12:21:50 UTC
Created attachment 113387 [details] [review]
Bug 23971: (QA follow-up) Fix punctuation, doubled up return and syspref order

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 82 Andrew Isherwood 2020-11-10 12:21:54 UTC
Created attachment 113388 [details] [review]
Bug 23971: (follow-up) Add unit tests

This commit adds unit tests for the logging done in Acquisitions.pm

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 83 Andrew Isherwood 2020-11-10 12:21:58 UTC
Created attachment 113389 [details] [review]
Bug 23971: (follow-up) Respond to feedback

As requested by Jonathan in commit #28:

- We now no longer pass a flag to CloseBasket indicating if the closure
was due to an EDI basket approval, we log the closure and approval as
separate events.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 84 Andrew Isherwood 2020-11-10 12:22:02 UTC
Created attachment 113390 [details] [review]
Bug 23971: (follow-up) Fix tests

Following a discussion with Tomás on Slack, this commit implements the
following suggestions:

- Switch from ->find to ->search in tests. ->find is only ever going to return 0
or 1 rows, which doesn't help us if there is breakage which results in
more than 1 row being returned, the test would fail due to ->find not
expecting more than 1 row, but we should be testing for it. So switching
to ->search returns all matching rows.
- Moved tests into their own subtest, they had the potential to pollute
surrounding tests
- Remove all action logs before each test, this ensures we're only
testing the result of the current test

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 85 Andrew Isherwood 2020-11-10 12:22:07 UTC
Created attachment 113391 [details] [review]
Bug 23971: (follow-up) Make changes for 26582

This commit makes changes necessary to allow this bug to be dependent on
Bug 23562:

- Move basket closure logging from C4::Acquisision::CloseBasket to Koha::Acquisition::Basket::close
- Move basket closure unit test from t/db_dependent/Acquisition.t to
t/db_dependent/Koha/Acquisition/Basket.t

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 86 Andrew Isherwood 2020-11-10 12:22:11 UTC
Created attachment 113392 [details] [review]
Bug 23971: (QA follow-up) New DBrev syntax

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 87 Andrew Isherwood 2020-11-10 12:22:15 UTC
Created attachment 113393 [details] [review]
Bug 23971: (follow-up) Log entire objects

As requested by Tomás and Jonathan, we now log the entire basket object
when logging an action.
Comment 88 Andrew Isherwood 2020-11-10 12:23:22 UTC
Hi Jonathan

I've now rebased. I've also added logging of the entire basket object as requested.

Revised test plan:

- Apply the patch
=> TEST: Observe there is a new "AcqLog" syspref
- Enable the AcqLog syspref
- Create a basket
=> TEST: Observe that an Acquisitions "Create an acquisitions basket" action log has been created containing:
  => Basket number in "Object" column
  => Borrowernumber of staff member in "Librarian" column
  => Full basket object details in "Info" column
=> TEST: Observe that an Acquisitions "Modify an acquisitions basket header" action log has been created containing:
  => Basket number in "Object" column
  => Borrowernumber of staff member in "Librarian" column
  => Full basket object details in "Info" column
- Modify a basket
=> TEST: Observe that an Acquisitions "Modify an acquisitions basket header" action log has been created containing:
  => Basket number in "Object" column
  => Borrowernumber of staff member in "Librarian" column
  => Full basket object details in "Info" column
- Approve a basket via EDI
=> TEST: Observe that an Acquisitions "Close an acquisitions basket" action log has been created containing:
  => Basket number in "Object" column
  => Borrowernumber of staff member in "Librarian" column
  => Full basket object details in "Info" column
=> TEST: Observe that an Acquisitions "Approve an acquisitions basket" action log has been created containing:
  => Basket number in "Object" column
  => Borrowernumber of staff member in "Librarian" column
  => Full basket object details in "Info" column
- Close a basket
=> TEST: Observe that an Acquisitions "Close an acquisitions basket" action log has been created containing:
  => Basket number in "Object" column
  => Borrowernumber of staff member in "Librarian" column
Comment 89 Jonathan Druart 2021-01-04 13:53:35 UTC
acqui/addorder.pl is logging a MODIFY_BASKET, is that expected?
Comment 90 Jonathan Druart 2021-01-04 13:55:08 UTC
I squashed the patches for review and found:

+++ b/acqui/basketgroup.pl
@@ -307,7 +307,8 @@ if ( $op eq "add" ) {
   my $basketno=$input->param('basketno');
   my $basketgroupid=$input->param('basketgroupid');
   ModBasket( { basketno => $basketno,
-                         basketgroupid => $basketgroupid } );
+                         basketgroupid => $basketgroupid,
+                         borrowernumber => $loggedinuser } );


Why are you adding borrowernumber?
Comment 91 Andrew Isherwood 2021-01-05 11:41:24 UTC
Created attachment 114857 [details] [review]
Bug 23971: Add logging for basket related actions

This patch adds logging for the following Acq actions:

- Basket creation
- Basket editing
- Basket approval (via EDI)
- Basket closure

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 92 Andrew Isherwood 2021-01-05 11:41:29 UTC
Created attachment 114858 [details] [review]
Bug 23971: (follow-up) Remove data duplication

In response to Séverine observations in comment #10, this patch removes
the duplicate logging of the borrowernumber

https://bugs.koha-community.org/show_bug.cgi?id=23971
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 93 Andrew Isherwood 2021-01-05 11:41:33 UTC
Created attachment 114859 [details] [review]
Bug 23971: (follow-up) Add additional basket logging

This patch adds additional basket logging as discussed in comment #14

- Basket reopening
- Basket users (manager) modification
- Basket header modification

Signed-off-by: Holly <hc@interleaf.ie>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 94 Andrew Isherwood 2021-01-05 11:41:37 UTC
Created attachment 114860 [details] [review]
Bug 23971: (QA follow-up) Fix punctuation, doubled up return and syspref order

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 95 Andrew Isherwood 2021-01-05 11:41:41 UTC
Created attachment 114861 [details] [review]
Bug 23971: (follow-up) Add unit tests

This commit adds unit tests for the logging done in Acquisitions.pm

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 96 Andrew Isherwood 2021-01-05 11:41:45 UTC
Created attachment 114862 [details] [review]
Bug 23971: (follow-up) Respond to feedback

As requested by Jonathan in commit #28:

- We now no longer pass a flag to CloseBasket indicating if the closure
was due to an EDI basket approval, we log the closure and approval as
separate events.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 97 Andrew Isherwood 2021-01-05 11:41:50 UTC
Created attachment 114863 [details] [review]
Bug 23971: (follow-up) Fix tests

Following a discussion with Tomás on Slack, this commit implements the
following suggestions:

- Switch from ->find to ->search in tests. ->find is only ever going to return 0
or 1 rows, which doesn't help us if there is breakage which results in
more than 1 row being returned, the test would fail due to ->find not
expecting more than 1 row, but we should be testing for it. So switching
to ->search returns all matching rows.
- Moved tests into their own subtest, they had the potential to pollute
surrounding tests
- Remove all action logs before each test, this ensures we're only
testing the result of the current test

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 98 Andrew Isherwood 2021-01-05 11:41:54 UTC
Created attachment 114864 [details] [review]
Bug 23971: (follow-up) Make changes for 26582

This commit makes changes necessary to allow this bug to be dependent on
Bug 23562:

- Move basket closure logging from C4::Acquisision::CloseBasket to Koha::Acquisition::Basket::close
- Move basket closure unit test from t/db_dependent/Acquisition.t to
t/db_dependent/Koha/Acquisition/Basket.t

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 99 Andrew Isherwood 2021-01-05 11:41:58 UTC
Created attachment 114865 [details] [review]
Bug 23971: (QA follow-up) New DBrev syntax

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 100 Andrew Isherwood 2021-01-05 11:42:03 UTC
Created attachment 114866 [details] [review]
Bug 23971: (follow-up) Log entire objects

As requested by Tomás and Jonathan, we now log the entire basket object
when logging an action.
Comment 101 Andrew Isherwood 2021-01-05 11:42:08 UTC
Created attachment 114867 [details] [review]
Bug 23971: (follow-up) Remove stale param

As highlighted by Jonathan in comment #90, we were passing
borrowernumber to ModBasket. This was a hangover from when we explicitly
logged the borrower, which was later discovered to be unnecessary
duplication, and was removed in commit "Remove data duplication".

This commit removes this unnecessary parameter.
Comment 102 Andrew Isherwood 2021-01-05 11:42:23 UTC
Hi Jonathan

Thanks for that.

I've rebased as I was getting conflicts on a couple of commits.

> acqui/addorder.pl is logging a MODIFY_BASKET, is that expected?

I believe so. addorder processes the submission from the "New order" form when adding an order to a basket, so logging the basket modification makes sense here I think.

> Why are you adding borrowernumber?

That was a hangover from when ModBasket was explicitly logging the borrower. This was highlighted as unnecessary duplication, so I removed it from the logging in the patch titled "Bug 23971: (follow-up) Remove data duplication", but missed removing it from the ModBasket call. I've now created a new commit that removes it, thanks for highlighting it.
Comment 103 Jonathan Druart 2021-01-06 09:59:09 UTC
Please double check the rebased patches, I get an error on tools/viewlog.pl

I think we should squash all the patches. They are pretty simple to read once squashed but the history makes them very hard to follow.

(In reply to Andrew Isherwood from comment #102)
> > acqui/addorder.pl is logging a MODIFY_BASKET, is that expected?
> 
> I believe so. addorder processes the submission from the "New order" form
> when adding an order to a basket, so logging the basket modification makes
> sense here I think.

It logs a "Modify an acquisitions basket" with the info of the basket when I create or update an order.
It does not make sense to me. Can you confirm with the sponsor please?

Should not we have ADD_ORDER/MODIFY_LOG instead?
Comment 104 Andrew Isherwood 2021-01-07 11:34:26 UTC
Created attachment 114905 [details] [review]
Bug 23971: Add logging for basket related actions

This patch adds logging for the following Acq actions:

- Basket creation
- Basket editing
- Basket approval (via EDI)
- Basket closure

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 105 Andrew Isherwood 2021-01-07 11:34:30 UTC
Created attachment 114906 [details] [review]
Bug 23971: (follow-up) Remove data duplication

In response to Séverine observations in comment #10, this patch removes
the duplicate logging of the borrowernumber

https://bugs.koha-community.org/show_bug.cgi?id=23971
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 106 Andrew Isherwood 2021-01-07 11:34:34 UTC
Created attachment 114907 [details] [review]
Bug 23971: (follow-up) Add additional basket logging

This patch adds additional basket logging as discussed in comment #14

- Basket reopening
- Basket users (manager) modification
- Basket header modification

Signed-off-by: Holly <hc@interleaf.ie>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 107 Andrew Isherwood 2021-01-07 11:34:38 UTC
Created attachment 114908 [details] [review]
Bug 23971: (QA follow-up) Fix punctuation, doubled up return and syspref order

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 108 Andrew Isherwood 2021-01-07 11:34:43 UTC
Created attachment 114909 [details] [review]
Bug 23971: (follow-up) Add unit tests

This commit adds unit tests for the logging done in Acquisitions.pm

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 109 Andrew Isherwood 2021-01-07 11:34:47 UTC
Created attachment 114910 [details] [review]
Bug 23971: (follow-up) Respond to feedback

As requested by Jonathan in commit #28:

- We now no longer pass a flag to CloseBasket indicating if the closure
was due to an EDI basket approval, we log the closure and approval as
separate events.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 110 Andrew Isherwood 2021-01-07 11:34:51 UTC
Created attachment 114911 [details] [review]
Bug 23971: (follow-up) Fix tests

Following a discussion with Tomás on Slack, this commit implements the
following suggestions:

- Switch from ->find to ->search in tests. ->find is only ever going to return 0
or 1 rows, which doesn't help us if there is breakage which results in
more than 1 row being returned, the test would fail due to ->find not
expecting more than 1 row, but we should be testing for it. So switching
to ->search returns all matching rows.
- Moved tests into their own subtest, they had the potential to pollute
surrounding tests
- Remove all action logs before each test, this ensures we're only
testing the result of the current test

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 111 Andrew Isherwood 2021-01-07 11:34:56 UTC
Created attachment 114912 [details] [review]
Bug 23971: (follow-up) Make changes for 26582

This commit makes changes necessary to allow this bug to be dependent on
Bug 23562:

- Move basket closure logging from C4::Acquisision::CloseBasket to Koha::Acquisition::Basket::close
- Move basket closure unit test from t/db_dependent/Acquisition.t to
t/db_dependent/Koha/Acquisition/Basket.t

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 112 Andrew Isherwood 2021-01-07 11:35:00 UTC
Created attachment 114913 [details] [review]
Bug 23971: (QA follow-up) New DBrev syntax

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 113 Andrew Isherwood 2021-01-07 11:35:04 UTC
Created attachment 114914 [details] [review]
Bug 23971: (follow-up) Log entire objects

As requested by Tomás and Jonathan, we now log the entire basket object
when logging an action.
Comment 114 Andrew Isherwood 2021-01-07 11:35:08 UTC
Created attachment 114915 [details] [review]
Bug 23971: (follow-up) Remove stale param

As highlighted by Jonathan in comment #90, we were passing
borrowernumber to ModBasket. This was a hangover from when we explicitly
logged the borrower, which was later discovered to be unnecessary
duplication, and was removed in commit "Remove data duplication".

This commit removes this unnecessary parameter.
Comment 115 Andrew Isherwood 2021-01-07 11:35:13 UTC
Created attachment 114916 [details] [review]
Bug 23971: (follow-up) Fix template typo

As mentioned in comment #103, there was a typo in viewlog.tt
Comment 116 Andrew Isherwood 2021-01-07 11:48:30 UTC
(In reply to Jonathan Druart from comment #103)
> Please double check the rebased patches, I get an error on tools/viewlog.pl

Good spot, sorry about that. Now fixed.

> I think we should squash all the patches. They are pretty simple to read
> once squashed but the history makes them very hard to follow.

Fair enough. Though I have found myself referring to individual patches during this process. Maybe squashing just prior to merging would be the best approach?

> It logs a "Modify an acquisitions basket" with the info of the basket when I
> create or update an order.
> It does not make sense to me. Can you confirm with the sponsor please?

The original spec agreed with the customer requests "Who edited the basket and when", this was in the context of the basket contents. So I believe what we're doing is correct.
Comment 117 Jonathan Druart 2021-01-07 14:02:08 UTC
I've asked Tomas another look on the last patch and the question about addorder.pl

> Does it make sense to have a MODIFY_BASKET log when we add/update an order (without any info on the order)?
Comment 118 Andrew Isherwood 2021-01-28 16:44:57 UTC
Created attachment 115976 [details] [review]
Bug 23971: Add logging for basket related actions

This patch adds logging for the following Acq actions:

- Basket creation
- Basket editing
- Basket approval (via EDI)
- Basket closure

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 119 Andrew Isherwood 2021-01-28 16:45:03 UTC
Created attachment 115977 [details] [review]
Bug 23971: (follow-up) Remove data duplication

In response to Séverine observations in comment #10, this patch removes
the duplicate logging of the borrowernumber

https://bugs.koha-community.org/show_bug.cgi?id=23971
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 120 Andrew Isherwood 2021-01-28 16:45:08 UTC
Created attachment 115978 [details] [review]
Bug 23971: (follow-up) Add additional basket logging

This patch adds additional basket logging as discussed in comment #14

- Basket reopening
- Basket users (manager) modification
- Basket header modification

Signed-off-by: Holly <hc@interleaf.ie>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 121 Andrew Isherwood 2021-01-28 16:45:13 UTC
Created attachment 115979 [details] [review]
Bug 23971: (QA follow-up) Fix punctuation, doubled up return and syspref order

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 122 Andrew Isherwood 2021-01-28 16:45:17 UTC
Created attachment 115980 [details] [review]
Bug 23971: (follow-up) Add unit tests

This commit adds unit tests for the logging done in Acquisitions.pm

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 123 Andrew Isherwood 2021-01-28 16:45:21 UTC
Created attachment 115981 [details] [review]
Bug 23971: (follow-up) Respond to feedback

As requested by Jonathan in commit #28:

- We now no longer pass a flag to CloseBasket indicating if the closure
was due to an EDI basket approval, we log the closure and approval as
separate events.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 124 Andrew Isherwood 2021-01-28 16:45:26 UTC
Created attachment 115982 [details] [review]
Bug 23971: (follow-up) Fix tests

Following a discussion with Tomás on Slack, this commit implements the
following suggestions:

- Switch from ->find to ->search in tests. ->find is only ever going to return 0
or 1 rows, which doesn't help us if there is breakage which results in
more than 1 row being returned, the test would fail due to ->find not
expecting more than 1 row, but we should be testing for it. So switching
to ->search returns all matching rows.
- Moved tests into their own subtest, they had the potential to pollute
surrounding tests
- Remove all action logs before each test, this ensures we're only
testing the result of the current test

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 125 Andrew Isherwood 2021-01-28 16:45:30 UTC
Created attachment 115983 [details] [review]
Bug 23971: (follow-up) Make changes for 26582

This commit makes changes necessary to allow this bug to be dependent on
Bug 23562:

- Move basket closure logging from C4::Acquisision::CloseBasket to Koha::Acquisition::Basket::close
- Move basket closure unit test from t/db_dependent/Acquisition.t to
t/db_dependent/Koha/Acquisition/Basket.t

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 126 Andrew Isherwood 2021-01-28 16:45:35 UTC
Created attachment 115984 [details] [review]
Bug 23971: (QA follow-up) New DBrev syntax

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 127 Andrew Isherwood 2021-01-28 16:45:39 UTC
Created attachment 115985 [details] [review]
Bug 23971: (follow-up) Log entire objects

As requested by Tomás and Jonathan, we now log the entire basket object
when logging an action.
Comment 128 Andrew Isherwood 2021-01-28 16:45:44 UTC
Created attachment 115986 [details] [review]
Bug 23971: (follow-up) Remove stale param

As highlighted by Jonathan in comment #90, we were passing
borrowernumber to ModBasket. This was a hangover from when we explicitly
logged the borrower, which was later discovered to be unnecessary
duplication, and was removed in commit "Remove data duplication".

This commit removes this unnecessary parameter.
Comment 129 Andrew Isherwood 2021-01-28 16:47:46 UTC
Rebased so I can rebase the dependent bug
Comment 130 Kyle M Hall 2021-04-09 15:51:30 UTC
Created attachment 119401 [details] [review]
Bug 23971: Add logging for basket related actions

This patch adds logging for the following Acq actions:

- Basket creation
- Basket editing
- Basket approval (via EDI)
- Basket closure

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 131 Kyle M Hall 2021-04-09 15:51:47 UTC
Created attachment 119402 [details] [review]
Bug 23971: Add logging for basket related actions

This patch adds logging for the following Acq actions:

- Basket creation
- Basket editing
- Basket approval (via EDI)
- Basket closure

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 132 Kyle M Hall 2021-04-09 15:52:44 UTC
Created attachment 119403 [details] [review]
Bug 23971: (follow-up) Remove data duplication

In response to Séverine observations in comment #10, this patch removes
the duplicate logging of the borrowernumber

https://bugs.koha-community.org/show_bug.cgi?id=23971
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 133 Kyle M Hall 2021-04-09 15:52:49 UTC
Created attachment 119404 [details] [review]
Bug 23971: (follow-up) Add additional basket logging

This patch adds additional basket logging as discussed in comment #14

- Basket reopening
- Basket users (manager) modification
- Basket header modification

Signed-off-by: Holly <hc@interleaf.ie>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 134 Kyle M Hall 2021-04-09 15:52:55 UTC
Created attachment 119405 [details] [review]
Bug 23971: (QA follow-up) Fix punctuation, doubled up return and syspref order

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 135 Kyle M Hall 2021-04-09 15:53:00 UTC
Created attachment 119406 [details] [review]
Bug 23971: (follow-up) Add unit tests

This commit adds unit tests for the logging done in Acquisitions.pm

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 136 Kyle M Hall 2021-04-09 15:53:05 UTC
Created attachment 119407 [details] [review]
Bug 23971: (follow-up) Respond to feedback

As requested by Jonathan in commit #28:

- We now no longer pass a flag to CloseBasket indicating if the closure
was due to an EDI basket approval, we log the closure and approval as
separate events.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 137 Kyle M Hall 2021-04-09 15:53:11 UTC
Created attachment 119408 [details] [review]
Bug 23971: (follow-up) Fix tests

Following a discussion with Tomás on Slack, this commit implements the
following suggestions:

- Switch from ->find to ->search in tests. ->find is only ever going to return 0
or 1 rows, which doesn't help us if there is breakage which results in
more than 1 row being returned, the test would fail due to ->find not
expecting more than 1 row, but we should be testing for it. So switching
to ->search returns all matching rows.
- Moved tests into their own subtest, they had the potential to pollute
surrounding tests
- Remove all action logs before each test, this ensures we're only
testing the result of the current test

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 138 Kyle M Hall 2021-04-09 15:53:16 UTC
Created attachment 119409 [details] [review]
Bug 23971: (follow-up) Make changes for 26582

This commit makes changes necessary to allow this bug to be dependent on
Bug 23562:

- Move basket closure logging from C4::Acquisision::CloseBasket to Koha::Acquisition::Basket::close
- Move basket closure unit test from t/db_dependent/Acquisition.t to
t/db_dependent/Koha/Acquisition/Basket.t

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 139 Kyle M Hall 2021-04-09 15:53:21 UTC
Created attachment 119410 [details] [review]
Bug 23971: (QA follow-up) New DBrev syntax

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 140 Kyle M Hall 2021-04-09 15:53:26 UTC
Created attachment 119411 [details] [review]
Bug 23971: (follow-up) Log entire objects

As requested by Tomás and Jonathan, we now log the entire basket object
when logging an action.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 141 Kyle M Hall 2021-04-09 15:53:31 UTC
Created attachment 119412 [details] [review]
Bug 23971: (follow-up) Remove stale param

As highlighted by Jonathan in comment #90, we were passing
borrowernumber to ModBasket. This was a hangover from when we explicitly
logged the borrower, which was later discovered to be unnecessary
duplication, and was removed in commit "Remove data duplication".

This commit removes this unnecessary parameter.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 142 Jonathan Druart 2021-04-16 11:55:06 UTC
Release notes are missing.
Comment 143 Jonathan Druart 2021-04-21 12:27:03 UTC
Created attachment 119944 [details] [review]
Bug 23971: Rename pref with AcquisitionLog
Comment 144 Jonathan Druart 2021-04-21 12:31:04 UTC
Please fill in the release notes fields with the list of what this patch set is going to log.
Comment 145 Jonathan Druart 2021-04-21 12:39:35 UTC
Created attachment 119946 [details] [review]
Bug 23971: Do not always flag AcquisitionLog as 'not enabled'
Comment 146 Jonathan Druart 2021-04-23 09:55:01 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 147 Jonathan Druart 2021-04-23 10:46:13 UTC
Created attachment 120048 [details] [review]
Bug 23971: Fix UsageStats.t

Hum, what's the point of these tests?
Comment 148 Jonathan Druart 2021-04-23 10:47:09 UTC
Follow-up pushed to master.
Comment 149 Fridolin Somers 2021-04-30 07:46:01 UTC
Enhancement not pushed to 20.11.x