[Koha-bugs] [Bug 5337] acq EAN search

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

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=5337

M. de Rooy <m.de.rooy at rijksmuseum.nl> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|Signed Off                  |Failed QA

--- Comment #30 from M. de Rooy <m.de.rooy at rijksmuseum.nl> ---
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
jean-manuel.broust at univ-lyon2.fr. 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.