Bug 4502

Summary: Perl error in acq budget planning
Product: Koha Reporter: Katrin Fischer <katrin.fischer>
Component: AcquisitionsAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Bugs List <koha-bugs>
Severity: normal    
Priority: P5 - low CC: bgkriegel, f.demians, jonathan.druart, kyle, liz, mtompset, tomascohen, veron
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 4502: Catch software error if no active budget defined
Bug 4502: Catch software error if no active budget defined
Bug 4502: Catch software error if no active budget defined
Bug 4502 - An attempt to make things more sensible.
Bug 4502 - An attempt to make things more sensible.
[SIGNED-OFF] Bug 4502: Catch software error if no active budget defined
[SIGNED-OFF] Bug 4502: An attempt to make things more sensible.
[SIGNED-OFF] Bug 4502: Catch software error if no active budget defined
[SIGNED-OFF] Bug 4502: An attempt to make things more sensible.
Bug 4502: Changing wording again...
Bug 4502: Catch software error if no active budget defined
Bug 4502: An attempt to make things more sensible.
Bug 4502: Changing wording again...
[SIGNED-OFF] Bug 4502: Changing wording again...
[PASSED QA] Bug 4502: Catch software error if no active budget defined
[PASSED QA] Bug 4502: An attempt to make things more sensible.
[PASSED QA] Bug 4502: Changing wording again...

Description Chris Cormack 2010-05-21 01:27:56 UTC


---- Reported by katrin.fischer@bsz-bw.de 2010-05-13 21:08:28 ----

After I added my first budget I tried 'Planning' > 'Plan by MONTHS'.

I was not sure how this is supposed to work, so I tried different settings for:  Show my funds only   	 
Show active funds only 	
Show actual/estimated values

Clicked on 'Select' and got a perl error message:

Usage: Date::Calc::Delta_YMD(year1, month1, day1, year2, month2, day2) at /home/katrin/kohaclone/admin/aqplan.pl line 250.

I can repeat this as long as there are no active budgets and no funds.

Tested with 3.01.00.134



--- Bug imported by chris@bigballofwax.co.nz 2010-05-21 01:27 UTC  ---

This bug was previously known as _bug_ 4502 at http://bugs.koha.org/cgi-bin/bugzilla3/show_bug.cgi?id=4502

Actual time not defined. Setting to 0.0
Setting qa contact to the default for this product.
   This bug either had no qa contact or an invalid one.

Comment 1 Jonathan Druart 2015-03-24 14:58:27 UTC Comment hidden (obsolete)
Comment 2 Mark Tompsett 2015-03-25 04:46:21 UTC
Comment on attachment 37181 [details] [review]
Bug 4502: Catch software error if no active budget defined

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

::: koha-tmpl/intranet-tmpl/prog/en/modules/admin/aqplan.tt
@@ +224,5 @@
>  
>  </form>
>  
> +        [% ELSIF not budget_period_id %]
> +<div class="dialog alert">No active budget defined. You should define on before continuing.</div>

"You should define one" -- there is a missing 'e'.
Sign off and post a typo patch? Or will you fix it, and I'll this off?
Comment 3 Mark Tompsett 2015-03-25 05:13:58 UTC
And yes, this whole section is ugly. HTML is poor. What is the bug number of the acquisitions re-write? And when might that make it into master?
Comment 4 Jonathan Druart 2015-03-25 08:11:52 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2015-03-25 08:15:50 UTC
(In reply to M. Tompsett from comment #2)
> Comment on attachment 37181 [details] [review] [review]
> Bug 4502: Catch software error if no active budget defined
> 
> Review of attachment 37181 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: koha-tmpl/intranet-tmpl/prog/en/modules/admin/aqplan.tt
> @@ +224,5 @@
> >  
> >  </form>
> >  
> > +        [% ELSIF not budget_period_id %]
> > +<div class="dialog alert">No active budget defined. You should define on before continuing.</div>
> 
> "You should define one" -- there is a missing 'e'.

Fixed, thanks.

(In reply to M. Tompsett from comment #3)
> And yes, this whole section is ugly. HTML is poor. What is the bug number of
> the acquisitions re-write? And when might that make it into master?

When you see I need more than 6 months to get the tax rewrite stuffs tested (not in master), I can't imagine how long a complete module rewrite would take.
Comment 6 Mark Tompsett 2015-03-30 04:17:40 UTC Comment hidden (obsolete)
Comment 7 Katrin Fischer 2015-04-05 20:53:06 UTC
Hm, something is not looking quite right in my test.

I marked all existing budgets as inactive. I created a new one without any fund and also marked it inactive.

I click on the inactive budget, select Planning and the perl error occurs without the patch.

With the patch, I do the same, example URL:
http://localhost:8080/cgi-bin/koha/admin/aqplan.pl?budget_period_id=8&authcat=BRANCHES

Message displayed:
No funds to display for this search criteria

When I know submit the filters, the budget selection seems to be forgotten:
http://localhost:8080/cgi-bin/koha/admin/aqplan.pl

And instead of displaying the new error message I see some fund values that maybe belong to one of the other budgets?

I know this is a messy script - but can we catch this easily maybe?
Comment 8 Mark Tompsett 2015-04-06 01:02:38 UTC
(In reply to Katrin Fischer from comment #7)
[ISSUE SNIPPED]
> 
> I know this is a messy script - but can we catch this easily maybe?

That's beyond the scope of this patch, as far as I can tell. I wouldn't fail this patch for the failing of the whole larger mess.
Comment 9 Katrin Fischer 2015-04-06 08:29:29 UTC
This patch introduces a new error message, that I have not been able to trigger in my tests. The Perl error is gone, which is great - but the information showing in my install now is also quite confusing.
Comment 10 Mark Tompsett 2015-04-06 14:50:09 UTC
Comment on attachment 37354 [details] [review]
Bug 4502: Catch software error if no active budget defined

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

::: admin/aqplan.pl
@@ +107,5 @@
>  $authcat = 'Asort1' if  not defined $authcat; # defaults to Asort if no authcat given
>  
> +# If no active budget is defined, budget_period_id is undef
> +# This is the behavior of GetBudgetPeriod
> +$authcat = '' unless $budget_period_id;

Perhaps it should be 'Asort1'?
Comment 11 Katrin Fischer 2015-04-06 14:54:56 UTC
Not sure if that makes sense... asort1 might not even be an existing authorised value.
Comment 12 Mark Tompsett 2015-04-06 23:57:36 UTC Comment hidden (obsolete)
Comment 13 Mark Tompsett 2015-04-07 00:30:44 UTC Comment hidden (obsolete)
Comment 14 Mark Tompsett 2015-04-07 00:32:46 UTC
How about these two together, Katrin?
Comment 15 Bernardo Gonzalez Kriegel 2015-05-12 13:25:07 UTC Comment hidden (obsolete)
Comment 16 Bernardo Gonzalez Kriegel 2015-05-12 13:26:32 UTC Comment hidden (obsolete)
Comment 17 Bernardo Gonzalez Kriegel 2015-05-12 13:28:07 UTC Comment hidden (obsolete)
Comment 18 Bernardo Gonzalez Kriegel 2015-05-12 13:28:16 UTC Comment hidden (obsolete)
Comment 19 Kyle M Hall 2015-05-22 14:50:49 UTC
I think the phrase:

That budget is not defined. You should select a defined budget to continue.

is a bit confusing. Perhaps it should be more like:

That budget does not exist! Please select a budget to continue.

or maybe this is better

No active budget set! Please activate a budget first.

Thoughts?
Comment 20 Jonathan Druart 2015-05-22 15:03:13 UTC
(In reply to Kyle M Hall from comment #19)
> I think the phrase:
> 
> That budget is not defined. You should select a defined budget to continue.
> 
> is a bit confusing. Perhaps it should be more like:
> 
> That budget does not exist! Please select a budget to continue.
> 
> or maybe this is better
> 
> No active budget set! Please activate a budget first.
> 
> Thoughts?

Feel free to update my wordings as you want :)
Comment 21 Mark Tompsett 2015-05-22 16:21:12 UTC
(In reply to Kyle M Hall from comment #19)
> Thoughts?

I vaguely recall there were two differing cases which triggered this message, but both were different and would require different messages.
Comment 22 Jonathan Druart 2015-09-22 11:12:13 UTC Comment hidden (obsolete)
Comment 23 Bernardo Gonzalez Kriegel 2015-10-30 14:00:27 UTC
Sorry, does not apply anymore

Applying: Bug 4502: Catch software error if no active budget defined
Applying: Bug 4502: An attempt to make things more sensible.
fatal: sha1 information is lacking or useless (admin/aqplan.pl).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 4502: An attempt to make things more sensible.
Comment 24 Jonathan Druart 2015-11-05 12:21:18 UTC Comment hidden (obsolete)
Comment 25 Jonathan Druart 2015-11-05 12:21:23 UTC Comment hidden (obsolete)
Comment 26 Jonathan Druart 2015-11-05 12:21:27 UTC Comment hidden (obsolete)
Comment 27 Bernardo Gonzalez Kriegel 2015-11-17 18:29:31 UTC Comment hidden (obsolete)
Comment 28 Katrin Fischer 2015-11-18 08:44:01 UTC
Created attachment 44945 [details] [review]
[PASSED QA] Bug 4502: Catch software error if no active budget defined

TEST PLAN
---------
0) Back up your database.
1) In mysql client:
   > DROP DATABASE {your koha database name};
   > CREATE DATABASE {your koha database name};
   > QUIT;
2) Go to the staff client, and install all the default
   and optional things -- except patrons. :)
3) Log into staff client.
4) Create a patron -> New Patron -> Staff
5) Enter data and Save
6) More -> Set Permissions
7) Make superlibrarian
8) Log out
9) Log in as new superlibrarian
10) Acquisitions -> Budgets -> New Budget
11) Enter a non-active budget with some funds.
    -- Once saved, it should list in the inactive budgets.
12) Click on the name.
13) Click on one of the Planning submenu options.
14) Click the 'Submit' button in the Filter area.
    -- This should trigger the blow up.
15) Apply the patch
16) Repeat steps 12-15
    -- The kaboom is avoided and a nice message given.

NOTE: This does not solve all the problems in this ugly, ugly
      module area. It does solve the one thing it is meant to:
      that nasty kaboom.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

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

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 29 Katrin Fischer 2015-11-18 08:44:17 UTC
Created attachment 44946 [details] [review]
[PASSED QA] Bug 4502: An attempt to make things more sensible.

The reason the budget_period_id was not defined was because in
two cases it was not passed! This patch adds those missing
parameters. And as a result, cuts out the attempt to default the
authcat to '' unless the budget_period_id is defined.

Additionally, the start and end months don't seem to be passed,
so rather than have it blow up, checking them forces the else
case logic.

budget_period_id is the budget id. If you have two budgets,
you can craft a URL to work with budget_period_id matching
those two ids. Anything else should trigger the new error which
was modified to reflect more of what the problem is.

Follow the test plan in comment #6. Feel free to also to attempt
crafting URLs and triggering errors.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Well, works and does not explode any more
No errors

To trigger the new message simply put /cgi-bin/koha/admin/aqplan.pl
on your staff page, an intriguing 'Planning for by Asort1' appears :)

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 30 Katrin Fischer 2015-11-18 08:44:21 UTC
Created attachment 44947 [details] [review]
[PASSED QA] Bug 4502: Changing wording again...

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Just a string patch, others are signed.
No errors

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 31 Tomás Cohen Arazi 2015-11-19 23:26:30 UTC
Patches pushed to master.

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