https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893
--- Comment #201 from David Gustafsson <[email protected]> --- (In reply to Joonas Kylmälä from comment #197) > 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. I for some reason read it as "die" should be replaced with "croak". I have not replaced them with exceptions instead. I think it's a bit of an anti-pattern to put a block of code into a function if only invoked in one location, so would prefer to leave the code as it is. About the syspref, sadly there is no persistent variable store in Koha that I know of. In other places where this is needed (like Koha version number), it is stored in the syspref-table. Fixed the grammatical error. I don't know how much time I have to spend on unit tests. I think there is close to 100% code coverage trough the other tests, but I recently checked this with cover and there are some minor execution-paths that are never reached. Which I could fix. If I where to write unit tests for all new helper-methods added it could take some time. -- You are receiving this mail because: You are watching all bug changes. _______________________________________________ Koha-bugs mailing list [email protected] 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/
