Bug 23971 - Add logging for basket related actions
Summary: Add logging for basket related actions
Status: Passed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement with 10 votes (vote)
Assignee: Andrew Isherwood
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
Depends on: 26582
Blocks: 22794 24190 25916
  Show dependency treegraph
 
Reported: 2019-11-05 10:57 UTC by Andrew Isherwood
Modified: 2020-10-23 18:39 UTC (History)
8 users (show)

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


Attachments
Bug 23971: Add Acq action logging (10.36 KB, patch)
2019-11-12 08:48 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23971: Add Acq action logging (11.35 KB, patch)
2019-11-12 09:10 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23971: Add Acq action logging (18.49 KB, patch)
2019-11-29 15:18 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23971 - Add logging for basket related actions (20.82 KB, patch)
2019-11-29 15:41 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23961: (follow-up) Remove data duplication (6.07 KB, patch)
2019-12-06 10:31 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23971: (follow-up) Add additional basket logging (3.71 KB, patch)
2019-12-06 11:36 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23971 - Add logging for basket related actions (20.82 KB, patch)
2020-03-06 10:28 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23971: (follow-up) Remove data duplication (6.07 KB, patch)
2020-03-06 10:28 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23971: (follow-up) Add additional basket logging (3.71 KB, patch)
2020-03-06 10:28 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23971: Add logging for basket related actions (21.01 KB, patch)
2020-04-29 10:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23971: (follow-up) Remove data duplication (6.07 KB, patch)
2020-04-29 10:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23971: (follow-up) Add additional basket logging (3.94 KB, patch)
2020-04-29 10:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23971: (follow-up) Add additional basket logging (3.99 KB, patch)
2020-04-29 14:06 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 23971: (QA follow-up) Fix punctuation, doubled up return and syspref order (11.10 KB, patch)
2020-08-23 21:25 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 23971: Add logging for basket related actions (20.95 KB, patch)
2020-10-06 12:43 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23971: (follow-up) Remove data duplication (6.07 KB, patch)
2020-10-06 12:43 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23971: (follow-up) Add additional basket logging (3.92 KB, patch)
2020-10-06 12:43 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23971: (QA follow-up) Fix punctuation, doubled up return and syspref order (11.08 KB, patch)
2020-10-06 12:44 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23971: (follow-up) Add unit tests (3.11 KB, patch)
2020-10-06 12:44 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23971: Add logging for basket related actions (20.99 KB, patch)
2020-10-19 09:57 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23971: (follow-up) Remove data duplication (6.07 KB, patch)
2020-10-19 09:57 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23971: (follow-up) Add additional basket logging (3.92 KB, patch)
2020-10-19 09:57 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23971: (QA follow-up) Fix punctuation, doubled up return and syspref order (11.12 KB, patch)
2020-10-19 09:57 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23971: (follow-up) Add unit tests (3.11 KB, patch)
2020-10-19 09:58 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23971: (follow-up) Respond to feedback (2.36 KB, patch)
2020-10-19 09:58 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23971: Add logging for basket related actions (20.99 KB, patch)
2020-10-23 14:30 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23971: (follow-up) Remove data duplication (6.07 KB, patch)
2020-10-23 14:30 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23971: (follow-up) Add additional basket logging (3.92 KB, patch)
2020-10-23 14:30 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23971: (QA follow-up) Fix punctuation, doubled up return and syspref order (11.12 KB, patch)
2020-10-23 14:30 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23971: (follow-up) Add unit tests (3.11 KB, patch)
2020-10-23 14:30 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23971: (follow-up) Respond to feedback (2.36 KB, patch)
2020-10-23 14:30 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23971: (follow-up) Fix tests (5.73 KB, patch)
2020-10-23 14:30 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23971: Add logging for basket related actions (20.99 KB, patch)
2020-10-23 14:47 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23971: (follow-up) Remove data duplication (6.07 KB, patch)
2020-10-23 14:47 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23971: (follow-up) Add additional basket logging (3.92 KB, patch)
2020-10-23 14:47 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23971: (QA follow-up) Fix punctuation, doubled up return and syspref order (11.12 KB, patch)
2020-10-23 14:47 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23971: (follow-up) Add unit tests (3.11 KB, patch)
2020-10-23 14:47 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23971: (follow-up) Respond to feedback (2.36 KB, patch)
2020-10-23 14:47 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23971: (follow-up) Fix tests (5.73 KB, patch)
2020-10-23 14:47 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23971: (follow-up) Make changes for 26582 (3.45 KB, patch)
2020-10-23 14:47 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23971: Add logging for basket related actions (21.07 KB, patch)
2020-10-23 18:34 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23971: (follow-up) Remove data duplication (6.14 KB, patch)
2020-10-23 18:34 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23971: (follow-up) Add additional basket logging (3.98 KB, patch)
2020-10-23 18:35 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23971: (QA follow-up) Fix punctuation, doubled up return and syspref order (11.18 KB, patch)
2020-10-23 18:35 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23971: (follow-up) Add unit tests (3.17 KB, patch)
2020-10-23 18:35 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23971: (follow-up) Respond to feedback (2.42 KB, patch)
2020-10-23 18:35 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23971: (follow-up) Fix tests (5.78 KB, patch)
2020-10-23 18:36 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23971: (follow-up) Make changes for 26582 (3.51 KB, patch)
2020-10-23 18:36 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23971: (QA follow-up) New DBrev syntax (1.09 KB, patch)
2020-10-23 18:37 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23971: (QA follow-up) New DBrev syntax (1.14 KB, patch)
2020-10-23 18:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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>