Bug 14639 - Extend Koha::MetadataRecord to handle serialization format
Summary: Extend Koha::MetadataRecord to handle serialization format
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 14645 15578
  Show dependency treegraph
 
Reported: 2015-08-03 14:36 UTC by Tomás Cohen Arazi
Modified: 2017-06-14 22:10 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 14639: (regression tests) Extend Koha::MetadataRecord to handle serialization format (3.51 KB, patch)
2015-08-03 14:56 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14639: Extend Koha::MetadataRecord to handle serialization format (1.64 KB, patch)
2015-08-03 14:56 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14639: (regression tests) Extend Koha::MetadataRecord to handle serialization format and record id (3.77 KB, patch)
2015-08-04 16:08 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14639: Extend Koha::MetadataRecord to handle serialization format and record id (3.01 KB, patch)
2015-08-04 16:08 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14639: (regression tests) Extend Koha::MetadataRecord to handle serialization format and record id (3.86 KB, patch)
2015-08-04 16:44 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14639: Extend Koha::MetadataRecord to handle serialization format and record id (3.01 KB, patch)
2015-08-04 16:44 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14639: (regression tests) Extend Koha::MetadataRecord to handle serialization format and record id (3.92 KB, patch)
2015-08-05 19:06 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 14639: Extend Koha::MetadataRecord to handle serialization format and record id (3.14 KB, patch)
2015-08-05 19:06 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 14639: (regression tests) Extend Koha::MetadataRecord to handle serialization format and record id (4.02 KB, patch)
2015-09-03 12:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14639: Extend Koha::MetadataRecord to handle serialization format and record id (3.24 KB, patch)
2015-09-03 12:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14639: (QA followup) make schema mandatory (2.61 KB, patch)
2015-09-03 13:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2015-08-03 14:36:39 UTC
Koha::MetadataRecord was introduced to abstract records, and carry information about the records. It is currently missing a way to know the serialization format used.
This is pretty important when it comes to using Koha::RecordProcessor to apply filters on records, as different filter could be needed depending on the serialization format: for example, a source could be a USMARC record and another one an XML record, both representing MARC21 metadata.

The same applies if you think of JSON records, etc.
Comment 1 Tomás Cohen Arazi 2015-08-03 14:56:33 UTC Comment hidden (obsolete)
Comment 2 Tomás Cohen Arazi 2015-08-03 14:56:38 UTC Comment hidden (obsolete)
Comment 3 Tomás Cohen Arazi 2015-08-04 16:08:18 UTC Comment hidden (obsolete)
Comment 4 Tomás Cohen Arazi 2015-08-04 16:08:25 UTC Comment hidden (obsolete)
Comment 5 Tomás Cohen Arazi 2015-08-04 16:44:00 UTC Comment hidden (obsolete)
Comment 6 Tomás Cohen Arazi 2015-08-04 16:44:06 UTC Comment hidden (obsolete)
Comment 7 M. Tompsett 2015-08-05 19:06:34 UTC Comment hidden (obsolete)
Comment 8 M. Tompsett 2015-08-05 19:06:38 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2015-08-18 15:29:05 UTC
Comment on attachment 41387 [details] [review]
Bug 14639: Extend Koha::MetadataRecord to handle serialization format and record id

Review of attachment 41387 [details] [review]:
-----------------------------------------------------------------

::: Koha/MetadataRecord.pm
@@ +84,5 @@
> +
> +    my $record = $params->{ record };
> +    my $schema = $params->{ schema } // 'marc21';
> +    my $format = $params->{ format } // 'MARC';
> +    my $id     = $params->{ id };

Don't you think these 4 params should be mandatory?
I would raise an exception if one of them is not given to the constructor.
Comment 10 Tomás Cohen Arazi 2015-08-18 16:22:45 UTC
(In reply to Jonathan Druart from comment #9)
> Comment on attachment 41387 [details] [review] [review]
> Bug 14639: Extend Koha::MetadataRecord to handle serialization format and
> record id
> 
> Review of attachment 41387 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: Koha/MetadataRecord.pm
> @@ +84,5 @@
> > +
> > +    my $record = $params->{ record };
> > +    my $schema = $params->{ schema } // 'marc21';
> > +    my $format = $params->{ format } // 'MARC';
> > +    my $id     = $params->{ id };
> 
> Don't you think these 4 params should be mandatory?
> I would raise an exception if one of them is not given to the constructor.

I agree. But my plan was to do that on a later stage, so the transition into using this is more smooth. So for now I just fallback to the current behaviour. Once this is in, I will provide the relevant regression tests for an API change.

Also, we need to get the exception handling code pushed before that too.
Comment 11 Jonathan Druart 2015-09-03 10:45:06 UTC
Ok, so just one thing prevents to pass QA I think.
The default value for schema should be C4::Context->preference('marcflavour').
And all calls where the pref is passed can be removed.
Don't you think?
Comment 12 Marcel de Rooy 2015-09-03 12:21:46 UTC
Created attachment 42312 [details] [review]
Bug 14639: (regression tests) Extend Koha::MetadataRecord to handle serialization format and record id

In order to use Koha::MetadataRecord as a container for moving records
around it is important to let it carry the serialization format
of the record object it was built with, so it is easier and cheaper to
make decisions about records.

This patch introduces regression tests for the changes to be made.
The 'format' param is introduced, and also sets default values:
  schema => 'marc21'
  format => 'MARC'

A new (optional) 'id' param is added so the record carries its own id outside
of it.

The default behaviour is preserved, and no changes are needed in places
Koha::MetadataRecord is used.
->new also returns undef if no record is passed, and raises a carped warning.

To test:
- Apply this test patch
- Run the new tests
  $ prove t/Koha_MetadataRecord.t
=> FAIL: Tests shoud fail as the changes are not implemented on Koha::MetadataRecord

Edit: made serialization format be upper-case to match what is used on Koha::Filter's

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 13 Marcel de Rooy 2015-09-03 12:21:50 UTC
Created attachment 42313 [details] [review]
Bug 14639: Extend Koha::MetadataRecord to handle serialization format and record id

The description of this changes is on the regression tests commit
message.

To test:
- Apply the test patch
- Run
  $ prove t/Koha_MetadataRecord.t
=> FAIL: Tests fail because changes are not implemented
- Apply this patch
- Run
  $ prove t/Koha_MetadataRecord.t
=> SUCCESS: tests pass
- Run
  $ prove t/Koha_Util_MARC.t
=> SUCCESS: it still passes
- Sign off :-D

NOTE: Tested as above. Read code. Seems to cover all cases.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 14 Jonathan Druart 2015-09-03 12:39:21 UTC
Marcel, I have let a comment 11, please don't ignore it.
Comment 15 Tomás Cohen Arazi 2015-09-03 13:19:37 UTC
Created attachment 42316 [details] [review]
Bug 14639: (QA followup) make schema mandatory

This patch makes the 'schema' param mandatory. It is passed in every
call on the current codebase, so it makes no harm now, but makes
the code less error-prone.

Tests for this situation are added to t/Koha_MetadataRecord.t (schema
param is omitted and new() returns undef and a carped warning).

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 16 Tomás Cohen Arazi 2015-09-07 15:13:08 UTC
Patches pushed to master.
Comment 17 Jonathan Druart 2015-09-22 12:54:17 UTC
It seems that this patch breaks some tests:

prove t/db_dependent/Koha_Authority.t
t/db_dependent/Koha_Authority.t .. 5/? 
#   Failed test 'Object authid is correct'
#   at t/db_dependent/Koha_Authority.t line 60.
#          got: undef
#     expected: '1'
# Looks like you failed 1 test of 11.
t/db_dependent/Koha_Authority.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/11 subtests 
        (less 3 skipped subtests: 7 okay)

Test Summary Report
-------------------
t/db_dependent/Koha_Authority.t (Wstat: 256 Tests: 11 Failed: 1)
  Failed test:  6
  Non-zero exit status: 1
Files=1, Tests=11,  2 wallclock secs ( 0.04 usr  0.00 sys +  1.24 cusr  0.06 csys =  1.34 CPU)
Result: FAIL
Comment 18 Tomás Cohen Arazi 2015-09-22 13:00:47 UTC
(In reply to Jonathan Druart from comment #17)
> It seems that this patch breaks some tests:
> 
> prove t/db_dependent/Koha_Authority.t
> t/db_dependent/Koha_Authority.t .. 5/? 
> #   Failed test 'Object authid is correct'
> #   at t/db_dependent/Koha_Authority.t line 60.
> #          got: undef
> #     expected: '1'
> # Looks like you failed 1 test of 11.
> t/db_dependent/Koha_Authority.t .. Dubious, test returned 1 (wstat 256,
> 0x100)
> Failed 1/11 subtests 
>         (less 3 skipped subtests: 7 okay)
> 
> Test Summary Report
> -------------------
> t/db_dependent/Koha_Authority.t (Wstat: 256 Tests: 11 Failed: 1)
>   Failed test:  6
>   Non-zero exit status: 1
> Files=1, Tests=11,  2 wallclock secs ( 0.04 usr  0.00 sys +  1.24 cusr  0.06
> csys =  1.34 CPU)
> Result: FAIL

I've tested them in Jessie and Trusty and cannot reproduce the failing tests.
Comment 19 Jonathan Druart 2015-09-22 13:18:23 UTC
Just retested with a clean DB and it passes correctly.
Comment 20 Jonathan Druart 2016-01-12 10:57:23 UTC
This patch set has introduced a big issue on Koha::Authority (now Koha::Metadata::Authority).
The authid method does not return the authid anymore...
The tests in t/db_dependent/Koha_Authority.t are bad: they skipped if the authid does not exist.
Comment 21 Jonathan Druart 2016-01-14 12:27:14 UTC
(In reply to Jonathan Druart from comment #20)
> This patch set has introduced a big issue on Koha::Authority (now
> Koha::Metadata::Authority).
> The authid method does not return the authid anymore...
> The tests in t/db_dependent/Koha_Authority.t are bad: they skipped if the
> authid does not exist.

Will be fixed by bug 15578.