http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11175 Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff --- Comment #12 from Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> --- (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.