Bug 12176 - Remove HTML from additem.pl
Summary: Remove HTML from additem.pl
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Bernardo Gonzalez Kriegel
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 11944 13596
Blocks: 15173
  Show dependency treegraph
 
Reported: 2014-05-03 21:59 UTC by Bernardo Gonzalez Kriegel
Modified: 2017-06-14 22:10 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 12176 - Remove HTML from additem.pl (10.65 KB, patch)
2014-05-03 22:04 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 12176 - Remove HTML from additem.pl (10.52 KB, patch)
2014-07-29 12:35 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 12176 - Remove HTML from additem.pl (10.58 KB, patch)
2014-08-19 09:26 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 12176 - Remove HTML from additem.pl (10.64 KB, patch)
2014-08-25 13:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12176: Remove HTML from additem.pl (11.16 KB, patch)
2015-05-19 17:06 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 12176: [QA Follow-up] Small additem adjustments (7.16 KB, patch)
2015-05-20 14:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 12176: Remove HTML from additem.pl (11.26 KB, patch)
2015-05-20 14:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 12176: [QA Follow-up] Small additem adjustments (7.16 KB, patch)
2015-05-20 14:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 12176: [QA Follow-up] Capitalization typo after eleventh hour (1.85 KB, patch)
2015-05-20 14:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 12176: Fix for missing field_value in select (3.46 KB, patch)
2015-05-23 12:05 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 12176: [QA Follow-up] Small additem adjustments (7.18 KB, patch)
2015-05-23 21:03 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 12176: [QA Follow-up] Capitalization typo after eleventh hour (1.86 KB, patch)
2015-05-23 21:03 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 12176: Fix for missing field_value in select (3.71 KB, patch)
2015-05-23 21:03 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Edit item highlight yellow (61.94 KB, image/png)
2015-05-26 09:18 UTC, Jonathan Druart
Details
Highlight (1.75 KB, patch)
2015-05-26 12:53 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 12176: Highlight edited item (1.90 KB, patch)
2015-05-26 12:58 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 12176: Remove HTML from additem.pl (11.29 KB, patch)
2015-05-26 12:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12176: [QA Follow-up] Small additem adjustments (7.25 KB, patch)
2015-05-26 12:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12176: [QA Follow-up] Capitalization typo after eleventh hour (1.93 KB, patch)
2015-05-26 12:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12176: Fix for missing field_value in select (3.77 KB, patch)
2015-05-26 12:59 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Bernardo Gonzalez Kriegel 2014-05-03 21:59:31 UTC
There is HTML code on additem.pl
In particular an untranslatable "Tag Editor"
Comment 1 Bernardo Gonzalez Kriegel 2014-05-03 22:04:34 UTC Comment hidden (obsolete)
Comment 2 Bernardo Gonzalez Kriegel 2014-07-29 12:35:30 UTC Comment hidden (obsolete)
Comment 3 Chris Cormack 2014-08-19 09:26:52 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2014-08-22 14:45:47 UTC
My 995$e is mapped to the a AV and I got the following dropdown list:

<option value="Langues étrangères"></option>
<option value="Ados">Ados</option>
<option value="Album">Album</option>
<option value="BD adulte">BD adulte</option>
<option value="BD enfant">BD enfant</option>
<option value="Bureau">Bureau</option>
<option value="Cédéroms adulte"></option>
<option value="Cédéroms enfant"></option>
<option value="Contes">Contes</option>

etc.

When the description contains a diacritic, the option is empty.

Marked as Failed QA.
Comment 5 Bernardo Gonzalez Kriegel 2014-08-25 13:41:24 UTC
(In reply to Jonathan Druart from comment #4)
> 
> When the description contains a diacritic, the option is empty.
> 
> Marked as Failed QA.

Right, it's true, encoding problems.
But this is one of the problems fixed by Bug 11944. 
Tested on top of that and it works :)

Can you test it that way?
I'm adding 11944 as a dependency.
Comment 6 Jonathan Druart 2014-08-25 13:48:09 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2014-08-25 13:52:45 UTC
Bug 11944 fixes the issue I raised on comment 4.
Comment 8 Tomás Cohen Arazi 2014-08-26 18:43:36 UTC
Patch pushed to master.

Thanks Bernardo!
Comment 9 Jonathan Druart 2014-08-27 07:15:29 UTC
(In reply to Tomás Cohen Arazi from comment #8)
> Patch pushed to master.

Tomás, without bug 11944, this patch introduces a regression (see comment 4), I am not sure it should have been pushed before it.
Comment 10 Tomás Cohen Arazi 2014-08-27 12:27:10 UTC
Jonathan, thanks for pointing this out. I have reverted this patch, and will push it when the UTF-8 work hits master.
Comment 11 Marcel de Rooy 2014-11-14 15:03:52 UTC
Adding a see also on this one to bug 10480 (work in progress). It will touch this part of code too in particular to item plugins. Not a hard rebase on either side..
Comment 12 Tomás Cohen Arazi 2015-01-14 13:16:37 UTC
Bernardo, could you please rebase this one? I want it :-D
Comment 13 M. Tompsett 2015-04-16 19:21:07 UTC
Make sure that bug 7673 is not regressed.
Comment 14 Tomás Cohen Arazi 2015-05-18 15:09:43 UTC
Ping
Comment 15 Bernardo Gonzalez Kriegel 2015-05-19 17:06:27 UTC Comment hidden (obsolete)
Comment 16 Marcel de Rooy 2015-05-20 12:30:55 UTC
Looking at this one now..
Comment 17 Marcel de Rooy 2015-05-20 14:21:10 UTC Comment hidden (obsolete)
Comment 18 Marcel de Rooy 2015-05-20 14:21:25 UTC Comment hidden (obsolete)
Comment 19 Marcel de Rooy 2015-05-20 14:21:29 UTC Comment hidden (obsolete)
Comment 20 Marcel de Rooy 2015-05-20 14:23:02 UTC
Thanks Bernardo for reviving this one.
The patch was fine already although I eventually added a list of seven points..
Hopefully the QAer of duty will not be too hard on us :)
Comment 21 Marcel de Rooy 2015-05-20 14:45:44 UTC Comment hidden (obsolete)
Comment 22 Bernardo Gonzalez Kriegel 2015-05-20 15:14:50 UTC
(In reply to Marcel de Rooy from comment #20)
> Thanks Bernardo for reviving this one.

Thank you!!

> 
> [1] Remove .hidden after [% avalue %] in additem.tt. (Typo)
a leftover from my debugging 

> [2] A closer look revealed that mv.avalue is useless too. An input element
>     does not contain any content. Putting it after the hidden element
>     in a non-visible context has no meaning.
Ok!
> [3] Change handling of select attributes readonly and disabled.

I was worried about this, since i think select can't have readonly
attribute, only disabled. But if disabled, it's content is not sent
back to GET/POST, so perhaps a hidden attribute with same name and
old value is needed.
Or may be now select can have readonly, but then we need to remove disabled.
I left both to not change old behavior

> [4] Remove unused variable $attributes_no_value_textarea
Good! I forgot them
But in my first patch i also removed "my $attributes_no_value"

> 
> Sorry that this small list kept growing :)
Perfect, thanks again!
Comment 23 Marcel de Rooy 2015-05-20 18:58:15 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #22)
> > [3] Change handling of select attributes readonly and disabled.
> 
> I was worried about this, since i think select can't have readonly
> attribute, only disabled. But if disabled, it's content is not sent
> back to GET/POST, so perhaps a hidden attribute with same name and
> old value is needed.
> Or may be now select can have readonly, but then we need to remove disabled.
> I left both to not change old behavior

Seems good to me. I recall seeing some code in the template that removes the disabled status when submitting the form ..
Comment 24 Jonathan Druart 2015-05-22 12:18:01 UTC
On editing an item, I get

Form not submitted because of the following problem(s)
------------------------------------------------------------------------------------

- 4 mandatory fields empty (highlighted)

Which is wrong, my 4 mandatory fields are filled.
Comment 25 Marcel de Rooy 2015-05-23 11:49:32 UTC
(In reply to Jonathan Druart from comment #24)
> On editing an item, I get
> 
> Form not submitted because of the following problem(s)
> -----------------------------------------------------------------------------
> -------
> 
> - 4 mandatory fields empty (highlighted)
> 
> Which is wrong, my 4 mandatory fields are filled.

Good catch. I see that something else is wrong not related to mandatory fields but indirectly causing this issue..
Comment 26 Marcel de Rooy 2015-05-23 12:05:24 UTC Comment hidden (obsolete)
Comment 27 Marcel de Rooy 2015-05-23 12:07:45 UTC
Bernardo:
Could you please retest with the latest patch attached and verify that we now resolved these issues (and add signoff)?

Could you also confirm that we do no longer need select parameters:
-                    -labels   => \%authorised_lib,
-                    -override => 1,
-                    -multiple => 0,
These were passed to CGI::scrolling_list in the past.

Thanks.
Comment 28 Bernardo Gonzalez Kriegel 2015-05-23 21:03:17 UTC Comment hidden (obsolete)
Comment 29 Bernardo Gonzalez Kriegel 2015-05-23 21:03:23 UTC Comment hidden (obsolete)
Comment 30 Bernardo Gonzalez Kriegel 2015-05-23 21:03:30 UTC Comment hidden (obsolete)
Comment 31 Bernardo Gonzalez Kriegel 2015-05-23 21:23:48 UTC
(In reply to Marcel de Rooy from comment #27)
> Could you please retest with the latest patch attached and verify that we
> now resolved these issues (and add signoff)?

Done :)

> 
> Could you also confirm that we do no longer need select parameters:
> -                    -labels   => \%authorised_lib,
This IS passed ( labels   => \%authorised_lib, )
and we need it.

> -                    -override => 1,
It's something from CGI, no needed

> -                    -multiple => 0,
multiple='false' is default, so no needed

> Thanks.

Thanks to you!
Comment 32 Jonathan Druart 2015-05-24 12:06:31 UTC
Sorry but I have found something else.
Before this patch, the item line which is currently edited was highlighted in yellow. After this patch, it does not anymore.
Comment 33 Marcel de Rooy 2015-05-25 09:28:23 UTC
(In reply to Jonathan Druart from comment #32)
> Sorry but I have found something else.
> Before this patch, the item line which is currently edited was highlighted
> in yellow. After this patch, it does not anymore.

Jonathan: I also remember the yellow line. But when I am testing in current clean master now, I do not see it anymore !
I am looking where it is triggered in the template/js ?
Comment 34 Marcel de Rooy 2015-05-25 09:52:51 UTC
(In reply to Marcel de Rooy from comment #33)
> (In reply to Jonathan Druart from comment #32)
> > Sorry but I have found something else.
> > Before this patch, the item line which is currently edited was highlighted
> > in yellow. After this patch, it does not anymore.
> 
> Jonathan: I also remember the yellow line. But when I am testing in current
> clean master now, I do not see it anymore !
> I am looking where it is triggered in the template/js ?

Found somethings else. See bug 14267.
But it seems that we did not have a yellow background in the input field. Note that the item currently edited is still yellow in the table.
Checked on older version too.
Please provide a screen shot from current master to convince me if I am not correct.
Back to Signed off.
Comment 35 Jonathan Druart 2015-05-26 09:18:18 UTC
(In reply to Marcel de Rooy from comment #33)
> (In reply to Jonathan Druart from comment #32)
> > Sorry but I have found something else.
> > Before this patch, the item line which is currently edited was highlighted
> > in yellow. After this patch, it does not anymore.
> 
> Jonathan: I also remember the yellow line. But when I am testing in current
> clean master now, I do not see it anymore !
> I am looking where it is triggered in the template/js ?

By item line I meant the tr tag in the items table.
Screenshot coming.
Comment 36 Jonathan Druart 2015-05-26 09:18:48 UTC Comment hidden (obsolete)
Comment 37 Jonathan Druart 2015-05-26 09:20:56 UTC
And I confirm that it does not work with this patch set.
Comment 38 Bernardo Gonzalez Kriegel 2015-05-26 10:44:09 UTC
(In reply to Jonathan Druart from comment #37)
> And I confirm that it does not work with this patch set.

Hey, it works... half of the time :)
Only highlights even items, odd ones remains un-highligthed.
Comment 39 Marcel de Rooy 2015-05-26 11:51:55 UTC
OK One misunderstanding resolved..
The items table highlight should not be that hard to fix.
Will have a look.
Comment 40 Marcel de Rooy 2015-05-26 12:53:18 UTC
(In reply to Jonathan Druart from comment #37)
> And I confirm that it does not work with this patch set.

I have the strong impression that it "does not work" without this patch set too :) It only works on the even items.
So imo this should not interfere with this report. But let me add a small followup for changing behavior (it should go on another report).
Comment 41 Marcel de Rooy 2015-05-26 12:53:38 UTC Comment hidden (obsolete)
Comment 42 Marcel de Rooy 2015-05-26 12:54:17 UTC
(In reply to Marcel de Rooy from comment #41)
> Created attachment 39513 [details] [review] [review]
> Highlight

Would this resolve your highlight problem, Jonathan? No clean patch btw..
Comment 43 Bernardo Gonzalez Kriegel 2015-05-26 12:58:56 UTC Comment hidden (obsolete)
Comment 44 Jonathan Druart 2015-05-26 12:59:31 UTC
Created attachment 39517 [details] [review]
Bug 12176: Remove HTML from additem.pl

This patch removes HTML code from additem.pl.

To test:
1. Check no regressions on Add/Edit/Save items
2. Update translation files for a language,
   check new strings "Tag editor" & "No popup" on staff PO file
3. Check it passes xt/tt_valid.t

Patch partially rebased, part rewritten.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jonathan Druart <jonathan.druart@koha-community.org>
Comment 45 Jonathan Druart 2015-05-26 12:59:35 UTC
Created attachment 39518 [details] [review]
Bug 12176: [QA Follow-up] Small additem adjustments

Adjusting a few small things and making qa tools happy:

[1] Remove .hidden after [% avalue %] in additem.tt. (Typo)
[2] A closer look revealed that mv.avalue is useless too. An input element
    does not contain any content. Putting it after the hidden element
    in a non-visible context has no meaning.
[3] Change handling of select attributes readonly and disabled.
[4] Remove unused variable $attributes_no_value_textarea
[5] Removed a comment with TODO referring to this report.
[6] Moving a duplicated TT variable (mv.javascript) outside IF statement.
[7] And finally could not resist this one: Moving strings Tag editor and
    No popup from script to template.  Plugins++
    NOTE: Most item plugins redirect click to focus. In that case there is
    no popup, but unfortunately the text Tag editor comes up.
    When you remove or rename function Click, No popup comes up. So it works.

Sorry that this small list kept growing :)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@koha-community.org>
Comment 46 Jonathan Druart 2015-05-26 12:59:39 UTC
Created attachment 39519 [details] [review]
Bug 12176: [QA Follow-up] Capitalization typo after eleventh hour

While cleaning up, still found this super tiny string typo :)
Yes, Tag Editor should now be Tag editor.

Test plan :)
Git grep on Tag editor and Tag Editor

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@koha-community.org>
Comment 47 Jonathan Druart 2015-05-26 12:59:43 UTC
Created attachment 39520 [details] [review]
Bug 12176: Fix for missing field_value in select

As Jonathan Druart discovered, we were still missing an important attribute
in the selects. Var mv.name was not filled, so the name was empty with nice
side-effects :)
While fixing this, it was also possible to delete some unused vars that Bernardo
already mentioned before: $attributes and $attributes_no_value.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

For me the simptom was different, when saving an edited item
fields were mangled beyond repear :(
No alert for empty mandatory fields.

But this last patch fix the problem.
Tested add/edit items
Cleaner pl file :)
No errors

Signed-off-by: Jonathan Druart <jonathan.druart@koha-community.org>
Comment 48 Jonathan Druart 2015-05-26 13:00:41 UTC
(In reply to Marcel de Rooy from comment #40)
> (In reply to Jonathan Druart from comment #37)
> > And I confirm that it does not work with this patch set.
> 
> I have the strong impression that it "does not work" without this patch set
> too :) It only works on the even items.
> So imo this should not interfere with this report. But let me add a small
> followup for changing behavior (it should go on another report).

Yes you are right, it does not work as expected on master.
So let's move it to its own bug report.
Comment 49 Tomás Cohen Arazi 2015-05-26 17:39:46 UTC
Patches pushed to master.

Good job Bernardo and Marcel!
Comment 50 Chris Cormack 2015-06-03 07:37:13 UTC
Pushed to 3.20.x will be in 3.20.1
Comment 51 Liz Rea 2015-06-07 22:50:00 UTC
Hi,

these patches will not apply cleanly on 3.18.x, please resubmit if necessary.

Cheers,
Liz