[Koha-bugs] [Bug 11175] Show the parent record's component parts in the detailed views.

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

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

Olli-Antti Kivilahti <olli-antti.kivilahti at jns.fi> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|Failed QA                   |Needs Signoff

--- Comment #12 from Olli-Antti Kivilahti <olli-antti.kivilahti at 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.