---- 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
Bumping up severity. I just noticed that new_acq *added* more uses of CGI::scrolling_list to C4/Budgets.pm.
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
Something for the hackfest at kohacon12 perhaps, lets kill the rest of these.
Created attachment 18538 [details] [review] Patch for occurence in request.pl Moves the display logic to the template. To Test place a hold use a oartial borrower name you should get a dropdown select list of borrowers and be able to select the borrower for the hold from there. The functionality should remain unimpaired after this patch is applied
Created attachment 18550 [details] [review] [SIGNED-OFF] Bug 766 Remove CGI::scrolling_list from request.pl Let the template take care of generating the display. Lets at least part of request.pl be more succinct. To test, place a hold and search for the patron using a partial name. You should get a dropdown select list of patrons and be able to select the patron to hold for from there. The functionality should remain unimpaired after this patch is applied. Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 18664 [details] [review] Bug 766 Remove CGI::scrolling_list from request.pl Let the template take care of generating the display. Lets at least part of request.pl be more succinct. To test, place a hold and search for the patron using a partial name. You should get a dropdown select list of patrons and be able to select the patron to hold for from there. The functionality should remain unimpaired after this patch is applied. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
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.
"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(
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!
Pushed to 3.10.x, will be in 3.10.8
Pushed to 3.8.x, will be in 3.8.15
Currently the oldest open bug... you know you want to fix it! ;)
Created attachment 25904 [details] [review] Bug 766: Move GetAuthvalueDropbox to C4::Koha GetAuthvalueDropbox is in C4::Budgets, this does not make sense. Only acqui/fetch_sort_dropbox.pl call GetAuthvalueDropbox without using C4::Koha.
Created attachment 25905 [details] [review] Bug 766: Create a plugin routine to build dropdown list On this way, dropdown list could be generated from the templates.
Created attachment 25906 [details] [review] Bug 766: Use the method plugin to generate sort dropdown list - memberentry Now we can use the method plugin from the memberentry form. Test plan: 1/ Create a patron and verify sort1 and sort2 dropdown list are correctly generated. 2/ Select values. 3/ Save. 4/ Edit the patron and verify the selected values are displayed.
Created attachment 25907 [details] [review] Bug 766: Cleaning: buildCGISort is not in use anymore buildCGISort can be deleted. Test plan: git grep buildCGISort should return no result.
What about something like that?
Note: Bug 7180 already removes some call to CGI::scrolling_list.
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.
Created attachment 26401 [details] [review] Bug 766: Use a tt include instead of a method in the AV plugin
(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.
I need tests and feedbacks on my patches to continue.
(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
Created attachment 27010 [details] [review] Bug 766: Move GetAuthvalueDropbox to C4::Koha GetAuthvalueDropbox is in C4::Budgets, this does not make sense. Only acqui/fetch_sort_dropbox.pl call GetAuthvalueDropbox without using C4::Koha.
Created attachment 27011 [details] [review] Bug 766: Create a plugin routine to build dropdown list On this way, dropdown list could be generated from the templates.
Created attachment 27012 [details] [review] Bug 766: Use the method plugin to generate sort dropdown list - memberentry Now we can use the method plugin from the memberentry form. Test plan: 1/ Create a patron and verify sort1 and sort2 dropdown list are correctly generated. 2/ Select values. 3/ Save. 4/ Edit the patron and verify the selected values are displayed.
Created attachment 27013 [details] [review] Bug 766: Cleaning: buildCGISort is not in use anymore buildCGISort can be deleted. Test plan: git grep buildCGISort should return no result.
Created attachment 27014 [details] [review] Bug 766: Use a tt include instead of a method in the AV plugin
(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?
(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?
(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
Created attachment 27044 [details] [review] [SIGNED-OFF] Bug 766: Move GetAuthvalueDropbox to C4::Koha GetAuthvalueDropbox is in C4::Budgets, this does not make sense. Only acqui/fetch_sort_dropbox.pl call GetAuthvalueDropbox without using C4::Koha. Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Created attachment 27045 [details] [review] [SIGNED-OFF] Bug 766: Create a plugin routine to build dropdown list On this way, dropdown list could be generated from the templates. Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Created attachment 27046 [details] [review] [SIGNED-OFF] Bug 766: Use the method plugin to generate sort dropdown list - memberentry Now we can use the method plugin from the memberentry form. Test plan: 1/ Create a patron and verify sort1 and sort2 dropdown list are correctly generated. 2/ Select values. 3/ Save. 4/ Edit the patron and verify the selected values are displayed. Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Works as described.
Created attachment 27047 [details] [review] [SIGNED-OFF] Bug 766: Cleaning: buildCGISort is not in use anymore buildCGISort can be deleted. Test plan: git grep buildCGISort should return no result. Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> No more buildCGISort
Created attachment 27048 [details] [review] [SIGNED-OFF] Bug 766: Use a tt include instead of a method in the AV plugin Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> This gives a koha-qa error, but on another user/env not. Really don't know
(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?
(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.
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 :)
(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.
Created attachment 27396 [details] [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.
(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 :(
Hi Bernardo, thx for testing :) I had problems with a value äüö I think.
(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)
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.
And the problem shoudl only appear without the last patch Jonathan attached.
Created attachment 27400 [details] [review] [SIGNED-OFF] 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. Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> No side effects.
Created attachment 27441 [details] Without last patch to fix encoding
Created attachment 27442 [details] With last patch
Created attachment 27443 [details] [review] [PASSED QA] Bug 766: Move GetAuthvalueDropbox to C4::Koha GetAuthvalueDropbox is in C4::Budgets, this does not make sense. Only acqui/fetch_sort_dropbox.pl call GetAuthvalueDropbox without using C4::Koha. Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 27444 [details] [review] [PASSED QA] Bug 766: Create a plugin routine to build dropdown list On this way, dropdown list could be generated from the templates. Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 27445 [details] [review] [PASSED QA] Bug 766: Use the method plugin to generate sort dropdown list - memberentry Now we can use the method plugin from the memberentry form. Test plan: 1/ Create a patron and verify sort1 and sort2 dropdown list are correctly generated. 2/ Select values. 3/ Save. 4/ Edit the patron and verify the selected values are displayed. Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Works as described. Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 27446 [details] [review] [PASSED QA] Bug 766: Cleaning: buildCGISort is not in use anymore buildCGISort can be deleted. Test plan: git grep buildCGISort should return no result. Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> No more buildCGISort Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 27447 [details] [review] [PASSED QA] Bug 766: Use a tt include instead of a method in the AV plugin Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> This gives a koha-qa error, but on another user/env not. Really don't know Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> I see the same error, but can't see where this is coming from, code seems to work ok. Works as described, no regressions found.
Created attachment 27448 [details] [review] [PASSED QA] 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. Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> No side effects. Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Fixes encoding problem, until this can be resolved properly.
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.
(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.
Created attachment 27479 [details] [review] Bug 766: Update POD for AuthorisedValues TT plugin
Created attachment 27495 [details] [review] [SIGNED-OFF] Bug 766: Update POD for AuthorisedValues TT plugin Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> POD is Ok.
Created attachment 27665 [details] [review] [PASSED QA] Bug 766: Move GetAuthvalueDropbox to C4::Koha GetAuthvalueDropbox is in C4::Budgets, this does not make sense. Only acqui/fetch_sort_dropbox.pl call GetAuthvalueDropbox without using C4::Koha. Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 27666 [details] [review] [PASSED QA] Bug 766: Create a plugin routine to build dropdown list On this way, dropdown list could be generated from the templates. Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 27667 [details] [review] [PASSED QA] Bug 766: Use the method plugin to generate sort dropdown list - memberentry Now we can use the method plugin from the memberentry form. Test plan: 1/ Create a patron and verify sort1 and sort2 dropdown list are correctly generated. 2/ Select values. 3/ Save. 4/ Edit the patron and verify the selected values are displayed. Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Works as described. Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 27668 [details] [review] [PASSED QA] Bug 766: Cleaning: buildCGISort is not in use anymore buildCGISort can be deleted. Test plan: git grep buildCGISort should return no result. Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> No more buildCGISort Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 27669 [details] [review] [PASSED QA] Bug 766: Use a tt include instead of a method in the AV plugin Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> This gives a koha-qa error, but on another user/env not. Really don't know Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> I see the same error, but can't see where this is coming from, code seems to work ok. Works as described, no regressions found. Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 27670 [details] [review] [PASSED QA] 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. Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> No side effects. Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Fixes encoding problem, until this can be resolved properly. Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Created attachment 27671 [details] [review] [PASSED QA] Bug 766: Update POD for AuthorisedValues TT plugin Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> POD is Ok. Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Pushed latest round to master. Thanks, Jonathan!
Created attachment 29870 [details] [review] Bug 766: Remove CGI::scrolling_list from aqplan.pl This patch removes 3 occurences on aqplan.pl To test: 1. You need a budget and a found 2. Apply the patch 3. Go to Aquisitions, click on your found 4. Select one of the options on pulldown Planning e.g. "Plan by months" 5. Now to the left there are two boxes, Filter and Export a) Filter: (1) pulldown below 'Select planning type' check that works selecting other values and submit This pulldown showed originally code types, untranslatable BTW. Added a TT block to allow translation and shows legends that match Planning pulldown, but not the order. b) Export: pulldowns for filetype (2) and delimiter (3) Those have originally one value each, and for me Export does not work, before or after patch, but check that pulldown exist. Notes: A) 5.a) I can change de order of main Planning pulldown, is harcoded. The values for Filter pulldown are sorted on aqplan.pl. Is one or the other. If desired, TT block can be moved to includes/budgets-admin-toolbar.inc (where hardcoded Planning lives) B) 5.b) Moved 'Export' legend inside it's box, like Filter. C) Finally: there is something strange, before or after patch. There is an empty option 'Planning by " if you select planning using main pulldown, it disappears if you select planning using Filter (not a mistery, using Planning fills $auth_cats_loop, using Filter submit button does not)
Created attachment 29871 [details] [review] Bug 766: Remove CGI::scrollinglist from marctagstructure.pl This patch removes the only instance in this file. Don't know if the feature is of some utility, seems that does not work (no use of auth values as indicators on MARC editor) To test: 1. Apply the patch 2. Go to Administration > MARC frameworks 3. Clic on MARC structure of any fw 4. Clic Edit on any tag 5. On tag editor check the pulldown 'Authorised value', that was replaced 6. Check for regressions: select a value, save, verify the value, change again, etc. Note: removed a SELECT, using now C4::Koha::GetAuthorisedValueCategories Do not we need an AuthorisedValues.pm? (on C4 or Koha)
Created attachment 29872 [details] [review] Bug 766: Remove CGI::scrollinglist from authorised_values.pl This patch removes the only instance in this file. Also fixes array sorting, removes some tabs, removes an unneeded $dbh handler (there is a global one). Makes use of C4::Koha::GetAuthorisedValueCategories. To test: 1. Apply the patch 2. Go to Administration > Authorised values 3. Replaced pulldown next to 'Show category' must show all (present & system) categories correctly ordered. 4. Select some and check for regressions, e.g. CCODE, Bsort2, YES_NO no changes must be observed
Created attachment 29877 [details] [review] Bug 766: Remove CGI::scrollinglist from marctagstructure.pl This patch removes the only instance in this file. Don't know if the feature is of some utility, seems that does not work (no use of auth values as indicators on MARC editor) To test: 1. Apply the patch 2. Go to Administration > MARC frameworks 3. Clic on MARC structure of any fw 4. Clic Edit on any tag 5. On tag editor check the pulldown 'Authorised value', that was replaced 6. Check for regressions: select a value, save, verify the value, change again, etc. Note: removed a SELECT, using now C4::Koha::GetAuthorisedValueCategories Do not we need an AuthorisedValues.pm? (on C4 or Koha)
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>
Created attachment 30404 [details] [review] Bug 766: Remove CGI::scrolling_list from aqplan.pl This patch removes 3 occurences on aqplan.pl To test: 1. You need a budget and a found 2. Apply the patch 3. Go to Aquisitions, click on your found 4. Select one of the options on pulldown Planning e.g. "Plan by months" 5. Now to the left there are two boxes, Filter and Export a) Filter: (1) pulldown below 'Select planning type' check that works selecting other values and submit This pulldown showed originally code types, untranslatable BTW. Added a TT block to allow translation and shows legends that match Planning pulldown, but not the order. b) Export: pulldowns for filetype (2) and delimiter (3) Those have originally one value each, and for me Export does not work, before or after patch, but check that pulldown exist. Notes: A) 5.a) I can change de order of main Planning pulldown, is harcoded. The values for Filter pulldown are sorted on aqplan.pl. Is one or the other. If desired, TT block can be moved to includes/budgets-admin-toolbar.inc (where hardcoded Planning lives) B) 5.b) Moved 'Export' legend inside it's box, like Filter. C) Finally: there is something strange, before or after patch. There is an empty option 'Planning by " if you select planning using main pulldown, it disappears if you select planning using Filter (not a mistery, using Planning fills $auth_cats_loop, using Filter submit button does not)
Created attachment 30405 [details] [review] Bug 766: Remove CGI::scrollinglist from authorised_values.pl This patch removes the only instance in this file. Also fixes array sorting, removes some tabs, removes an unneeded $dbh handler (there is a global one). Makes use of C4::Koha::GetAuthorisedValueCategories. To test: 1. Apply the patch 2. Go to Administration > Authorised values 3. Replaced pulldown next to 'Show category' must show all (present & system) categories correctly ordered. 4. Select some and check for regressions, e.g. CCODE, Bsort2, YES_NO no changes must be observed
Created attachment 30406 [details] [review] Bug 766: Remove CGI::scrollinglist from marctagstructure.pl This patch removes the only instance in this file. Don't know if the feature is of some utility, seems that does not work (no use of auth values as indicators on MARC editor) To test: 1. Apply the patch 2. Go to Administration > MARC frameworks 3. Clic on MARC structure of any fw 4. Clic Edit on any tag 5. On tag editor check the pulldown 'Authorised value', that was replaced 6. Check for regressions: select a value, save, verify the value, change again, etc. Note: removed a SELECT, using now C4::Koha::GetAuthorisedValueCategories Do not we need an AuthorisedValues.pm? (on C4 or Koha)
(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.
Created attachment 30428 [details] [review] [SIGNED-OFF] Bug 766: Remove CGI::scrolling_list from aqplan.pl This patch removes 3 occurences on aqplan.pl To test: 1. You need a budget and a found 2. Apply the patch 3. Go to Aquisitions, click on your found 4. Select one of the options on pulldown Planning e.g. "Plan by months" 5. Now to the left there are two boxes, Filter and Export a) Filter: (1) pulldown below 'Select planning type' check that works selecting other values and submit This pulldown showed originally code types, untranslatable BTW. Added a TT block to allow translation and shows legends that match Planning pulldown, but not the order. b) Export: pulldowns for filetype (2) and delimiter (3) Those have originally one value each, and for me Export does not work, before or after patch, but check that pulldown exist. Notes: A) 5.a) I can change de order of main Planning pulldown, is harcoded. The values for Filter pulldown are sorted on aqplan.pl. Is one or the other. If desired, TT block can be moved to includes/budgets-admin-toolbar.inc (where hardcoded Planning lives) B) 5.b) Moved 'Export' legend inside it's box, like Filter. C) Finally: there is something strange, before or after patch. There is an empty option 'Planning by " if you select planning using main pulldown, it disappears if you select planning using Filter (not a mistery, using Planning fills $auth_cats_loop, using Filter submit button does not) Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 30430 [details] [review] [SIGNED-OFF] Bug 766: Remove CGI::scrollinglist from authorised_values.pl This patch removes the only instance in this file. Also fixes array sorting, removes some tabs, removes an unneeded $dbh handler (there is a global one). Makes use of C4::Koha::GetAuthorisedValueCategories. To test: 1. Apply the patch 2. Go to Administration > Authorised values 3. Replaced pulldown next to 'Show category' must show all (present & system) categories correctly ordered. 4. Select some and check for regressions, e.g. CCODE, Bsort2, YES_NO no changes must be observed Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 30432 [details] [review] [SIGNED-OFF] Bug 766: Remove CGI::scrollinglist from marctagstructure.pl This patch removes the only instance in this file. Don't know if the feature is of some utility, seems that does not work (no use of auth values as indicators on MARC editor) To test: 1. Apply the patch 2. Go to Administration > MARC frameworks 3. Clic on MARC structure of any fw 4. Clic Edit on any tag 5. On tag editor check the pulldown 'Authorised value', that was replaced 6. Check for regressions: select a value, save, verify the value, change again, etc. Note: removed a SELECT, using now C4::Koha::GetAuthorisedValueCategories Do not we need an AuthorisedValues.pm? (on C4 or Koha) Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 30510 [details] [review] [PASSED QA] Bug 766: Remove CGI::scrolling_list from aqplan.pl This patch removes 3 occurences on aqplan.pl To test: 1. You need a budget and a fund 2. Apply the patch 3. Go to Aquisitions, click on your fund 4. Select one of the options on pulldown Planning e.g. "Plan by months" 5. Now to the left there are two boxes, Filter and Export a) Filter: (1) pulldown below 'Select planning type' check that works selecting other values and submit This pulldown showed originally code types, untranslatable BTW. Added a TT block to allow translation and shows legends that match Planning pulldown, but not the order. b) Export: pulldowns for filetype (2) and delimiter (3) Those have originally one value each, and for me Export does not work, before or after patch, but check that pulldown exist. Notes: A) 5.a) I can change de order of main Planning pulldown, is harcoded. The values for Filter pulldown are sorted on aqplan.pl. Is one or the other. If desired, TT block can be moved to includes/budgets-admin-toolbar.inc (where hardcoded Planning lives) B) 5.b) Moved 'Export' legend inside it's box, like Filter. C) Finally: there is something strange, before or after patch. There is an empty option 'Planning by " if you select planning using main pulldown, it disappears if you select planning using Filter (not a mistery, using Planning fills $auth_cats_loop, using Filter submit button does not) Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Looks like this needs more work, but no regressions found that were caused by the patch. Passes tests and QA script.
Created attachment 30511 [details] [review] [PASSED QA] Bug 766: Remove CGI::scrollinglist from authorised_values.pl This patch removes the only instance in this file. Also fixes array sorting, removes some tabs, removes an unneeded $dbh handler (there is a global one). Makes use of C4::Koha::GetAuthorisedValueCategories. To test: 1. Apply the patch 2. Go to Administration > Authorised values 3. Replaced pulldown next to 'Show category' must show all (present & system) categories correctly ordered. 4. Select some and check for regressions, e.g. CCODE, Bsort2, YES_NO no changes must be observed Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Works as described, no problems found. Passes tests and QA script.
Created attachment 30512 [details] [review] [PASSED QA] Bug 766: Remove CGI::scrollinglist from marctagstructure.pl This patch removes the only instance in this file. Don't know if the feature is of some utility, seems that does not work (no use of auth values as indicators on MARC editor) To test: 1. Apply the patch 2. Go to Administration > MARC frameworks 3. Clic on MARC structure of any fw 4. Clic Edit on any tag 5. On tag editor check the pulldown 'Authorised value', that was replaced 6. Check for regressions: select a value, save, verify the value, change again, etc. Note: removed a SELECT, using now C4::Koha::GetAuthorisedValueCategories Do not we need an AuthorisedValues.pm? (on C4 or Koha) Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Works as described, passes tests and QA script.
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.
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(
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(
(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 :)
A long story coming to a happy ending - thx Bernardo!
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.