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.
Created attachment 43392 [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.
Created attachment 44262 [details] [review] [SIGNED-OFF] 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
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?
(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?
What is the status of this Bug?
(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.
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.
You will need to provide tests for this change.
Note that a AV could be "" or 0 :-/
(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?
(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.
It is ok, but we need regression tests. Also, please mark it targets master.
Created attachment 49601 [details] [review] Bug 15009 - Adds unit tests for C4::Budget::GetBudgetAuthCats
Created attachment 50293 [details] [review] Bug 15009 - Adds unit tests for C4::Budget::GetBudgetAuthCats Signed-off-by: Marc Véron <veron@veron.ch>
Signing off for unit tests.
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>
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>
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>
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>
Pushed to master for the May 2016 release. Spasibo Philippe!
Patch pushed to 3.22.x, will be in 3.22.7