Bug 766

Summary: Omnibus: Use of CGI::scrolling_list prevents customization in template
Product: Koha Reporter: Owen Leonard <oleonard>
Component: TemplatesAssignee: Bernardo Gonzalez Kriegel <bgkriegel>
Status: CLOSED FIXED QA Contact: Bugs List <koha-bugs>
Severity: major    
Priority: P5 - low CC: bgkriegel, chris, chrish, colin.campbell, gmcharlt, jonathan.druart, katrin.fischer, tomascohen
Version: master   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12176
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12177
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12180
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11638
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 5082, 6181, 12612, 12613, 12614, 12687, 12694, 12695, 12696, 12704, 12706, 12707, 13748, 14275, 14279    
Bug Blocks:    
Attachments: Patch for occurence in request.pl
[SIGNED-OFF] Bug 766 Remove CGI::scrolling_list from request.pl
Bug 766 Remove CGI::scrolling_list from request.pl
Bug 766: Move GetAuthvalueDropbox to C4::Koha
Bug 766: Create a plugin routine to build dropdown list
Bug 766: Use the method plugin to generate sort dropdown list - memberentry
Bug 766: Cleaning: buildCGISort is not in use anymore
Bug 766: Use a tt include instead of a method in the AV plugin
Bug 766: Move GetAuthvalueDropbox to C4::Koha
Bug 766: Create a plugin routine to build dropdown list
Bug 766: Use the method plugin to generate sort dropdown list - memberentry
Bug 766: Cleaning: buildCGISort is not in use anymore
Bug 766: Use a tt include instead of a method in the AV plugin
[SIGNED-OFF] Bug 766: Move GetAuthvalueDropbox to C4::Koha
[SIGNED-OFF] Bug 766: Create a plugin routine to build dropdown list
[SIGNED-OFF] Bug 766: Use the method plugin to generate sort dropdown list - memberentry
[SIGNED-OFF] Bug 766: Cleaning: buildCGISort is not in use anymore
[SIGNED-OFF] Bug 766: Use a tt include instead of a method in the AV plugin
Bug 766: Fix encoding issues [ REVERT ME on pushing bug 11944 ]
[SIGNED-OFF] Bug 766: Fix encoding issues [ REVERT ME on pushing bug 11944 ]
Without last patch to fix encoding
With last patch
[PASSED QA] Bug 766: Move GetAuthvalueDropbox to C4::Koha
[PASSED QA] Bug 766: Create a plugin routine to build dropdown list
[PASSED QA] Bug 766: Use the method plugin to generate sort dropdown list - memberentry
[PASSED QA] Bug 766: Cleaning: buildCGISort is not in use anymore
[PASSED QA] Bug 766: Use a tt include instead of a method in the AV plugin
[PASSED QA] Bug 766: Fix encoding issues [ REVERT ME on pushing bug 11944 ]
Bug 766: Update POD for AuthorisedValues TT plugin
[SIGNED-OFF] Bug 766: Update POD for AuthorisedValues TT plugin
[PASSED QA] Bug 766: Move GetAuthvalueDropbox to C4::Koha
[PASSED QA] Bug 766: Create a plugin routine to build dropdown list
[PASSED QA] Bug 766: Use the method plugin to generate sort dropdown list - memberentry
[PASSED QA] Bug 766: Cleaning: buildCGISort is not in use anymore
[PASSED QA] Bug 766: Use a tt include instead of a method in the AV plugin
[PASSED QA] Bug 766: Fix encoding issues [ REVERT ME on pushing bug 11944 ]
[PASSED QA] Bug 766: Update POD for AuthorisedValues TT plugin
Bug 766: Remove CGI::scrolling_list from aqplan.pl
Bug 766: Remove CGI::scrollinglist from marctagstructure.pl
Bug 766: Remove CGI::scrollinglist from authorised_values.pl
Bug 766: Remove CGI::scrollinglist from marctagstructure.pl
Bug 766: Remove CGI::scrolling_list from aqplan.pl
Bug 766: Remove CGI::scrollinglist from authorised_values.pl
Bug 766: Remove CGI::scrollinglist from marctagstructure.pl
[SIGNED-OFF] Bug 766: Remove CGI::scrolling_list from aqplan.pl
[SIGNED-OFF] Bug 766: Remove CGI::scrollinglist from authorised_values.pl
[SIGNED-OFF] Bug 766: Remove CGI::scrollinglist from marctagstructure.pl
[PASSED QA] Bug 766: Remove CGI::scrolling_list from aqplan.pl
[PASSED QA] Bug 766: Remove CGI::scrollinglist from authorised_values.pl
[PASSED QA] Bug 766: Remove CGI::scrollinglist from marctagstructure.pl

Description Chris Cormack 2010-05-20 23:35:01 UTC


---- Reported by oleonard@myacpl.org 2004-04-02 09:07:11 ----

The use of CGI::scrolling_list (in itemtypes.plugin, for instance) prevents
template authors from customizing the select box.  If the values are put into a
TMPL_LOOP, the template itself can build the form element with more flexibility.



---- Additional Comments From mason.james@liblime.com 2007-06-19 14:11:08 ----

(In reply to comment #0)
> The use of CGI::scrolling_list (in itemtypes.plugin, for instance) prevents
> template authors from customizing the select box.  If the values are put into a
> TMPL_LOOP, the template itself can build the form element with more flexibility.

This bug might need an example template file or url, 

do you have one Owen?



---- Additional Comments From oleonard@myacpl.org 2007-06-23 09:21:52 ----

One example of a common one: opac\opac-addbookbybiblionumber.pl. You can't add a <label> to the drop-down list of existing shelves because the script generates no id attribute for the <select> that means clicking on the label doesn't give focus to the drop-down. It's a minor task to add it to the script, but it shows how the scrolling_list removes control from the template writer.

There could be other instances where the template writer prefers to display the form in another way: a series of radio buttons instead of a drop-down. If the selection data can be packaged as a template loop, the template-writer can make that decision rather than the script-writer. Of course that's probably a rare case.





---- Additional Comments From joe.atzberger@liblime.com 2008-04-17 11:01:58 ----

FYI: Removed scrolling_list from several reports.



---- Additional Comments From chris@bigballofwax.co.nz 2009-12-10 09:08:40 ----

Files still using this
acqui/neworderempty.pl
admin/auth_subfields_structure.pl
admin/koha2marclinks.pl
admin/aqplan.pl
admin/authorised_values.pl
admin/auth_tag_structure.pl
admin/marctagstructure.pl
authorities/detail.pl
authorities/authorities.pl
C4/Biblio.pm
C4/Reports.pm
C4/Input.pm
C4/Budgets.pm
catalogue/MARCdetail.pl
catalogue/labeledMARCdetail.pl
cataloguing/additem.pl
cataloguing/value_builder/unimarc_field_225a.pl
cataloguing/value_builder/unimarc_field_4XX.pl
cataloguing/addbiblio.pl
circ/circulation.pl
members/memberentry.pl
opac/opac-addbybiblionumber.pl
reports/cat_issues_top.pl
reports/itemtypes.plugin
reports/issues_by_borrower_category.plugin
reports/borrowers_out.pl
reports/serials_stats.pl
reports/issues_avg_stats.pl
reports/acquisitions_stats.pl
reports/borrowers_stats.pl
reports/issues_stats.pl
reserve/request.pl
serials/claims.pl
tools/batchMod.pl
virtualshelves/addbybiblionumber.pl






---- Additional Comments From colin.campbell@ptfs-europe.com 2010-01-07 09:42:23 ----

Also C4/Input.pm has the subroutine buildCGIsort which is a wrapper around this used in members/memberentry.pl which needs removal



--- Bug imported by chris@bigballofwax.co.nz 2010-05-20 23:35 UTC  ---

This bug was previously known as _bug_ 766 at http://bugs.koha.org/cgi-bin/bugzilla3/show_bug.cgi?id=766

Actual time not defined. Setting to 0.0

Comment 1 Galen Charlton 2010-10-30 05:49:42 UTC
Bumping up severity.  I just noticed that new_acq *added* more uses of CGI::scrolling_list to C4/Budgets.pm.
Comment 2 Colin Campbell 2011-04-13 16:17:57 UTC
As bumping up the priority did not seem to get a response. I've created a new bug 6181 to clear out the instances from C4::Budgets
Comment 3 Chris Cormack 2012-04-08 07:11:53 UTC
Something for the hackfest at kohacon12 perhaps, lets kill the rest of these.
Comment 4 Colin Campbell 2013-05-31 07:55:23 UTC Comment hidden (obsolete)
Comment 5 Owen Leonard 2013-05-31 15:25:55 UTC Comment hidden (obsolete)
Comment 6 Chris Cormack 2013-06-05 09:06:34 UTC Comment hidden (obsolete)
Comment 7 Galen Charlton 2013-06-05 14:33:33 UTC
Comment on attachment 18664 [details] [review]
Bug 766 Remove CGI::scrolling_list from request.pl

Latest patch pushed to master.  Thanks, Colin!

Resetting bug status; only a few left.
Comment 8 Galen Charlton 2013-06-05 14:36:37 UTC
"Few" is a relative term, I suppose:

./C4/Reports.pm:    return CGI::scrolling_list(
./C4/Input.pm:        $CGISort= CGI::scrolling_list(
./C4/Items.pm:                    $subfield_data{marc_value} = CGI::scrolling_list(
./acqui/fetch_sort_dropbox.pl:    $ret_html = CGI::scrolling_list(
./admin/marc_subfields_structure.pl:        $row_data{tab} = CGI::scrolling_list(
./admin/marc_subfields_structure.pl:        $row_data{kohafield}    = CGI::scrolling_list(
./admin/marc_subfields_structure.pl:        $row_data{authorised_value} = CGI::scrolling_list(
./admin/marc_subfields_structure.pl:        $row_data{value_builder} = CGI::scrolling_list(
./admin/marc_subfields_structure.pl:        $row_data{authtypes} = CGI::scrolling_list(
./admin/marc_subfields_structure.pl:        $row_data{tab} = CGI::scrolling_list(
./admin/marc_subfields_structure.pl:        $row_data{kohafield}    = CGI::scrolling_list(
./admin/marc_subfields_structure.pl:        $row_data{value_builder} = CGI::scrolling_list(
./admin/marc_subfields_structure.pl:        $row_data{authorised_value} = CGI::scrolling_list(
./admin/marc_subfields_structure.pl:        $row_data{authtypes} = CGI::scrolling_list(
./admin/marctagstructure.pl:    my $authorised_value  = CGI::scrolling_list(-name=>'authorised_value',
./admin/authorised_values.pl:   my $tab_list = CGI::scrolling_list(-name=>'searchfield',
./admin/aqplan.pl:my $tab_list = CGI::scrolling_list(
./admin/aqplan.pl:my $CGIextChoice = CGI::scrolling_list(
./admin/aqplan.pl:my $CGIsepChoice = CGI::scrolling_list(
./admin/auth_subfields_structure.pl:            $row_data{tab} = CGI::scrolling_list(-name=>'tab',
./admin/auth_subfields_structure.pl:            $row_data{ohidden} = CGI::scrolling_list(-name=>'ohidden',
./admin/auth_subfields_structure.pl:            #$row_data{ihidden} = CGI::scrolling_list(-name=>'ihidden',
./admin/auth_subfields_structure.pl:            #$row_data{ehidden} = CGI::scrolling_list(-name=>'ehidden',
./admin/auth_subfields_structure.pl:            $row_data{kohafield}= CGI::scrolling_list( -name=>"kohafield",
./admin/auth_subfields_structure.pl:            $row_data{authorised_value}  = CGI::scrolling_list(-name=>'authorised_value',
./admin/auth_subfields_structure.pl:            $row_data{frameworkcode}  = CGI::scrolling_list(-name=>'frameworkcode',
./admin/auth_subfields_structure.pl:            $row_data{value_builder}  = CGI::scrolling_list(-name=>'value_builder',
./admin/auth_subfields_structure.pl:            $row_data{tab} = CGI::scrolling_list(-name=>'tab',
./admin/auth_subfields_structure.pl:            $row_data{ohidden} = CGI::scrolling_list(-name=>'ohidden',
./admin/auth_subfields_structure.pl:            #$row_data{ihidden} = CGI::scrolling_list(-name=>'ihidden',
./admin/auth_subfields_structure.pl:            #$row_data{ehidden} = CGI::scrolling_list(-name=>'ehidden',
./admin/auth_subfields_structure.pl:            $row_data{kohafield}= CGI::scrolling_list( -name=>'kohafield',
./admin/auth_subfields_structure.pl:            $row_data{frameworkcode}  = CGI::scrolling_list(-name=>'frameworkcode',
./admin/auth_subfields_structure.pl:            $row_data{authorised_value}  = CGI::scrolling_list(-name=>'authorised_value',
./admin/auth_subfields_structure.pl:            $row_data{value_builder}  = CGI::scrolling_list(-name=>'value_builder',
./admin/auth_tag_structure.pl:  my $authorised_value  = CGI::scrolling_list(-name=>'authorised_value',
./admin/koha2marclinks.pl:        my $marclist = CGI::scrolling_list(
./admin/koha2marclinks.pl:        tablename => CGI::scrolling_list(
./circ/circulation.pl:    $CGIselectborrower = CGI::scrolling_list(
./tools/batchMod.pl:      $subfield_data{marc_value} =CGI::scrolling_list(      # FIXME: factor out scrolling_list
./authorities/authorities.pl:    return CGI::scrolling_list(
./catalogue/labeledMARCdetail.pl:my $framework=CGI::scrolling_list( -name     => 'Frameworks',
./cataloguing/value_builder/unimarc_field_225a.pl:    my $collection = CGI::scrolling_list(
./cataloguing/value_builder/unimarc_field_4XX.pl:        my $CGIitemtype = CGI::scrolling_list(
./cataloguing/value_builder/unimarc_field_4XX.pl:        my $CGIpublisher = CGI::scrolling_list(
./cataloguing/value_builder/marc21_linking_section.pl:             my $CGIitemtype = CGI::scrolling_list(
./cataloguing/value_builder/marc21_linking_section.pl:              my $CGIpublisher = CGI::scrolling_list(
./cataloguing/addbiblio.pl:    return CGI::scrolling_list(
./cataloguing/additem.pl:                $subfield_data{marc_value} =CGI::scrolling_list(      # FIXME: factor out scrolling_list
./members/memberentry.pl:    $CGIorganisations = CGI::scrolling_list( -id => 'organisations',
./reports/catalogue_stats.pl:#  my $CGIlccn=CGI::scrolling_list( -name     => 'Filter',
./reports/serials_stats.pl:     my $CGIextChoice=CGI::scrolling_list(
./reports/issues_stats.pl:my $CGIextChoice=CGI::scrolling_list(
./reports/cat_issues_top.pl:    my $CGIextChoice=CGI::scrolling_list(
./reports/reserves_stats.pl:my $CGIextChoice=CGI::scrolling_list(
./reports/borrowers_out.pl:    my $CGIextChoice=CGI::scrolling_list(
./reports/acquisitions_stats.pl:    my $CGIItemTypes = CGI::scrolling_list(
./reports/acquisitions_stats.pl:    my $CGIBudget = CGI::scrolling_list(
./reports/acquisitions_stats.pl:    my $CGISort1 = CGI::scrolling_list(
./reports/acquisitions_stats.pl:    my $CGISort2 = CGI::scrolling_list(
./reports/acquisitions_stats.pl:    my $CGIextChoice = CGI::scrolling_list(
./reports/borrowers_stats.pl:   my $CGIextChoice=CGI::scrolling_list(
./reports/guided_reports.pl:                    $input =CGI::scrolling_list(      # FIXME: factor out scrolling_list
./reports/issues_avg_stats.pl:    my $CGIBorCat=CGI::scrolling_list( -name     => 'Filter',
./reports/issues_avg_stats.pl:    my $CGIItemTypes=CGI::scrolling_list( -name     => 'Filter',
./reports/issues_avg_stats.pl:    my $CGISort1=CGI::scrolling_list( -name     => 'Filter',
./reports/issues_avg_stats.pl:    my $CGISort2=CGI::scrolling_list( -name     => 'Filter',
./reports/issues_avg_stats.pl:    my $CGIextChoice=CGI::scrolling_list(
Comment 9 Tomás Cohen Arazi 2013-07-02 18:03:09 UTC
The patch for request.pl and request.tt has been pushed to 3.12.x as a dependency for bug 10464, will be in 3.12.2.

Thanks Colin!
Comment 10 Bernardo Gonzalez Kriegel 2013-07-13 17:53:59 UTC
Pushed to 3.10.x, will be in 3.10.8
Comment 11 Chris Hall 2013-07-18 09:02:07 UTC
Pushed to 3.8.x, will be in 3.8.15
Comment 12 Katrin Fischer 2013-12-06 01:16:26 UTC
Currently the oldest open bug... you know you want to fix it! ;)
Comment 13 Jonathan Druart 2014-03-06 16:14:46 UTC Comment hidden (obsolete)
Comment 14 Jonathan Druart 2014-03-06 16:14:54 UTC Comment hidden (obsolete)
Comment 15 Jonathan Druart 2014-03-06 16:15:00 UTC Comment hidden (obsolete)
Comment 16 Jonathan Druart 2014-03-06 16:15:09 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2014-03-06 16:16:54 UTC
What about something like that?
Comment 18 Jonathan Druart 2014-03-06 16:20:41 UTC
Note: Bug 7180 already removes some call to CGI::scrolling_list.
Comment 19 Galen Charlton 2014-03-10 21:04:38 UTC
Comment on attachment 25905 [details] [review]
Bug 766: Create a plugin routine to build dropdown list

Review of attachment 25905 [details] [review]:
-----------------------------------------------------------------

I can't say I quite like this approach, as, well, it prevents customization in the templates by embedding HTML in the code. :)

I think a Template Toolkit include that accepts parameters would be better.
Comment 20 Jonathan Druart 2014-03-17 11:37:51 UTC Comment hidden (obsolete)
Comment 21 Jonathan Druart 2014-03-17 11:38:55 UTC
(In reply to Galen Charlton from comment #19)
> I think a Template Toolkit include that accepts parameters would be better.

Some think like the last submitted patch?

Note that this patch introduces an encoding issue with the current master.
Comment 22 Jonathan Druart 2014-04-11 10:06:34 UTC
I need tests and feedbacks on my patches to continue.
Comment 23 Bernardo Gonzalez Kriegel 2014-04-11 11:03:54 UTC
(In reply to Jonathan Druart from comment #22)
> I need tests and feedbacks on my patches to continue.

4th patch has small conflict, seems that present version on master is shorter

koha-qa complains

 FAIL   koha-tmpl/intranet-tmpl/prog/en/includes/av-build-dropbox.inc
   OK     forbidden patterns
   OK     tt_valid
   FAIL   valid_template
                Undefined subroutine &C4::Koha::GetAuthvalueDropbox called at /home/bgkriegel/kohaclone/Koha/Template/Plugin/AuthorisedValues.pm line 49.

But works
Comment 24 Jonathan Druart 2014-04-11 11:33:29 UTC Comment hidden (obsolete)
Comment 25 Jonathan Druart 2014-04-11 11:33:33 UTC Comment hidden (obsolete)
Comment 26 Jonathan Druart 2014-04-11 11:33:52 UTC Comment hidden (obsolete)
Comment 27 Jonathan Druart 2014-04-11 11:34:09 UTC Comment hidden (obsolete)
Comment 28 Jonathan Druart 2014-04-11 11:34:13 UTC Comment hidden (obsolete)
Comment 29 Jonathan Druart 2014-04-11 11:34:42 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #23)
> 4th patch has small conflict, seems that present version on master is shorter

Hum, I don't get the conflict (just Auto-merging members/memberentry.pl)
and no qa errors raised. Could you please retry with these rebased patches?
Comment 30 Owen Leonard 2014-04-11 14:41:20 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #23)
> koha-qa complains
> 
>  FAIL   koha-tmpl/intranet-tmpl/prog/en/includes/av-build-dropbox.inc
>    OK     forbidden patterns
>    OK     tt_valid
>    FAIL   valid_template
>                 Undefined subroutine &C4::Koha::GetAuthvalueDropbox called
> at /home/bgkriegel/kohaclone/Koha/Template/Plugin/AuthorisedValues.pm line
> 49.

I get this same error from the QA script, but everything seems to work. Is it an inaccurate error?
Comment 31 Bernardo Gonzalez Kriegel 2014-04-11 21:32:35 UTC
(In reply to Owen Leonard from comment #30)
> 
> I get this same error from the QA script, but everything seems to work. Is
> it an inaccurate error?

I think it is, but don't know why. 
With another user/repo/etc. gives no error, like Jonathan.
For me it works
Comment 32 Bernardo Gonzalez Kriegel 2014-04-11 21:46:44 UTC Comment hidden (obsolete)
Comment 33 Bernardo Gonzalez Kriegel 2014-04-11 21:46:55 UTC Comment hidden (obsolete)
Comment 34 Bernardo Gonzalez Kriegel 2014-04-11 21:47:07 UTC Comment hidden (obsolete)
Comment 35 Bernardo Gonzalez Kriegel 2014-04-11 21:47:19 UTC Comment hidden (obsolete)
Comment 36 Bernardo Gonzalez Kriegel 2014-04-11 21:47:29 UTC Comment hidden (obsolete)
Comment 37 Jonathan Druart 2014-04-14 07:47:29 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #36)
> This gives a koha-qa error, but on another user/env not.
> Really don't know

Maybe your koha-qa repository is not up-to-date?
Comment 38 Bernardo Gonzalez Kriegel 2014-04-14 16:07:38 UTC
(In reply to Jonathan Druart from comment #37)
> Maybe your koha-qa repository is not up-to-date?

Was not up to date, but I've updated and got same message. 
Anyway, with other user I have 

testing 5 commit(s) (applied to db3d4a0 'Bug 11563: (follow-up) add noEnterSub')

 OK     members/mod_debarment.pl
 OK     xt/tt_valid.t
 OK     koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/additem.tt
 OK     koha-tmpl/intranet-tmpl/prog/en/modules/members/mancredit.tt
 OK     koha-tmpl/intranet-tmpl/prog/en/modules/members/maninvoice.tt
 OK     koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt

Could be some environment thing.
Comment 39 Katrin Fischer 2014-04-18 09:00:03 UTC
This works as described and I am happy to see buildCGIsort go. 

Tested:
- List of authorized values on funds page still works as expected.
- Member details page still shows the list of valus defined for
  bsort1 and bsort2 correctly. Values can be edited, saved, and removed.
  Library restrictions also still work here.
    
BUT: As Jonathan noted, currently diacritics in the bsort1, bsort2 pull downs in the member entry form will be broken. This is because of a dependency on bug 11944, as I understand it.

I think this is probably ready to go in now, if we 'fixed' the encoding, while bug 11944 will take a bit longer. Would it be an option to apply one of the old fixes here and undo it on 11944?

One other idea: should it be possible to add an id to the pull down for better customization? I see that you can set a class, but sometimes you might want to do both - a class for several elements and an id for the individual.

Setting in discussion for now, I don't want to fail and I am not quite ready to pass it yet :)
Comment 40 Jonathan Druart 2014-04-22 11:38:41 UTC
(In reply to Katrin Fischer from comment #39)
> BUT: As Jonathan noted, currently diacritics in the bsort1, bsort2 pull
> downs in the member entry form will be broken. This is because of a
> dependency on bug 11944, as I understand it.

A patch is coming.

> I think this is probably ready to go in now, if we 'fixed' the encoding,
> while bug 11944 will take a bit longer.

The idea of bug 11944 is to have several persons testing/contributing in order to quickly have a working patch set.
Then it would be better if it doesn't take a long time to be tested/pushed

> One other idea: should it be possible to add an id to the pull down for
> better customization? I see that you can set a class, but sometimes you
> might want to do both - a class for several elements and an id for the
> individual.

The id attribute is already generated, it is the "name" parameter.
Comment 41 Jonathan Druart 2014-04-22 11:39:00 UTC Comment hidden (obsolete)
Comment 42 Bernardo Gonzalez Kriegel 2014-04-22 12:21:48 UTC
(In reply to Jonathan Druart from comment #41)
> Created attachment 27396 [details] [review] [review]
> Bug 766: Fix encoding issues [ REVERT ME on pushing bug 11944 ]
> 
> This patch deals with the current master code. It should be removed when
> patches for bug 11944 will be pushed.

Well, how can be tested? 
I don't have problems with Bsort1/2 utf8 descriptions, with/without patch.
Can/Can't have utf8 values (arabic/chinese) with/without patch. They look good, can be assigned and saved, but on edition they are not selected

Or perhaps I do not understand what to test :(
Comment 43 Katrin Fischer 2014-04-22 12:24:05 UTC
Hi Bernardo, thx for testing :) I had problems with a value äüö I think.
Comment 44 Bernardo Gonzalez Kriegel 2014-04-22 13:13:09 UTC
(In reply to Katrin Fischer from comment #43)
> Hi Bernardo, thx for testing :) I had problems with a value äüö I think.

If value has diacritics, value="ÄÄ", and description "ÄÄ", I see ��
with value="AA", description "ÄÄ", I see "ÄÄ". Can't understand, we
show labels in both cases (moremember.pl?borrowernumber=NNN)
Comment 45 Katrin Fischer 2014-04-22 13:18:21 UTC
Hm, let me think. I had äöü in the description of my bsort2 value and it did show up correctly on the detail page, but the pull down on the edit form was not correct.
Comment 46 Katrin Fischer 2014-04-22 13:20:34 UTC
And the problem shoudl only appear without the last patch Jonathan attached.
Comment 47 Bernardo Gonzalez Kriegel 2014-04-22 13:40:33 UTC Comment hidden (obsolete)
Comment 48 Katrin Fischer 2014-04-23 05:09:12 UTC Comment hidden (obsolete)
Comment 49 Katrin Fischer 2014-04-23 05:09:30 UTC Comment hidden (obsolete)
Comment 50 Katrin Fischer 2014-04-23 05:22:44 UTC Comment hidden (obsolete)
Comment 51 Katrin Fischer 2014-04-23 05:22:48 UTC Comment hidden (obsolete)
Comment 52 Katrin Fischer 2014-04-23 05:22:51 UTC Comment hidden (obsolete)
Comment 53 Katrin Fischer 2014-04-23 05:22:54 UTC Comment hidden (obsolete)
Comment 54 Katrin Fischer 2014-04-23 05:22:57 UTC Comment hidden (obsolete)
Comment 55 Katrin Fischer 2014-04-23 05:23:00 UTC Comment hidden (obsolete)
Comment 56 Katrin Fischer 2014-04-23 05:23:53 UTC
This works nicely for me. I am ready to pass QA, but I had missed before, that we will need some unit tests to cover the routine and possibly the Plugin.

Also, the POD of the TT plugin should be updated to explain the new functionality. 

Leaving this in 'SIGNED OFF' right now.
Comment 57 Jonathan Druart 2014-04-23 13:45:33 UTC
(In reply to Katrin Fischer from comment #56)
> This works nicely for me. I am ready to pass QA, but I had missed before,
> that we will need some unit tests to cover the routine and possibly the
> Plugin.

I just moved the routine, no change done. So I am not sure it should be needed.
It would be great to have unit tests for authorized values (see bug 10363).

> Also, the POD of the TT plugin should be updated to explain the new
> functionality. 

Will do.
Comment 58 Jonathan Druart 2014-04-23 13:46:30 UTC Comment hidden (obsolete)
Comment 59 Bernardo Gonzalez Kriegel 2014-04-23 15:22:26 UTC Comment hidden (obsolete)
Comment 60 Katrin Fischer 2014-04-26 21:15:26 UTC Comment hidden (obsolete)
Comment 61 Katrin Fischer 2014-04-26 21:15:30 UTC Comment hidden (obsolete)
Comment 62 Katrin Fischer 2014-04-26 21:15:34 UTC Comment hidden (obsolete)
Comment 63 Katrin Fischer 2014-04-26 21:15:37 UTC Comment hidden (obsolete)
Comment 64 Katrin Fischer 2014-04-26 21:15:41 UTC Comment hidden (obsolete)
Comment 65 Katrin Fischer 2014-04-26 21:15:44 UTC Comment hidden (obsolete)
Comment 66 Katrin Fischer 2014-04-26 21:15:48 UTC Comment hidden (obsolete)
Comment 67 Galen Charlton 2014-05-04 23:17:45 UTC
Pushed latest round to master.  Thanks, Jonathan!
Comment 68 Bernardo Gonzalez Kriegel 2014-07-19 00:50:17 UTC Comment hidden (obsolete)
Comment 69 Bernardo Gonzalez Kriegel 2014-07-19 02:13:18 UTC Comment hidden (obsolete)
Comment 70 Bernardo Gonzalez Kriegel 2014-07-19 03:45:03 UTC Comment hidden (obsolete)
Comment 71 Bernardo Gonzalez Kriegel 2014-07-19 18:02:53 UTC Comment hidden (obsolete)
Comment 72 Owen Leonard 2014-07-30 16:27:11 UTC
Some comments about attachment 29870 [details] [review], attachment 29872 [details] [review], and attachment 29877 [details] [review]: Although not necessarily required by the HTML5 standard, we do still follow XHTML-style rule for markup.

That means that instead of <option value="foo" selected>
We need to write <option value="foo" selected="selected">

And values should be quoted: <option value="foo">
And not <option value=foo>
Comment 73 Bernardo Gonzalez Kriegel 2014-07-31 14:05:14 UTC Comment hidden (obsolete)
Comment 74 Bernardo Gonzalez Kriegel 2014-07-31 14:05:22 UTC Comment hidden (obsolete)
Comment 75 Bernardo Gonzalez Kriegel 2014-07-31 14:05:29 UTC Comment hidden (obsolete)
Comment 76 Bernardo Gonzalez Kriegel 2014-07-31 14:08:06 UTC
(In reply to Owen Leonard from comment #72)
> That means that instead of <option value="foo" selected>
> We need to write <option value="foo" selected="selected">
> 
> And values should be quoted: <option value="foo">
> And not <option value=foo>

Thanks Owen, fixed now. 
I'll add a followup to some others that have been pushed, and fix also others awaiting sings.
Comment 77 Owen Leonard 2014-07-31 17:38:50 UTC Comment hidden (obsolete)
Comment 78 Owen Leonard 2014-07-31 18:49:37 UTC Comment hidden (obsolete)
Comment 79 Owen Leonard 2014-07-31 18:55:50 UTC Comment hidden (obsolete)
Comment 80 Katrin Fischer 2014-08-03 10:03:35 UTC Comment hidden (obsolete)
Comment 81 Katrin Fischer 2014-08-03 10:03:50 UTC Comment hidden (obsolete)
Comment 82 Katrin Fischer 2014-08-03 10:04:09 UTC Comment hidden (obsolete)
Comment 83 Tomás Cohen Arazi 2014-08-13 13:29:19 UTC
Patches pushed to master.

Thanks Bernardo!

Note: Resetting bug status. Any remaining wil have its own bug report and this omnibus will be closed when all get closed.
Comment 84 Katrin Fischer 2014-10-10 22:55:15 UTC
Looks like we are getting closer!

cataloguing/additem.pl:                $subfield_data{marc_value} =CGI::scrolling_list(      # FIXME: factor out scrolling_list
reports/acquisitions_stats.pl:    my $CGIItemTypes = CGI::scrolling_list(
reports/acquisitions_stats.pl:    my $CGIBudget = CGI::scrolling_list(
reports/acquisitions_stats.pl:    my $CGISort1 = CGI::scrolling_list(
reports/acquisitions_stats.pl:    my $CGISort2 = CGI::scrolling_list(
reports/acquisitions_stats.pl:    my $CGIextChoice = CGI::scrolling_list(
reports/guided_reports.pl:                    $input =CGI::scrolling_list(      # FIXME: factor out scrolling_list
reports/issues_avg_stats.pl:    my $CGIBorCat=CGI::scrolling_list( -name     => 'Filter',
reports/issues_avg_stats.pl:    my $CGIItemTypes=CGI::scrolling_list( -name     => 'Filter',
reports/issues_avg_stats.pl:    my $CGISort1=CGI::scrolling_list( -name     => 'Filter',
reports/issues_avg_stats.pl:    my $CGISort2=CGI::scrolling_list( -name     => 'Filter',
reports/issues_avg_stats.pl:    my $CGIextChoice=CGI::scrolling_list(
Comment 85 Katrin Fischer 2015-05-25 09:47:10 UTC
3 left... can we finish those please?

git grep CGI::scrolling_list
cataloguing/additem.pl:                $subfield_data{marc_value} =CGI::scrolling_list(@scrparam);
reports/guided_reports.pl:                    $input =CGI::scrolling_list(      # FIXME: factor out
reports/issues_avg_stats.pl:    my $CGIBorCat=CGI::scrolling_list( -name     => 'Filter',
reports/issues_avg_stats.pl:    my $CGIItemTypes=CGI::scrolling_list( -name     => 'Filter',
reports/issues_avg_stats.pl:    my $CGISort1=CGI::scrolling_list( -name     => 'Filter',
reports/issues_avg_stats.pl:    my $CGISort2=CGI::scrolling_list( -name     => 'Filter',
reports/issues_avg_stats.pl:    my $CGIextChoice=CGI::scrolling_list(
Comment 86 Bernardo Gonzalez Kriegel 2015-05-26 18:01:46 UTC
(In reply to Katrin Fischer from comment #85)
> 3 left... can we finish those please?
> 
> git grep CGI::scrolling_list
> cataloguing/additem.pl:                $subfield_data{marc_value}
> =CGI::scrolling_list(@scrparam);

Bug 12176, in master!

> reports/guided_reports.pl:                    $input =CGI::scrolling_list(  
> # FIXME: factor out

Bug 14275

> reports/issues_avg_stats.pl:    my $CGIBorCat=CGI::scrolling_list( -name    
> => 'Filter',
> reports/issues_avg_stats.pl:    my $CGIItemTypes=CGI::scrolling_list( -name 
> => 'Filter',
> reports/issues_avg_stats.pl:    my $CGISort1=CGI::scrolling_list( -name    
> => 'Filter',
> reports/issues_avg_stats.pl:    my $CGISort2=CGI::scrolling_list( -name    
> => 'Filter',
> reports/issues_avg_stats.pl:    my $CGIextChoice=CGI::scrolling_list(

Bug 14279

No more :)
Comment 87 Katrin Fischer 2015-05-31 12:10:24 UTC
A long story coming to a happy ending - thx Bernardo!
Comment 88 Tomás Cohen Arazi 2015-06-01 17:44:45 UTC
Many thanks to everyone involved on this bug and its dependencies.

And special thanks to Bernardo! Who took a lot of time writing/rebasing/rewriting this patches so we can get rid of HTML in Perl code all over the place.