http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11395
M. de Rooy <m.de.r...@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA --- Comment #121 from M. de Rooy <m.de.r...@rijksmuseum.nl> --- QA Comment: The new code looks quite good to me. I have some minor comments as to design issues (no blockers). My only concerns about code concentrate on changes to marc_mod_templates. Since this report actually is a new interface on top of those rules, I would not oppose pushing it without the patches changing marc_mod_templates. If we want to push it with those changes (last two now), I think we should first make some corrections (see below). It is still somewhat unfinished now. Aside from that, I would also suggest changes to marc mod templates to prevent unneeded record saving (with ModBiblio). And aside from that, I would love to see additional code for indicators in Koha/SimpleMarc.pm (containing the actual routines for the record updates). My conclusion: I am setting the status back to Passed QA to pass the ball to Tomas. If you also feel that we should wait for an improvement of the last two patches, please set to FQA. Or push without those two? === Details: tools/batch_record_modification.pl: Design may still need some work (no blocker). E.g. label before the selected marc mod template (position&length, color). Typing an empty line in the biblio number field gives the warning: This biblionumber does not exist in the database. (I would personally ignore the whitespace, not warn here). In the selected biblio number table you have Preview MARC. Just a style issue again: It made me wonder if we had two links there. But it is only one. Note that your column title is already Preview. In line with Cataloging Search, please remove the Preview string in the table and just keep MARC. About Preview: It was not clear to me at first sight that this preview would show a modified record. I interpreted this view as a way to check the record just before updating it. But it actually is a way to test the result of the rule. If you have another name that makes this intuitively more clear, you are welcome :) The results list every biblio number successfully modified. This list could be very long. Since I gave Koha the biblio numbers (in a file or via text box), I would be more interested in the numbers where an error came up. At the same time not all "successfully modified" records have actually been modified. Which ones were really changed? That may however be considered outside the scope of this report and would need some code adjustments in MarcModificationTemplates.pm to make the copy/edit/delete routines return some meaningful value. In that case it would not be necessary in this code to always run ModBiblio (with less [theoretical] risks to hurt data that you did not want to change in the first place).. *** Bug-11395-Raise-an-alert-if-control-field This patch deals with changes in marc mod templates itself. Note that I can still add a rule like: Update field 001$a with value XX (But 001 has no subfields.) Delete field 001$b When moving 022$a to 001, I still have the warning: Both subfields should be filled or empty. Add field 001 value XX: Not allowed, needs subfield !! 001 has not subfields. etc. When you allow rules like this, you can easily generate errors like: The biblio 1 has not been modified. An error occurred on modifying it. (The error was: Can't use an undefined value as an ARRAY reference at /usr/local/share/perl5/MARC/Field.pm line 453. , see the Koha logfile for more information) -- You are receiving this mail because: You are watching all bug changes. _______________________________________________ Koha-bugs mailing list Koha-bugs@lists.koha-community.org 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/