Bug 13941 - Add ids to <body> tag in staff client where missing
Summary: Add ids to <body> tag in staff client where missing
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Frédéric Demians
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-04-02 09:17 UTC by Katrin Fischer
Modified: 2015-12-03 22:11 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 13941 [1/2] Fix <body> tags missing id/class (28.85 KB, patch)
2015-04-13 12:49 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 13941 [2/2] Fix <body> tags missing id/class (28.85 KB, patch)
2015-04-13 12:50 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 13941 [1/2] Test <body> tag with id/class attributes (3.53 KB, patch)
2015-04-13 12:50 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 13941 [2/2] Fix <body> tags missing id/class (28.35 KB, patch)
2015-04-13 15:16 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 13941 [1/2] Test <body> tag with id/class attributes (3.59 KB, patch)
2015-04-13 15:16 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 13941 [2/2] Fix <body> tags missing id/class (27.83 KB, patch)
2015-04-13 18:04 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 13941 [1/2] Test <body> tag with id/class attributes (3.59 KB, patch)
2015-04-13 18:04 UTC, Frédéric Demians
Details | Diff | Splinter Review
[Signed-off] Bug 13941 [1/2] Test <body> tag with id/class attributes (3.78 KB, patch)
2015-04-13 20:31 UTC, Marc Véron
Details | Diff | Splinter Review
[Signed-off] Bug 13941 [2/2] Fix <body> tags missing id/class (28.03 KB, patch)
2015-04-13 20:35 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 13941 [2/2] Fix <body> tags missing id/class (35.18 KB, patch)
2015-04-14 14:30 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 13941 [1/2] Test <body> tag with id/class attributes (4.17 KB, patch)
2015-04-14 14:30 UTC, Frédéric Demians
Details | Diff | Splinter Review
[Signed-off] Bug 13941 [1/2] Test <body> tag with id/class attributes (4.35 KB, patch)
2015-04-15 10:18 UTC, Marc Véron
Details | Diff | Splinter Review
[Signed-off] Bug 13941 [2/2] Fix <body> tags missing id/class (35.38 KB, patch)
2015-04-15 10:20 UTC, Marc Véron
Details | Diff | Splinter Review
[PASSED QA] Bug 13941 [1/2] Test <body> tag with id/class attributes (4.41 KB, patch)
2015-04-15 21:34 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 13941 [2/2] Fix <body> tags missing id/class (35.43 KB, patch)
2015-04-15 21:35 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 13941: QA Follow-up - fix for acqui/addorder.tt (977 bytes, patch)
2015-04-15 21:35 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 Katrin Fischer 2015-04-02 09:17:08 UTC
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.
Comment 1 Frédéric Demians 2015-04-13 09:35:17 UTC
Are you talking about id/class attributes of <body> exclusively, or about other tags?
Comment 2 Katrin Fischer 2015-04-13 10:00:53 UTC
Hi Frederic, thx for asking - I have clarified the title of the bug.
I meant the <body> tag here.
Comment 3 Frédéric Demians 2015-04-13 12:08:39 UTC
Several TT files have <body> without id/class. We need a test.
Comment 4 Frédéric Demians 2015-04-13 12:49:26 UTC Comment hidden (obsolete)
Comment 5 Frédéric Demians 2015-04-13 12:50:41 UTC Comment hidden (obsolete)
Comment 6 Frédéric Demians 2015-04-13 12:50:51 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2015-04-13 13:55:57 UTC
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 8 Jonathan Druart 2015-04-13 14:02:36 UTC
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 &rsaquo; Course reserves &rsaquo; 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?
Comment 9 Jonathan Druart 2015-04-13 14:03:22 UTC
Marked as Failed QA, at least for typo.
Comment 10 Frédéric Demians 2015-04-13 14:14:33 UTC
(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?
Comment 11 Frédéric Demians 2015-04-13 14:24:27 UTC
(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.
Comment 12 Jonathan Druart 2015-04-13 14:51:28 UTC
(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.
Comment 13 Frédéric Demians 2015-04-13 15:13:16 UTC
> > > 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.
Comment 14 Frédéric Demians 2015-04-13 15:16:29 UTC Comment hidden (obsolete)
Comment 15 Frédéric Demians 2015-04-13 15:16:50 UTC Comment hidden (obsolete)
Comment 16 Marc Véron 2015-04-13 17:42:59 UTC
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.
Comment 17 Frédéric Demians 2015-04-13 18:04:18 UTC Comment hidden (obsolete)
Comment 18 Frédéric Demians 2015-04-13 18:04:38 UTC Comment hidden (obsolete)
Comment 19 Frédéric Demians 2015-04-13 18:05:26 UTC
(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.
Comment 20 Marc Véron 2015-04-13 20:31:43 UTC Comment hidden (obsolete)
Comment 21 Marc Véron 2015-04-13 20:35:20 UTC Comment hidden (obsolete)
Comment 22 Jonathan Druart 2015-04-14 10:56:47 UTC
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.
Comment 23 Frédéric Demians 2015-04-14 12:07:58 UTC
(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.
Comment 24 Jonathan Druart 2015-04-14 13:22:18 UTC
(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?
Comment 25 Frédéric Demians 2015-04-14 13:57:01 UTC
(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.
Comment 26 Frédéric Demians 2015-04-14 14:30:19 UTC Comment hidden (obsolete)
Comment 27 Frédéric Demians 2015-04-14 14:30:42 UTC Comment hidden (obsolete)
Comment 28 Marc Véron 2015-04-15 10:18:41 UTC Comment hidden (obsolete)
Comment 29 Marc Véron 2015-04-15 10:20:40 UTC Comment hidden (obsolete)
Comment 30 Katrin Fischer 2015-04-15 21:31:45 UTC
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.
Comment 31 Katrin Fischer 2015-04-15 21:34:19 UTC
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>
Comment 32 Katrin Fischer 2015-04-15 21:35:25 UTC
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>
Comment 33 Katrin Fischer 2015-04-15 21:35:59 UTC
Created attachment 37930 [details] [review]
Bug 13941: QA Follow-up - fix for acqui/addorder.tt
Comment 34 Katrin Fischer 2015-04-15 21:36:56 UTC
I have written a follow-up for the problem described in the last comment - please check.
Comment 35 Frédéric Demians 2015-04-16 05:52:01 UTC
(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.
Comment 36 Tomás Cohen Arazi 2015-04-24 13:06:00 UTC
Patches pushed to master.

Thanks Frederic!