Bug 36093 - Fix missing array reference in provider rollup reports
Summary: Fix missing array reference in provider rollup reports
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: ERM (show other bugs)
Version: Main
Hardware: All All
: P5 - low trivial (vote)
Assignee: Matt Blenkinsop
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-02-14 12:28 UTC by Matt Blenkinsop
Modified: 2024-05-28 20:09 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00,23.11.06


Attachments
Bug 36093: Fix missing array reference (1.14 KB, patch)
2024-02-14 12:30 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 36093: Fix missing array reference (1.19 KB, patch)
2024-02-15 11:03 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 36093: Add unit test and fix QA tests (9.24 KB, patch)
2024-03-01 10:32 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 36093: (QA follow-up) Fix unit tests (3.72 KB, patch)
2024-04-19 09:03 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 36093: Fix missing array reference (1.28 KB, patch)
2024-04-19 09:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 36093: Add unit test and fix QA tests (9.32 KB, patch)
2024-04-19 09:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 36093: (QA follow-up) Fix unit tests (3.80 KB, patch)
2024-04-19 09:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Blenkinsop 2024-02-14 12:28:25 UTC
An array has not been correctly referenced when passed into a function when creating a provider rollup report
Comment 1 Matt Blenkinsop 2024-02-14 12:30:28 UTC
Created attachment 162145 [details] [review]
Bug 36093: Fix missing array reference

This patch adds an array reference where it was previously missed.

Test plan:
Review patch diff and observe that the array reference is now correctly added
Comment 2 PTFS Europe Sandboxes 2024-02-15 11:03:27 UTC
Created attachment 162199 [details] [review]
Bug 36093: Fix missing array reference

This patch adds an array reference where it was previously missed.

Test plan:
Review patch diff and observe that the array reference is now correctly added

Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com>
Comment 3 Jonathan Druart 2024-02-29 09:08:24 UTC
Can you provide a test to cover this change please?
Comment 4 Matt Blenkinsop 2024-03-01 10:32:35 UTC
Created attachment 162655 [details] [review]
Bug 36093: Add unit test and fix QA tests

prove t/db_dependent/api/v1/erm_custom_reports.t
Comment 5 Jonathan Druart 2024-03-25 13:32:05 UTC
1. You are not supposed to tidy the whole pm to make the QA tests pass
2. Why do you set date_uploaded in the tests, that's not needed.
3. +    Koha::ERM::EUsage::MonthlyUsages->search->delete;
You should not need to remove data before tests. You can count how many are there before the tests then do a sum.
Comment 6 Jonathan Druart 2024-03-25 13:33:04 UTC
(In reply to Jonathan Druart from comment #5)
> 1. You are not supposed to tidy the whole pm to make the QA tests pass

Don't consider it a blocker, but keep in mind for next time. If you want to tidy the whole ERM namespace please do it on a separate bug.
Comment 7 Matt Blenkinsop 2024-04-19 09:01:44 UTC
Completely missed this QA review! 

(In reply to Jonathan Druart from comment #5)
> 1. You are not supposed to tidy the whole pm to make the QA tests pass

Noted, will bear in mind for future patches

> 2. Why do you set date_uploaded in the tests, that's not needed.

This can be removed

> 3. +    Koha::ERM::EUsage::MonthlyUsages->search->delete;
> You should not need to remove data before tests. You can count how many are
> there before the tests then do a sum.

I think probably when I wrote this test I had some data in the system already that was corrupting the test figures? I'll remove these lines and re-test
Comment 8 Matt Blenkinsop 2024-04-19 09:03:21 UTC
Created attachment 165156 [details] [review]
Bug 36093: (QA follow-up) Fix unit tests

This patch removes some unnecessary lines from the test file
Comment 9 Marcel de Rooy 2024-04-19 09:45:47 UTC
Created attachment 165159 [details] [review]
Bug 36093: Fix missing array reference

This patch adds an array reference where it was previously missed.

Test plan:
Review patch diff and observe that the array reference is now correctly added

Signed-off-by: Jonathan Field <jonathan.field@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 10 Marcel de Rooy 2024-04-19 09:45:49 UTC
Created attachment 165160 [details] [review]
Bug 36093: Add unit test and fix QA tests

prove t/db_dependent/api/v1/erm_custom_reports.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 11 Marcel de Rooy 2024-04-19 09:45:52 UTC
Created attachment 165161 [details] [review]
Bug 36093: (QA follow-up) Fix unit tests

This patch removes some unnecessary lines from the test file

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 12 Katrin Fischer 2024-04-19 16:07:39 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 13 Fridolin Somers 2024-05-23 13:55:54 UTC
Pushed to 23.11.x for 23.11.06
Comment 14 Lucas Gass 2024-05-28 20:09:19 UTC
Doesn't apply cleanly in 23.05.x, no backport.