http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=5337
M. de Rooy <[email protected]> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #30 from M. de Rooy <[email protected]> --- QA Comment: Although I agree with Ian that a more general solution would be welcome, I do not oppose this patch. But this problem will come up again. And if we continuously would say "Next time we must solve it better..", we will never come there. So, it would be nice to trigger that discussion now. But who will take care of the generic housekeeping solution.. Specific comments on this patch below. Note that Paul wanted to pass qa already on it, but I unfortunately disagree. Note that it was signed-off by [email protected]. The patch commit does not show that however. Please fix that too. The atomicupdate file could be removed (adding the field is done in updatedatabase)? The version number there should be filled with XXX instead of 005. This is a BLOCKER. Why do you need a index in deletedbiblioitems on ean? Please clarify. The commit reads: However, if you already have records with ean, you will have to run misc/batchRebuildBiblioTables.pl to populate biblioitems.ean. Should this be published too in the update print statement or anywhere else too? Routine GetSubscriptions: You add line: $sqlwhere .= ( $sqlwhere ? " AND " : " WHERE " ) . "(" . join( ") OR (", @sqlstrings ) . ")"; Please add an additional ( and ) around this construct, just as is done a few lines above it. This prevents wrong interpretation of the whole expression. Since you are adding parameters to existing routines: Did you check if you did not miss any function call for the two routines changed? You should grep on the name only. Do not assume that the name always is immediately followed by parenthesis. Could be whitespace also. Note that I find instances of test scripts referring to the function that you do not change (e.g. t/db_dependent/Serials.t). Please verify. BLOCKER too. Please clarify: Why a span if biblionumber passed and otherwise a label in neworderempty.tt change? Changing status to reflect need for corrections and clarification. -- 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/
