Bug 15009 - Planning dropdown button in aqbudget can have empty line
Summary: Planning dropdown button in aqbudget can have empty line
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: System Administration (show other bugs)
Version: Main
Hardware: All All
: P5 - low trivial
Assignee: Blou
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-10-13 19:58 UTC by Blou
Modified: 2016-12-05 21:25 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Planning dropdown having useless fourth row (76.30 KB, image/png)
2015-10-13 19:58 UTC, Blou
Details
Bug 15009 - Planning dropdown button in aqbudget can have empty line (1.42 KB, patch)
2015-10-13 20:19 UTC, Blou
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 15009 - Planning dropdown button in aqbudget can have empty line (1.53 KB, patch)
2015-10-30 23:56 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 15009 - Adds unit tests for C4::Budget::GetBudgetAuthCats (2.24 KB, patch)
2016-03-25 21:11 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 15009 - Adds unit tests for C4::Budget::GetBudgetAuthCats (2.38 KB, patch)
2016-04-17 08:20 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 15009 - Planning dropdown button in aqbudget can have empty line (1.59 KB, patch)
2016-04-18 09:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15009 - Adds unit tests for C4::Budget::GetBudgetAuthCats (2.45 KB, patch)
2016-04-18 09:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15009: Correctly populate planning filter (1.05 KB, patch)
2016-04-18 09:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15009: QA follow-up (5.30 KB, patch)
2016-04-18 09:41 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 Blou 2015-10-13 19:58:39 UTC
Created attachment 43391 [details]
Planning dropdown having useless fourth row

When displaying a budget, the Planning button in the admin toolbar displays

Plan by months
Plan by libraries
Plan by item types
Plan by

The last one is empty, due to C4::Budgets::GetBudgetAuthCats returning an empty field if the budget has no sort defined.
Comment 1 Blou 2015-10-13 20:19:00 UTC Comment hidden (obsolete)
Comment 2 Bernardo Gonzalez Kriegel 2015-10-30 23:56:59 UTC Comment hidden (obsolete)
Comment 3 Katrin Fischer 2015-11-04 00:31:25 UTC
This removes the empty entry and the test plan for this works. But under which conditions would the entry show up again? 
Since the sort categories can be different for each fund I wonder if this can work. Can you please take a look?
Comment 4 Charles Farmer 2016-01-25 19:43:56 UTC
(In reply to Katrin Fischer from comment #3)
> This removes the empty entry and the test plan for this works. But under
> which conditions would the entry show up again? 
> Since the sort categories can be different for each fund I wonder if this
> can work. Can you please take a look?

From a quick search for C4::Budgets::GetBudgetAuthCats, these values are only used in the 'Planning' button of the acquisition module. They don't seem to show up anywhere else.

Even if the sort categories are different from a budget to another, additional statistics on a budget do show up in the 'Planning' button, which is the desired behaviour. For example, if I had the statistic 'CCODE' to my fund, the option shows up after the patch.

Does it answer your question?
Comment 5 Marc Véron 2016-02-22 10:12:24 UTC
What is the status of this Bug?
Comment 6 Charles Farmer 2016-02-22 15:17:55 UTC
(In reply to Marc Véron from comment #5)
> What is the status of this Bug?

Good question. It has a signed off patch, but after Katrin's inquiry, I wasn't sure where we were heading next.

Let's just say 'Signed off' for now.
Comment 7 Katrin Fischer 2016-02-22 15:33:27 UTC
Sorry, I had intended to come back on this - I have a bit of a backlog with bugs to comment/test on again :(

Signed off is ok by me.
Comment 8 Jonathan Druart 2016-02-29 14:41:25 UTC
You will need to provide tests for this change.
Comment 9 Jonathan Druart 2016-02-29 14:41:44 UTC
Note that a AV could be "" or 0 :-/
Comment 10 Charles Farmer 2016-02-29 15:07:45 UTC
(In reply to Jonathan Druart from comment #9)
> Note that a AV could be "" or 0 :-/

Could you be more specific about what that entails? Is it a good thing, or a bad thing?
Comment 11 Jonathan Druart 2016-02-29 15:28:17 UTC
(In reply to Charles Farmer from comment #10)
> (In reply to Jonathan Druart from comment #9)
> > Note that a AV could be "" or 0 :-/
> 
> Could you be more specific about what that entails? Is it a good thing, or a
> bad thing?

When you create an authorised value, you can set it with "" or "0".
You are testing if the aqbudgets.sort*_authcat is evaluated to true, that means 0 and "" won't be displayed.
I don't have a quick solution to suggest. I don't think we should accept "" as a valid authorised value category, but it's outside the scope of this bug.
Comment 12 Tomás Cohen Arazi (tcohen) 2016-03-23 15:12:09 UTC
It is ok, but we need regression tests. Also, please mark it targets master.
Comment 13 Charles Farmer 2016-03-25 21:11:37 UTC Comment hidden (obsolete)
Comment 14 Marc Véron 2016-04-17 08:20:06 UTC Comment hidden (obsolete)
Comment 15 Marc Véron 2016-04-17 08:20:44 UTC
Signing off for unit tests.
Comment 16 Jonathan Druart 2016-04-18 09:40:41 UTC
Created attachment 50314 [details] [review]
Bug 15009 - Planning dropdown button in aqbudget can have empty line

When displaying a budget, the Planning button in the admin toolbar displays

Plan by months
Plan by libraries
Plan by item types
Plan by

The last one is empty, due to C4::Budgets::GetBudgetAuthCats returning an empty field if the budget has no sort defined.
This prevents returning an array with empty element(s)

TEST:
1) Admin -> Budgets
2) Select a budget
   a) you must have '' (empty) in your aqbudgets.sort1_authcat field.
   b) edit the budget (direct DB or interface) to get that.
3) Click on Planning dropdown, see the "Plan by <nothing> " entry.
4) apply the patch, revalidate.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

No more empty option
No errors

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 17 Jonathan Druart 2016-04-18 09:40:45 UTC
Created attachment 50315 [details] [review]
Bug 15009 - Adds unit tests for C4::Budget::GetBudgetAuthCats

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 18 Jonathan Druart 2016-04-18 09:41:02 UTC
Created attachment 50316 [details] [review]
Bug 15009: Correctly populate planning filter

The filter for planning values was populated with empty strings.
Note that it's not caused by previous patches.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 19 Jonathan Druart 2016-04-18 09:41:06 UTC
Created attachment 50317 [details] [review]
Bug 15009: QA follow-up

This patch adds a test to check the unicity of auth cats, simplify
the GetBudgetAuthCats subroutine and make it return an arrayref of scalar
instead of an arrayref of hashref with only 1 key.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 20 Kyle M Hall (khall) 2016-04-29 13:03:08 UTC
Pushed to master for the May 2016 release. Spasibo Philippe!
Comment 21 Julian Maurice 2016-05-16 12:01:59 UTC
Patch pushed to 3.22.x, will be in 3.22.7