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.
Created attachment 27152 [details] [review] Bug 12091 - Ugly HTML corrects for /cgi-bin/koha/admin/auth_subfields_structure.pl Untested. I know it cleans up the HTML, but with the renaming of things and the like, it needs to be thoroughly tested still.
Working on a rebase because of bug 11508.
Created attachment 27674 [details] [review] Bug 12091 - Rebased, but untested still. This cleans up the HTML in auth_subfields_structure.pl by: - removing some more CGI::scrolling_lists. - fixing some attributes - removing some attributes - and using a handy-dandy validation plug-in that Owen pointed me at the other day.
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 :)
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. :)
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.
Created attachment 30767 [details] [review] Bug 12091 - Part that should work This cleans up the HTML in auth_subfields_structure.pl and auth_subfields_structure.tt by: - fixing some attributes - removing some attributes - cleaning up list building code - 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!
Created attachment 30768 [details] [review] Bug 12091 - Part that should work 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!
Created attachment 30773 [details] [review] Bug 12091 - Part that should work 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!
Created attachment 30774 [details] [review] Bug 12091 - Part that should work 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!
Maybe 11 errors / 11 warnings, lot less than before. :)
Created attachment 30778 [details] [review] [SIGNED-OFF] 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
Created attachment 30779 [details] [review] Bug 12091: Followup to fix remaining validation errors This patch fix all remaining errors. There was a for clause to add multiple new subfields, perhaps old code because now we can only add one subfield at a time, in that block id of html elements where numbered again from '1', and some elements lack proper id numbering, and one of them has incorrect label. I notice this on another patch but left things as they were trying not to broke anything. To test 1. Follow test plan of previous patch, there must be no errors, 1 warning 2. Add a new subfield, check subfield is added without problems
(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.
I recall fixing that ties into saving problems. It was a big unraveling mess. I will confirm and test later.
(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.
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.
Patch pushed to master. Thanks Mark!