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

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

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

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.

I think Severine is right - logging the reopening of a basket would be really helpful. Andrew, could this be added to this patch set?

I think the missing pref could be considered a "Failed QA" as we should stick with existing patterns and it makes sense here.

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

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

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

(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 !).

> 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

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

(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.

(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.

(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.

(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

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

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.

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

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

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

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.

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

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

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

Seems there was a problem with applying this bug, which the latest patches attached by Martin should fix. Hopefully this will now pass QA.

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>

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)

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.

Created attachment 108962 [details] [review]
Bug 23971: (QA follow-up) Fix punctuation, doubled up return and syspref order

(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?

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

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

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>

Created attachment 111303 [details] [review]
Bug 23971: (QA follow-up) Fix punctuation, doubled up return and syspref order

Created attachment 111304 [details] [review]
Bug 23971: (follow-up) Add unit tests

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

(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

(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?

(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"

(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!

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

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

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

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>

Created attachment 111958 [details] [review]
Bug 23971: (QA follow-up) Fix punctuation, doubled up return and syspref order

Created attachment 111959 [details] [review]
Bug 23971: (follow-up) Add unit tests

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

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.

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).

(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 :-)

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

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

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>

Created attachment 112280 [details] [review]
Bug 23971: (QA follow-up) Fix punctuation, doubled up return and syspref order

Created attachment 112281 [details] [review]
Bug 23971: (follow-up) Add unit tests

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

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.

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

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

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

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>

Created attachment 112287 [details] [review]
Bug 23971: (QA follow-up) Fix punctuation, doubled up return and syspref order

Created attachment 112288 [details] [review]
Bug 23971: (follow-up) Add unit tests

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

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.

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

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

Hi Tomás

I've now made the changes we discussed on Slack. Should be ready for you again now.

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>

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>

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>

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>

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>

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>

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>

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>

Created attachment 112306 [details] [review]
Bug 23971: (QA follow-up) New DBrev syntax

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

Created attachment 112307 [details] [review]
Bug 23971: (QA follow-up) New DBrev syntax

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

I still feel the logs should include the objects, following what's been done so far with the rest of the logs.

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.

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>

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>

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>

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>

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>

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>

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>

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>

Created attachment 113392 [details] [review]
Bug 23971: (QA follow-up) New DBrev syntax

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

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.

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

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

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?

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>

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>

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>

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>

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>

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>

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>

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>

Created attachment 114865 [details] [review]
Bug 23971: (QA follow-up) New DBrev syntax

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

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.

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.

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.

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?

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>

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>

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>

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>

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>

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>

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>

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>

Created attachment 114913 [details] [review]
Bug 23971: (QA follow-up) New DBrev syntax

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

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.

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.

Created attachment 114916 [details] [review]
Bug 23971: (follow-up) Fix template typo

As mentioned in comment #103, there was a typo in viewlog.tt

(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.

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)?

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>

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>

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>

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>

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>

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>

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>

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>

Created attachment 115984 [details] [review]
Bug 23971: (QA follow-up) New DBrev syntax

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

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.

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.

Rebased so I can rebase the dependent bug

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

Release notes are missing.

Created attachment 119944 [details] [review]
Bug 23971: Rename pref with AcquisitionLog

Please fill in the release notes fields with the list of what this patch set is going to log.

Created attachment 119946 [details] [review]
Bug 23971: Do not always flag AcquisitionLog as 'not enabled'

Pushed to master for 21.05, thanks to everybody involved!

Created attachment 120048 [details] [review]
Bug 23971: Fix UsageStats.t

Hum, what's the point of these tests?

Follow-up pushed to master.

Enhancement not pushed to 20.11.x