https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893
--- Comment #197 from Joonas Kylmälä <joonas.kylm...@helsinki.fi> --- Thanks for the patch! Instead of croak or die the exceptions need to be used, e.g. Koha::Exceptions::Exception->throw(""). Please do a git diff origin/master..HEAD to see all the dies and croaks (assuming HEAD contains your patches). And the many for loops I mentioned are in the subroutine marc_records_to_documents Koha/SearchEngine/Elasticsearch.pm – though take this as an optional thing to fix as I know this bug is blocking quite many other things. Continuing still code review... About installer/data/mysql/atomicupdate/bug_19893_elasticsearch_index_status_sysprefs.sql – a) is it a good idea to have these as a syspref (even though the user cannot see them)? b) if it's a good idea then the sysprefs should also be in installer/data/mysql/sysprefs.sql because otherwise in a new Koha installation the syspref will be missing. In the file Koha/SearchEngine/Elasticsearch.pm: s/string on the format/string in the format/ Then we also need a test plan for this code. Like what steps need to be taken to index authorities and biblios and what should be the expected result. Unit tests (https://wiki.koha-community.org/wiki/Coding_Guidelines#PERL17:_Unit_tests_are_required_.28updated_Apr_26.2C_2017.29): at least decode_record_from_result is missing one If all the above mentioned things get fixed then I'm probably ready to sign-off but I still need to test this code actually before that. -- You are receiving this mail because: You are watching all bug changes. _______________________________________________ Koha-bugs mailing list Koha-bugs@lists.koha-community.org http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/