Bug 10442 - Remove references to non-standard "error" class
Summary: Remove references to non-standard "error" class
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Owen Leonard
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-06-10 14:14 UTC by Owen Leonard
Modified: 2020-01-06 20:14 UTC (History)
8 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 10442 - Remove references to non-standard "error" class (13.37 KB, patch)
2013-06-10 14:44 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 10442 - Remove references to non-standard "error" class (13.52 KB, patch)
2013-06-29 06:16 UTC, Liz Rea
Details | Diff | Splinter Review
[PASSED QA] Bug 10442 - Remove references to non-standard "error" class (13.84 KB, patch)
2013-06-30 10:01 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 10442: Remove references to non-standard "error" class (10.91 KB, patch)
2018-03-13 14:50 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 10442: Remove references to non-standard "error" class (10.78 KB, patch)
2018-07-16 18:48 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 10442: Remove references to non-standard "error" class (10.81 KB, patch)
2018-08-13 13:51 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 10442: Remove references to non-standard "error" class (10.86 KB, patch)
2018-09-04 16:10 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 10442: Remove references to non-standard "error" class (10.98 KB, patch)
2018-10-30 11:33 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 10442: Remove references to non-standard "error" class (11.04 KB, patch)
2018-10-31 06:59 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-06-10 14:14:25 UTC
There are a few references in the templates to a very old "error" class which should not be used anymore. Generally speaking '<div class="dialog alert">' should be used instead.
Comment 1 Owen Leonard 2013-06-10 14:44:59 UTC Comment hidden (obsolete)
Comment 2 Srdjan Jankovic 2013-06-11 02:59:54 UTC
I'm not sure that replacing & with &amp; in href was in order
Comment 3 Kyle M Hall 2013-06-14 12:00:12 UTC
Owen, can you please address Srjdan's comment 2?
Comment 4 Owen Leonard 2013-06-25 13:11:23 UTC
(In reply to Srdjan Jankovic from comment #2)
> I'm not sure that replacing & with &amp; in href was in order

While not strictly within the scope of the bug, unescaped ampersands are a violation of coding guidelines (invalid HTML) and should be corrected.
Comment 5 Liz Rea 2013-06-29 06:16:04 UTC Comment hidden (obsolete)
Comment 6 Katrin Fischer 2013-06-30 10:01:04 UTC Comment hidden (obsolete)
Comment 7 Galen Charlton 2013-07-01 01:56:24 UTC
Comment on attachment 19296 [details] [review]
[PASSED QA] Bug 10442 - Remove references to non-standard "error" class

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

::: koha-tmpl/intranet-tmpl/prog/en/modules/acqui/neworderempty.tt
@@ -171,5 @@
>      [% END %]
>  </h2>
>  
> -<div class="error" style="display:none"></div>
> -

I'm not sure whether simply removing this is OK.  There is code in additem.js that looks for the existence of a div whose CSS class is "error"; if such a div is not present, an error message (e.g., when trying to add a duplicate item) would have no place to be displayed.

Setting bug back to "Signed-off" for QA to look into this.  If I'm right, additem.js will need to be modified, e.g., to use a div ID rather than a div class to locate the placeholder for an error message.

It may be necessary to apply the patch for bug 10483 to be able to test a situation where attempting to add an item during the process of creating a new order results in an error.
Comment 8 Srdjan Jankovic 2013-07-10 01:03:16 UTC
> While not strictly within the scope of the bug, unescaped ampersands are a
> violation of coding guidelines (invalid HTML) and should be corrected.

Even in href? I'm not sure about that.
Comment 9 Owen Leonard 2018-03-13 14:50:47 UTC Comment hidden (obsolete)
Comment 10 Katrin Fischer 2018-07-08 16:41:07 UTC
Patch doesn't apply, can you please rebase?
Comment 11 Owen Leonard 2018-07-16 18:48:32 UTC Comment hidden (obsolete)
Comment 12 Owen Leonard 2018-08-13 13:51:36 UTC Comment hidden (obsolete)
Comment 13 Owen Leonard 2018-09-04 16:10:04 UTC Comment hidden (obsolete)
Comment 14 Andrew Isherwood 2018-10-30 11:33:41 UTC Comment hidden (obsolete)
Comment 15 Katrin Fischer 2018-10-31 06:59:07 UTC
Created attachment 81646 [details] [review]
Bug 10442: Remove references to non-standard "error" class

The CSS for <div class="error"> is obsolete and should not be used. This
patch removes the definition from the main CSS file and corrects
instances of its use in the templates to the standard <div class="dialog
alert">.

To test:

- In Acquisitions -> Late orders, locate an order from a vendor which
  doesn't have an email address. Selecting that order and clicking
  "Claim" should trigger an error dialog, "This vendor has no email." It
  should be styled correctly.

- With  AcqCreateItem set to "when placing an order," add to an existing
  order using the "From a new (empty) record" option. Add two items with
  identical barcodes and submit the form. A error should show at
  the top of the page.

- With AcqCreateItem set to "when receiving an order," receive an order
  and add two items with identical barcodes. Submitting the form should
  trigger an error message at the top of the page.

With the remaining cases I don't know how to trigger the errors in
question, so a visual check of the changes may be required:

- Administration -> Funds -> "You are not authorized to modify this
  fund"
- Administration -> Search engine configuration
  (/admin/searchengine/elasticsearch/mappgings.pl) -> Various
  modification errors.
- With the  AutoEmailOpacUser preference set to "send," adding a patron
  without an email address can trigger an error, "This member has no
  email."
- With plugins enabled, and installed, there are error messages
  displayed under various circumstances.

Signed-off-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 16 Nick Clemens 2018-10-31 15:02:20 UTC
Awesome work all!

Pushed to master for 18.11
Comment 17 Martin Renvoize 2018-11-09 15:53:57 UTC
Pushed to 18.05.x for 18.05.06
Comment 18 Fridolin Somers 2018-11-28 13:33:37 UTC
Pushed to 17.11.x for 17.11.13