Some time ago we added ids and classes to all the pages in the staff client to make it easier to customize. The newly added modules are missing those and we should add them. So far I noticed they are missing on the course reserves pages, but a search would probably turn up some more.
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!