Description
Owen Leonard
2013-06-10 14:14:25 UTC
Created attachment 18824 [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">. This patch also corrects some minor HTML validity and capitalization errors in the changed templates. Testing each error condition in all the modified temlates is probably overkill for the minor changes made in this patch. Markup has simply been changed to use alternate existing styles. Visual confirmation of the markup changes is probably sufficient. I'm not sure that replacing & with & in href was in order Owen, can you please address Srjdan's comment 2? (In reply to Srdjan Jankovic from comment #2) > I'm not sure that replacing & with & 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. Created attachment 19281 [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">. This patch also corrects some minor HTML validity and capitalization errors in the changed templates. Testing each error condition in all the modified temlates is probably overkill for the minor changes made in this patch. Markup has simply been changed to use alternate existing styles. Visual confirmation of the markup changes is probably sufficient. Signed-off-by: Liz Rea <wizzyrea@gmail.com> confirmed the changes - no obvious errors, functionality seems fine. Created attachment 19296 [details] [review] [PASSED QA] 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">. This patch also corrects some minor HTML validity and capitalization errors in the changed templates. Testing each error condition in all the modified temlates is probably overkill for the minor changes made in this patch. Markup has simply been changed to use alternate existing styles. Visual confirmation of the markup changes is probably sufficient. Signed-off-by: Liz Rea <wizzyrea@gmail.com> confirmed the changes - no obvious errors, functionality seems fine. Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Passes all tests and QA script. I read the changes and did a few tests in Koha. Some of the error conditions are pretty hard to reproduce, I did manage for lateorders, but didn't succeed for memberentrygen and parcel. But changes appear to be save and I spotted no problems, so passing QA. Patch also fixes a few capitalization errors. 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.
> 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.
Created attachment 72785 [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. Patch doesn't apply, can you please rebase? Created attachment 77022 [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. Created attachment 77742 [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. Created attachment 78413 [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. Created attachment 81615 [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> 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> Awesome work all! Pushed to master for 18.11 Pushed to 18.05.x for 18.05.06 Pushed to 17.11.x for 17.11.13 |