Bug 9924 - Simplify and rename patron card creator error message include
Summary: Simplify and rename patron card creator error message include
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Owen Leonard
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-03-25 16:20 UTC by Owen Leonard
Modified: 2014-12-07 20:02 UTC (History)
2 users (show)

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


Attachments
Bug 9924 - Simplify and rename patron card creator error message include (12.27 KB, patch)
2013-03-25 16:53 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 9924 - Simplify and rename patron card creator error message include (12.24 KB, patch)
2013-04-12 16:22 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 9924 - Simplify and rename patron card creator error message include (12.23 KB, patch)
2013-08-21 16:39 UTC, Owen Leonard
Details | Diff | Splinter Review
[PASSED QA] Bug 9924 - Simplify and rename patron card creator error message include (12.29 KB, patch)
2013-08-25 17:00 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 9924: (follow-up) ensure that correct error message is displayed (7.17 KB, patch)
2013-09-08 07:40 UTC, Galen Charlton
Details | Diff | Splinter Review
[PASSED QA] Bug 9924: (follow-up) ensure that correct error message is displayed (7.57 KB, patch)
2013-09-08 08:04 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Owen Leonard 2013-03-25 16:20:36 UTC
There is an include file, error-messages.inc, which is used by a few patron card creator scripts. It uses some custom JavaScript to create a closable dialog for showing error messages. This can more simply be implemented as a standard error message box and provide a more consistent experience to users.

In addition, the include's name "error-messages.inc" doesn't properly convey that it is used exclusively by patron card creator pages.
Comment 1 Owen Leonard 2013-03-25 16:53:15 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2013-04-12 16:22:55 UTC Comment hidden (obsolete)
Comment 3 Katrin Fischer 2013-06-16 09:07:44 UTC
Hi Owen,
not sure if it's a false positive, but the QA script found something to complain about:

 * koha-tmpl/intranet-tmpl/prog/en/modules/patroncards/edit-batch.tt        FAIL
	forbidden patterns          OK
	tt_valid                    OK
	valid_template              FAIL
		patroncards-batches-toolbar.inc: not found

Could you please take a look?
Comment 4 Owen Leonard 2013-08-05 14:08:46 UTC
Could you look at this again Katrin? I'm not getting any errors from the QA script when I run it.
Comment 5 Katrin Fischer 2013-08-17 22:11:31 UTC
Hm this is weird, I am still getting the error running the latest version of the QA script. Trying to see what is causing it.
Comment 6 Katrin Fischer 2013-08-17 22:29:00 UTC
Comment on attachment 17404 [details] [review]
Bug 9924 - Simplify and rename patron card creator error message include

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

::: koha-tmpl/intranet-tmpl/prog/en/modules/patroncards/edit-batch.tt
@@ +152,4 @@
>                  </div>
>  
>  
> +                    [% INCLUDE 'patroncards-batches-toolbar.inc' %]

Hi Owen,

I think it's this line - and I can't spot a file with this name in the includes directory?
Comment 7 Owen Leonard 2013-08-21 16:39:45 UTC Comment hidden (obsolete)
Comment 8 Owen Leonard 2013-08-21 16:47:26 UTC
(In reply to Katrin Fischer from comment #6)
> > +                    [% INCLUDE 'patroncards-batches-toolbar.inc' %]

I think this was caused by a merge error following Bug 9697. I have corrected the patch but kept the signoff. I hope that's okay.
Comment 9 Katrin Fischer 2013-08-25 17:00:09 UTC
Created attachment 20590 [details] [review]
[PASSED QA] Bug 9924 - Simplify and rename patron card creator error message include

The patron card creator error message include uses a non-standard method
for displaying error messages, and is poorly-named.

This patch converts the method of displaying error messages for various
patron card creator options to the standard one ('<div class="dialog
alert">') and renames the include file to make it clear that it relates
only to patron card creator operations.

To test, perform various operations:

- Go to 'manage images' and try to upload a file which exceeds the
  500KBfile size limit
- Go to the edit batch page and manually append an error code to the
  URL:  /cgi-bin/koha/patroncards/edit-batch.pl?op=new&error=403
- Go to one of the manage pages and manually append an error code to the
  URL:
  /cgi-bin/koha/patroncards/manage.pl?card_element=profile&error=201

Correct display of an error message indicates that the include file is
being found.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Passes test plan, test suite and QA script.
Comment 10 Galen Charlton 2013-09-08 07:40:29 UTC Comment hidden (obsolete)
Comment 11 Galen Charlton 2013-09-08 07:42:03 UTC
In the course of testing this, I discovered that the patron card errors were broken, probably since the switch to Template Toolkit -- numeric template variable names are not a Good Thing.  The follow-up patch I just attached tries to fix this.

Setting to signed off for QA review of the follow-up.
Comment 12 Katrin Fischer 2013-09-08 08:04:26 UTC
Created attachment 20875 [details] [review]
[PASSED QA] Bug 9924: (follow-up) ensure that correct error message is displayed

(Apparently) unlike HTML::Template::Pro, Template Toolkit doesn't like
template variables that are entirely numeric -- in conditionals, it
consider them integers, most of which are Perl true.

This patch changes this by setting the error variable to the error
value.

To test:

[1] Run the test plan from the previous patch.  In each
    case, verify that the error message is specifically applicable
    to the test.  For example, if you try uplaoading a patron image
    that is larger than 500KB, the error message displayed should
    specifically say so.

Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Thank you Galen for catching this. Error messages showing up
now are much more specific and according to the error codes given.
I testd uploading a file larger then 500KB and triggered several
error messages giving the error code in the URL:
/cgi-bin/koha/patroncards/manage.pl?card_element=profile&error=201

All tests and QA script pass.
Comment 13 Galen Charlton 2013-09-08 19:20:26 UTC
Pushed to master.  Thanks, Owen!