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.

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.

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

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.

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

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

Do we need this patch? The funds are in sorted order even without this patch in acqui/acqui-home.pl.

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

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

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

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.

(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

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>

Patch pushed to master.

Thanks Jonathan!

This patch has been pushed to 3.20.x, will be in 3.20.6.

Pushed to 3.18.13, and released.