Created attachment 59663 [details] [review] Bug 18014: General update of AuthoritiesMarc.t Test plan: Run t/db_dependent/AuthoritiesMarc.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 59664 [details] [review] Bug 18014: Add test to AuthoritiesMarc.t to expose problem in AddAuthority Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 59665 [details] [review] Bug 18014: AddAuthority should respect AUTO_INCREMENT Instead of using the MAX(authid)+1 logic, AddAuthority should just save the record and get the new id. The authid column is an autoincrement. This eliminates problems where a newly assigned authid also refers to a previously deleted record. (And it will not cause problems when refining the dontmerge functionality on report 9988.) Note: ModAuthority also calls AddAuthority to update an existing record; in that case we should not create a new record even if the record should not be found any more (which should be exceptional). This patch also simplifies handling of 001 in the authority record: in all cases this field is updated now; no need to check its contents. Test plan: [1] Run t/db_dependent/AuthoritiesMarc.t [2] Add a new authority record via the interface Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
*** Bug 12734 has been marked as a duplicate of this bug. ***
Created attachment 59667 [details] [review] Bug 18014: General update of AuthoritiesMarc.t Modern::Perl, Koha::Database, etc. Test plan: Run t/db_dependent/AuthoritiesMarc.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 59668 [details] [review] Bug 18014: Add test to AuthoritiesMarc.t to expose problem in AddAuthority Since AddAuthority uses max(id)+1 logic to produce the next authid, authority id's will be reused when you delete the last record. This may be a source of problems and will be addressed on the next patch. This patch add a test to expose the problem. Test plan: [1] Run t/db_dependent/AuthoritiesMarc.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 59669 [details] [review] Bug 18014: AddAuthority should respect AUTO_INCREMENT Instead of using the MAX(authid)+1 logic, AddAuthority should just save the record and get the new id. The authid column is an autoincrement. This eliminates problems where a newly assigned authid also refers to a previously deleted record. (And it will not cause problems when refining the dontmerge functionality on report 9988.) Note: ModAuthority also calls AddAuthority to update an existing record; in that case we should not create a new record even if the record should not be found any more (which should be exceptional). This patch also simplifies handling of 001 in the authority record: in all cases this field is updated now; no need to check its contents. Test plan: [1] Run t/db_dependent/AuthoritiesMarc.t [2] Add a new authority record via the interface Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment on attachment 59669 [details] [review] Bug 18014: AddAuthority should respect AUTO_INCREMENT Review of attachment 59669 [details] [review]: ----------------------------------------------------------------- ::: t/db_dependent/AuthoritiesMarc.t @@ +201,5 @@ > my $id2 = AddAuthority( $record, undef, 'GEOGR_NAME' ); > + isnt( $id1, $id2, 'Do not return the same id again' ); > + t::lib::Mocks::mock_preference( 'marcflavour', 'UNIMARC' ); > + my $id3 = AddAuthority( $record, undef, 'GEOGR_NAME' ); > + is( $id3 > 0, 1, 'Tested AddAuthority with UNIMARC' ); ok($id3>0,'...') would have been better.
Comment on attachment 59669 [details] [review] Bug 18014: AddAuthority should respect AUTO_INCREMENT Review of attachment 59669 [details] [review]: ----------------------------------------------------------------- ::: t/db_dependent/AuthoritiesMarc.t @@ -198,5 @@ > my $record = C4::AuthoritiesMarc::GetAuthority(1); > my $id1 = AddAuthority( $record, undef, 'GEOGR_NAME' ); > DelAuthority( $id1 ); > my $id2 = AddAuthority( $record, undef, 'GEOGR_NAME' ); > - is( $id1, $id2, 'FIXME: Got the same id back, let\'s fix that behavior' ); This should have been isnt in the first place, and the second patch should be expecting a failure. :)
Created attachment 59700 [details] [review] Bug 18014: General update of AuthoritiesMarc.t Modern::Perl, Koha::Database, etc. Test plan: Run t/db_dependent/AuthoritiesMarc.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Created attachment 59701 [details] [review] Bug 18014: Add test to AuthoritiesMarc.t to expose problem in AddAuthority Since AddAuthority uses max(id)+1 logic to produce the next authid, authority id's will be reused when you delete the last record. This may be a source of problems and will be addressed on the next patch. This patch add a test to expose the problem. Test plan: [1] Run t/db_dependent/AuthoritiesMarc.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Created attachment 59702 [details] [review] Bug 18014: AddAuthority should respect AUTO_INCREMENT Instead of using the MAX(authid)+1 logic, AddAuthority should just save the record and get the new id. The authid column is an autoincrement. This eliminates problems where a newly assigned authid also refers to a previously deleted record. (And it will not cause problems when refining the dontmerge functionality on report 9988.) Note: ModAuthority also calls AddAuthority to update an existing record; in that case we should not create a new record even if the record should not be found any more (which should be exceptional). This patch also simplifies handling of 001 in the authority record: in all cases this field is updated now; no need to check its contents. Test plan: [1] Run t/db_dependent/AuthoritiesMarc.t [2] Add a new authority record via the interface Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Didn't test just the first patch by itself, but tested 1&2 together and all three together. That's why I commented like I dd in comment #8 and comment #9. But the end product is good. :)
(In reply to M. Tompsett from comment #13) > Didn't test just the first patch by itself, but tested 1&2 together and all > three together. That's why I commented like I dd in comment #8 and comment > #9. But the end product is good. :) Thanks for your feedback and signoff !
Created attachment 59869 [details] [review] Bug 18014: General update of AuthoritiesMarc.t Modern::Perl, Koha::Database, etc. Test plan: Run t/db_dependent/AuthoritiesMarc.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 59870 [details] [review] Bug 18014: Add test to AuthoritiesMarc.t to expose problem in AddAuthority Since AddAuthority uses max(id)+1 logic to produce the next authid, authority id's will be reused when you delete the last record. This may be a source of problems and will be addressed on the next patch. This patch add a test to expose the problem. Test plan: [1] Run t/db_dependent/AuthoritiesMarc.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 59871 [details] [review] Bug 18014: AddAuthority should respect AUTO_INCREMENT Instead of using the MAX(authid)+1 logic, AddAuthority should just save the record and get the new id. The authid column is an autoincrement. This eliminates problems where a newly assigned authid also refers to a previously deleted record. (And it will not cause problems when refining the dontmerge functionality on report 9988.) Note: ModAuthority also calls AddAuthority to update an existing record; in that case we should not create a new record even if the record should not be found any more (which should be exceptional). This patch also simplifies handling of 001 in the authority record: in all cases this field is updated now; no need to check its contents. Test plan: [1] Run t/db_dependent/AuthoritiesMarc.t [2] Add a new authority record via the interface Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 59880 [details] [review] Bug 18014: General update of AuthoritiesMarc.t Modern::Perl, Koha::Database, etc. Test plan: Run t/db_dependent/AuthoritiesMarc.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 59881 [details] [review] Bug 18014: Add test to AuthoritiesMarc.t to expose problem in AddAuthority Since AddAuthority uses max(id)+1 logic to produce the next authid, authority id's will be reused when you delete the last record. This may be a source of problems and will be addressed on the next patch. This patch add a test to expose the problem. Test plan: [1] Run t/db_dependent/AuthoritiesMarc.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 59882 [details] [review] Bug 18014: AddAuthority should respect AUTO_INCREMENT Instead of using the MAX(authid)+1 logic, AddAuthority should just save the record and get the new id. The authid column is an autoincrement. This eliminates problems where a newly assigned authid also refers to a previously deleted record. (And it will not cause problems when refining the dontmerge functionality on report 9988.) Note: ModAuthority also calls AddAuthority to update an existing record; in that case we should not create a new record even if the record should not be found any more (which should be exceptional). This patch also simplifies handling of 001 in the authority record: in all cases this field is updated now; no need to check its contents. Test plan: [1] Run t/db_dependent/AuthoritiesMarc.t [2] Add a new authority record via the interface Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 59883 [details] [review] Bug 18014: QA follow-up: Use ok() instead of is() for '>' comparison Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Oops, I didn't see Nick already did QA earlier. I will obsolete my patches (except for the followup) and de-obsolete Nick's patches.
Pushed to master for 17.05, thanks Julian, Marcel!
'Smells' a bit of possible behaviour change. Marcel, what do you think? Not pushing this for 16.11.04, might reconsider later.
(In reply to Katrin Fischer from comment #24) > 'Smells' a bit of possible behaviour change. Marcel, what do you think? > Not pushing this for 16.11.04, might reconsider later. Fixing a bug is on itself a behavior change :) The only change is a good one: the deleted authid will no longer be reused. We may assume that no one relies on such wrong behavior. This would be imo an candidate for backporting.
Bit hard to admit, because you said noone would do that... But actually we enforce auth_id to be the ID from the union catalog - that way we can import the records "prelinked" as the number in $9 of the bibliographic record matches the auth_id of Koha. I admit, this is not a totally recommendable way of doing things... but there might be other strange things out there. Not sure if it would affect our workflow at the moment.
(In reply to Katrin Fischer from comment #26) > Bit hard to admit, because you said noone would do that... But actually we > enforce auth_id to be the ID from the union catalog - that way we can import > the records "prelinked" as the number in $9 of the bibliographic record > matches the auth_id of Koha. I admit, this is not a totally recommendable > way of doing things... but there might be other strange things out there. > Not sure if it would affect our workflow at the moment. Never say never :)
*** Bug 17980 has been marked as a duplicate of this bug. ***