[Koha-bugs] [Bug 19893] Alternative optimized indexing for Elasticsearch

[bugzilla-daemon at bugs.koha-community.org]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19893

--- Comment #201 from David Gustafsson <glasklas at 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.