Bug 7308

Summary: new column in admin/aqbudgets
Product: Koha Reporter: claire.hernandez <claire.hernandez>
Component: AcquisitionsAssignee: Julian Maurice <julian.maurice>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: bgkriegel, fcapovilla, fridolin.somers, gmcharlt, jonathan.druart, julian.maurice, katrin.fischer, kyle, m.de.rooy, mathsabypro, mglavica, paul.poulain
Version: master   
Hardware: All   
OS: All   
Change sponsored?: Sponsored Patch complexity: Small patch
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 7163    
Attachments: Show ordered amount in aqbudgets.pl
Bug 7308: rework aqbudgets.pl table
Bug 7308: Show ordered amount in aqbudgets.pl
Bug 7308: rework aqbudgets.pl table
Bug 7308: rework aqbudgets.pl table
Bug 7308: Show ordered amount in aqbudgets.pl
Bug 7308: rework aqbudgets.pl table
[PASSED QA] Bug 7308: Show ordered amount in aqbudgets.pl
[PASSED QA] Bug 7308: rework aqbudgets.pl table
Bug 7308 - Follow up: removing line breaks for better translatability
potential source of confusion if parent and child funds both have orders
Bug 7308: change color style for fund child
[PASSED QA] Bug 7308 - Follow up: removing line breaks for better translatability
[PASSED QA] Bug 7308: change color style for fund child

Description claire.hernandez@biblibre.com 2011-12-02 10:21:15 UTC
display a column in admin/aqbudgets "Base level ordered"	 (BibLibre MT5835)
Comment 1 Julian Maurice 2012-04-13 08:45:40 UTC Comment hidden (obsolete)
Comment 2 Marijana Glavica 2012-06-10 08:51:25 UTC
I tested the patch. "Base level ordered" column is now visible. But there are few more things which maybe should be changed in this table:

1. Amount shown in "Base-level remaining" column should be same as "Avail" column in acqui/acqui-home.pl, i.e. remaining = allocated - ordered - spent.

2. We should have the same wording for this two columns for consistency reasons (available or remaining)

3. "Base-level remaining" column should also have sorting feature (Claire noticed that)
Comment 3 Julian Maurice 2012-07-06 08:55:46 UTC
I'm not sure what to do for 1.:
In acqui/acqui-home.pl: Avail = allocated - (ordered + spent) (ordered and spent do not include child funds)
In admin/aqbudgets.pl: Remaining = allocated - spent (here, spent include child funds, but column indicate "Base-level remaining")

What should we have in these columns?
Comment 4 Julian Maurice 2012-07-18 11:26:13 UTC
Switching to 'in discussion'
Comment 5 Marijana Glavica 2012-07-18 18:43:46 UTC
Sorry Maurice for me not being clear.

I took another look to admin/aqbudgets.pl and I found more confusing stuff, so I will try to make it clear what I think this table should look like to be really useful. I am looking at my production data.

admin/aqbudgets.pl should have the following columns:

Base-level allocated (or just Allocated)
Base-level ordered
Total sub-levels ordered
Base-level spent
Total sub-levels spent
Base-level available
Total sub-levels available 

Base-level is always calculated for one level, without children. 
Total sub-levels should include child funds. 
Available is calculated as "allocated - (ordered + spent)".

So, this means that:
- "Total allocated" should be removed because currently it shows the same data as "Base-level alloceted".
- three new columns should be added: "Base-level ordered", "Total sub-levels ordered" and "Total sub-levels available";
- wording should be changed from "remaining" to "available" (consistency with acqui/acqui-home.pl)
Comment 6 Julian Maurice 2012-08-07 13:47:59 UTC
This sounds ok(In reply to comment #5)
> Sorry Maurice for me not being clear.
I prefer my firstname: Julian ;-)

> 
> I took another look to admin/aqbudgets.pl and I found more confusing stuff,
> so I will try to make it clear what I think this table should look like to
> be really useful. I am looking at my production data.
> 
> admin/aqbudgets.pl should have the following columns:
> 
> Base-level allocated (or just Allocated)
> Base-level ordered
> Total sub-levels ordered
> Base-level spent
> Total sub-levels spent
> Base-level available
> Total sub-levels available 
> 
> Base-level is always calculated for one level, without children. 
> Total sub-levels should include child funds. 
> Available is calculated as "allocated - (ordered + spent)".
> 
> So, this means that:
> - "Total allocated" should be removed because currently it shows the same
> data as "Base-level alloceted".
> - three new columns should be added: "Base-level ordered", "Total sub-levels
> ordered" and "Total sub-levels available";
> - wording should be changed from "remaining" to "available" (consistency
> with acqui/acqui-home.pl)

This sounds ok for me.
Can anyone else confirm that this is what we want in these tables?
Comment 7 Julian Maurice 2012-10-11 08:58:02 UTC
I'll start working on it.
Comment 8 Julian Maurice 2012-10-11 12:17:00 UTC Comment hidden (obsolete)
Comment 9 Bernardo Gonzalez Kriegel 2013-02-17 16:19:36 UTC
Patch doesn't apply
Applying: Bug 7308: Show ordered amount in aqbudgets.pl
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/aqbudgets.tt
Auto-merging admin/aqbudgets.pl
CONFLICT (content): Merge conflict in admin/aqbudgets.pl
Auto-merging C4/Budgets.pm
Failed to merge in the changes.
Patch failed at 0001 Bug 7308: Show ordered amount in aqbudgets.pl
Comment 10 Julian Maurice 2013-03-11 08:20:46 UTC Comment hidden (obsolete)
Comment 11 Julian Maurice 2013-03-11 08:21:42 UTC Comment hidden (obsolete)
Comment 12 Julian Maurice 2013-03-11 08:22:16 UTC
Patches rebased on master.
Comment 13 Paul Poulain 2013-03-19 13:11:22 UTC
Patch tested with a sandbox, by Cedric Vita <cedric.vita@dracenie.com>
Comment 14 Paul Poulain 2013-03-19 13:11:37 UTC Comment hidden (obsolete)
Comment 15 Fridolin Somers 2013-03-19 14:08:09 UTC Comment hidden (obsolete)
Comment 16 Fridolin Somers 2013-03-19 14:09:04 UTC Comment hidden (obsolete)
Comment 17 Marcel de Rooy 2013-06-10 06:57:55 UTC
syntax error at /usr/share/koha/testclone/admin/aqbudgets.pl line 323, near "/) "
syntax error at /usr/share/koha/testclone/admin/aqbudgets.pl line 327, near "$_}"
Global symbol "@loop" requires explicit package name at /usr/share/koha/testclone/admin/aqbudgets.pl line 352.
Unmatched right curly bracket at /usr/share/koha/testclone/admin/aqbudgets.pl line 357, at end of line
syntax error at /usr/share/koha/testclone/admin/aqbudgets.pl line 357, near "}"
Unmatched right curly bracket at /usr/share/koha/testclone/admin/aqbudgets.pl line 376, at end of line
syntax error at /usr/share/koha/testclone/admin/aqbudgets.pl line 376, near "}"
Execution of /usr/share/koha/testclone/admin/aqbudgets.pl aborted due to compilation errors.

Failed QA
Comment 18 Julian Maurice 2013-06-13 13:01:43 UTC
I do not reproduce these errors.
Here's what I've done:

$ git checkout -b bug/7308 && git fetch origin && git reset --hard origin/master
$ git bz apply 7308
Bug 7308 - new column in admin/aqbudgets

Bug 7308: Show ordered amount in aqbudgets.pl
Apply? [yn] y

Applying: Bug 7308: Show ordered amount in aqbudgets.pl
Bug 7308: rework aqbudgets.pl table
Apply? [yn] y
            
Applying: Bug 7308: rework aqbudgets.pl table
$ perl -cw admin/aqbudgets.pl
admin/aqbudgets.pl syntax OK
$ git log --oneline -3
ebd30fe Bug 7308: rework aqbudgets.pl table
72e2c0a Bug 7308: Show ordered amount in aqbudgets.pl
acd4b21 Bug 10311 - Holds queue ignores item-level holds where only one items exists
$ perl -v | head -n2

This is perl, v5.10.1 (*) built for x86_64-linux-gnu-thread-multi


Looking at the source file, the code looks good too.
Could you retry please?
Comment 19 Jonathan Druart 2013-07-02 10:37:38 UTC
I confirm I don't get any error.
Comment 20 Julian Maurice 2013-07-02 11:38:25 UTC
Reset to Signed off
Comment 21 Katrin Fischer 2013-07-21 08:27:35 UTC
Created attachment 19824 [details] [review]
[PASSED QA] Bug 7308: Show ordered amount in aqbudgets.pl

Signed-off-by: C├ędric Vita <cedric.vita@dracenie.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 22 Katrin Fischer 2013-07-21 08:27:46 UTC
Created attachment 19825 [details] [review]
[PASSED QA] Bug 7308: rework aqbudgets.pl table

admin/aqbudgets.pl should have the following columns:

Base-level allocated (or just Allocated)
Base-level ordered
Total sub-levels ordered
Base-level spent
Total sub-levels spent
Base-level available
Total sub-levels available

Base-level is always calculated for one level, without children.
Total sub-levels should include child funds.
Available is calculated as "allocated - (ordered + spent)".

Signed-off-by: Cedric Vita <cedric.vita@dracenie.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Seems to work alright for me.
Passes QA script and tests, after I fixed 2 tabs in admin/aqbudgets.pl.

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 23 Katrin Fischer 2013-07-21 08:28:15 UTC Comment hidden (obsolete)
Comment 24 Galen Charlton 2013-07-22 19:37:20 UTC
Created attachment 19859 [details]
potential source of confusion if parent and child funds both have orders

Overall, I think the patch looks good, but there's a case where I think it leads to a confusing display.

In the screenshot, fund abc has $10.45 ordered against it, but it also has a child fund, def, that has $14.97 ordered against it.

The base-level ordered and total-ordered make sense, individually, but when you look at the whole picture, a librarian could be excused for asking why the total of the "total ordered" column is $33.97, not $48.94 (= $25.42 + 14.97 + $8.55).

Does anybody have any suggestions on how this could be made more clear?  Setting to in discussion.
Comment 25 Paul Poulain 2013-07-23 06:45:11 UTC
(In reply to Galen Charlton from comment #24)
> Does anybody have any suggestions on how this could be made more clear? 
> Setting to in discussion.

Pushed or in discussion ? status mistaken ?
Comment 26 Jonathan Druart 2013-10-28 11:20:23 UTC
Galen,
I don't see a good solution here.
Maybe can we change the color for child cells?
I think there is a deep ergonomic issue on this page and should be partially rewrite (always use DT and insert a way to always see funds as a tree).
I will submit a followup in order to revive this bug.
Comment 27 Jonathan Druart 2013-10-28 11:21:26 UTC Comment hidden (obsolete)
Comment 28 Jonathan Druart 2013-10-28 11:22:38 UTC
Switch to "needs QA", waiting for a QAer point of view.
Comment 29 Katrin Fischer 2013-12-26 14:50:40 UTC
Ok, it looks like Jonathan and me can't do QA here - could someone else take this on please? :)
Comment 30 Kyle M Hall 2014-01-31 12:29:15 UTC
Created attachment 24936 [details] [review]
[PASSED QA] Bug 7308 - Follow up: removing line breaks for better translatability

Because of the lines breaks in the table headings the strings
were separated in translation. Example:

Base-level allocated
 - Base-level
 - allocated

If you look at a smaller screen the table headings will still break
without the additional <br />.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 31 Kyle M Hall 2014-01-31 12:29:32 UTC
Created attachment 24937 [details] [review]
[PASSED QA] Bug 7308: change color style for fund child

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 32 Galen Charlton 2014-04-21 04:17:59 UTC
Pushed to master, along with a follow-up that introduces a descriptive CSS class (child_fund_amount) and uses italics rather than a subtle font color difference to indicate that child fund amounts do not participate in the totals.

Thanks, Julian and Jonathan!
Comment 33 simith.doliveira 2014-08-29 19:43:57 UTC
*** Bug 11823 has been marked as a duplicate of this bug. ***
Comment 34 Fridolin Somers 2014-09-10 14:40:44 UTC
Pushed to 3.14.x, will be in 3.14.11