|
Description
Katrin Fischer
2015-04-02 09:17:08 UTC
Are you talking about id/class attributes of <body> exclusively, or about other tags? Hi Frederic, thx for asking - I have clarified the title of the bug. I meant the <body> tag here. Several TT files have <body> without id/class. We need a test. Created attachment 37733 [details] [review] Bug 13941 [1/2] Fix <body> tags missing id/class Created attachment 37734 [details] [review] Bug 13941 [2/2] Fix <body> tags missing id/class Created attachment 37735 [details] [review] Bug 13941 [1/2] Test <body> tag with id/class attributes Test: - Apply the first patch [1/2] containg a new test checking <body> tags in all templates - Run the test: prove xt/tt_valid.t - You get a list of templates with invalid <body> tags. - Apply the second patch [2/2] fixing all invalid <body> tags. - Re-run the test: It passes. Comment on attachment 37735 [details] [review] Bug 13941 [1/2] Test <body> tag with id/class attributes Review of attachment 37735 [details] [review]: ----------------------------------------------------------------- ::: xt/tt_valid.t @@ +41,4 @@ > } > close $dh; > > +use YAML; Seems useless. @@ +56,5 @@ > + { > + description => '<body> tag with id and class attributes', > + check => sub { > + my ($self, $name, $token) = @_; > + my $_ = $token->{_string}; Hum... Sure about this? :) Comment on attachment 37734 [details] [review] Bug 13941 [2/2] Fix <body> tags missing id/class Review of attachment 37734 [details] [review]: ----------------------------------------------------------------- ::: koha-tmpl/intranet-tmpl/prog/en/modules/common/patron_search.tt @@ +135,4 @@ > </script> > > </head> > +<body id="common_patron_seach" id="common"> typo "search" ::: koha-tmpl/intranet-tmpl/prog/en/modules/course_reserves/add_items-step1.tt @@ +2,4 @@ > <title>Koha › Course reserves › Add items</title> > [% INCLUDE 'doc-head-close.inc' %] > </head> > +<body id="courses_items_step1" class="course"> why not couse_add_items_step1? ::: koha-tmpl/intranet-tmpl/prog/en/modules/course_reserves/add_items-step2.tt @@ +3,4 @@ > [% INCLUDE 'doc-head-close.inc' %] > </head> > > +<body id="courses_items_step2" class="course"> idem. ::: koha-tmpl/intranet-tmpl/prog/en/modules/course_reserves/course-details.tt @@ +32,4 @@ > > </head> > > +<body id="courses_details" class="course"> Shouldn't be courses_course_details? ::: koha-tmpl/intranet-tmpl/prog/en/modules/course_reserves/course-reserves.tt @@ +14,4 @@ > </script> > > </head> > +<body id="courses_reserves" class="course"> Shouldn't be courses_course_reserves? ::: koha-tmpl/intranet-tmpl/prog/en/modules/patron_lists/add-modify.tt @@ +19,4 @@ > > </head> > > +<body id="patlist_add_modify" class="pat patlist"> Shouldn't be patlist*s*? ::: koha-tmpl/intranet-tmpl/prog/en/modules/patron_lists/list.tt @@ +65,4 @@ > > </head> > > +<body id="patlist_list" class="pat patlist"> Shouldn't be patlist*s*? ::: koha-tmpl/intranet-tmpl/prog/en/modules/patron_lists/lists.tt @@ +24,4 @@ > > </head> > > +<body id="patlist_lists" class="pat patlist"> Shouldn't be patlist*s*? ::: koha-tmpl/intranet-tmpl/prog/en/modules/plugins/plugins-disabled.tt @@ +5,4 @@ > [% INCLUDE 'calendar.inc' %] > </head> > > +<body id="plugins_disbled" class="plugins"> typo Shouldn't be plugins_plugins_disabled? ::: koha-tmpl/intranet-tmpl/prog/en/modules/plugins/plugins-home.tt @@ +5,4 @@ > [% INCLUDE 'calendar.inc' %] > </head> > > +<body id="plugins_home" class="plugins"> Shouldn't be plugins_plugins_home? ::: koha-tmpl/intranet-tmpl/prog/en/modules/plugins/plugins-upload.tt @@ +5,4 @@ > [% INCLUDE 'calendar.inc' %] > </head> > > +<body id="plugins_upload" class="plugins"> Shouldn't be plugins_plugins_upload? Marked as Failed QA, at least for typo. (In reply to Jonathan Druart from comment #7) Thanks for testing. > Comment on attachment 37735 [details] [review] [review] > Bug 13941 [1/2] Test <body> tag with id/class attributes > > Review of attachment 37735 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: xt/tt_valid.t > @@ +41,4 @@ > > } > > close $dh; > > > > +use YAML; > > Seems useless. Sure. > @@ +56,5 @@ > > + { > > + description => '<body> tag with id and class attributes', > > + check => sub { > > + my ($self, $name, $token) = @_; > > + my $_ = $token->{_string}; > > Hum... Sure about this? :) Yes. Do you see anything wrong/strange? (In reply to Jonathan Druart from comment #8) > Comment on attachment 37734 [details] [review] [review] > Bug 13941 [2/2] Fix <body> tags missing id/class > > Review of attachment 37734 [details] [review] [review]: Your points are well taken. You have good eyesight. But this: > ::: koha-tmpl/intranet-tmpl/prog/en/modules/patron_lists/add-modify.tt > @@ +19,4 @@ > > > > </head> > > > > +<body id="patlist_add_modify" class="pat patlist"> > > Shouldn't be patlist*s*? I don't understand. (In reply to Frédéric Demians from comment #11) > (In reply to Jonathan Druart from comment #8) > > Comment on attachment 37734 [details] [review] [review] [review] > > Bug 13941 [2/2] Fix <body> tags missing id/class > > > > Review of attachment 37734 [details] [review] [review] [review]: > > Your points are well taken. You have good eyesight. > > But this: > > > ::: koha-tmpl/intranet-tmpl/prog/en/modules/patron_lists/add-modify.tt > > @@ +19,4 @@ > > > > > > </head> > > > > > > +<body id="patlist_add_modify" class="pat patlist"> > > > > Shouldn't be patlist*s*? > > I don't understand. The module is plural, not the id (patron_lists vs patlist). See plugins and tools. But if you prefer patlist, go for it :) (In reply to Frédéric Demians from comment #10) > (In reply to Jonathan Druart from comment #7) > > Thanks for testing. > > > Comment on attachment 37735 [details] [review] [review] [review] > > Bug 13941 [1/2] Test <body> tag with id/class attributes > > > > Review of attachment 37735 [details] [review] [review] [review]: > > ----------------------------------------------------------------- > > > > ::: xt/tt_valid.t > > @@ +41,4 @@ > > > } > > > close $dh; > > > > > > +use YAML; > > > > Seems useless. > > Sure. > > > @@ +56,5 @@ > > > + { > > > + description => '<body> tag with id and class attributes', > > > + check => sub { > > > + my ($self, $name, $token) = @_; > > > + my $_ = $token->{_string}; > > > > Hum... Sure about this? :) > > Yes. Do you see anything wrong/strange? At least a warning :) Use of my $_ is experimental at xt/tt_valid.t line 61. > > > Shouldn't be patlist*s*? > > > > I don't understand. > > The module is plural, not the id (patron_lists vs patlist). > See plugins and tools. > But if you prefer patlist, go for it :) I understand now your *s*. I wasn't reading it properly... I will keep it as it is if you don't mind. It's already a sort of shortcut. > Use of my $_ is experimental at xt/tt_valid.t line 61. Ok. I don't get it on Perl 5.14, but it's a valid warning. Created attachment 37759 [details] [review] Bug 13941 [2/2] Fix <body> tags missing id/class Created attachment 37760 [details] [review] Bug 13941 [1/2] Test <body> tag with id/class attributes Test: - Apply the first patch [1/2] containg a new test checking <body> tags in all templates - Run the test: prove xt/tt_valid.t - You get a list of templates with invalid <body> tags. - Apply the second patch [2/2] fixing all invalid <body> tags. - Re-run the test: It passes. Patch 2/2 does not apply on current master because koha-tmpl/opac-tmpl/bootstrap/en/modules/sco/receipt.tt no longer exists. Applying: Bug 13941 [2/2] Fix <body> tags missing id/class Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... error: refusing to lose untracked file at 'koha-tmpl/opac-tmpl/bootstrap/en/modules/sco/receipt.tt' CONFLICT (modify/delete): koha-tmpl/opac-tmpl/bootstrap/en/modules/sco/receipt.tt deleted in HEAD and modified in Bug 13941 [2/2] Fix <body> tags missing id/class. Version Bug 13941 [2/2] Fix <body> tags missing id/class of koha-tmpl/opac-tmpl/bootstrap/en/modules/sco/receipt.tt left in tree. Created attachment 37774 [details] [review] Bug 13941 [2/2] Fix <body> tags missing id/class Created attachment 37775 [details] [review] Bug 13941 [1/2] Test <body> tag with id/class attributes Test: - Apply the first patch [1/2] containg a new test checking <body> tags in all templates - Run the test: prove xt/tt_valid.t - You get a list of templates with invalid <body> tags. - Apply the second patch [2/2] fixing all invalid <body> tags. - Re-run the test: It passes. (In reply to Marc Véron from comment #16) > Patch 2/2 does not apply on current master because > koha-tmpl/opac-tmpl/bootstrap/en/modules/sco/receipt.tt no longer exists. Thanks. Conflict solved. Created attachment 37781 [details] [review] [Signed-off] Bug 13941 [1/2] Test <body> tag with id/class attributes Test: - Apply the first patch [1/2] containg a new test checking <body> tags in all templates - Run the test: prove xt/tt_valid.t - You get a list of templates with invalid <body> tags. - Apply the second patch [2/2] fixing all invalid <body> tags. - Re-run the test: It passes. Followed test plan. Works as expected. Signed-off-by: Marc Véron <veron@veron.ch> Created attachment 37782 [details] [review] [Signed-off] Bug 13941 [2/2] Fix <body> tags missing id/class Followed test plan (Comment #18), works as expected. Signed-off-by: Marc Véron <veron@veron.ch> Comment on attachment 37781 [details] [review] [Signed-off] Bug 13941 [1/2] Test <body> tag with id/class attributes Review of attachment 37781 [details] [review]: ----------------------------------------------------------------- ::: xt/tt_valid.t @@ +55,5 @@ > + description => '<body> tag with id and class attributes', > + check => sub { > + my ($self, $name, $token) = @_; > + return if $name =~ /bodytag\.inc/; > + $_ = $token->{_string}; Sorry but I still don't understand why this is useful. (In reply to Jonathan Druart from comment #22) > Comment on attachment 37781 [details] [review] [review] > [Signed-off] Bug 13941 [1/2] Test <body> tag with id/class attributes > > Review of attachment 37781 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: xt/tt_valid.t > @@ +55,5 @@ > > + description => '<body> tag with id and class attributes', > > + check => sub { > > + my ($self, $name, $token) = @_; > > + return if $name =~ /bodytag\.inc/; > > + $_ = $token->{_string}; > > Sorry but I still don't understand why this is useful. The sub complete code is: my ($self, $name, $token) = @_; return if $name =~ /bodytag\.inc/; $_ = $token->{_string}; push @{$self->{errors}->{$name}}, $token->{_lc} if /^<body/ && ! /id=".+"/ && ! /class=".+"/; The $_ is used in the last line which should have been coded like that without $_ assignation: if $token->{_string} =~ $/^<body/ && ! $token->{_string} =~ /id=".+"/ && ! $token->{_string} =~ /class=".+"/; Hope this explanation doesn't obfuscate more something which isn't already that clear. (In reply to Frédéric Demians from comment #23) > (In reply to Jonathan Druart from comment #22) > > Comment on attachment 37781 [details] [review] [review] [review] > > [Signed-off] Bug 13941 [1/2] Test <body> tag with id/class attributes > > > > Review of attachment 37781 [details] [review] [review] [review]: > > ----------------------------------------------------------------- > > > > ::: xt/tt_valid.t > > @@ +55,5 @@ > > > + description => '<body> tag with id and class attributes', > > > + check => sub { > > > + my ($self, $name, $token) = @_; > > > + return if $name =~ /bodytag\.inc/; > > > + $_ = $token->{_string}; > > > > Sorry but I still don't understand why this is useful. > > The sub complete code is: > > my ($self, $name, $token) = @_; > return if $name =~ /bodytag\.inc/; > $_ = $token->{_string}; > push @{$self->{errors}->{$name}}, $token->{_lc} > if /^<body/ && ! /id=".+"/ && ! /class=".+"/; > > The $_ is used in the last line which should have been coded like that > without $_ assignation: > > if $token->{_string} =~ $/^<body/ && > ! $token->{_string} =~ /id=".+"/ && > ! $token->{_string} =~ /class=".+"/; > > Hope this explanation doesn't obfuscate more something which isn't already > that clear. Sorry, read to quickly the push statement. I personally think it's a bad idea to assign the default variable, but I don't want to impose that. Without the second patch applied, I get: % prove xt/tt_valid.t xt/tt_valid.t .. Can't use string ("</head>") as a HASH ref while "strict refs" in use at xt/tt_valid.t line 60. # Looks like your test exited with 255 before it could output anything. xt/tt_valid.t .. Dubious, test returned 255 (wstat 65280, 0xff00) Failed 3/3 subtests Test Summary Report ------------------- xt/tt_valid.t (Wstat: 65280 Tests: 0 Failed: 0) Non-zero exit status: 255 Parse errors: Bad plan. You planned 3 tests but ran 0. Files=1, Tests=0, 0 wallclock secs ( 0.02 usr 0.00 sys + 0.10 cusr 0.00 csys = 0.12 CPU) Result: FAIL If I apply the second one, and remove an id from a body tag: % git diff diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basketgroup.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basketgroup.tt index 74bedcb..ffd1c1b 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basketgroup.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basketgroup.tt @@ -123,7 +123,7 @@ function submitForm(form) { //]]> </script> </head> -<body id="acq_basketgroup" class="acq"> +<body class="acq"> [% INCLUDE 'header.inc' %] [% INCLUDE 'acquisitions-search.inc' %] % prove xt/tt_valid.t xt/tt_valid.t .. ok All tests successful. Files=1, Tests=3, 2 wallclock secs ( 0.02 usr 0.00 sys + 2.76 cusr 0.00 csys = 2.78 CPU) Result: PASS Shouldn't it fail? (In reply to Jonathan Druart from comment #24) > Sorry, read to quickly the push statement. > I personally think it's a bad idea to assign the default variable, but I > don't want to impose that. I agree with you. Since I have to fix the patch due to you other comment, I will stop using default variable which doesn't add readability in these case. > Without the second patch applied, I get: > % prove xt/tt_valid.t > xt/tt_valid.t .. Can't use string ("</head>") as a HASH ref while "strict > refs" in use at xt/tt_valid.t line 60. It comes from fixing the warning you reported. I will send new version of the UT. > Shouldn't it fail? Yes, it should. Created attachment 37823 [details] [review] Bug 13941 [2/2] Fix <body> tags missing id/class Created attachment 37824 [details] [review] Bug 13941 [1/2] Test <body> tag with id/class attributes Test: - Apply the first patch [1/2] containing a new test checking <body> tags in all templates - Run the test: prove xt/tt_valid.t - You get a list of templates with invalid <body> tags. - Apply the second patch [2/2] fixing all invalid <body> tags. - Re-run the test: It passes. Created attachment 37896 [details] [review] [Signed-off] Bug 13941 [1/2] Test <body> tag with id/class attributes Test: - Apply the first patch [1/2] containing a new test checking <body> tags in all templates - Run the test: prove xt/tt_valid.t - You get a list of templates with invalid <body> tags. - Apply the second patch [2/2] fixing all invalid <body> tags. - Re-run the test: It passes. Folowed test plan, works as expected Signed-off-by: Marc Véron <veron@veron.ch> Created attachment 37897 [details] [review] [Signed-off] Bug 13941 [2/2] Fix <body> tags missing id/class Followed test plan from patch 1/2, works as expected. Signed-off-by: Marc Véron <veron@veron.ch> Comment on attachment 37897 [details] [review] [Signed-off] Bug 13941 [2/2] Fix <body> tags missing id/class Review of attachment 37897 [details] [review]: ----------------------------------------------------------------- ::: koha-tmpl/intranet-tmpl/prog/en/modules/acqui/addorder.tt @@ +3,4 @@ > [% INCLUDE "doc-head-close.inc" %] > </head> > > +<body id="a" class="acq"> Typo? I think should be 'acq_addorder'. @@ +7,5 @@ > [% INCLUDE "header.inc" %] > > <div id="doc3" class="yui-t7"> > > +<div id="acq_addorder" class="acq"> This change doesn't look right - see above. Created attachment 37927 [details] [review] [PASSED QA] Bug 13941 [1/2] Test <body> tag with id/class attributes Test: - Apply the first patch [1/2] containing a new test checking <body> tags in all templates - Run the test: prove xt/tt_valid.t - You get a list of templates with invalid <body> tags. - Apply the second patch [2/2] fixing all invalid <body> tags. - Re-run the test: It passes. Folowed test plan, works as expected Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 37929 [details] [review] [PASSED QA] Bug 13941 [2/2] Fix <body> tags missing id/class Followed test plan from patch 1/2, works as expected. Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 37930 [details] [review] Bug 13941: QA Follow-up - fix for acqui/addorder.tt I have written a follow-up for the problem described in the last comment - please check. (In reply to Katrin Fischer from comment #34) > I have written a follow-up for the problem described in the last comment - > please check. Looks good to me. I suspect other templates may have inconsistent id/class with their file name and module. I thought about improving the UT to detect that cases, but changing id/class that are here since a while may broke local JavaScript developed by libraries. Patches pushed to master. Thanks Frederic! |