https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893 --- Comment #201 from David Gustafsson <glasklas@gmail.com> --- (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.