[Bug 17380] New: Cannot edit authorities with authtypecode = default
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17380 Bug ID: 17380 Summary: Cannot edit authorities with authtypecode = default Change sponsored?: --- Product: Koha Version: 16.05 Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: MARC Authority data support Assignee: gmcharlt@gmail.com Reporter: joachim.ganseman@ehb.be QA Contact: testopia@bugs.koha-community.org Created attachment 55947 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=55947&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17380 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@bugs.koha-c | |ommunity.org --- Comment #1 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Could you give us the exact error with the file and line number where it crashes? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17380 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl Attachment #55947|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17380 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|gmcharlt@gmail.com |m.de.rooy@rijksmuseum.nl Status|NEW |ASSIGNED Version|16.05 |master --- Comment #2 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Confirm the problem. Small fix coming -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17380 --- Comment #3 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 60592 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=60592&action=edit Bug 17380: Merge saves with wrong authtypecode -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17380 --- Comment #4 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Still needs some polishing. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17380 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nick@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17380 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=18801 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17380 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |18801 Depends on| |18811 Attachment #60592|0 |1 is obsolete| | Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18801 [Bug 18801] Merging authorities has an invalid 'Default' type in the merge framework selector https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18811 [Bug 18811] Visibility settings inconsistent between framework and authority editor -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17380 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Cannot edit authorities |Resolve several problems |with authtypecode = default |related to Default | |authority framework -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17380 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |17908 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17908 [Bug 17908] Authority merge omnibus -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17380 --- Comment #5 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 64952 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=64952&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17380 --- Comment #6 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 64953 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=64953&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17380 --- Comment #7 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 64954 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=64954&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17380 --- Comment #8 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 64955 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=64955&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17380 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17380 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #64952|0 |1 is obsolete| | --- Comment #9 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 66277 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=66277&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17380 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #64953|0 |1 is obsolete| | --- Comment #10 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 66278 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=66278&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17380 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #64954|0 |1 is obsolete| | --- Comment #11 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 66279 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=66279&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17380 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #64955|0 |1 is obsolete| | --- Comment #12 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 66280 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=66280&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17380 --- Comment #13 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Trivial rebase -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17380 Josef Moravec <josef.moravec@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17380 Josef Moravec <josef.moravec@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #66277|0 |1 is obsolete| | Attachment #66278|0 |1 is obsolete| | Attachment #66279|0 |1 is obsolete| | Attachment #66280|0 |1 is obsolete| | --- Comment #14 from Josef Moravec <josef.moravec@gmail.com> --- Created attachment 66313 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=66313&action=edit [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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17380 --- Comment #15 from Josef Moravec <josef.moravec@gmail.com> --- Created attachment 66314 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=66314&action=edit [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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17380 --- Comment #16 from Josef Moravec <josef.moravec@gmail.com> --- Created attachment 66315 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=66315&action=edit [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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17380 --- Comment #17 from Josef Moravec <josef.moravec@gmail.com> --- Created attachment 66316 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=66316&action=edit [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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17380 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |nick@bywatersolutions.com |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17380 --- Comment #18 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 66750 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=66750&action=edit Bug 17380: (QA followup) Fix parameter in tests -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17380 --- Comment #19 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 66751 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=66751&action=edit Bug 17380: (Alternate proposal) Report error to user instead of throwing exception Internal server errors are good, user feedback is better -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17380 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |In Discussion --- Comment #20 from Nick Clemens <nick@bywatersolutions.com> --- Marcel - I fixed a problem in tests and provide an alternate to the exceptions, does this seem reasonable? -Nick -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17380 --- Comment #21 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Nick Clemens from comment #18)
Created attachment 66750 [details] [review] Bug 17380: (QA followup) Fix parameter in tests
This has to do with the pushed enhancement that changes prototype of GetMarcBiblio. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17380 --- Comment #22 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 66796 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=66796&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17380 --- Comment #23 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 66797 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=66797&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17380 --- Comment #24 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 66798 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=66798&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17380 --- Comment #25 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 66799 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=66799&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17380 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #66750|0 |1 is obsolete| | --- Comment #26 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 66800 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=66800&action=edit Bug 17380: (QA followup) Fix parameter in tests Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17380 --- Comment #27 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 66801 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=66801&action=edit 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'. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17380 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17380 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #66313|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17380 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #66314|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17380 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #66315|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17380 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #66316|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17380 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #66751|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17380 --- Comment #28 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17380 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17380 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #66796|0 |1 is obsolete| | Attachment #66797|0 |1 is obsolete| | Attachment #66798|0 |1 is obsolete| | Attachment #66799|0 |1 is obsolete| | Attachment #66800|0 |1 is obsolete| | Attachment #66801|0 |1 is obsolete| | --- Comment #29 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 66912 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=66912&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17380 --- Comment #30 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 66913 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=66913&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17380 --- Comment #31 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 66914 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=66914&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17380 --- Comment #32 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 66915 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=66915&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17380 --- Comment #33 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 66916 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=66916&action=edit Bug 17380: (QA followup) Fix parameter in tests Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17380 --- Comment #34 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 66917 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=66917&action=edit 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'. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17380 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master --- Comment #35 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Pushed to master for 17.11, thanks to everybody involved! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17380 Fridolin SOMERS <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Medium patch Status|Pushed to Master |Pushed to Stable CC| |fridolin.somers@biblibre.co | |m --- Comment #36 from Fridolin SOMERS <fridolin.somers@biblibre.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17380 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |katrin.fischer@bsz-bw.de --- Comment #37 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Doesn't apply cleanly on 16.11.x, please rebase if you want this to be included. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17380 --- Comment #38 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17380 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Stable |RESOLVED Resolution|--- |FIXED --- Comment #39 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Looking at Marcel's comments, this won't get pushed to 16.11.x. Closing. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17380 Bug 17380 depends on bug 18811, which changed state. Bug 18811 Summary: Visibility settings inconsistent between framework and authority editor https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18811 What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Stable |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org