Bug 24161 - Add ability to track the claim dates of later orders
Summary: Add ability to track the claim dates of later orders
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact:
URL:
Keywords:
Depends on: 12502 24388
Blocks: 32187 24163 30539
  Show dependency treegraph
 
Reported: 2019-12-03 19:41 UTC by Jonathan Druart
Modified: 2022-11-15 08:57 UTC (History)
5 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This enhancement adds the ability to track multiple claim dates for a late order and exposes this audit record via the late orders and basket pages.
Version(s) released in:
20.05.00


Attachments
Bug 24161: DB changes (4.00 KB, patch)
2020-01-09 16:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24161: DBIC changes (4.76 KB, patch)
2020-01-09 16:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24161: Add the Koha::Acquisition::Order::Claim[s] classes (3.15 KB, patch)
2020-01-09 16:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24161: Keep tracks of late orders claims (30.03 KB, patch)
2020-01-09 16:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24161: Remove GetLateOrders (6.37 KB, patch)
2020-01-09 16:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24161: Add tests (10.92 KB, patch)
2020-01-09 16:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24161: Keep tracks of late orders claims (30.04 KB, patch)
2020-01-16 20:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24161: Remove GetLateOrders (6.37 KB, patch)
2020-01-16 20:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24161: Add tests (10.92 KB, patch)
2020-01-16 20:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24161: Add tests (10.76 KB, patch)
2020-02-03 11:44 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24161: DB changes (4.04 KB, patch)
2020-02-06 11:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24161: DBIC changes (4.74 KB, patch)
2020-02-06 11:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24161: Add the Koha::Acquisition::Order::Claim[s] classes (3.15 KB, patch)
2020-02-06 11:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24161: Keep tracks of late orders claims (30.04 KB, patch)
2020-02-06 11:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24161: Remove GetLateOrders (6.37 KB, patch)
2020-02-06 11:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24161: Add tests (10.62 KB, patch)
2020-02-06 11:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24161: DB changes (4.03 KB, patch)
2020-03-31 09:50 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24161: DBIC changes (4.73 KB, patch)
2020-03-31 09:50 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24161: Add the Koha::Acquisition::Order::Claim[s] classes (3.14 KB, patch)
2020-03-31 09:50 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24161: Keep tracks of late orders claims (29.91 KB, patch)
2020-03-31 09:50 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24161: Remove GetLateOrders (6.37 KB, patch)
2020-03-31 09:50 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24161: Add tests (10.61 KB, patch)
2020-03-31 09:50 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24161: DBIC manual changes (835 bytes, patch)
2020-04-01 12:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24161: DBIC manual changes (1.30 KB, patch)
2020-04-01 12:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24161: Keep tracks of late orders claims (29.90 KB, patch)
2020-04-23 11:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24161: Remove GetLateOrders (6.37 KB, patch)
2020-04-23 11:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24161: Add tests (10.61 KB, patch)
2020-04-23 11:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24161: DBIC manual changes (1.30 KB, patch)
2020-04-23 11:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24161: Display claimed dates vertically (989 bytes, patch)
2020-04-23 11:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24161: DB changes (4.12 KB, patch)
2020-04-23 12:21 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 24161: DBIC changes (4.82 KB, patch)
2020-04-23 12:21 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 24161: Add the Koha::Acquisition::Order::Claim[s] classes (3.23 KB, patch)
2020-04-23 12:21 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 24161: Keep tracks of late orders claims (30.06 KB, patch)
2020-04-23 12:22 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 24161: Remove GetLateOrders (6.45 KB, patch)
2020-04-23 12:22 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 24161: Add tests (10.70 KB, patch)
2020-04-23 12:22 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 24161: DBIC manual changes (1.38 KB, patch)
2020-04-23 12:22 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 24161: Display claimed dates vertically (1.04 KB, patch)
2020-04-23 12:22 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 24161: Fix license statements (3.55 KB, patch)
2020-05-02 13:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24161: (follow-up) DB changes (1.22 KB, patch)
2020-05-02 13:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24161: DB changes (4.18 KB, patch)
2020-05-02 17:25 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 24161: DBIC changes (4.88 KB, patch)
2020-05-02 17:25 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 24161: Add the Koha::Acquisition::Order::Claim[s] classes (3.29 KB, patch)
2020-05-02 17:25 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 24161: Keep tracks of late orders claims (30.12 KB, patch)
2020-05-02 17:25 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 24161: Remove GetLateOrders (6.51 KB, patch)
2020-05-02 17:25 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 24161: Add tests (10.76 KB, patch)
2020-05-02 17:25 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 24161: DBIC manual changes (1.44 KB, patch)
2020-05-02 17:25 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 24161: Display claimed dates vertically (1.10 KB, patch)
2020-05-02 17:25 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 24161: Fix license statements (3.61 KB, patch)
2020-05-02 17:26 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 24161: (follow-up) DB changes (1.28 KB, patch)
2020-05-02 17:26 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 24161: (follow-up) Fix typo in POD (1.05 KB, patch)
2020-05-04 11:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24161: (follow-up) DBIC changes (1015 bytes, patch)
2020-05-04 11:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24161: (follow-up) Typo in POD (847 bytes, patch)
2020-05-04 12:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24161: Fix failing test (when running slow) (1.31 KB, patch)
2020-05-13 07:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24161: (follow-up) Fix failing test (when running slow) (1.18 KB, patch)
2020-05-13 14:11 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2019-12-03 19:41:58 UTC
* Allow multiple claim dates to be associated with an individual order line
 * Show multiple claim dates against an order line in the late orders page
 * Show multiple claim dates against an order line in the basket page
Comment 1 Jonathan Druart 2020-01-09 16:32:19 UTC
Created attachment 97114 [details] [review]
Bug 24161: DB changes

Sponsored-by: Cork Institute of Technology
Comment 2 Jonathan Druart 2020-01-09 16:32:23 UTC
Created attachment 97115 [details] [review]
Bug 24161: DBIC changes

Sponsored-by: Cork Institute of Technology
Comment 3 Jonathan Druart 2020-01-09 16:32:27 UTC
Created attachment 97116 [details] [review]
Bug 24161: Add the Koha::Acquisition::Order::Claim[s] classes

Sponsored-by: Cork Institute of Technology
Comment 4 Jonathan Druart 2020-01-09 16:32:30 UTC
Created attachment 97117 [details] [review]
Bug 24161: Keep tracks of late orders claims

So far we only record the number of claims and the date of the last
claim, in the aqorders table.
To keep track of the different claim dates, this patchset is going to
make the following DB changes:
  * Create a new table 'aqorders_claims' (id, ordernumber, claimed_on)
  * Remove the two columns from the aqorders table: claims_count and
  claimed_date

This will allow to display the different claim dates where needed: on
the late orders page, and the basket page.

To avoid additional fetches of Koha::Acquisition::Orders, GetLateOrders
has been moved to Koha::Acquisition::Orders->filter_by_late
That way we are going to add consistency, robustness, and cover the
feature with new tests.

Test plan:
0/ Create a bunch of new orders. Make sure they are from different
vendor (with different delivery time).
1/ Go to the late orders page and claim some orders
2/ Reclaim some of those orders
3/ Confirm that you can see the different claim dates for a given orders
(the history of the late orders claims is kept and displayed)

4/ Bonus point: Regression tests:

a. Modify the closedate of the basket in the database. That
will allow you to make sure the patch set did not introduce regressions.
It would be good to test the different filters on the late orders page:
 * delay
 * Estimated delivery date from/to
 * Vendor

b. Confirm that the subtotal and the total values from the late orders
page is correct.

c. Test the update database entry: do not apply these patches, claims
some orders against master. Apply the patch, execute the update DB entry
then confirm that the number of claims is correct (note that the dates
will not as it is not possible to guess them).

QA note: the branchcode parameter has been removed from filter_by_late.
At first glance it seems that it was not used.

Sponsored-by: Cork Institute of Technology
Comment 5 Jonathan Druart 2020-01-09 16:32:34 UTC
Created attachment 97118 [details] [review]
Bug 24161: Remove GetLateOrders

Sponsored-by: Cork Institute of Technology
Comment 6 Jonathan Druart 2020-01-09 16:32:38 UTC
Created attachment 97119 [details] [review]
Bug 24161: Add tests

Sponsored-by: Cork Institute of Technology
Comment 7 Katrin Fischer 2020-01-16 07:22:52 UTC
Hi Jonathan, working on this tree - can you please do a quick rebase/check?
Comment 8 Jonathan Druart 2020-01-16 20:59:02 UTC
Created attachment 97470 [details] [review]
Bug 24161: Keep tracks of late orders claims

So far we only record the number of claims and the date of the last
claim, in the aqorders table.
To keep track of the different claim dates, this patchset is going to
make the following DB changes:
  * Create a new table 'aqorders_claims' (id, ordernumber, claimed_on)
  * Remove the two columns from the aqorders table: claims_count and
  claimed_date

This will allow to display the different claim dates where needed: on
the late orders page, and the basket page.

To avoid additional fetches of Koha::Acquisition::Orders, GetLateOrders
has been moved to Koha::Acquisition::Orders->filter_by_late
That way we are going to add consistency, robustness, and cover the
feature with new tests.

Test plan:
0/ Create a bunch of new orders. Make sure they are from different
vendor (with different delivery time).
1/ Go to the late orders page and claim some orders
2/ Reclaim some of those orders
3/ Confirm that you can see the different claim dates for a given orders
(the history of the late orders claims is kept and displayed)

4/ Bonus point: Regression tests:

a. Modify the closedate of the basket in the database. That
will allow you to make sure the patch set did not introduce regressions.
It would be good to test the different filters on the late orders page:
 * delay
 * Estimated delivery date from/to
 * Vendor

b. Confirm that the subtotal and the total values from the late orders
page is correct.

c. Test the update database entry: do not apply these patches, claims
some orders against master. Apply the patch, execute the update DB entry
then confirm that the number of claims is correct (note that the dates
will not as it is not possible to guess them).

QA note: the branchcode parameter has been removed from filter_by_late.
At first glance it seems that it was not used.

Sponsored-by: Cork Institute of Technology
Comment 9 Jonathan Druart 2020-01-16 20:59:06 UTC
Created attachment 97471 [details] [review]
Bug 24161: Remove GetLateOrders

Sponsored-by: Cork Institute of Technology
Comment 10 Jonathan Druart 2020-01-16 20:59:09 UTC
Created attachment 97472 [details] [review]
Bug 24161: Add tests

Sponsored-by: Cork Institute of Technology
Comment 11 Katrin Fischer 2020-01-22 07:04:12 UTC
I had already started here yesterday, so trying to clean up my notes:

+=head3 authorizer

Wonder if we should make this 'creator', as we have discussed for the API endpoint.

Missing description in the updatedatabase:
+    print "Upgrade to $DBversion done (Bug XXXXX - description)\n";


Both not blocking further testing!
Comment 12 Jonathan Druart 2020-02-03 11:44:26 UTC
Created attachment 98280 [details] [review]
Bug 24161: Add tests

Sponsored-by: Cork Institute of Technology
Comment 13 Jonathan Druart 2020-02-06 11:28:27 UTC
Created attachment 98522 [details] [review]
Bug 24161: DB changes

Sponsored-by: Cork Institute of Technology
Comment 14 Jonathan Druart 2020-02-06 11:28:31 UTC
Created attachment 98523 [details] [review]
Bug 24161: DBIC changes

Sponsored-by: Cork Institute of Technology
Comment 15 Jonathan Druart 2020-02-06 11:28:35 UTC
Created attachment 98524 [details] [review]
Bug 24161: Add the Koha::Acquisition::Order::Claim[s] classes

Sponsored-by: Cork Institute of Technology
Comment 16 Jonathan Druart 2020-02-06 11:28:39 UTC
Created attachment 98525 [details] [review]
Bug 24161: Keep tracks of late orders claims

So far we only record the number of claims and the date of the last
claim, in the aqorders table.
To keep track of the different claim dates, this patchset is going to
make the following DB changes:
  * Create a new table 'aqorders_claims' (id, ordernumber, claimed_on)
  * Remove the two columns from the aqorders table: claims_count and
  claimed_date

This will allow to display the different claim dates where needed: on
the late orders page, and the basket page.

To avoid additional fetches of Koha::Acquisition::Orders, GetLateOrders
has been moved to Koha::Acquisition::Orders->filter_by_late
That way we are going to add consistency, robustness, and cover the
feature with new tests.

Test plan:
0/ Create a bunch of new orders. Make sure they are from different
vendor (with different delivery time).
1/ Go to the late orders page and claim some orders
2/ Reclaim some of those orders
3/ Confirm that you can see the different claim dates for a given orders
(the history of the late orders claims is kept and displayed)

4/ Bonus point: Regression tests:

a. Modify the closedate of the basket in the database. That
will allow you to make sure the patch set did not introduce regressions.
It would be good to test the different filters on the late orders page:
 * delay
 * Estimated delivery date from/to
 * Vendor

b. Confirm that the subtotal and the total values from the late orders
page is correct.

c. Test the update database entry: do not apply these patches, claims
some orders against master. Apply the patch, execute the update DB entry
then confirm that the number of claims is correct (note that the dates
will not as it is not possible to guess them).

QA note: the branchcode parameter has been removed from filter_by_late.
At first glance it seems that it was not used.

Sponsored-by: Cork Institute of Technology
Comment 17 Jonathan Druart 2020-02-06 11:28:43 UTC
Created attachment 98526 [details] [review]
Bug 24161: Remove GetLateOrders

Sponsored-by: Cork Institute of Technology
Comment 18 Jonathan Druart 2020-02-06 11:28:47 UTC
Created attachment 98527 [details] [review]
Bug 24161: Add tests

Sponsored-by: Cork Institute of Technology
Comment 19 Angela 2020-03-18 18:38:24 UTC
The development which adds multiple claim dates is available for testing on
the following sandbox:

http://staff-bug24161.sandboxes.ptfs-europe.co.uk. To ensure it is working correctly please follow these steps:

1. Create multiple orders against two or more vendors and close the baskets
2. On the late orders page, claim some of the orders
3. The date of the claim will be displayed in the "Claimed date" column
4. Make more claims against the same items
5. You should now see multiple dates listed in the "Claimed date" column

Nothing happens when claim button on late orders page clicked.  Error message "connect to localhost failed (Connection refused) connect to localhost failed connect to localhost failed (Connection refused) no (more) retries!"
Comment 20 Jonathan Druart 2020-03-19 13:31:07 UTC
(In reply to Angela from comment #19)
Hello Angela,
You get this error because there is no MTA (mail system) configured on the sandbox.
Comment 21 Martin Renvoize 2020-03-31 09:50:21 UTC
Created attachment 102111 [details] [review]
Bug 24161: DB changes

Sponsored-by: Cork Institute of Technology
Comment 22 Martin Renvoize 2020-03-31 09:50:25 UTC
Created attachment 102112 [details] [review]
Bug 24161: DBIC changes

Sponsored-by: Cork Institute of Technology
Comment 23 Martin Renvoize 2020-03-31 09:50:28 UTC
Created attachment 102113 [details] [review]
Bug 24161: Add the Koha::Acquisition::Order::Claim[s] classes

Sponsored-by: Cork Institute of Technology
Comment 24 Martin Renvoize 2020-03-31 09:50:31 UTC
Created attachment 102114 [details] [review]
Bug 24161: Keep tracks of late orders claims

So far we only record the number of claims and the date of the last
claim, in the aqorders table.
To keep track of the different claim dates, this patchset is going to
make the following DB changes:
  * Create a new table 'aqorders_claims' (id, ordernumber, claimed_on)
  * Remove the two columns from the aqorders table: claims_count and
  claimed_date

This will allow to display the different claim dates where needed: on
the late orders page, and the basket page.

To avoid additional fetches of Koha::Acquisition::Orders, GetLateOrders
has been moved to Koha::Acquisition::Orders->filter_by_late
That way we are going to add consistency, robustness, and cover the
feature with new tests.

Test plan:
0/ Create a bunch of new orders. Make sure they are from different
vendor (with different delivery time).
1/ Go to the late orders page and claim some orders
2/ Reclaim some of those orders
3/ Confirm that you can see the different claim dates for a given orders
(the history of the late orders claims is kept and displayed)

4/ Bonus point: Regression tests:

a. Modify the closedate of the basket in the database. That
will allow you to make sure the patch set did not introduce regressions.
It would be good to test the different filters on the late orders page:
 * delay
 * Estimated delivery date from/to
 * Vendor

b. Confirm that the subtotal and the total values from the late orders
page is correct.

c. Test the update database entry: do not apply these patches, claims
some orders against master. Apply the patch, execute the update DB entry
then confirm that the number of claims is correct (note that the dates
will not as it is not possible to guess them).

QA note: the branchcode parameter has been removed from filter_by_late.
At first glance it seems that it was not used.

Sponsored-by: Cork Institute of Technology
Comment 25 Martin Renvoize 2020-03-31 09:50:34 UTC
Created attachment 102115 [details] [review]
Bug 24161: Remove GetLateOrders

Sponsored-by: Cork Institute of Technology
Comment 26 Martin Renvoize 2020-03-31 09:50:38 UTC
Created attachment 102116 [details] [review]
Bug 24161: Add tests

Sponsored-by: Cork Institute of Technology
Comment 27 Martin Renvoize 2020-03-31 09:50:51 UTC
Rebased
Comment 28 Martin Renvoize 2020-04-01 09:30:13 UTC
Updated PTFS-E sandboxes to support email.. emails will not be delivered externally but will go into a log file which can be accessed via the 'Logs' menu in the sandbox UI
Comment 29 Jonathan Druart 2020-04-01 12:31:34 UTC
Created attachment 102220 [details] [review]
Bug 24161: DBIC manual changes

No idea what's happening with the current_timestamp, but previous change
was wrong.
Comment 30 Jonathan Druart 2020-04-01 12:48:27 UTC
Created attachment 102222 [details] [review]
Bug 24161: DBIC manual changes

No idea what's happening with the current_timestamp, but previous change
was wrong.
Comment 31 Martin Renvoize 2020-04-23 10:27:04 UTC
Feedback from sponsor testing.

(1) The 'claimed date' field lists the claimed dates side by side, could they be stacked vertically instead with most recent at the top?
(2) The 'estimated delivery date from - to' filter appears to be broken
(3) The 'vendor' filter does not list all vendors?
Comment 32 Jonathan Druart 2020-04-23 11:42:25 UTC
Created attachment 103567 [details] [review]
Bug 24161: Keep tracks of late orders claims

So far we only record the number of claims and the date of the last
claim, in the aqorders table.
To keep track of the different claim dates, this patchset is going to
make the following DB changes:
  * Create a new table 'aqorders_claims' (id, ordernumber, claimed_on)
  * Remove the two columns from the aqorders table: claims_count and
  claimed_date

This will allow to display the different claim dates where needed: on
the late orders page, and the basket page.

To avoid additional fetches of Koha::Acquisition::Orders, GetLateOrders
has been moved to Koha::Acquisition::Orders->filter_by_late
That way we are going to add consistency, robustness, and cover the
feature with new tests.

Test plan:
0/ Create a bunch of new orders. Make sure they are from different
vendor (with different delivery time).
1/ Go to the late orders page and claim some orders
2/ Reclaim some of those orders
3/ Confirm that you can see the different claim dates for a given orders
(the history of the late orders claims is kept and displayed)

4/ Bonus point: Regression tests:

a. Modify the closedate of the basket in the database. That
will allow you to make sure the patch set did not introduce regressions.
It would be good to test the different filters on the late orders page:
 * delay
 * Estimated delivery date from/to
 * Vendor

b. Confirm that the subtotal and the total values from the late orders
page is correct.

c. Test the update database entry: do not apply these patches, claims
some orders against master. Apply the patch, execute the update DB entry
then confirm that the number of claims is correct (note that the dates
will not as it is not possible to guess them).

QA note: the branchcode parameter has been removed from filter_by_late.
At first glance it seems that it was not used.

Sponsored-by: Cork Institute of Technology
Comment 33 Jonathan Druart 2020-04-23 11:42:30 UTC
Created attachment 103568 [details] [review]
Bug 24161: Remove GetLateOrders

Sponsored-by: Cork Institute of Technology
Comment 34 Jonathan Druart 2020-04-23 11:42:34 UTC
Created attachment 103569 [details] [review]
Bug 24161: Add tests

Sponsored-by: Cork Institute of Technology
Comment 35 Jonathan Druart 2020-04-23 11:42:38 UTC
Created attachment 103570 [details] [review]
Bug 24161: DBIC manual changes

No idea what's happening with the current_timestamp, but previous change
was wrong.
Comment 36 Jonathan Druart 2020-04-23 11:42:43 UTC
Created attachment 103571 [details] [review]
Bug 24161: Display claimed dates vertically
Comment 37 Jonathan Druart 2020-04-23 11:43:39 UTC
(In reply to Martin Renvoize from comment #31)
> Feedback from sponsor testing.
> 
> (1) The 'claimed date' field lists the claimed dates side by side, could
> they be stacked vertically instead with most recent at the top?

Done in a follow-up patch

> (2) The 'estimated delivery date from - to' filter appears to be broken

Indeed! Patch amended ("Keep tracks of late orders claims")

> (3) The 'vendor' filter does not list all vendors?

It seems to be a bug in master, can you report this separately?
Comment 38 Martin Renvoize 2020-04-23 12:06:56 UTC
Thanks for the quick followups Jonathan.

Yes, I did wonder about that final one perhaps being in master.. I'll report that separately now you've varified.
Comment 39 Martin Renvoize 2020-04-23 12:14:47 UTC
Bug 25266 added
Comment 40 PTFS Europe Sandboxes 2020-04-23 12:21:51 UTC
Created attachment 103588 [details] [review]
Bug 24161: DB changes

Sponsored-by: Cork Institute of Technology

Signed-off-by: Angela O'Connor Desmond <angela.oconnordesmond@staff.ittralee.ie>
Comment 41 PTFS Europe Sandboxes 2020-04-23 12:21:54 UTC
Created attachment 103589 [details] [review]
Bug 24161: DBIC changes

Sponsored-by: Cork Institute of Technology

Signed-off-by: Angela O'Connor Desmond <angela.oconnordesmond@staff.ittralee.ie>
Comment 42 PTFS Europe Sandboxes 2020-04-23 12:21:58 UTC
Created attachment 103590 [details] [review]
Bug 24161: Add the Koha::Acquisition::Order::Claim[s] classes

Sponsored-by: Cork Institute of Technology

Signed-off-by: Angela O'Connor Desmond <angela.oconnordesmond@staff.ittralee.ie>
Comment 43 PTFS Europe Sandboxes 2020-04-23 12:22:01 UTC
Created attachment 103591 [details] [review]
Bug 24161: Keep tracks of late orders claims

So far we only record the number of claims and the date of the last
claim, in the aqorders table.
To keep track of the different claim dates, this patchset is going to
make the following DB changes:
  * Create a new table 'aqorders_claims' (id, ordernumber, claimed_on)
  * Remove the two columns from the aqorders table: claims_count and
  claimed_date

This will allow to display the different claim dates where needed: on
the late orders page, and the basket page.

To avoid additional fetches of Koha::Acquisition::Orders, GetLateOrders
has been moved to Koha::Acquisition::Orders->filter_by_late
That way we are going to add consistency, robustness, and cover the
feature with new tests.

Test plan:
0/ Create a bunch of new orders. Make sure they are from different
vendor (with different delivery time).
1/ Go to the late orders page and claim some orders
2/ Reclaim some of those orders
3/ Confirm that you can see the different claim dates for a given orders
(the history of the late orders claims is kept and displayed)

4/ Bonus point: Regression tests:

a. Modify the closedate of the basket in the database. That
will allow you to make sure the patch set did not introduce regressions.
It would be good to test the different filters on the late orders page:
 * delay
 * Estimated delivery date from/to
 * Vendor

b. Confirm that the subtotal and the total values from the late orders
page is correct.

c. Test the update database entry: do not apply these patches, claims
some orders against master. Apply the patch, execute the update DB entry
then confirm that the number of claims is correct (note that the dates
will not as it is not possible to guess them).

QA note: the branchcode parameter has been removed from filter_by_late.
At first glance it seems that it was not used.

Sponsored-by: Cork Institute of Technology

Signed-off-by: Angela O'Connor Desmond <angela.oconnordesmond@staff.ittralee.ie>
Comment 44 PTFS Europe Sandboxes 2020-04-23 12:22:05 UTC
Created attachment 103592 [details] [review]
Bug 24161: Remove GetLateOrders

Sponsored-by: Cork Institute of Technology

Signed-off-by: Angela O'Connor Desmond <angela.oconnordesmond@staff.ittralee.ie>
Comment 45 PTFS Europe Sandboxes 2020-04-23 12:22:08 UTC
Created attachment 103593 [details] [review]
Bug 24161: Add tests

Sponsored-by: Cork Institute of Technology

Signed-off-by: Angela O'Connor Desmond <angela.oconnordesmond@staff.ittralee.ie>
Comment 46 PTFS Europe Sandboxes 2020-04-23 12:22:12 UTC
Created attachment 103594 [details] [review]
Bug 24161: DBIC manual changes

No idea what's happening with the current_timestamp, but previous change
was wrong.

Signed-off-by: Angela O'Connor Desmond <angela.oconnordesmond@staff.ittralee.ie>
Comment 47 PTFS Europe Sandboxes 2020-04-23 12:22:15 UTC
Created attachment 103595 [details] [review]
Bug 24161: Display claimed dates vertically

Signed-off-by: Angela O'Connor Desmond <angela.oconnordesmond@staff.ittralee.ie>
Comment 48 Katrin Fischer 2020-05-02 03:12:21 UTC
1) 2 small issues in the QA script:

 FAIL	Koha/Acquisition/Order/Claim.pm
   OK	  critic
   FAIL	  forbidden patterns
		forbidden pattern: Incorrect license statement (using postal address), may be a false positive if the file is coming from outside Koha (bug 24545) (line 16)
alid

 FAIL	Koha/Acquisition/Order/Claims.pm
   OK	  critic
   FAIL	  forbidden patterns
		forbidden pattern: Incorrect license statement (using postal address), may be a false positive if the file is coming from outside Koha (bug 24545) (line 16)

2) Database documentation

Can you please add notes to the entry in kohastructure.sql?

I am still testing, but will need to create some more data in my db first.
Comment 49 Katrin Fischer 2020-05-02 12:36:29 UTC
3) In my testing, the vendor pull down was empty, although there showed orders in my results. Can you please check? I was testing with an order made today, oder 0 days ago, estimated delivery date in the range I searched for (between May 1 and May 6).

It should be that when you select a certain vendor in the filters, you can do a 'check all' and claim them all at once (see bug 7930), but can't test atm.

4) You mention a removed branch parameter. I was wondering if there was some kind of permissions thing going on here that would only allow you to claim your own late orders (permission order_manage_all maybe, but more likely the older independentbranches?).
Comment 50 Jonathan Druart 2020-05-02 13:57:56 UTC
Created attachment 104190 [details] [review]
Bug 24161: Fix license statements
Comment 51 Jonathan Druart 2020-05-02 13:58:01 UTC
Created attachment 104191 [details] [review]
Bug 24161: (follow-up) DB changes
Comment 52 Jonathan Druart 2020-05-02 13:59:05 UTC
(In reply to Katrin Fischer from comment #48)

Thanks Katrin for having a look at this!

> 1) 2 small issues in the QA script:
> 
>  FAIL	Koha/Acquisition/Order/Claim.pm
>    OK	  critic
>    FAIL	  forbidden patterns
> 		forbidden pattern: Incorrect license statement (using postal address), may
> be a false positive if the file is coming from outside Koha (bug 24545)
> (line 16)
> alid
> 
>  FAIL	Koha/Acquisition/Order/Claims.pm
>    OK	  critic
>    FAIL	  forbidden patterns
> 		forbidden pattern: Incorrect license statement (using postal address), may
> be a false positive if the file is coming from outside Koha (bug 24545)
> (line 16)

Fixed!

> 2) Database documentation
> 
> Can you please add notes to the entry in kohastructure.sql?

Done!

> I am still testing, but will need to create some more data in my db first.

(In reply to Katrin Fischer from comment #49)
> 3) In my testing, the vendor pull down was empty, although there showed
> orders in my results. Can you please check? I was testing with an order made
> today, oder 0 days ago, estimated delivery date in the range I searched for
> (between May 1 and May 6).
> 
> It should be that when you select a certain vendor in the filters, you can
> do a 'check all' and claim them all at once (see bug 7930), but can't test
> atm.

Yes, Martin opened bug 25266 for that, it's not directly related to these patches. I had trouble finding out a solution.

> 4) You mention a removed branch parameter. I was wondering if there was some
> kind of permissions thing going on here that would only allow you to claim
> your own late orders (permission order_manage_all maybe, but more likely the
> older independentbranches?).

The (reverse) path is:
GetLateOrders has "branch" as third parameter, which is sent by lateorders.pl and it's expecting to come from the lateorders.tt template. But there is no "branch" parameter in that form.
Comment 53 Katrin Fischer 2020-05-02 17:25:21 UTC
Created attachment 104207 [details] [review]
Bug 24161: DB changes

Sponsored-by: Cork Institute of Technology

Signed-off-by: Angela O'Connor Desmond <angela.oconnordesmond@staff.ittralee.ie>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 54 Katrin Fischer 2020-05-02 17:25:27 UTC
Created attachment 104208 [details] [review]
Bug 24161: DBIC changes

Sponsored-by: Cork Institute of Technology

Signed-off-by: Angela O'Connor Desmond <angela.oconnordesmond@staff.ittralee.ie>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 55 Katrin Fischer 2020-05-02 17:25:33 UTC
Created attachment 104209 [details] [review]
Bug 24161: Add the Koha::Acquisition::Order::Claim[s] classes

Sponsored-by: Cork Institute of Technology

Signed-off-by: Angela O'Connor Desmond <angela.oconnordesmond@staff.ittralee.ie>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 56 Katrin Fischer 2020-05-02 17:25:38 UTC
Created attachment 104210 [details] [review]
Bug 24161: Keep tracks of late orders claims

So far we only record the number of claims and the date of the last
claim, in the aqorders table.
To keep track of the different claim dates, this patchset is going to
make the following DB changes:
  * Create a new table 'aqorders_claims' (id, ordernumber, claimed_on)
  * Remove the two columns from the aqorders table: claims_count and
  claimed_date

This will allow to display the different claim dates where needed: on
the late orders page, and the basket page.

To avoid additional fetches of Koha::Acquisition::Orders, GetLateOrders
has been moved to Koha::Acquisition::Orders->filter_by_late
That way we are going to add consistency, robustness, and cover the
feature with new tests.

Test plan:
0/ Create a bunch of new orders. Make sure they are from different
vendor (with different delivery time).
1/ Go to the late orders page and claim some orders
2/ Reclaim some of those orders
3/ Confirm that you can see the different claim dates for a given orders
(the history of the late orders claims is kept and displayed)

4/ Bonus point: Regression tests:

a. Modify the closedate of the basket in the database. That
will allow you to make sure the patch set did not introduce regressions.
It would be good to test the different filters on the late orders page:
 * delay
 * Estimated delivery date from/to
 * Vendor

b. Confirm that the subtotal and the total values from the late orders
page is correct.

c. Test the update database entry: do not apply these patches, claims
some orders against master. Apply the patch, execute the update DB entry
then confirm that the number of claims is correct (note that the dates
will not as it is not possible to guess them).

QA note: the branchcode parameter has been removed from filter_by_late.
At first glance it seems that it was not used.

Sponsored-by: Cork Institute of Technology

Signed-off-by: Angela O'Connor Desmond <angela.oconnordesmond@staff.ittralee.ie>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 57 Katrin Fischer 2020-05-02 17:25:44 UTC
Created attachment 104211 [details] [review]
Bug 24161: Remove GetLateOrders

Sponsored-by: Cork Institute of Technology

Signed-off-by: Angela O'Connor Desmond <angela.oconnordesmond@staff.ittralee.ie>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 58 Katrin Fischer 2020-05-02 17:25:49 UTC
Created attachment 104212 [details] [review]
Bug 24161: Add tests

Sponsored-by: Cork Institute of Technology

Signed-off-by: Angela O'Connor Desmond <angela.oconnordesmond@staff.ittralee.ie>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 59 Katrin Fischer 2020-05-02 17:25:54 UTC
Created attachment 104213 [details] [review]
Bug 24161: DBIC manual changes

No idea what's happening with the current_timestamp, but previous change
was wrong.

Signed-off-by: Angela O'Connor Desmond <angela.oconnordesmond@staff.ittralee.ie>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 60 Katrin Fischer 2020-05-02 17:25:59 UTC
Created attachment 104214 [details] [review]
Bug 24161: Display claimed dates vertically

Signed-off-by: Angela O'Connor Desmond <angela.oconnordesmond@staff.ittralee.ie>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 61 Katrin Fischer 2020-05-02 17:26:04 UTC
Created attachment 104215 [details] [review]
Bug 24161: Fix license statements

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 62 Katrin Fischer 2020-05-02 17:26:09 UTC
Created attachment 104216 [details] [review]
Bug 24161: (follow-up) DB changes

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 63 Martin Renvoize 2020-05-04 08:30:50 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 64 Jonathan Druart 2020-05-04 11:30:11 UTC
Created attachment 104258 [details] [review]
Bug 24161: (follow-up) Fix typo in POD

#   Failed test 'POD test for Koha/Acquisition/Orders.pm'
 #   at /usr/share/perl5/Test/Pod.pm line 187.
 # Koha/Acquisition/Orders.pm (43): Non-ASCII character seen before =encoding in 'uivalent'. Assuming UTF-8
xt/author/podcorrectness.t .. 622/829 # Looks like you failed 1 test of 829.
Comment 65 Jonathan Druart 2020-05-04 11:33:49 UTC
Created attachment 104259 [details] [review]
Bug 24161: (follow-up) DBIC changes

Define koha_object[s]_class methods
Comment 66 Martin Renvoize 2020-05-04 11:49:52 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 67 Tomás Cohen Arazi 2020-05-04 12:16:25 UTC
Created attachment 104265 [details] [review]
Bug 24161: (follow-up) Typo in POD

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 68 Joy Nelson 2020-05-11 20:40:04 UTC
not backported to 19.11.x
Comment 69 Jonathan Druart 2020-05-13 07:59:35 UTC
Created attachment 104826 [details] [review]
Bug 24161: Fix failing test (when running slow)

We "mocked" the claim date to make sure we were not going to hit this
problem, but it's not done on the correct claim, it should be done on
the last one (so second).

from Koha::Acq::Order->claimed_date

294     my $last_claim = $self->claims->last;
295     return unless $last_claim;
296     return $last_claim->claimed_on;
Comment 70 Martin Renvoize 2020-05-13 08:07:20 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 71 Jonathan Druart 2020-05-13 14:11:45 UTC
Created attachment 104848 [details] [review]
Bug 24161: (follow-up) Fix failing test (when running slow)

The previous assumption was wrong, we just need to return the last one.
The tests mean to have 2 different values in the claimed_on dates.
Comment 72 Martin Renvoize 2020-05-13 15:06:22 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 73 Katrin Fischer 2022-04-01 07:59:54 UTC
I am not sure if the database for this one works correctly. It looks like the timestamp was always set to the 'update date' instead of the last known claim date as it should have been. :(

<pre>
$DBversion = '19.12.00.085';
if( CheckVersion( $DBversion ) ) {
    unless ( TableExists( 'aqorders_claims' ) ) {
        $dbh->do(q|
            CREATE TABLE aqorders_claims (
                id int(11) AUTO_INCREMENT,
                ordernumber INT(11) NOT NULL,
                claimed_on TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP,
                PRIMARY KEY (id),
                CONSTRAINT aqorders_claims_ibfk_1 FOREIGN KEY (ordernumber) REFERENCES aqorders (ordernumber) ON DELETE CASCADE ON UPDATE CASCADE
            ) ENGINE = InnoDB DEFAULT CHARSET = utf8mb4 COLLATE = utf8mb4_unicode_ci
        |);

        my $orders = $dbh->selectall_arrayref(q|
            SELECT ordernumber, claims_count, claimed_date
            FROM aqorders
            WHERE claims_count > 0
        |, { Slice => {} });
        my $insert_claim_sth = $dbh->prepare(q|
            INSERT INTO aqorders_claims (ordernumber, claimed_on)
            VALUES (?,?)
        |);

        for my $order ( @$orders ) {
            for my $claim (1..$order->{claims_count}) {
                $insert_claim_sth->execute($order->{ordernumber}, $order->{claimed_on});
            }
        }

        $dbh->do(q|ALTER TABLE aqorders DROP COLUMN claims_count, DROP COLUMN claimed_date|);
    }

    NewVersion( $DBversion, 24161, "Add new join table aqorders_claims to keep track of claims");
}
Comment 74 Jonathan Druart 2022-04-05 09:45:08 UTC
(In reply to Katrin Fischer from comment #73)
> I am not sure if the database for this one works correctly. It looks like
> the timestamp was always set to the 'update date' instead of the last known
> claim date as it should have been. :(
> 
> <pre>
> $DBversion = '19.12.00.085';
> if( CheckVersion( $DBversion ) ) {
>     unless ( TableExists( 'aqorders_claims' ) ) {
>         $dbh->do(q|
>             CREATE TABLE aqorders_claims (
>                 id int(11) AUTO_INCREMENT,
>                 ordernumber INT(11) NOT NULL,
>                 claimed_on TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP,
>                 PRIMARY KEY (id),
>                 CONSTRAINT aqorders_claims_ibfk_1 FOREIGN KEY (ordernumber)
> REFERENCES aqorders (ordernumber) ON DELETE CASCADE ON UPDATE CASCADE
>             ) ENGINE = InnoDB DEFAULT CHARSET = utf8mb4 COLLATE =
> utf8mb4_unicode_ci
>         |);
> 
>         my $orders = $dbh->selectall_arrayref(q|
>             SELECT ordernumber, claims_count, claimed_date
>             FROM aqorders
>             WHERE claims_count > 0
>         |, { Slice => {} });
>         my $insert_claim_sth = $dbh->prepare(q|
>             INSERT INTO aqorders_claims (ordernumber, claimed_on)
>             VALUES (?,?)
>         |);
> 
>         for my $order ( @$orders ) {
>             for my $claim (1..$order->{claims_count}) {
>                 $insert_claim_sth->execute($order->{ordernumber},
> $order->{claimed_on});
>             }
>         }
> 
>         $dbh->do(q|ALTER TABLE aqorders DROP COLUMN claims_count, DROP
> COLUMN claimed_date|);
>     }
> 
>     NewVersion( $DBversion, 24161, "Add new join table aqorders_claims to
> keep track of claims");
> }

Looks like the following line
22065                 $insert_claim_sth->execute($order->{ordernumber}, $order->{claimed_on});
should have used $order->{claimed_on} instead.
Comment 75 Katrin Fischer 2022-04-10 22:06:14 UTC
(In reply to Jonathan Druart from comment #74)
> (In reply to Katrin Fischer from comment #73)
> Looks like the following line
> 22065                 $insert_claim_sth->execute($order->{ordernumber},
> $order->{claimed_on});
> should have used $order->{claimed_on} instead.

Did you mean claimed_date maybe?
Comment 76 Jonathan Druart 2022-04-21 09:17:11 UTC
(In reply to Katrin Fischer from comment #75)
> (In reply to Jonathan Druart from comment #74)
> > (In reply to Katrin Fischer from comment #73)
> > Looks like the following line
> > 22065                 $insert_claim_sth->execute($order->{ordernumber},
> > $order->{claimed_on});
> > should have used $order->{claimed_on} instead.
> 
> Did you mean claimed_date maybe?

Yes!...
Comment 77 Jonathan Druart 2022-05-31 05:58:07 UTC
Reported on bug 30539