Bug 17380 - Resolve several problems related to Default authority framework
Summary: Resolve several problems related to Default authority framework
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: MARC Authority data support (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Marcel de Rooy
QA Contact: Nick Clemens
URL:
Whiteboard:
Keywords:
Depends on: 18801 18811
Blocks: 17908
  Show dependency treegraph
 
Reported: 2016-09-29 21:44 UTC by joachim.ganseman
Modified: 2017-10-30 13:18 UTC (History)
5 users (show)

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


Attachments
Example: after trying to merge PERSO_NAME and TOPIC_TERM "Rossini", the PERSO_NAME record is now authtypecoded as "default" and cannot be edited anymore (53.83 KB, image/jpeg)
2016-09-29 21:44 UTC, joachim.ganseman
Details
Bug 17380: Merge saves with wrong authtypecode (3.01 KB, patch)
2017-02-23 09:04 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17380: Add some checks around Authorities::Types->find (3.24 KB, patch)
2017-07-10 08:40 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17380: Do not use GuessAuthTypeCode in MetadataRecord::Authority (3.55 KB, patch)
2017-07-10 08:40 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17380: Graceful resolution of missing reporting tag in merge (6.04 KB, patch)
2017-07-10 08:40 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17380: Do not allow Default template in merge form (10.49 KB, patch)
2017-07-10 08:40 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17380: Add some checks around Authorities::Types->find (3.23 KB, patch)
2017-08-21 14:27 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17380: Do not use GuessAuthTypeCode in MetadataRecord::Authority (3.55 KB, patch)
2017-08-21 14:27 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17380: Graceful resolution of missing reporting tag in merge (6.04 KB, patch)
2017-08-21 14:27 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17380: Do not allow Default template in merge form (10.49 KB, patch)
2017-08-21 14:27 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17380: Add some checks around Authorities::Types->find (3.27 KB, patch)
2017-08-22 10:40 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17380: Do not use GuessAuthTypeCode in MetadataRecord::Authority (3.58 KB, patch)
2017-08-22 10:40 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17380: Graceful resolution of missing reporting tag in merge (6.07 KB, patch)
2017-08-22 10:40 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17380: Do not allow Default template in merge form (10.52 KB, patch)
2017-08-22 10:40 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17380: (QA followup) Fix parameter in tests (1.00 KB, patch)
2017-09-01 14:41 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17380: (Alternate proposal) Report error to user instead of throwing exception (2.81 KB, patch)
2017-09-01 14:41 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17380: Add some checks around Authorities::Types->find (3.30 KB, patch)
2017-09-04 08:29 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17380: Do not use GuessAuthTypeCode in MetadataRecord::Authority (3.61 KB, patch)
2017-09-04 08:29 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17380: Graceful resolution of missing reporting tag in merge (6.09 KB, patch)
2017-09-04 08:29 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17380: Do not allow Default template in merge form (10.54 KB, patch)
2017-09-04 08:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17380: (QA followup) Fix parameter in tests (1.09 KB, patch)
2017-09-04 08:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17380: [QA Follow-up] Report error to user instead of throwing exception (3.12 KB, patch)
2017-09-04 08:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17380: Add some checks around Authorities::Types->find (3.31 KB, patch)
2017-09-07 09:52 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17380: Do not use GuessAuthTypeCode in MetadataRecord::Authority (3.63 KB, patch)
2017-09-07 09:52 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17380: Graceful resolution of missing reporting tag in merge (6.10 KB, patch)
2017-09-07 09:52 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17380: Do not allow Default template in merge form (10.56 KB, patch)
2017-09-07 09:52 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17380: (QA followup) Fix parameter in tests (1.05 KB, patch)
2017-09-07 09:52 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17380: [QA Follow-up] Report error to user instead of throwing exception (3.08 KB, patch)
2017-09-07 09:52 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description joachim.ganseman 2016-09-29 21:44:49 UTC
Created attachment 55947 [details]
Example: after trying to merge PERSO_NAME and TOPIC_TERM "Rossini", the PERSO_NAME record is now authtypecoded as "default" and cannot be edited anymore

Editing authorities whose authtypecode is not set to a common value is not possible in Koha 16.05 .

This occurs e.g. after trying to (accidentally) merge a PERSO_NAME and TOPIC_TERM authority into one. The resulting authority will have its authtypecode set to "default".

Upon trying to correct such an authority, it turns out to be impossible to edit the said authority. An error very similar to 'Can't call method "authtypecode" on an undefined value' appears instead of the editor.

This occurs even when there are no authority frameworks defined (i.e. all in the default frameworks), in Koha 16.05 on Debian.
Comment 1 Jonathan Druart 2016-10-05 07:51:29 UTC
Could you give us the exact error with the file and line number where it crashes?
Comment 2 Marcel de Rooy 2017-02-23 09:00:10 UTC
Confirm the problem. Small fix coming
Comment 3 Marcel de Rooy 2017-02-23 09:04:37 UTC
Created attachment 60592 [details] [review]
Bug 17380: Merge saves with wrong authtypecode
Comment 4 Marcel de Rooy 2017-02-23 09:04:57 UTC
Still needs some polishing.
Comment 5 Marcel de Rooy 2017-07-10 08:40:31 UTC
Created attachment 64952 [details] [review]
Bug 17380: Add some checks around Authorities::Types->find

Resolves:
Use of uninitialized value $biblio_fields in scalar chop at authorities/detail.pl line 212.
Can't call method "authtypetext" on an undefined value at authorities/detail.pl line 216.
Can't call method "authtypecode" on an undefined value at authorities/detail.pl line 180.

NOTE: Some of these problems have actually been resolved now by bugs 18801
and 18811, but it is still better imo to have these checks.

Test plan:
[1] Verify unchanged behavior. Search for some authorities on authorities.pl
    and click on the details of a record.
[2] Open an authorities detail page and change the authid in the URL to a
    not existing number. Instead of an internal server error, you should see
    a message like "The authority record you requested does not exist".

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 6 Marcel de Rooy 2017-07-10 08:40:36 UTC
Created attachment 64953 [details] [review]
Bug 17380: Do not use GuessAuthTypeCode in MetadataRecord::Authority

If we got an authtypecode from the database and this value is not NULL
since the table column does not allow it, there is no need to call
GuessAuthTypeCode for empty string (read: Default framework) in the
sub get_from_authid.

Furthermore, we remove three Koha::MetadataRecord::Authority->new calls.
They are useless, since we do not pass a record. It just generates:
  No record passed at authorities/merge.pl line 96.
  Can't bless non-reference value at Koha/MetadataRecord/Authority.pm line 66.
Instead we throw an ObjectNotFound exception.

Test plan:
[1] Run t/db_dependent/Koha_Authority.t
[2] Interface will be tested in the following patches.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 7 Marcel de Rooy 2017-07-10 08:40:39 UTC
Created attachment 64954 [details] [review]
Bug 17380: Graceful resolution of missing reporting tag in merge

Altough this patch deals with a mostly hypothetical case and this report
makes it practically impossible anymore to merge with records in the
Default framework (having no reporting tag), we can make the code of
sub merge still a bit more robust here.

If you would merge biblio records from one authtype to another and the new
framework would not have a reporting tag, before this patch the result would
be data loss. Merge would handle this request as a delete. This patch makes
merge handle it differently: instead of clearing the biblio records, it
keeps $a and $9 in order to make a future corrective merge possible.

Note: The additional condition on line 1468 for $tags_using_authtype
makes sure that we do not select all fields when the authtype should
unexpectedly be empty string (Default). This prevents crashing on
a "Control fields do not have subfields" error.

Test plan:
[1] Run t/db_dependent/Authorities/Merge.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 8 Marcel de Rooy 2017-07-10 08:40:43 UTC
Created attachment 64955 [details] [review]
Bug 17380: Do not allow Default template in merge form

This patch makes the following changes:
[1] Removes Default from the template list. We should not merge with the
    Default framework, since it does not have a reporting tag.
[2] Rearranges the error section in the template. It is confusing to have
    two error sections in this template. The error CANNOT_MOVE is not used.
    The error FRAMEWORK_NOT_SELECTED is replaced by WRONG_FRAMEWORK.
[3] Do not allow to merge a record with itself.
[4] Check if the merge reference record still contains any MARC tags.
[5] Additional polishing: Simplify passing frameworks to template. Remove
    an unused Koha::Authority::Types->search. Remove obsolete POD header
    for functions from the script.

Test plan:
[1] Select two authorities to merge. Verify that you cannot select Default
    anymore as framework for the reference record.
[2] Reproduce error WRONG_COUNT by adding another authid=999 in the URL
    after you selected two authority records for merging.
[3] Remove the third authid from the URL and change the first or second
    authid into an unexisting record id. You should generate an Internal
    Server Error. The log should show the exception message.
[4] Merge two authorities. Deselect all MARC tags. Should trigger the
    error EMPTY_MARC in the template.
[5] Select the same authority record twice for merging. Should trigger the
    error DESTRUCTIVE_MERGE in the template.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 9 Marcel de Rooy 2017-08-21 14:27:13 UTC
Created attachment 66277 [details] [review]
Bug 17380: Add some checks around Authorities::Types->find

Resolves:
Use of uninitialized value $biblio_fields in scalar chop at authorities/detail.pl line 212.
Can't call method "authtypetext" on an undefined value at authorities/detail.pl line 216.
Can't call method "authtypecode" on an undefined value at authorities/detail.pl line 180.

NOTE: Some of these problems have actually been resolved now by bugs 18801
and 18811, but it is still better imo to have these checks.

Test plan:
[1] Verify unchanged behavior. Search for some authorities on authorities.pl
    and click on the details of a record.
[2] Open an authorities detail page and change the authid in the URL to a
    not existing number. Instead of an internal server error, you should see
    a message like "The authority record you requested does not exist".

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 10 Marcel de Rooy 2017-08-21 14:27:17 UTC
Created attachment 66278 [details] [review]
Bug 17380: Do not use GuessAuthTypeCode in MetadataRecord::Authority

If we got an authtypecode from the database and this value is not NULL
since the table column does not allow it, there is no need to call
GuessAuthTypeCode for empty string (read: Default framework) in the
sub get_from_authid.

Furthermore, we remove three Koha::MetadataRecord::Authority->new calls.
They are useless, since we do not pass a record. It just generates:
  No record passed at authorities/merge.pl line 96.
  Can't bless non-reference value at Koha/MetadataRecord/Authority.pm line 66.
Instead we throw an ObjectNotFound exception.

Test plan:
[1] Run t/db_dependent/Koha_Authority.t
[2] Interface will be tested in the following patches.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 11 Marcel de Rooy 2017-08-21 14:27:21 UTC
Created attachment 66279 [details] [review]
Bug 17380: Graceful resolution of missing reporting tag in merge

Altough this patch deals with a mostly hypothetical case and this report
makes it practically impossible anymore to merge with records in the
Default framework (having no reporting tag), we can make the code of
sub merge still a bit more robust here.

If you would merge biblio records from one authtype to another and the new
framework would not have a reporting tag, before this patch the result would
be data loss. Merge would handle this request as a delete. This patch makes
merge handle it differently: instead of clearing the biblio records, it
keeps $a and $9 in order to make a future corrective merge possible.

Note: The additional condition on line 1468 for $tags_using_authtype
makes sure that we do not select all fields when the authtype should
unexpectedly be empty string (Default). This prevents crashing on
a "Control fields do not have subfields" error.

Test plan:
[1] Run t/db_dependent/Authorities/Merge.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 12 Marcel de Rooy 2017-08-21 14:27:26 UTC
Created attachment 66280 [details] [review]
Bug 17380: Do not allow Default template in merge form

This patch makes the following changes:
[1] Removes Default from the template list. We should not merge with the
    Default framework, since it does not have a reporting tag.
[2] Rearranges the error section in the template. It is confusing to have
    two error sections in this template. The error CANNOT_MOVE is not used.
    The error FRAMEWORK_NOT_SELECTED is replaced by WRONG_FRAMEWORK.
[3] Do not allow to merge a record with itself.
[4] Check if the merge reference record still contains any MARC tags.
[5] Additional polishing: Simplify passing frameworks to template. Remove
    an unused Koha::Authority::Types->search. Remove obsolete POD header
    for functions from the script.

Test plan:
[1] Select two authorities to merge. Verify that you cannot select Default
    anymore as framework for the reference record.
[2] Reproduce error WRONG_COUNT by adding another authid=999 in the URL
    after you selected two authority records for merging.
[3] Remove the third authid from the URL and change the first or second
    authid into an unexisting record id. You should generate an Internal
    Server Error. The log should show the exception message.
[4] Merge two authorities. Deselect all MARC tags. Should trigger the
    error EMPTY_MARC in the template.
[5] Select the same authority record twice for merging. Should trigger the
    error DESTRUCTIVE_MERGE in the template.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 13 Marcel de Rooy 2017-08-21 14:27:54 UTC
Trivial rebase
Comment 14 Josef Moravec 2017-08-22 10:40:21 UTC
Created attachment 66313 [details] [review]
[SIGNED-OFF] Bug 17380: Add some checks around Authorities::Types->find

Resolves:
Use of uninitialized value $biblio_fields in scalar chop at authorities/detail.pl line 212.
Can't call method "authtypetext" on an undefined value at authorities/detail.pl line 216.
Can't call method "authtypecode" on an undefined value at authorities/detail.pl line 180.

NOTE: Some of these problems have actually been resolved now by bugs 18801
and 18811, but it is still better imo to have these checks.

Test plan:
[1] Verify unchanged behavior. Search for some authorities on authorities.pl
    and click on the details of a record.
[2] Open an authorities detail page and change the authid in the URL to a
    not existing number. Instead of an internal server error, you should see
    a message like "The authority record you requested does not exist".

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

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 15 Josef Moravec 2017-08-22 10:40:24 UTC
Created attachment 66314 [details] [review]
[SIGNED-OFF] Bug 17380: Do not use GuessAuthTypeCode in MetadataRecord::Authority

If we got an authtypecode from the database and this value is not NULL
since the table column does not allow it, there is no need to call
GuessAuthTypeCode for empty string (read: Default framework) in the
sub get_from_authid.

Furthermore, we remove three Koha::MetadataRecord::Authority->new calls.
They are useless, since we do not pass a record. It just generates:
  No record passed at authorities/merge.pl line 96.
  Can't bless non-reference value at Koha/MetadataRecord/Authority.pm line 66.
Instead we throw an ObjectNotFound exception.

Test plan:
[1] Run t/db_dependent/Koha_Authority.t
[2] Interface will be tested in the following patches.

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

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 16 Josef Moravec 2017-08-22 10:40:28 UTC
Created attachment 66315 [details] [review]
[SIGNED-OFF] Bug 17380: Graceful resolution of missing reporting tag in merge

Altough this patch deals with a mostly hypothetical case and this report
makes it practically impossible anymore to merge with records in the
Default framework (having no reporting tag), we can make the code of
sub merge still a bit more robust here.

If you would merge biblio records from one authtype to another and the new
framework would not have a reporting tag, before this patch the result would
be data loss. Merge would handle this request as a delete. This patch makes
merge handle it differently: instead of clearing the biblio records, it
keeps $a and $9 in order to make a future corrective merge possible.

Note: The additional condition on line 1468 for $tags_using_authtype
makes sure that we do not select all fields when the authtype should
unexpectedly be empty string (Default). This prevents crashing on
a "Control fields do not have subfields" error.

Test plan:
[1] Run t/db_dependent/Authorities/Merge.t

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

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 17 Josef Moravec 2017-08-22 10:40:32 UTC
Created attachment 66316 [details] [review]
[SIGNED-OFF] Bug 17380: Do not allow Default template in merge form

This patch makes the following changes:
[1] Removes Default from the template list. We should not merge with the
    Default framework, since it does not have a reporting tag.
[2] Rearranges the error section in the template. It is confusing to have
    two error sections in this template. The error CANNOT_MOVE is not used.
    The error FRAMEWORK_NOT_SELECTED is replaced by WRONG_FRAMEWORK.
[3] Do not allow to merge a record with itself.
[4] Check if the merge reference record still contains any MARC tags.
[5] Additional polishing: Simplify passing frameworks to template. Remove
    an unused Koha::Authority::Types->search. Remove obsolete POD header
    for functions from the script.

Test plan:
[1] Select two authorities to merge. Verify that you cannot select Default
    anymore as framework for the reference record.
[2] Reproduce error WRONG_COUNT by adding another authid=999 in the URL
    after you selected two authority records for merging.
[3] Remove the third authid from the URL and change the first or second
    authid into an unexisting record id. You should generate an Internal
    Server Error. The log should show the exception message.
[4] Merge two authorities. Deselect all MARC tags. Should trigger the
    error EMPTY_MARC in the template.
[5] Select the same authority record twice for merging. Should trigger the
    error DESTRUCTIVE_MERGE in the template.

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

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 18 Nick Clemens 2017-09-01 14:41:10 UTC
Created attachment 66750 [details] [review]
Bug 17380: (QA followup) Fix parameter in tests
Comment 19 Nick Clemens 2017-09-01 14:41:14 UTC
Created attachment 66751 [details] [review]
Bug 17380: (Alternate proposal) Report error to user instead of throwing exception

Internal server errors are good, user feedback is better
Comment 20 Nick Clemens 2017-09-01 14:42:10 UTC
Marcel - I fixed a problem in tests and provide an alternate to the exceptions, does this seem reasonable?

-Nick
Comment 21 Marcel de Rooy 2017-09-04 08:15:05 UTC
(In reply to Nick Clemens from comment #18)
> Created attachment 66750 [details] [review] [review]
> Bug 17380: (QA followup) Fix parameter in tests

This has to do with the pushed enhancement that changes prototype of GetMarcBiblio.
Comment 22 Marcel de Rooy 2017-09-04 08:29:50 UTC
Created attachment 66796 [details] [review]
Bug 17380: Add some checks around Authorities::Types->find

Resolves:
Use of uninitialized value $biblio_fields in scalar chop at authorities/detail.pl line 212.
Can't call method "authtypetext" on an undefined value at authorities/detail.pl line 216.
Can't call method "authtypecode" on an undefined value at authorities/detail.pl line 180.

NOTE: Some of these problems have actually been resolved now by bugs 18801
and 18811, but it is still better imo to have these checks.

Test plan:
[1] Verify unchanged behavior. Search for some authorities on authorities.pl
    and click on the details of a record.
[2] Open an authorities detail page and change the authid in the URL to a
    not existing number. Instead of an internal server error, you should see
    a message like "The authority record you requested does not exist".

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

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 23 Marcel de Rooy 2017-09-04 08:29:54 UTC
Created attachment 66797 [details] [review]
Bug 17380: Do not use GuessAuthTypeCode in MetadataRecord::Authority

If we got an authtypecode from the database and this value is not NULL
since the table column does not allow it, there is no need to call
GuessAuthTypeCode for empty string (read: Default framework) in the
sub get_from_authid.

Furthermore, we remove three Koha::MetadataRecord::Authority->new calls.
They are useless, since we do not pass a record. It just generates:
  No record passed at authorities/merge.pl line 96.
  Can't bless non-reference value at Koha/MetadataRecord/Authority.pm line 66.
Instead we throw an ObjectNotFound exception.

Test plan:
[1] Run t/db_dependent/Koha_Authority.t
[2] Interface will be tested in the following patches.

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

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 24 Marcel de Rooy 2017-09-04 08:29:58 UTC
Created attachment 66798 [details] [review]
Bug 17380: Graceful resolution of missing reporting tag in merge

Altough this patch deals with a mostly hypothetical case and this report
makes it practically impossible anymore to merge with records in the
Default framework (having no reporting tag), we can make the code of
sub merge still a bit more robust here.

If you would merge biblio records from one authtype to another and the new
framework would not have a reporting tag, before this patch the result would
be data loss. Merge would handle this request as a delete. This patch makes
merge handle it differently: instead of clearing the biblio records, it
keeps $a and $9 in order to make a future corrective merge possible.

Note: The additional condition on line 1468 for $tags_using_authtype
makes sure that we do not select all fields when the authtype should
unexpectedly be empty string (Default). This prevents crashing on
a "Control fields do not have subfields" error.

Test plan:
[1] Run t/db_dependent/Authorities/Merge.t

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

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 25 Marcel de Rooy 2017-09-04 08:30:02 UTC
Created attachment 66799 [details] [review]
Bug 17380: Do not allow Default template in merge form

This patch makes the following changes:
[1] Removes Default from the template list. We should not merge with the
    Default framework, since it does not have a reporting tag.
[2] Rearranges the error section in the template. It is confusing to have
    two error sections in this template. The error CANNOT_MOVE is not used.
    The error FRAMEWORK_NOT_SELECTED is replaced by WRONG_FRAMEWORK.
[3] Do not allow to merge a record with itself.
[4] Check if the merge reference record still contains any MARC tags.
[5] Additional polishing: Simplify passing frameworks to template. Remove
    an unused Koha::Authority::Types->search. Remove obsolete POD header
    for functions from the script.

Test plan:
[1] Select two authorities to merge. Verify that you cannot select Default
    anymore as framework for the reference record.
[2] Reproduce error WRONG_COUNT by adding another authid=999 in the URL
    after you selected two authority records for merging.
[3] Remove the third authid from the URL and change the first or second
    authid into an unexisting record id. You should generate an Internal
    Server Error. The log should show the exception message.
[4] Merge two authorities. Deselect all MARC tags. Should trigger the
    error EMPTY_MARC in the template.
[5] Select the same authority record twice for merging. Should trigger the
    error DESTRUCTIVE_MERGE in the template.

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

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 26 Marcel de Rooy 2017-09-04 08:30:08 UTC
Created attachment 66800 [details] [review]
Bug 17380: (QA followup) Fix parameter in tests

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 27 Marcel de Rooy 2017-09-04 08:30:12 UTC
Created attachment 66801 [details] [review]
Bug 17380: [QA Follow-up] Report error to user instead of throwing exception

Internal server errors are good, user feedback is better.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tiny change: removed the obsolete 'use Koha::Exceptions'.
Comment 28 Marcel de Rooy 2017-09-04 08:33:24 UTC
(In reply to Nick Clemens from comment #20)
> Marcel - I fixed a problem in tests and provide an alternate to the
> exceptions, does this seem reasonable?
> 
> -Nick

Yes, looks good to me. 
Did you finish QA on this one with these adjustments?
If so, please dont forget to add a signoff and change status.
Thanks.
Comment 29 Nick Clemens 2017-09-07 09:52:37 UTC
Created attachment 66912 [details] [review]
Bug 17380: Add some checks around Authorities::Types->find

Resolves:
Use of uninitialized value $biblio_fields in scalar chop at authorities/detail.pl line 212.
Can't call method "authtypetext" on an undefined value at authorities/detail.pl line 216.
Can't call method "authtypecode" on an undefined value at authorities/detail.pl line 180.

NOTE: Some of these problems have actually been resolved now by bugs 18801
and 18811, but it is still better imo to have these checks.

Test plan:
[1] Verify unchanged behavior. Search for some authorities on authorities.pl
    and click on the details of a record.
[2] Open an authorities detail page and change the authid in the URL to a
    not existing number. Instead of an internal server error, you should see
    a message like "The authority record you requested does not exist".

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

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 30 Nick Clemens 2017-09-07 09:52:40 UTC
Created attachment 66913 [details] [review]
Bug 17380: Do not use GuessAuthTypeCode in MetadataRecord::Authority

If we got an authtypecode from the database and this value is not NULL
since the table column does not allow it, there is no need to call
GuessAuthTypeCode for empty string (read: Default framework) in the
sub get_from_authid.

Furthermore, we remove three Koha::MetadataRecord::Authority->new calls.
They are useless, since we do not pass a record. It just generates:
  No record passed at authorities/merge.pl line 96.
  Can't bless non-reference value at Koha/MetadataRecord/Authority.pm line 66.
Instead we throw an ObjectNotFound exception.

Test plan:
[1] Run t/db_dependent/Koha_Authority.t
[2] Interface will be tested in the following patches.

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

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 31 Nick Clemens 2017-09-07 09:52:43 UTC
Created attachment 66914 [details] [review]
Bug 17380: Graceful resolution of missing reporting tag in merge

Altough this patch deals with a mostly hypothetical case and this report
makes it practically impossible anymore to merge with records in the
Default framework (having no reporting tag), we can make the code of
sub merge still a bit more robust here.

If you would merge biblio records from one authtype to another and the new
framework would not have a reporting tag, before this patch the result would
be data loss. Merge would handle this request as a delete. This patch makes
merge handle it differently: instead of clearing the biblio records, it
keeps $a and $9 in order to make a future corrective merge possible.

Note: The additional condition on line 1468 for $tags_using_authtype
makes sure that we do not select all fields when the authtype should
unexpectedly be empty string (Default). This prevents crashing on
a "Control fields do not have subfields" error.

Test plan:
[1] Run t/db_dependent/Authorities/Merge.t

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

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 32 Nick Clemens 2017-09-07 09:52:46 UTC
Created attachment 66915 [details] [review]
Bug 17380: Do not allow Default template in merge form

This patch makes the following changes:
[1] Removes Default from the template list. We should not merge with the
    Default framework, since it does not have a reporting tag.
[2] Rearranges the error section in the template. It is confusing to have
    two error sections in this template. The error CANNOT_MOVE is not used.
    The error FRAMEWORK_NOT_SELECTED is replaced by WRONG_FRAMEWORK.
[3] Do not allow to merge a record with itself.
[4] Check if the merge reference record still contains any MARC tags.
[5] Additional polishing: Simplify passing frameworks to template. Remove
    an unused Koha::Authority::Types->search. Remove obsolete POD header
    for functions from the script.

Test plan:
[1] Select two authorities to merge. Verify that you cannot select Default
    anymore as framework for the reference record.
[2] Reproduce error WRONG_COUNT by adding another authid=999 in the URL
    after you selected two authority records for merging.
[3] Remove the third authid from the URL and change the first or second
    authid into an unexisting record id. You should generate an Internal
    Server Error. The log should show the exception message.
[4] Merge two authorities. Deselect all MARC tags. Should trigger the
    error EMPTY_MARC in the template.
[5] Select the same authority record twice for merging. Should trigger the
    error DESTRUCTIVE_MERGE in the template.

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

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 33 Nick Clemens 2017-09-07 09:52:49 UTC
Created attachment 66916 [details] [review]
Bug 17380: (QA followup) Fix parameter in tests

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 34 Nick Clemens 2017-09-07 09:52:52 UTC
Created attachment 66917 [details] [review]
Bug 17380: [QA Follow-up] Report error to user instead of throwing exception

Internal server errors are good, user feedback is better.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tiny change: removed the obsolete 'use Koha::Exceptions'.
Comment 35 Jonathan Druart 2017-09-12 15:11:33 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 36 Fridolin SOMERS 2017-10-23 14:13:11 UTC
Pushed to 17.05.x, will be in 17.05.06.

Needed to revert "Bug 17380: (QA followup) Fix parameter in tests" because C4::Biblio::GetMarcBiblio has not changed in 17.05.x
Comment 37 Katrin Fischer 2017-10-29 17:45:49 UTC
Doesn't apply cleanly on 16.11.x, please rebase if you want this to be included.
Comment 38 Marcel de Rooy 2017-10-30 11:26:25 UTC
(In reply to Katrin Fischer from comment #37)
> Doesn't apply cleanly on 16.11.x, please rebase if you want this to be
> included.

See comment on the dependency 18811.
Recommend to close it now.
Comment 39 Katrin Fischer 2017-10-30 13:18:36 UTC
Looking at Marcel's comments, this won't get pushed to 16.11.x. Closing.