Bug 14388 - add sorting to funds
Summary: add sorting to funds
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL: cgi-bin/koha/acqui/acqui-home.pl
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-06-15 17:59 UTC by Nicole C. Engard
Modified: 2017-06-14 22:10 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 14388: Funds should be sorted by budget_code (3.19 KB, patch)
2015-07-02 15:44 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14388: Funds should be sorted by budget_code (3.19 KB, patch)
2015-07-09 07:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14388: Funds should be sorted by budget_code (3.26 KB, patch)
2015-10-30 18:42 UTC, Nicole C. Engard
Details | Diff | Splinter Review
Budget order without the patch (29.19 KB, image/png)
2015-11-06 07:49 UTC, j.kylmala
Details
[PASSED QA] Bug 14388: Funds should be sorted by budget_code (3.32 KB, patch)
2015-11-08 21:57 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nicole C. Engard 2015-06-15 17:59:37 UTC
The list of funds on this page (cgi-bin/koha/acqui/acqui-home.pl) has no sorting in place. It should sort on fund name or code or something useful to help people find the fund they want.
Comment 1 Jonathan Druart 2015-07-02 15:44:59 UTC Comment hidden (obsolete)
Comment 2 M. Tompsett 2015-07-03 00:03:30 UTC
Comment on attachment 40765 [details] [review]
Bug 14388: Funds should be sorted by budget_code

Review of attachment 40765 [details] [review]:
-----------------------------------------------------------------

::: t/db_dependent/Budgets.t
@@ +428,5 @@
>  );
>  
>  $budget_hierarchy        = GetBudgetHierarchy($budget_period_id);
> +is( $budget_hierarchy->[0]->{children}->[0]->{budget_name}, 'budget_11', 'GetBudgetHierarchy should return budgets ordered by name, first child is budget_11' );
> +is( $budget_hierarchy->[0]->{children}->[1]->{budget_name}, 'budget_12', 'GetBudgetHierarchy should return budgets ordered by name, first child is budget_12' );

second child, not first. Please fix typo.
Comment 3 M. Tompsett 2015-07-03 00:10:19 UTC
I was not able to duplicate issue with Debian Jessie on Master. Though, this patch clearly forces sorting.

Additionally, tests failed to run with my initial data set. Requires CPL.
Comment 4 Jonathan Druart 2015-07-09 07:32:23 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2015-07-09 07:34:05 UTC
(In reply to M. Tompsett from comment #2)
> second child, not first. Please fix typo.

Fixed, thanks

(In reply to M. Tompsett from comment #3)
> I was not able to duplicate issue with Debian Jessie on Master. Though, this
> patch clearly forces sorting.

They are currently sorted by budget_id.

> Additionally, tests failed to run with my initial data set. Requires CPL.

Which is the case for almost all tests.
Comment 6 M. Tompsett 2015-07-09 09:49:53 UTC
I wrote:
> I was not able to duplicate issue with Debian Jessie on Master.
> Though, this patch clearly forces sorting.

(In reply to Jonathan Druart from comment #5)
> They are currently sorted by budget_id.

My screen says otherwise. I'm wondering if this is a hashes are stored in a random order issue, because it ordered 2,1,3 on my screen without the patch.
Comment 7 j.kylmala 2015-10-13 08:39:37 UTC
(In reply to M. Tompsett from comment #6)
> I wrote:
> > I was not able to duplicate issue with Debian Jessie on Master.
> > Though, this patch clearly forces sorting.
> 
> (In reply to Jonathan Druart from comment #5)
> > They are currently sorted by budget_id.
> 
> My screen says otherwise. I'm wondering if this is a hashes are stored in a
> random order issue, because it ordered 2,1,3 on my screen without the patch.

Mine too. But this happened only when I had first applied the patch and then retracted it. Before I applied the patch they were ordered by the budget_id.
Comment 8 j.kylmala 2015-10-13 09:13:58 UTC
(In reply to Joonas Kylmälä from comment #7)
> (In reply to M. Tompsett from comment #6)
> > I wrote:
> > > I was not able to duplicate issue with Debian Jessie on Master.
> > > Though, this patch clearly forces sorting.
> > 
> > (In reply to Jonathan Druart from comment #5)
> > > They are currently sorted by budget_id.
> > 
> > My screen says otherwise. I'm wondering if this is a hashes are stored in a
> > random order issue, because it ordered 2,1,3 on my screen without the patch.
> 
> Mine too. But this happened only when I had first applied the patch and then
> retracted it. Before I applied the patch they were ordered by the budget_id.

It happened because in acqui-home.tt there is a javascript line which states:
        'aaSortingFixed': [[ 1, 'asc' ]],

So the table shown to the user is sorted with that. I'm just wondering why it started to only work after I applied the patch...
Comment 9 Nicole C. Engard 2015-10-30 18:42:16 UTC Comment hidden (obsolete)
Comment 10 j.kylmala 2015-11-04 10:44:25 UTC
Do we need this patch? The funds are in sorted order even without this patch in acqui/acqui-home.pl.
Comment 11 Jonathan Druart 2015-11-05 10:07:51 UTC
(In reply to Joonas Kylmälä from comment #10)
> Do we need this patch? The funds are in sorted order even without this patch
> in acqui/acqui-home.pl.

No, they are not.
Comment 12 j.kylmala 2015-11-06 07:49:33 UTC
Created attachment 44538 [details]
Budget order without the patch

Jonathan, isn't it here ordered by budget code? Is the patch trying to have the list in this order as shown in the picture? If not, could you please explain me what it tries to do. (I understand that the patch changes the order in the html level (before JS))
Comment 13 Jonathan Druart 2015-11-06 10:09:31 UTC
(In reply to Joonas Kylmälä from comment #12)
> Created attachment 44538 [details]
> Budget order without the patch
> 
> Jonathan, isn't it here ordered by budget code? Is the patch trying to have
> the list in this order as shown in the picture? If not, could you please
> explain me what it tries to do. (I understand that the patch changes the
> order in the html level (before JS))

You have only 1 fund per budget, so you cannot see the sort issue :)
Try and create 4 funds: fund_1_1, fund_0_1, fund_1_0, fund_0_0 (in this order) for the same budget.
Comment 14 Jonathan Druart 2015-11-06 10:11:47 UTC
And keep in mind:
A budget is related to the aqbudgetperiods table and the $budget_period variable, a fund is related to the budgets table and the $budget variable.
Comment 15 j.kylmala 2015-11-06 14:18:40 UTC
(In reply to Jonathan Druart from comment #13)
> You have only 1 fund per budget, so you cannot see the sort issue :)
> Try and create 4 funds: fund_1_1, fund_0_1, fund_1_0, fund_0_0 (in this
> order) for the same budget.

Oh, I see. I thought all this time that it was about sorting the different budgets... And now I see that even the title says funds :-D
Comment 16 Katrin Fischer 2015-11-08 21:57:50 UTC
Created attachment 44624 [details] [review]
[PASSED QA] Bug 14388: Funds should be sorted by budget_code

Before this patch, the funds were sorted by budget_id, which does not
make any sense.

This patch adds a sort by budget_code on the fund list (acqui/acqui-home.pl and
admin/aqbudgets.pl)

Test plan:
On both pages (acqui/acqui-home.pl and admin/aqbudgets.pl) confirm that
the funds are now sorted by fund code (DB column budget_code)

Signed-off-by: Nicole Engard <nengard@bywatersolutions.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 17 Tomás Cohen Arazi 2015-11-09 15:53:21 UTC
Patch pushed to master.

Thanks Jonathan!
Comment 18 Frédéric Demians 2015-11-29 20:37:28 UTC
This patch has been pushed to 3.20.x, will be in 3.20.6.
Comment 19 Liz Rea 2016-01-06 21:07:29 UTC
Pushed to 3.18.13, and released.