http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11175
Olli-Antti Kivilahti <[email protected]> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff --- Comment #12 from Olli-Antti Kivilahti <[email protected]> --- (In reply to Katrin Fischer from comment #7) > Hi, > > starting with a code review: Thank you for the effort Katrin! It is much appreciated! > > - Small thing: Please break your commit messages into shorter lines so > they are easier to read in a terminal window. I hope it's better. I also hope with shorter lines you don't mean the dreaded 80 character maximum. > > - Please provide a follow-up for the Bootstrap theme. As prog and > CCSR are going to be deprecated this is mandatory now. > Follow up provided! > - Database update adding the new system preference is missing. > Provided! > - The way this is coded it's a MARC21 specific feature. The rcn index > and 001 > $w linking are not MARC agnostic. Please check for the > marcflavor in your code and include a note > about the limitation to MARC21 in the syspref description. > Added a note to the system preference. Didn't add any syspref checks to the business layer. Uncertain if rcn is unused in UNIMARC. If rcn-index is unpopulated, then should cause zero extra overhead. If rcn is populated in UNIMARC, then this feature should work? > - Some of the comments look like TODOs - maybe something you want to > take another look at? > Apologies for that. I think it was a classical mistake. > - Creating our 'own' XML in a MARCXML record doesn't look right to me. I > would much prefer if that could be expressed in normal MARC21 fields > and subfields. > I guess I could differentiate from the searched component part records the marc field they use to make the linking. All the data is available since the found records are converted into MARC::Record objects. Then this data could be injected to appropriate MARC::Fields to construct a proper marc record. If this behaviour is wanted maybe it is better to create them when these component part relations are defined? Thus these links would be preserved during MARC-export function. The effort however doesn't justify the perceived benefits in my opinion. If you can demonstrate the benefits (like displaying different kinds of component relations under different heading) a follow-up can be prepared and designed. Also a "own" XML in MARCXML is already there inside the <sysprefs>-tag. > - With the heading "Child records" you assume that it will be always > a component part, but that's not true. 001 > $w linkings are used > to express a lot of different relationships between records. I think > putting a bit more thought into the various relationships could be > good here. > You are very correct. I went to the MARC21 manual and looked into the various aspects of linkings. I changed the terminology to "child" -> "component part". > Also the QA script points out some issues: > > FAIL C4/XSLT.pm > FAIL pod > *** ERROR: Unknown command 'head' in file C4/XSLT.pm > *** ERROR: Spurious text after =cut in file C4/XSLT.pm I don't know from where these are coming from? > OK forbidden patterns > OK valid > FAIL critic > # Variables::ProhibitConditionalDeclarations: Got 1 > violation(s). Fixed this issue. perlcritic++ Happy to deliver! -- 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/
