Bug 12091 - HTML from auth_subfields_structure.tt doesn't pass firefox's html validator
Summary: HTML from auth_subfields_structure.tt doesn't pass firefox's html validator
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Staff interface (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Mark Tompsett
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-04-15 23:45 UTC by Mark Tompsett
Modified: 2015-06-04 23:33 UTC (History)
4 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 12091 - Ugly HTML corrects for /cgi-bin/koha/admin/auth_subfields_structure.pl (18.38 KB, patch)
2014-04-15 23:47 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 12091 - Rebased, but untested still. (15.59 KB, patch)
2014-04-27 04:27 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 12091 - Part that should work (10.73 KB, patch)
2014-08-13 17:05 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 12091 - Part that should work (10.71 KB, patch)
2014-08-13 17:08 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 12091 - Part that should work (10.78 KB, patch)
2014-08-13 20:33 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 12091 - Part that should work (11.38 KB, patch)
2014-08-13 21:15 UTC, Mark Tompsett
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 12091: HTML from auth_subfields_structure.tt doesn't pass firefox's html validator (11.57 KB, patch)
2014-08-14 00:11 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 12091: Followup to fix remaining validation errors (3.19 KB, patch)
2014-08-14 00:11 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[PASSED QA] Bug 12091: HTML from auth_subfields_structure.tt doesn't pass firefox's html validator (11.79 KB, patch)
2014-08-15 10:28 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 Mark Tompsett 2014-04-15 23:45:24 UTC
After installing the 0.9.5.8 HTML Validator plugin for Firefox, as suggested by Owen, I noticed a lot of errors for /cgi-bin/koha/admin/auth_subfields_structure.pl while testing another bug. The goal of this patch is to bring the validations errors to as close to 0 as possible. I think my untested patch gets 2 warnings. I have to confirm that the screen still functions properly.
Comment 1 Mark Tompsett 2014-04-15 23:47:33 UTC Comment hidden (obsolete)
Comment 2 Mark Tompsett 2014-04-27 00:32:02 UTC
Working on a rebase because of bug 11508.
Comment 3 Mark Tompsett 2014-04-27 04:27:24 UTC Comment hidden (obsolete)
Comment 4 Bernardo Gonzalez Kriegel 2014-04-27 23:49:30 UTC
Mark, no problem to test but...

1. Test plan is missing
2. I think that we must try (fervently) not to put HTML in scripts
So, this kind of construct

$row_data{repeatable} = "<input id=\"repeatable$i\" type=\"checkbox\" " .
      ( $data->{'repeatable'} ? 'checked="checked" ' : '') .
      "name=\"repeatable$i\">

it's calling for a FaliedQA :(

Perhaps can be replaced by

(in script)
$row_data{repeatable} = {
 id => repeatable$i,
 name => repeatable$i,
 repeatable => $data->{'repeatable'},
}

(in tt)
[% IF ( ...row.repeatable ) %]
[% IF ( ...row.repeatable.repeatable ) %]
<input id="[%- ...row.repeatable.id -%]" type="checkbox" name="[%- ...row.repeatable.name -%]" checked="checked">
[% ELSE %]
<input id="[%- ...row.repeatable.id -%]" type="checkbox" name="[%- ...row.repeatable.name -%]">
[% END %]
[% END %]

or something similar
3. Bug status, it's ready to be signed? Then change that :)
Comment 5 Mark Tompsett 2014-04-28 02:39:54 UTC
That's why the name of the patch is untested and the status is still new. And yes, after getting it validating HTML-wise, I realized that HTML in scripts is bad. As for a test plan, trust me, it will be lengthy and thorough when it is finished being tested so as to make testing very simple. I'm just using bugzilla as my offsite backup, so I can do things like "git reset --hard origin/master" and not worry about losing the patch. :)
Comment 6 Mark Tompsett 2014-08-13 16:56:37 UTC
Well, another bug that Bernardo Gonzalez Kriegel cleaned up (bug 12614) made me have to rework this. I'm still concerned about the duplicate ID stuff, and if the data is actually being saved properly.
Comment 7 Mark Tompsett 2014-08-13 17:05:32 UTC Comment hidden (obsolete)
Comment 8 Mark Tompsett 2014-08-13 17:08:19 UTC Comment hidden (obsolete)
Comment 9 Mark Tompsett 2014-08-13 20:33:26 UTC Comment hidden (obsolete)
Comment 10 Mark Tompsett 2014-08-13 21:15:09 UTC Comment hidden (obsolete)
Comment 11 Mark Tompsett 2014-08-13 21:15:51 UTC
Maybe 11 errors / 11 warnings, lot less than before. :)
Comment 12 Bernardo Gonzalez Kriegel 2014-08-14 00:11:20 UTC Comment hidden (obsolete)
Comment 13 Bernardo Gonzalez Kriegel 2014-08-14 00:11:36 UTC Comment hidden (obsolete)
Comment 14 Bernardo Gonzalez Kriegel 2014-08-14 00:12:56 UTC
(In reply to M. Tompsett from comment #11)
> Maybe 11 errors / 11 warnings, lot less than before. :)

Mark,
I signed your patch and added a followup, I you can please test it.
Comment 15 Mark Tompsett 2014-08-14 01:00:19 UTC
I recall fixing that ties into saving problems. It was a big unraveling mess. I will confirm and test later.
Comment 16 Bernardo Gonzalez Kriegel 2014-08-14 10:44:53 UTC
(In reply to M. Tompsett from comment #15)
> I recall fixing that ties into saving problems. It was a big unraveling
> mess. I will confirm and test later.

Ok, forget about it.
Comment 17 Katrin Fischer 2014-08-15 10:28:32 UTC
Created attachment 30826 [details] [review]
[PASSED QA] Bug 12091: HTML from auth_subfields_structure.tt doesn't pass firefox's html validator

This cleans up the HTML in auth_subfields_structure.pl and
auth_subfields_structure.tt by:
- fixing some attributes
- removing some attributes
- and using a handy-dandy validation plug-in that Owen pointed
  me at the other day.

TEST PLAN
---------
 1) Install HTML Validator Plugin (Html Validator 0.9.5.8 Firefox addon)
 2) Log in to staff client
 3) Koha administration
 4) Authority types (on the right, 5 down from Catalog header)
 5) Click 'MARC structure' of any auth type
 6) Click 'subfields' (I believe for any Tag >= 010)
 7) Click 'Edit subfields'
 8) Right click and select 'View Page Source'
    -- Lots of HTML validation errors
 9) View the 'Authorized value:' and 'Thesaurus:' drop down lists.
    -- Patch cleans up code, so lists should remain unchanged
       after applying patch
10) Apply the patch
11) Refresh the page (make sure it isn't cached!)
12) Right click and select 'View Page Source'
    -- It should be down to 10 errors / 11 warnings.
       NOTE: I think this means there is a code problem with
             actually editing authorities properly!

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Work as described, much less validation errors, no koha-qa errors

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
CÃleans up code, also removes SQL in favor of using a method.
No regressions found.
Comment 18 Tomás Cohen Arazi 2014-08-15 18:00:10 UTC
Patch pushed to master.

Thanks Mark!