Created attachment 118461 [details] [review]
Bug 27981: Add option to automatically set 001 to the biblionumber

This patch adds a new system preference:
autoControlNumber

The option "biblionumber" will set field 001 to the biblionumber when you create a new record
or edit an existing record

If set to 'OFF' the 001 field wil not be touched

To test:
 1 - Apply patches and updatedatabase
 2 - Create a new record with no 001 field
 3 - Save and view the MAC, confirm there is no 001
 4 - Set the system preference to 'biblionumber'
 5 - Edit the record you created previously
 6 - Note the 001 is prepopulated with the biblionumber
 7 - Delete the field
 8 - Save the record
 9 - View the MARC, the 001 is filled with biblionumber
10 - Edit the record
11 - Set the 001 to a different value "Not the biblionumber"
12 - Save
13 - View the marc and confirm the value you entered is retained
14 - Edit a record with an existing 001 that is not the biblionumber
15 - Save and confirm 001 is not updated

*** Bug 9921 has been marked as a duplicate of this bug. ***

# Subtest: AddBiblio
    1..4
    not ok 1 - expected warnings when adding too long LCCN
    #   Failed test 'expected warnings when adding too long LCCN'
    #   at t/db_dependent/Biblio.t line 61.
    # found warning: Use of uninitialized value in string eq at /usr/share/koha/C4/Biblio.pm line 2630.
    # expected to find warning: (?^u:Data too long for column 'lccn')
    # expected to find warning: (?^u:Data too long for column 'lccn')
    not ok 2 - AddBiblio returns undef for biblionumber if something went wrong
    #   Failed test 'AddBiblio returns undef for biblionumber if something went wrong'
    #   at t/db_dependent/Biblio.t line 63.
    #          got: '1572'
    #     expected: undef
    not ok 3 - AddBiblio returns undef for biblioitemnumber if something went wrong
    #   Failed test 'AddBiblio returns undef for biblioitemnumber if something went wrong'
    #   at t/db_dependent/Biblio.t line 65.
    #          got: '1508'
    #     expected: undef
    not ok 4 - No biblio should have been added if something went wrong
    #   Failed test 'No biblio should have been added if something went wrong'
    #   at t/db_dependent/Biblio.t line 68.
    #          got: '1031'
    #     expected: '1030'
    # Looks like you failed 4 tests of 4.
not ok 2 - AddBiblio
#   Failed test 'AddBiblio'
#   at t/db_dependent/Biblio.t line 70.

Created attachment 118513 [details] [review]
Bug 27981: (follow-up) Set pref off for existing tests

I don't recreate this, can you test against master too? 
I don't see how these changes would case these failures

(In reply to Marcel de Rooy from comment #3)
> #   Failed test 'AddBiblio'
> #   at t/db_dependent/Biblio.t line 70.

The patch doesn't apply:

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 27981: Add option to automatically set 001 to the biblionumber
Applying: Bug 27981: (follow-up) Set pref off for existing tests
error: sha1 information is lacking or useless (members/memberentry.pl).
error: could not build fake ancestor
Patch failed at 0001 Bug 27981: (follow-up) Set pref off for existing tests

Needs to be rebased?

Created attachment 119704 [details] [review]
Bug 27981: Add option to automatically set 001 to the biblionumber

This patch adds a new system preference:
autoControlNumber

The option "biblionumber" will set field 001 to the biblionumber when you create a new record
or edit an existing record

If set to 'OFF' the 001 field wil not be touched

To test:
 1 - Apply patches and updatedatabase
 2 - Create a new record with no 001 field
 3 - Save and view the MAC, confirm there is no 001
 4 - Set the system preference to 'biblionumber'
 5 - Edit the record you created previously
 6 - Note the 001 is prepopulated with the biblionumber
 7 - Delete the field
 8 - Save the record
 9 - View the MARC, the 001 is filled with biblionumber
10 - Edit the record
11 - Set the 001 to a different value "Not the biblionumber"
12 - Save
13 - View the marc and confirm the value you entered is retained
14 - Edit a record with an existing 001 that is not the biblionumber
15 - Save and confirm 001 is not updated

Created attachment 119705 [details] [review]
Bug 27981: (follow-up) Set pref off for existing tests

Created attachment 119717 [details] [review]
Bug 27981: (follow-up) Set pref off for existing tests

The patches applies now. I have tested it and can confirm that it works
as intended.

Signed-off-by: Frank Hansen <frank.hansen@ub.lu.se>

Created attachment 121805 [details] [review]
Bug 27981: (follow-up) Set pref off for existing tests

The patches applies now. I have tested it and can confirm that it works
as intended.

Signed-off-by: Frank Hansen <frank.hansen@ub.lu.se>
Signed-off-by: Salman Ali <salman@outaouais>

 FAIL   members/memberentry.pl
   OK     critic
   FAIL   forbidden patterns
                forbidden pattern: Data::Dumper::Dumper (line 112)

Processing additional checks

        * Signed-off-by line 'Signed-off-by: Salman Ali <salman@outaouais>' seems invalid

This email adress does not work, Salman ;)

Note that the Biblio.t test already fails without this patch..

+        unless($record->field('001')){
+            $record->insert_fields_ordered(MARC::Field->new('001', $biblionumber));
+        }elsif($record->field('001')->data() eq 'biblionumber'){
+            $record->field('001')->update($biblionumber);
+        }

Please explain.
Why do you test eq 'biblionumber' ??
Why not always overwrite 001 with $biblionumber if the pref is set ?

(In reply to Marcel de Rooy from comment #14)
> +        unless($record->field('001')){
> +            $record->insert_fields_ordered(MARC::Field->new('001',
> $biblionumber));
> +        }elsif($record->field('001')->data() eq 'biblionumber'){
> +            $record->field('001')->update($biblionumber);
> +        }
> 
> Please explain.
> Why do you test eq 'biblionumber' ??
> Why not always overwrite 001 with $biblionumber if the pref is set ?

Similar to autobarcode or automembernum - we will default, but if you force a new value we let it stand - I think allowing the librarian to force a different number allows for collections that fall outside the norm.

It could be useful for an OAI set for instance

(In reply to Nick Clemens from comment #15)
> (In reply to Marcel de Rooy from comment #14)
> > +        unless($record->field('001')){
> > +            $record->insert_fields_ordered(MARC::Field->new('001',
> > $biblionumber));
> > +        }elsif($record->field('001')->data() eq 'biblionumber'){
> > +            $record->field('001')->update($biblionumber);
> > +        }
> > 
> > Please explain.
> > Why do you test eq 'biblionumber' ??
> > Why not always overwrite 001 with $biblionumber if the pref is set ?
> 
> Similar to autobarcode or automembernum - we will default, but if you force
> a new value we let it stand - I think allowing the librarian to force a
> different number allows for collections that fall outside the norm.
> 
> It could be useful for an OAI set for instance

Maybe you do not understand my point?
Read this line:
> > +        }elsif($record->field('001')->data() eq 'biblionumber'){

You are comparing with the string biblionumber !

Might start to get what you mean btw..

The problem with leaving a number might be when you are cloning records with Edit as new. When you do not modify that 001, it is probably wrong.

This still needs more attention, I am afraid?

+        }elsif($record->field('001')->data() eq 'biblionumber'){

How and when will the 001 ever be string biblionumber ?

Created attachment 122280 [details] [review]
Bug 27981: (follow-up) Handle duplciation and remove debug

There were some additional lines in the commits on accident

I add a 'duplication' option to svc/bib and pass the parameter from rancor
to remove the 001 if autoControlNumber is set

I add code to addbiblio.pl to remove 001 in standard editor

To test:
1 - Follow above test plan
2 - Edit a record as duplicate when using the advanced editor
3 - Confirm the 001 does not load, but record saves correctly
4 - Edit the record
5 - Switch to 'basic editor'
6 - Save, then view record
7 - Edit as duplicate in basic editor
8 - Confirm the 001 is removed
9 - Confirm the 001 is added on save

(In reply to Marcel de Rooy from comment #20)
> +        }elsif($record->field('001')->data() eq 'biblionumber'){
> 
> How and when will the 001 ever be string biblionumber ?

Either I was thinking this would be a way to force it, or I was editing my conditionals and got confused between syspref and record data. Removed in any case

(In reply to Marcel de Rooy from comment #18)
> The problem with leaving a number might be when you are cloning records with
> Edit as new. When you do not modify that 001, it is probably wrong.

New patch, back to NSO

Created attachment 122301 [details] [review]
Bug 27981: (follow-up) Handle duplciation and remove debug

There were some additional lines in the commits on accident

I add a 'duplication' option to svc/bib and pass the parameter from rancor
to remove the 001 if autoControlNumber is set

I add code to addbiblio.pl to remove 001 in standard editor

To test:
1 - Follow above test plan
2 - Edit a record as duplicate when using the advanced editor
3 - Confirm the 001 does not load, but record saves correctly
4 - Edit the record
5 - Switch to 'basic editor'
6 - Save, then view record
7 - Edit as duplicate in basic editor
8 - Confirm the 001 is removed
9 - Confirm the 001 is added on save

Signed-off-by: hakam <hakam@inlibro.com>

Hi Hakam, should this be switched to signed off? 

I notice you only reattached the last patch. Usually we will add sign-off lines to all patches and then reattach them all with them. If you need help on how to do so, please let us know.

(In reply to Katrin Fischer from comment #25)
> Hi Hakam, should this be switched to signed off? 
> 
> I notice you only reattached the last patch. Usually we will add sign-off
> lines to all patches and then reattach them all with them. If you need help
> on how to do so, please let us know.

Hakam signed off on two patches

(In reply to Katrin Fischer from comment #25)
> Hi Hakam, should this be switched to signed off? 
> 
> I notice you only reattached the last patch. Usually we will add sign-off
> lines to all patches and then reattach them all with them. If you need help
> on how to do so, please let us know.

Oops it is somebody else :)

I will pickup QA again here

# Subtest: AddBiblio
    1..5
    not ok 1 - expected warnings when adding too long LCCN
    #   Failed test 'expected warnings when adding too long LCCN'
    #   at t/db_dependent/Biblio.t line 61.
    # found warning: Use of uninitialized value in string eq at /usr/share/koha/C4/Biblio.pm line 2632.
    # expected to find warning: (?^u:Data too long for column 'lccn')
    # expected to find warning: (?^u:Data too long for column 'lccn')
    not ok 2 - AddBiblio returns undef for biblionumber if something went wrong
    #   Failed test 'AddBiblio returns undef for biblionumber if something went wrong'
    #   at t/db_dependent/Biblio.t line 63.
    #          got: '129'
    #     expected: undef
    not ok 3 - AddBiblio returns undef for biblioitemnumber if something went wrong
    #   Failed test 'AddBiblio returns undef for biblioitemnumber if something went wrong'
    #   at t/db_dependent/Biblio.t line 65.
    #          got: '129'
    #     expected: undef
    not ok 4 - No biblio should have been added if something went wrong
    #   Failed test 'No biblio should have been added if something went wrong'
    #   at t/db_dependent/Biblio.t line 68.
    #          got: '3'
    #     expected: '2'
    not ok 5 - The biblionumber is correctly passed to BiblioAutoLink
    #   Failed test 'The biblionumber is correctly passed to BiblioAutoLink'
    #   at t/db_dependent/Biblio.t line 81.
    # found warning: Use of uninitialized value in string eq at /usr/share/koha/C4/Biblio.pm line 2632.
    # found warning: My biblionumber is 130 and my frameworkcode is  at t/db_dependent/Biblio.t line 78.
    # expected to find warning: (?^u:My biblionumber is \d+ and my frameworkcode is )
    # Looks like you failed 5 tests of 5.
not ok 2 - AddBiblio

Please cleanup your patches!

diff --git a/members/memberentry.pl b/members/memberentry.pl
index abae2ec53e..da8a324c78 100755
--- a/members/memberentry.pl
+++ b/members/memberentry.pl
@@ -109,9 +109,7 @@ $template->param( relationships => scalar $patron->guarantor_relationships ) if

 my @relations = split /\|/, C4::Context->preference('borrowerRelationship'), -1;
 @relations = ('') unless @relations;
-warn Data::Dumper::Dumper( \@relations );
 my $empty_relationship_allowed = grep {$_ eq ""} @relations;
-warn $empty_relationship_allowed;
 $template->param( empty_relationship_allowed => $empty_relationship_allowed );

 my $guarantor_id = $input->param('guarantor_id');

This really does not need to be in any of these patches.

Please amend the first patch where you delete the buggy lines too ;)

Created attachment 122344 [details] [review]
Bug 27981: Add option to automatically set 001 to the biblionumber

This patch adds a new system preference:
autoControlNumber

The option "biblionumber" will set field 001 to the biblionumber when you create a new record
or edit an existing record

If set to 'OFF' the 001 field wil not be touched

When duplicating a record the 001 will be removed if autoControlNumber is set
A 'duplication' option is add to svc/bib and rancor passes the parameter,
the basic editor simply removes the field

To test:
 1 - Apply patches and updatedatabase
 2 - Create a new record with no 001 field
 3 - Save and view the MAC, confirm there is no 001
 4 - Set the system preference to 'biblionumber'
 5 - Edit the record you created previously
 6 - Note the 001 is prepopulated with the biblionumber
 7 - Delete the field
 8 - Save the record
 9 - View the MARC, the 001 is filled with biblionumber
10 - Edit the record
11 - Set the 001 to a different value "Not the biblionumber"
12 - Save
13 - View the marc and confirm the value you entered is retained
14 - Edit a record with an existing 001 that is not the biblionumber
15 - Save and confirm 001 is not updated

To testi duplication:
1 - Edit a record as duplicate when using the advanced editor
2 - Confirm the 001 does not load, but record saves correctly
3 - Edit the record
4 - Switch to 'basic editor'
5 - Save, then view record
6 - Edit as duplicate in basic editor
7 - Confirm the 001 is removed
8 - Confirm the 001 is added on save

(In reply to Marcel de Rooy from comment #30)
> Please cleanup your patches!
> Please amend the first patch where you delete the buggy lines too ;)

Patches squashed, back to NSO


(In reply to Marcel de Rooy from comment #29)

>     # found warning: Use of uninitialized value in string eq at
> /usr/share/koha/C4/Biblio.pm line 2632.
>     # found warning: My biblionumber is 130 and my frameworkcode is  at
> t/db_dependent/Biblio.t line 78.
>     # expected to find warning: (?^u:My biblionumber is \d+ and my
> frameworkcode is )
>     # Looks like you failed 5 tests of 5.
> not ok 2 - AddBiblio

I get the extra warning if i run the test before the DB update, but all is good after

Created attachment 122356 [details] [review]
Bug 27981: (QA follow-up) Adjust atomic update

[1] Insert ignore.
[2] Remove references to bug 9921.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

(In reply to Nick Clemens from comment #32)
> I get the extra warning if i run the test before the DB update, but all is
> good after

Still have it.

> A 'duplication' option is add to svc/bib and rancor passes the parameter,
> the basic editor simply removes the field

I dont think that we should go that way. The changes to svc/bib do not look good to me. The strange d parameter in the path svc/bib/d1 is rather hacky.

Personally, I would rather go away from the whole path_info tric to get the biblionumber and just have a real biblionumber parameter. But we should not spend too much time on this old service script when we have an API.

Instead of changing svc/bib, could you move the change to where rancor saves a duplicated record?

Adding Jonathan to the discussion.

(In reply to Marcel de Rooy from comment #34)
> (In reply to Nick Clemens from comment #32)
> > I get the extra warning if i run the test before the DB update, but all is
> > good after
> 
> Still have it.

Forget this one: strict sql mode stuff..

Yes, that's definitely hacky and it will complicate the move when we will replace svc/bib with a REST API call.

Nick: There is also another point still. If we update 001, we should update the orgcode too in 003.
See also bug 28491 for authority counterpart.

Created attachment 122672 [details] [review]
Bug 27981: Add option to automatically set 001 to the biblionumber

This patch adds a new system preference:
autoControlNumber

The option "biblionumber" will set field 001 to the biblionumber when you create a new record
or edit an existing record

If set to 'OFF' the 001 field wil not be touched

When duplicating a record the 001 will be removed if autoControlNumber is set

To test:
 1 - Apply patches and updatedatabase
 2 - Create a new record with no 001 field
 3 - Save and view the MAC, confirm there is no 001
 4 - Set the system preference to 'biblionumber'
 5 - Edit the record you created previously
 6 - Note the 001 is prepopulated with the biblionumber
 7 - Delete the field
 8 - Save the record
 9 - View the MARC, the 001 is filled with biblionumber
10 - Edit the record
11 - Set the 001 to a different value "Not the biblionumber"
12 - Save
13 - View the marc and confirm the value you entered is retained
14 - Edit a record with an existing 001 that is not the biblionumber
15 - Save and confirm 001 is not updated

To test duplication:
1 - Edit a record as duplicate when using the advanced editor
2 - Confirm the 001 does not load, but record saves correctly
3 - Edit the record
4 - Switch to 'basic editor'
5 - Save, then view record
6 - Edit as duplicate in basic editor
7 - Confirm the 001 is removed
8 - Confirm the 001 is added on save

(In reply to Marcel de Rooy from comment #38)
> Nick: There is also another point still. If we update 001, we should update
> the orgcode too in 003.
> See also bug 28491 for authority counterpart.

adding orgcode feels like scope creep - file another bug?

(In reply to Nick Clemens from comment #40)
> (In reply to Marcel de Rooy from comment #38)
> > Nick: There is also another point still. If we update 001, we should update
> > the orgcode too in 003.
> > See also bug 28491 for authority counterpart.
> 
> adding orgcode feels like scope creep - file another bug?

Done. See also

Commit promises:
The option "biblionumber" will set field 001 to the biblionumber when you create a new record     or edit an existing record

Create works, modify does not.

Created attachment 122674 [details] [review]
Bug 27981: (QA follow-up) Adjust atomic update

[1] Insert ignore.
[2] Remove references to bug 9921.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 122675 [details] [review]
Bug 27981: Add option to automatically set 001 to the biblionumber

This patch adds a new system preference:
autoControlNumber

The option "biblionumber" will set field 001 to the biblionumber when you create a new record
or edit an existing record

If set to 'OFF' the 001 field wil not be touched

When duplicating a record the 001 will be removed if autoControlNumber is set

To test:
 1 - Apply patches and updatedatabase
 2 - Create a new record with no 001 field
 3 - Save and view the MAC, confirm there is no 001
 4 - Set the system preference to 'biblionumber'
 5 - Edit the record you created previously
 6 - Note the 001 is prepopulated with the biblionumber
 7 - Delete the field
 8 - Save the record
 9 - View the MARC, the 001 is filled with biblionumber
10 - Edit the record
11 - Set the 001 to a different value "Not the biblionumber"
12 - Save
13 - View the marc and confirm the value you entered is retained
14 - Edit a record with an existing 001 that is not the biblionumber
15 - Save and confirm 001 is not updated

To test duplication:
1 - Edit a record as duplicate when using the advanced editor
2 - Confirm the 001 does not load, but record saves correctly
3 - Edit the record
4 - Switch to 'basic editor'
5 - Save, then view record
6 - Edit as duplicate in basic editor
7 - Confirm the 001 is removed
8 - Confirm the 001 is added on save

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

(In reply to Marcel de Rooy from comment #42)
> Commit promises:
> The option "biblionumber" will set field 001 to the biblionumber when you
> create a new record     or edit an existing record
> 
> Create works, modify does not.

And it contradicts the test plan:
    14 - Edit a record with an existing 001 that is not the biblionumber
    15 - Save and confirm 001 is not updated

Thanks for working on this, Nick. This is a feature I've long wanted in Koha.

I've just taken a quick peek at the code, so just going to ask for some clarification.

At a glance, it looks like a 001 will be assigned for original cataloguing or copy cataloguing using the Cataloguing UI (even if it already has a 001), is that right? That sounds good to me.

I wonder about Staged MARC Imports though. At a glance, I think those will retain their original 001 and won't get updated. I think that might be problematic. 

Same goes for the APIs.

Created attachment 124279 [details] [review]
Bug 27981: (QA follow-up) Adjust atomic update

[1] Insert ignore.
[2] Remove references to bug 9921.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 124280 [details] [review]
Bug 27981: Add option to automatically set 001 to the biblionumber

This patch adds a new system preference:
autoControlNumber

The option "biblionumber" will set field 001 to the biblionumber when you create a new record
or edit an existing record and clear the 001 field.

If a value is present in 001 when saving the record it will be preserved.

If set to 'OFF' the 001 field wil not be touched

When duplicating a record the 001 will be removed if autoControlNumber is set

To test:
 1 - Apply patches and updatedatabase
 2 - Create a new record with no 001 field
 3 - Save and view the MAC, confirm there is no 001
 4 - Set the system preference to 'biblionumber'
 5 - Edit the record you created previously
 6 - Note the 001 is prepopulated with the biblionumber
 7 - Delete the field
 8 - Save the record
 9 - View the MARC, the 001 is filled with biblionumber
10 - Edit the record
11 - Set the 001 to a different value "Not the biblionumber"
12 - Save
13 - View the marc and confirm the value you entered is retained
14 - Edit a record with an existing 001 that is not the biblionumber
15 - Save and confirm 001 is not updated

To test duplication:
1 - Edit a record as duplicate when using the advanced editor
2 - Confirm the 001 does not load, but record saves correctly
3 - Edit the record
4 - Switch to 'basic editor'
5 - Save, then view record
6 - Edit as duplicate in basic editor
7 - Confirm the 001 is removed
8 - Confirm the 001 is added on save

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 124281 [details] [review]
Bug 27981: Adjust imported records, svc/import_bibs, records from Z3950

(In reply to Marcel de Rooy from comment #45)
> (In reply to Marcel de Rooy from comment #42)
> > Commit promises:
> > The option "biblionumber" will set field 001 to the biblionumber when you
> > create a new record     or edit an existing record
> > 
> > Create works, modify does not.
> 
> And it contradicts the test plan:
>     14 - Edit a record with an existing 001 that is not the biblionumber
>     15 - Save and confirm 001 is not updated

Updated the commit message

(In reply to David Cook from comment #46)
> Thanks for working on this, Nick. This is a feature I've long wanted in Koha.
> 
> I've just taken a quick peek at the code, so just going to ask for some
> clarification.
> 
> At a glance, it looks like a 001 will be assigned for original cataloguing
> or copy cataloguing using the Cataloguing UI (even if it already has a 001),
> is that right? That sounds good to me.
> 
> I wonder about Staged MARC Imports though. At a glance, I think those will
> retain their original 001 and won't get updated. I think that might be
> problematic. 
> 
> Same goes for the APIs.

Updated importing and svc/import_bibs

I think this needs to be Needs Signoff.
If someone signs off on the last patch, I probably could do the QA again ;)

Is the sequence of the attached patch files correct? (QA follow-up on top)

Created attachment 128223 [details] [review]
Bug 27981: DB update

Current status: Needs Signoff

Created attachment 128224 [details] [review]
Bug 27981: Add option to automatically set 001 to the biblionumber

This patch adds a new system preference:
autoControlNumber

The option "biblionumber" will set field 001 to the biblionumber when you create a new record
or edit an existing record and clear the 001 field.

If a value is present in 001 when saving the record it will be preserved.

If set to 'OFF' the 001 field wil not be touched

When duplicating a record the 001 will be removed if autoControlNumber is set

To test:
 1 - Apply patches and updatedatabase
 2 - Create a new record with no 001 field
 3 - Save and view the MAC, confirm there is no 001
 4 - Set the system preference to 'biblionumber'
 5 - Edit the record you created previously
 6 - Note the 001 is prepopulated with the biblionumber
 7 - Delete the field
 8 - Save the record
 9 - View the MARC, the 001 is filled with biblionumber
10 - Edit the record
11 - Set the 001 to a different value "Not the biblionumber"
12 - Save
13 - View the marc and confirm the value you entered is retained
14 - Edit a record with an existing 001 that is not the biblionumber
15 - Save and confirm 001 is not updated

To test duplication:
1 - Edit a record as duplicate when using the advanced editor
2 - Confirm the 001 does not load, but record saves correctly
3 - Edit the record
4 - Switch to 'basic editor'
5 - Save, then view record
6 - Edit as duplicate in basic editor
7 - Confirm the 001 is removed
8 - Confirm the 001 is added on save

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 128225 [details] [review]
Bug 27981: Adjust imported records, svc/import_bibs, records from Z3950

Created attachment 132034 [details] [review]
Bug 27981: DB update

Created attachment 132035 [details] [review]
Bug 27981: Add option to automatically set 001 to the biblionumber

This patch adds a new system preference:
autoControlNumber

The option "biblionumber" will set field 001 to the biblionumber when you create a new record
or edit an existing record and clear the 001 field.

If a value is present in 001 when saving the record it will be preserved.

If set to 'OFF' the 001 field wil not be touched

When duplicating a record the 001 will be removed if autoControlNumber is set

To test:
 1 - Apply patches and updatedatabase
 2 - Create a new record with no 001 field
 3 - Save and view the MAC, confirm there is no 001
 4 - Set the system preference to 'biblionumber'
 5 - Edit the record you created previously
 6 - Note the 001 is prepopulated with the biblionumber
 7 - Delete the field
 8 - Save the record
 9 - View the MARC, the 001 is filled with biblionumber
10 - Edit the record
11 - Set the 001 to a different value "Not the biblionumber"
12 - Save
13 - View the marc and confirm the value you entered is retained
14 - Edit a record with an existing 001 that is not the biblionumber
15 - Save and confirm 001 is not updated

To test duplication:
1 - Edit a record as duplicate when using the advanced editor
2 - Confirm the 001 does not load, but record saves correctly
3 - Edit the record
4 - Switch to 'basic editor'
5 - Save, then view record
6 - Edit as duplicate in basic editor
7 - Confirm the 001 is removed
8 - Confirm the 001 is added on save

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 132036 [details] [review]
Bug 27981: Adjust imported records, svc/import_bibs, records from Z3950

I tested this feature, everything works as expected / documented, but I (currently) don't have time to set up a proper sign-off workflow ...

Created attachment 134536 [details] [review]
Bug 27981: DB update

Signed-off-by: Thomas Klausner <domm@plix.at>

Created attachment 134537 [details] [review]
Bug 27981: Add option to automatically set 001 to the biblionumber

This patch adds a new system preference:
autoControlNumber

The option "biblionumber" will set field 001 to the biblionumber when you create a new record
or edit an existing record and clear the 001 field.

If a value is present in 001 when saving the record it will be preserved.

If set to 'OFF' the 001 field wil not be touched

When duplicating a record the 001 will be removed if autoControlNumber is set

To test:
 1 - Apply patches and updatedatabase
 2 - Create a new record with no 001 field
 3 - Save and view the MAC, confirm there is no 001
 4 - Set the system preference to 'biblionumber'
 5 - Edit the record you created previously
 6 - Note the 001 is prepopulated with the biblionumber
 7 - Delete the field
 8 - Save the record
 9 - View the MARC, the 001 is filled with biblionumber
10 - Edit the record
11 - Set the 001 to a different value "Not the biblionumber"
12 - Save
13 - View the marc and confirm the value you entered is retained
14 - Edit a record with an existing 001 that is not the biblionumber
15 - Save and confirm 001 is not updated

To test duplication:
1 - Edit a record as duplicate when using the advanced editor
2 - Confirm the 001 does not load, but record saves correctly
3 - Edit the record
4 - Switch to 'basic editor'
5 - Save, then view record
6 - Edit as duplicate in basic editor
7 - Confirm the 001 is removed
8 - Confirm the 001 is added on save

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Thomas Klausner <domm@plix.at>

Created attachment 134538 [details] [review]
Bug 27981: Adjust imported records, svc/import_bibs, records from Z3950

Signed-off-by: Thomas Klausner <domm@plix.at>

Created attachment 134539 [details] [review]
Bug 27981: (follow-up) Correct variable

Thx Nick for adding the sign-off lines :)

This is a really interesting dev I've been often asked about. But: Tests are currently failing:

prove t/db_dependent/Biblio.t
t/db_dependent/Biblio.t .. 8/15     # Looks like you planned 3 tests but ran 1.

#   Failed test 'autoControlNumber tests'
#   at t/db_dependent/Biblio.t line 846.
Can't use string ("1") as a HASH ref while "strict refs" in use at /kohadevbox/koha/C4/Biblio.pm line 362.
# Looks like your test exited with 255 just after 15.
t/db_dependent/Biblio.t .. Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 1/15 subtests 

Test Summary Report
-------------------
t/db_dependent/Biblio.t (Wstat: 65280 Tests: 15 Failed: 1)
  Failed test:  15
  Non-zero exit status: 255

Created attachment 136022 [details] [review]
Bug 27981: (follow-up) Fix tests

*** Bug 23832 has been marked as a duplicate of this bug. ***

Created attachment 140092 [details] [review]
Bug 27981: DB update

Signed-off-by: Thomas Klausner <domm@plix.at>

Signed-off-by: Andrew Nugged <nugged@gmail.com>

Created attachment 140093 [details] [review]
Bug 27981: Add option to automatically set 001 to the biblionumber

This patch adds a new system preference:
autoControlNumber

The option "biblionumber" will set field 001 to the biblionumber when you create a new record
or edit an existing record and clear the 001 field.

If a value is present in 001 when saving the record it will be preserved.

If set to 'OFF' the 001 field wil not be touched

When duplicating a record the 001 will be removed if autoControlNumber is set

To test:
 1 - Apply patches and updatedatabase
 2 - Create a new record with no 001 field
 3 - Save and view the MAC, confirm there is no 001
 4 - Set the system preference to 'biblionumber'
 5 - Edit the record you created previously
 6 - Note the 001 is prepopulated with the biblionumber
 7 - Delete the field
 8 - Save the record
 9 - View the MARC, the 001 is filled with biblionumber
10 - Edit the record
11 - Set the 001 to a different value "Not the biblionumber"
12 - Save
13 - View the marc and confirm the value you entered is retained
14 - Edit a record with an existing 001 that is not the biblionumber
15 - Save and confirm 001 is not updated

To test duplication:
1 - Edit a record as duplicate when using the advanced editor
2 - Confirm the 001 does not load, but record saves correctly
3 - Edit the record
4 - Switch to 'basic editor'
5 - Save, then view record
6 - Edit as duplicate in basic editor
7 - Confirm the 001 is removed
8 - Confirm the 001 is added on save

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Thomas Klausner <domm@plix.at>

Signed-off-by: Andrew Nugged <nugged@gmail.com>

Created attachment 140094 [details] [review]
Bug 27981: Adjust imported records, svc/import_bibs, records from Z3950

Signed-off-by: Thomas Klausner <domm@plix.at>

Signed-off-by: Andrew Nugged <nugged@gmail.com>

Created attachment 140095 [details] [review]
Bug 27981: (follow-up) Correct variable

Signed-off-by: Andrew Nugged <nugged@gmail.com>

Created attachment 140096 [details] [review]
Bug 27981: (follow-up) Fix tests

Signed-off-by: Andrew Nugged <nugged@gmail.com>

Created attachment 140097 [details] [review]
Bug 27981: (QA follow-up) Unify JavaScript to false/true from 0/1/""

Make remove_control_num var and JS KohaBackend.GetRecord calls to have
only false/true instead of mixed 0,1, and "" in different places.

Signed-off-by: Andrew Nugged <nugged@gmail.com>

Did QA & Rebased to the latest master.

Also added 1 final patch (small polishing step) to use true/false everywhere to unify JS because,

    KohaBackend.GetRecord( id, remove_control_num, callback );

called in one place with remove_control_num to be 0 or 1,
and in another place:

    KohaBackend.GetRecord( id, "", callback );

let's have it true/false, so the next developer will get from the context that this variable is only boolean.

This bugfix code (including my last patch) was already used in our productions for more than a month as a hotfix, and works well.

Future development note, but this is not in the scope of the CURRENT TICKET:

Our librarians also noted that there's a 003 field which might also be auto-filled with some hardcoded value for this autofilled 001. They also noted that "However, it should be the case that the combination of 001 and 003 is always unique.").

Created attachment 140502 [details] [review]
Bug 27981: (QA follow-up) GetMarcBiblio was removed from codebase

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Pushed to master for 22.11.

Nice work everyone, thanks!

*** Bug 29694 has been marked as a duplicate of this bug. ***