|
Description
Chris Cormack
2010-05-21 01:27:56 UTC
Created attachment 37181 [details] [review] Bug 4502: Catch software error if no active budget defined To reproduce: 0/ On a fresh install (so no budget defined), create a budget and don't mark it as active. 1/ Go on the budget detail page > Plannin > By months 2/ Click on the 'Select' button (on the left) Without this patch, you get a software error: Usage: Date::Calc::Delta_YMD(year1, month1, day1, year2, month2, day2) With this patch, you get a sweet alert message inviting you to create an active budget. Note: This script is a mess... 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? 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? Created attachment 37203 [details] [review] Bug 4502: Catch software error if no active budget defined To reproduce: 0/ On a fresh install (so no budget defined), create a budget and don't mark it as active. 1/ Go on the budget detail page > Plannin > By months 2/ Click on the 'Select' button (on the left) Without this patch, you get a software error: Usage: Date::Calc::Delta_YMD(year1, month1, day1, year2, month2, day2) With this patch, you get a sweet alert message inviting you to create an active budget. Note: This script is a mess... (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. Created attachment 37354 [details] [review] 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> 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? (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. 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 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'? Not sure if that makes sense... asort1 might not even be an existing authorised value. Created attachment 37533 [details] [review] 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. Created attachment 37534 [details] [review] 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. How about these two together, Katrin? Created attachment 39081 [details] [review] [SIGNED-OFF] 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> Created attachment 39082 [details] [review] [SIGNED-OFF] 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 :) Created attachment 39083 [details] [review] [SIGNED-OFF] 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> Created attachment 39084 [details] [review] [SIGNED-OFF] 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 :) 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? (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 :) (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. Created attachment 42771 [details] [review] Bug 4502: Changing wording again... 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. Created attachment 44498 [details] [review] 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> Created attachment 44499 [details] [review] 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 :) Created attachment 44500 [details] [review] Bug 4502: Changing wording again... Created attachment 44932 [details] [review] [SIGNED-OFF] Bug 4502: Changing wording again... Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Just a string patch, others are signed. No errors 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> 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> 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> Patches pushed to master. Thanks Jonathan and Mark! This patch has been pushed to 3.20.x, will be in 3.20.6. Pushed to 3.18.13, and released. |