At the moment, admin/admin-home.pl shows the cataloguing search with check out, check in, renew and catalogue search options. It seems more appropriate to show the prefs-admin search here instead.
Created attachment 58278 [details] [review] Bug 17793: Make sysprefs search show on Admin home page To test: 1) Go to Administration 2) Notice Catalogue search at the top - seems out of place. 3) Apply patch and refresh page. 4) Notice admin / sysprefs search now shows and is more appropriate. 5) Confirm searching for sysprefs still works Sponsored-by: Catalyst IT
Created attachment 58327 [details] [review] Bug 17793: Make sysprefs search show on all Admin pages This patch is amended to use the sysprefs search on all Administration pages that do not have their own custom search. To test: 1) Go to Administration 2) Notice Catalogue search at the top - seems out of place. 3) Apply patch and refresh page. 4) Notice admin / sysprefs search now shows and is more appropriate. 5) Confirm searching for sysprefs still works This patch affects the following pages: - admin-home.pl - audio_alerts.pl - authtypes.pl - auth_tag_structure.pl - authorised_values.pl - biblio_framework.pl - marctagstructure.pl - branch_transfer_limits.pl - branches.pl - checkmarc.pl - classsources.pl - columns_settings.pl - didyoumean.pl - edi_accounts.pl - edi_ean_accounts.pl - fieldmapping.pl - item_circulation_alerts.pl - items_search_fields.pl - items_search_field.pl - item_types.pl - koha2marklinks.pl - matching-rules.pl - oai_sets.pl - oai_set_mappings.pl - patron-attr-types.pl - smart-rules.pl - transport-cost-matrix.pl Sponsored-by: Catalyst IT
Looks good to me, but found one missing: sms_providers.pl
Created attachment 58371 [details] [review] Bug 17793: Make sysprefs search show on all Admin pages This patch is amended to use the sysprefs search on all Administration pages that do not have their own custom search. To test: 1) Go to Administration 2) Notice Catalogue search at the top - seems out of place. 3) Apply patch and refresh page. 4) Notice admin / sysprefs search now shows and is more appropriate. 5) Confirm searching for sysprefs still works This patch affects the following pages: - admin-home.pl - audio_alerts.pl - authtypes.pl - auth_tag_structure.pl - authorised_values.pl - biblio_framework.pl - marctagstructure.pl - branch_transfer_limits.pl - branches.pl - checkmarc.pl - classsources.pl - columns_settings.pl - didyoumean.pl - edi_accounts.pl - edi_ean_accounts.pl - fieldmapping.pl - item_circulation_alerts.pl - items_search_fields.pl - items_search_field.pl - item_types.pl - koha2marklinks.pl - matching-rules.pl - oai_sets.pl - oai_set_mappings.pl - patron-attr-types.pl - smart-rules.pl - transport-cost-matrix.pl - sms_providers.pl Sponsored-by: Catalyst IT
Created attachment 58405 [details] [review] Bug 17793: Test Cases There was no form of testing for the change. In order to test the perl-side of the changes, I just went to a variety of URLs to see if they have the expected three tabs. TEST PLAN --------- 1) Apply this patch 2) At the bash prompt, export KOHA_INTRANET_URL=http://localhost:8080/ -- Or whatever yours is. With or without the trailing / 3) prove -v t/db_dependent/www/prefs-admin_search.t -- before applying the other patch, there should be failures. 4) Apply both patches 5) prove -v t/db_dependent/www/prefs-admin_search.t -- there should be no failures now. 6) run koha qa test tools
Created attachment 58406 [details] [review] Bug 17793: Test Cases There was no form of testing for the change. In order to test the perl-side of the changes, I just went to a variety of URLs to see if they have the expected three tabs. TEST PLAN --------- 1) Apply this patch 2) At the bash prompt, export KOHA_INTRANET_URL=http://localhost:8080/ -- Or whatever yours is. With or without the trailing / 3) prove -v t/db_dependent/www/prefs-admin_search.t -- before applying the other patch, there should be failures. 4) Apply both patches 5) prove -v t/db_dependent/www/prefs-admin_search.t -- there should be no failures now. 6) run koha qa test tools Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Created attachment 58407 [details] [review] Bug 17793: Make sysprefs search show on all Admin pages This patch is amended to use the sysprefs search on all Administration pages that do not have their own custom search. To test: 1) Go to Administration 2) Notice Catalogue search at the top - seems out of place. 3) Apply patch and refresh page. 4) Notice admin / sysprefs search now shows and is more appropriate. 5) Confirm searching for sysprefs still works This patch affects the following pages: - admin-home.pl - audio_alerts.pl - authtypes.pl - auth_tag_structure.pl - authorised_values.pl - biblio_framework.pl - marctagstructure.pl - branch_transfer_limits.pl - branches.pl - checkmarc.pl - classsources.pl - columns_settings.pl - didyoumean.pl - edi_accounts.pl - edi_ean_accounts.pl - fieldmapping.pl - item_circulation_alerts.pl - items_search_fields.pl - items_search_field.pl - item_types.pl - koha2marklinks.pl - matching-rules.pl - oai_sets.pl - oai_set_mappings.pl - patron-attr-types.pl - smart-rules.pl - transport-cost-matrix.pl - sms_providers.pl Sponsored-by: Catalyst IT Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Follow the test plan in comment #6 and sign off. :)
Marc, your tests don't work for me in koha-shell on kohadevbox, but the feature itself works nicely. not ok 191 - Expecting no \#checkin_search # Failed test 'Expecting no #checkin_search' # at t/db_dependent/www/prefs-admin_search.t line 99. # searched: "<!DOCTYPE html>\x{0a}<!-- TEMPLATE FILE: sms_providers."... # and found: "#checkin_search" # at position: 10048 (line 278 column 33) not ok 192 - Expecting no \#renew_search # Failed test 'Expecting no #renew_search' # at t/db_dependent/www/prefs-admin_search.t line 100. # searched: "<!DOCTYPE html>\x{0a}<!-- TEMPLATE FILE: sms_providers."... # and found: "#renew_search" # at position: 10115 (line 279 column 33) not ok 193 - Checking sms_providers.pl failed! # Failed test 'Checking sms_providers.pl failed!' # at t/db_dependent/www/prefs-admin_search.t line 94. 1..193 # Looks like you failed 109 tests of 193. Maybe we should fix them on another bug so this can move forward? Aleisha, failing because I found some more missing - quick to fix: /cgi-bin/koha/plugins/plugins-home.pl /cgi-bin/koha/admin/searchengine/elasticsearch/mappings.pl
Comment on attachment 58406 [details] [review] Bug 17793: Test Cases moved to bug 17864, as per Katrin's suggestion. I'll tweak up the test coverage once the other two cases Katrin mentioned are dealt with.
Created attachment 58670 [details] [review] [SIGNED-OFF] Bug 17793: Test Cases There was no form of testing for the change. In order to test the perl-side of the changes, I just went to a variety of URLs to see if they have the expected three tabs. TEST PLAN --------- 1) Apply this patch 2) At the bash prompt, export KOHA_INTRANET_URL=http://localhost:8080/ -- Or whatever yours is. With or without the trailing / 3) prove -v t/db_dependent/www/prefs-admin_search.t -- before applying the other patch, there should be failures. 4) Apply both patches 5) prove -v t/db_dependent/www/prefs-admin_search.t -- there should be no failures now. 6) run koha qa test tools Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Sorry, I didn't notice, the changes here... Katrin, the test is working fine in koha-shell on kohadevbox for me... I noticed the missing elasticsearch mapping page, and think we should add it to clone-rules.pl too - it's not used directly, but can be, so for the case, it doesn't hurt to make the same change I think...
I ran: $ git grep -l cat-search.inc | grep admin This means I believe the outstanding ones are (koha-tmpl/intranet-tmpl/prog/en/modules/): - admin/auth_subfields_structure.tt - admin/clone-rules.tt - admin/marc_subfields_structure.tt - admin/searchengine/elasticsearch/mappings.tt - plugins/plugins-home.tt
Created attachment 58684 [details] [review] Bug 17793: Follow up for identified missing changes I ran: $ git grep -l cat-search.inc | grep admin This means I believe the outstanding ones are (koha-tmpl/intranet-tmpl/prog/en/modules/): - admin/auth_subfields_structure.tt - admin/clone-rules.tt - admin/marc_subfields_structure.tt - admin/searchengine/elasticsearch/mappings.tt One other was recommended by Katrin in comment #9: - plugins/plugins-home.tt
Created attachment 59507 [details] [review] [SIGNED-OFF] Bug 17793: Follow up for identified missing changes I ran: $ git grep -l cat-search.inc | grep admin This means I believe the outstanding ones are (koha-tmpl/intranet-tmpl/prog/en/modules/): - admin/auth_subfields_structure.tt - admin/clone-rules.tt - admin/marc_subfields_structure.tt - admin/searchengine/elasticsearch/mappings.tt One other was recommended by Katrin in comment #9: - plugins/plugins-home.tt Signed-off-by: Owen Leonard <oleonard@myacpl.org>
So we are going to loose the 'check in' and 'renew' actions from the header search and the syspref search will become the default one. Does everybody agree with that?
(In reply to Jonathan Druart from comment #16) > So we are going to loose the 'check in' and 'renew' actions from the header > search and the syspref search will become the default one. > > Does everybody agree with that? When you are in the admin section, that makes more sense. So yes, I agree with that.
I think it is somewhat arbitrary choice. If you leave checkout, I would have preferred to leave checkin too e.g. But the syspref search is handy. So I do not disagree :)
Hm, what about syspref|checkout|checkin|catsearch combination?
Created attachment 59608 [details] [review] Bug 17793: Make sysprefs search show on all Admin pages This patch is amended to use the sysprefs search on all Administration pages that do not have their own custom search. To test: 1) Go to Administration 2) Notice Catalogue search at the top - seems out of place. 3) Apply patch and refresh page. 4) Notice admin / sysprefs search now shows and is more appropriate. 5) Confirm searching for sysprefs still works This patch affects the following pages: - admin-home.pl - audio_alerts.pl - authtypes.pl - auth_tag_structure.pl - authorised_values.pl - biblio_framework.pl - marctagstructure.pl - branch_transfer_limits.pl - branches.pl - checkmarc.pl - classsources.pl - columns_settings.pl - didyoumean.pl - edi_accounts.pl - edi_ean_accounts.pl - fieldmapping.pl - item_circulation_alerts.pl - items_search_fields.pl - items_search_field.pl - item_types.pl - koha2marklinks.pl - matching-rules.pl - oai_sets.pl - oai_set_mappings.pl - patron-attr-types.pl - smart-rules.pl - transport-cost-matrix.pl - sms_providers.pl Sponsored-by: Catalyst IT Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 59609 [details] [review] Bug 17793: Follow up for identified missing changes I ran: $ git grep -l cat-search.inc | grep admin This means I believe the outstanding ones are (koha-tmpl/intranet-tmpl/prog/en/modules/): - admin/auth_subfields_structure.tt - admin/clone-rules.tt - admin/marc_subfields_structure.tt - admin/searchengine/elasticsearch/mappings.tt One other was recommended by Katrin in comment #9: - plugins/plugins-home.tt Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
(In reply to Josef Moravec from comment #19) > Hm, what about > > syspref|checkout|checkin|catsearch combination? Oops, did not see your comment before passing QA. Anybody has opinions?
(In reply to Jonathan Druart from comment #22) > Oops, did not see [Josef Moravec's] comment before passing QA. > Anybody has opinions? A new enhancement that actually has configurable search bar letting you also set the default active tab? In short, let this pass, and fix it elsewhere.
Pushed to master for 17.05, thanks Aleisha, Mark!
This won't get ported back to 16.11.x as it is an enhancement.