Bug 37380 - Move GetMarcControlnumber to Koha namespace
Summary: Move GetMarcControlnumber to Koha namespace
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Tomás Cohen Arazi (tcohen)
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on:
Blocks: 17628
  Show dependency treegraph
 
Reported: 2024-07-16 18:19 UTC by Tomás Cohen Arazi (tcohen)
Modified: 2024-12-05 03:28 UTC (History)
4 users (show)

See Also:
GIT URL:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.11.00
Circulation function:


Attachments
Bug 37380: Unit tests (2.36 KB, patch)
2024-07-16 18:44 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 37380: Add get_control_number() to the metadata extractor (1.34 KB, patch)
2024-07-16 18:44 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 37380: Replace uses of GetMarcControlnumber for get_control_number() (12.65 KB, patch)
2024-07-16 18:44 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 37380: Unit tests (2.40 KB, patch)
2024-07-26 19:00 UTC, Sam Lau
Details | Diff | Splinter Review
Bug 37380: Add get_control_number() to the metadata extractor (1.38 KB, patch)
2024-07-26 19:00 UTC, Sam Lau
Details | Diff | Splinter Review
Bug 37380: Replace uses of GetMarcControlnumber for get_control_number() (12.67 KB, patch)
2024-07-26 19:00 UTC, Sam Lau
Details | Diff | Splinter Review
Bug 37380: Unit tests (2.60 KB, patch)
2024-09-05 18:45 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 37380: Add get_control_number() to the metadata extractor (1.52 KB, patch)
2024-09-05 18:45 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 37380: Replace uses of GetMarcControlnumber for get_control_number() (13.12 KB, patch)
2024-09-05 18:45 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 37380: Unit tests (2.67 KB, patch)
2024-09-09 11:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 37380: Add get_control_number() to the metadata extractor (1.59 KB, patch)
2024-09-09 11:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 37380: Replace uses of GetMarcControlnumber for get_control_number() (13.19 KB, patch)
2024-09-09 11:48 UTC, Jonathan Druart
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 (tcohen) 2024-07-16 18:19:47 UTC
Easy one to move.
Comment 1 Tomás Cohen Arazi (tcohen) 2024-07-16 18:44:52 UTC
Created attachment 169058 [details] [review]
Bug 37380: Unit tests
Comment 2 Tomás Cohen Arazi (tcohen) 2024-07-16 18:44:55 UTC
Created attachment 169059 [details] [review]
Bug 37380: Add get_control_number() to the metadata extractor

This simple patch implements C4::Biblio::GetMarcControlnumber in the
right spot on the Koha::* namespace.

To test:
1. Apply this patches
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/Koha/Biblio/Metadata/Extractor/MARC.t
=> SUCCESS: Tests pass!
3. Sign off :-D
Comment 3 Tomás Cohen Arazi (tcohen) 2024-07-16 18:44:57 UTC
Created attachment 169060 [details] [review]
Bug 37380: Replace uses of GetMarcControlnumber for get_control_number()
Comment 4 Sam Lau 2024-07-26 19:00:33 UTC
Created attachment 169777 [details] [review]
Bug 37380: Unit tests

Signed-off-by: Sam Lau <samalau@gmail.com>
Comment 5 Sam Lau 2024-07-26 19:00:35 UTC
Created attachment 169778 [details] [review]
Bug 37380: Add get_control_number() to the metadata extractor

This simple patch implements C4::Biblio::GetMarcControlnumber in the
right spot on the Koha::* namespace.

To test:
1. Apply this patches
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/Koha/Biblio/Metadata/Extractor/MARC.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Sam Lau <samalau@gmail.com>
Comment 6 Sam Lau 2024-07-26 19:00:37 UTC
Created attachment 169779 [details] [review]
Bug 37380: Replace uses of GetMarcControlnumber for get_control_number()

Signed-off-by: Sam Lau <samalau@gmail.com>
Comment 7 Jonathan Druart 2024-09-05 14:11:12 UTC
+                my $control_num = $biblio->get_control_number();

This is not correct.
Comment 8 Jonathan Druart 2024-09-05 14:12:53 UTC
+        is( $extractor->get_control_number, undef, 'Returns undef if no 001' );

-    is( $controlnumber, '', 'GetMarcControlnumber handles records without 001' );

Change in behaviour here, did you investigate it?
Comment 9 Tomás Cohen Arazi (tcohen) 2024-09-05 17:53:09 UTC
(In reply to Jonathan Druart from comment #7)
> +                my $control_num = $biblio->get_control_number();
> 
> This is not correct.

You're right!

(In reply to Jonathan Druart from comment #8)
> +        is( $extractor->get_control_number, undef, 'Returns undef if no
> 001' );
> 
> -    is( $controlnumber, '', 'GetMarcControlnumber handles records without
> 001' );
> 
> Change in behaviour here, did you investigate it?

I did! But I will re-do just in case!
Comment 10 Tomás Cohen Arazi (tcohen) 2024-09-05 18:45:09 UTC
Created attachment 171099 [details] [review]
Bug 37380: Unit tests

Signed-off-by: Sam Lau <samalau@gmail.com>
Comment 11 Tomás Cohen Arazi (tcohen) 2024-09-05 18:45:12 UTC
Created attachment 171100 [details] [review]
Bug 37380: Add get_control_number() to the metadata extractor

This simple patch implements C4::Biblio::GetMarcControlnumber in the
right spot on the Koha::* namespace.

To test:
1. Apply this patches
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/Koha/Biblio/Metadata/Extractor/MARC.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Sam Lau <samalau@gmail.com>
Comment 12 Tomás Cohen Arazi (tcohen) 2024-09-05 18:45:15 UTC
Created attachment 171101 [details] [review]
Bug 37380: Replace uses of GetMarcControlnumber for get_control_number()

Signed-off-by: Sam Lau <samalau@gmail.com>
Comment 13 Tomás Cohen Arazi (tcohen) 2024-09-05 18:53:52 UTC
Hi, I rolled back with making it return undef. As I noticed the tradeoff was not good. We ended up having to add `// q{}` in a bunch of places, which is not worth. Tests and code have been adjusting to go back to the old behavior.

I wanted to share that I made some exhaustive tests on MARC::Record and MARC::Field behavior...

```
perl -MDDP \
     -MMARC::File::XML \
     -MMARC::Record \
     -MMARC::Field \
-e 'my $f = MARC::Field->new( "001", undef); p($f->data); my $r = MARC::Record->new(); $r->append_fields($f); $r->append_fields(MARC::Field->new("245", "", "", a => "Stuff")); my $xml = $r->as_xml; $r = MARC::Record->new_from_xml($xml); p($r->as_xml);'
```

My conclusion is that MARC::Field allows adding `undef` values but it will then warn when appending them. But I also found out that once the record gets serialized as XML, the `undef` is replaced by an empty string and that's how it is going to stay forever, with our code. So having `undef` is a weird artificially created scenario.
Comment 14 Jonathan Druart 2024-09-09 11:48:39 UTC
Created attachment 171189 [details] [review]
Bug 37380: Unit tests

Signed-off-by: Sam Lau <samalau@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 15 Jonathan Druart 2024-09-09 11:48:42 UTC
Created attachment 171190 [details] [review]
Bug 37380: Add get_control_number() to the metadata extractor

This simple patch implements C4::Biblio::GetMarcControlnumber in the
right spot on the Koha::* namespace.

To test:
1. Apply this patches
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/Koha/Biblio/Metadata/Extractor/MARC.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Sam Lau <samalau@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 16 Jonathan Druart 2024-09-09 11:48:44 UTC
Created attachment 171191 [details] [review]
Bug 37380: Replace uses of GetMarcControlnumber for get_control_number()

Signed-off-by: Sam Lau <samalau@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 17 Jonathan Druart 2024-09-09 11:48:53 UTC
Looking at catalogue/export.pl:

my $marc   = $biblio->metadata->record({ embed_items => 1 });

my $metadata_extractor = $biblio->metadata_extractor;

We are going to fetch metadata twice. We should certainly improve it at some point. Not considering blocker for this change however.
Comment 18 Katrin Fischer 2024-09-16 08:46:13 UTC
I originally introduced this method in bug 5430 which which seems like ages ago now. But I won't miss it :)
Comment 19 Katrin Fischer 2024-09-16 08:54:04 UTC
Pushed for 24.11!

Well done everyone, thank you!
Comment 20 Katrin Fischer 2024-11-22 09:45:21 UTC
Skipping for release notes as this is quite technical and kind of self-explanatory to developers.
Comment 21 David Nind 2024-12-05 03:28:07 UTC
This is more developer focused, and is not covered in the manual - so no update to the manual required.