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.
Could you give us the exact error with the file and line number where it crashes?
Confirm the problem. Small fix coming
Created attachment 60592 [details] [review] Bug 17380: Merge saves with wrong authtypecode
Still needs some polishing.
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>
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>
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>
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>
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>
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>
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>
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>
Trivial rebase
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>
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>
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>
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>
Created attachment 66750 [details] [review] Bug 17380: (QA followup) Fix parameter in tests
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
Marcel - I fixed a problem in tests and provide an alternate to the exceptions, does this seem reasonable? -Nick
(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.
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>
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>
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>
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>
Created attachment 66800 [details] [review] Bug 17380: (QA followup) Fix parameter in tests Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
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'.
(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.
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>
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>
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>
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>
Created attachment 66916 [details] [review] Bug 17380: (QA followup) Fix parameter in tests Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
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'.
Pushed to master for 17.11, thanks to everybody involved!
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
Doesn't apply cleanly on 16.11.x, please rebase if you want this to be included.
(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.
Looking at Marcel's comments, this won't get pushed to 16.11.x. Closing.