From Nick Clemens on IRC: "Worth mentioning - you can't edit the 942 of an authority when creating, but it appears you can when editing, though you can't save it - confusing as end user." We should keep in mind that AddAuthority overwrites 942c when adding or modifying auth records. When adding a record: Since you choose the framework, we should not allow to change 942c here. The current behavior is fine; the field should normally be hidden via the framework. (We could even enforce it if it would not be hidden?) When modifying a record: The authorities.pl script (incorrectly) shows hidden fields when filled (which actually is weird). Since 942c is overwritten, this is useless. A closer look reveals that the visibility checks in authorities.pl do not completely correspond with the values used in auth_subfields_structure.pl. Auth_subfields_structure offers only two possibilities: 0=Show all and -5=Hide all. The code in authorities.pl now checks <=-4 or >=5 (probably remains from older code). Note that the sql install scripts initialize 942c to 8. The proposed patch adds more consistency between auth_subfields_structure.pl and authorities.pl. A non-zero value will be interpreted as hidden. IMPORTANT NOTE: This solution will remove values in hidden fields from the MARC record. Imo it is theoretically the best solution, but if we do not want to take that risk, we could also remove the two next statements at line 407 and 423 and add a specific next for 942c (enforcing to always hide it).
In the previous comment 942c must be 942a.
Created attachment 64314 [details] [review] Bug 18811: Change visibility checks in authorities.pl The check is now <=-4 or >=5, but the framework uses 0 for Show all and -5 for Hide all. (Note that sql installer scripts also use 8.) When modifying an authority, the script also showed hidden fields when filled, since it did not check the hidden field but only the tab field. NOTE: The proposed solution restores consistency, but will remove hidden fields from the MARC record. Test plan: [1] Set field 942a to Show all in an authority framework. [2] Open a new record in this framework and verify that you see 942a. [3] Edit an existing record in this framework and verify again. [4] Set field 942a now to Hide all in this framework. [5] Open a new record in this framework and verify that 942a is hidden. [6] Edit an existing record in this framework and verify again.
Nick, Julian or Jonathan: Would you have any comment on the *risk* of this solution?
(In reply to Marcel de Rooy from comment #3) > Nick, Julian or Jonathan: > Would you have any comment on the *risk* of this solution? This mail did not come through due to a mail problem that specific day. Second try :)
Created attachment 64541 [details] [review] Bug 18811: Add Koha Objects for authority tags and subfields Trivial copy and paste activity. Will be used in a later patch. No test plan needed.
Created attachment 64542 [details] [review] Bug 18811: Add a script for checking authority data in hidden fields If you edit an authority record while having data in hidden fields or subfields, that data will be lost now. This script can help you to unhide some fields and prevent data loss. Test plan: [1] Add a PERSO_NAME record. Fill e.g. 100b. [2] Hide 100b in the PERSO_NAME framework. [3] Run auth_show_hidden_data.pl and verify that it reports 100b in the PERSO_NAME framework.
Created attachment 64570 [details] [review] Bug 18811: Change visibility checks in authorities.pl The check is now <=-4 or >=5, but the framework uses 0 for Show all and -5 for Hide all. (Note that sql installer scripts also use 8.) When modifying an authority, the script also showed hidden fields when filled, since it did not check the hidden field but only the tab field. NOTE: The proposed solution restores consistency, but will remove hidden fields from the MARC record. Test plan: [1] Set field 942a to Show all in an authority framework. [2] Open a new record in this framework and verify that you see 942a. [3] Edit an existing record in this framework and verify again. [4] Set field 942a now to Hide all in this framework. [5] Open a new record in this framework and verify that 942a is hidden. [6] Edit an existing record in this framework and verify again. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 64571 [details] [review] Bug 18811: Add Koha Objects for authority tags and subfields Trivial copy and paste activity. Will be used in a later patch. No test plan needed. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 64572 [details] [review] Bug 18811: Add a script for checking authority data in hidden fields If you edit an authority record while having data in hidden fields or subfields, that data will be lost now. This script can help you to unhide some fields and prevent data loss. Test plan: [1] Add a PERSO_NAME record. Fill e.g. 100b. [2] Hide 100b in the PERSO_NAME framework. [3] Run auth_show_hidden_data.pl and verify that it reports 100b in the PERSO_NAME framework. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 64573 [details] [review] Bug 18811: Atomic update to print warning at upgrade time The warning encourages people to run the auth_show_hidden_data script to check for data in hidden fields and adjust their frameworks. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
In order to overcome the possible risk of data loss in hidden fields, I have added a maintenance script to search these fields. A user can decide to make some of these fields visible in the respective frameworks. Also a warning is printed at upgrade time.
When I run perl misc/maintenance/auth_show_hidden_data.pl -c I get this error: DBIx::Class::ResultSet::find(): find() expects either a column/value hashref, or a list of values corresponding to the columns of the specified unique constraint 'primary' at /home/vagrant/kohaclone/Koha/Objects.pm line 85 Checked Authorities schema and all seemed well.
(In reply to Michael Cabus from comment #12) > When I run perl misc/maintenance/auth_show_hidden_data.pl -c I get this > error: > > DBIx::Class::ResultSet::find(): find() expects either a column/value > hashref, or a list of values corresponding to the columns of the specified > unique constraint 'primary' at /home/vagrant/kohaclone/Koha/Objects.pm line > 85 > > Checked Authorities schema and all seemed well. Hi Michael, Thanks for testing. I guess that you are using kohadevbox to test, but I wonder if you have a configuration issue. Line 85 of Koha/Objects in master is not the DBIx find call, but only the first line of the sub with me. Please check if your paths are in order. The perl library path should also point to current master not to the older instance installed on your devbox. Do you use dev_install in your koha-conf? Marcel
Created attachment 66309 [details] [review] [SIGNED-OFF] Bug 18811: Change visibility checks in authorities.pl The check is now <=-4 or >=5, but the framework uses 0 for Show all and -5 for Hide all. (Note that sql installer scripts also use 8.) When modifying an authority, the script also showed hidden fields when filled, since it did not check the hidden field but only the tab field. NOTE: The proposed solution restores consistency, but will remove hidden fields from the MARC record. Test plan: [1] Set field 942a to Show all in an authority framework. [2] Open a new record in this framework and verify that you see 942a. [3] Edit an existing record in this framework and verify again. [4] Set field 942a now to Hide all in this framework. [5] Open a new record in this framework and verify that 942a is hidden. [6] Edit an existing record in this framework and verify again. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 66310 [details] [review] [SIGNED-OFF] Bug 18811: Add Koha Objects for authority tags and subfields Trivial copy and paste activity. Will be used in a later patch. No test plan needed. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 66311 [details] [review] [SIGNED-OFF] Bug 18811: Add a script for checking authority data in hidden fields If you edit an authority record while having data in hidden fields or subfields, that data will be lost now. This script can help you to unhide some fields and prevent data loss. Test plan: [1] Add a PERSO_NAME record. Fill e.g. 100b. [2] Hide 100b in the PERSO_NAME framework. [3] Run auth_show_hidden_data.pl and verify that it reports 100b in the PERSO_NAME framework. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 66312 [details] [review] [SIGNED-OFF] Bug 18811: Atomic update to print warning at upgrade time The warning encourages people to run the auth_show_hidden_data script to check for data in hidden fields and adjust their frameworks. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 66708 [details] [review] Bug 18811: Change visibility checks in authorities.pl The check is now <=-4 or >=5, but the framework uses 0 for Show all and -5 for Hide all. (Note that sql installer scripts also use 8.) When modifying an authority, the script also showed hidden fields when filled, since it did not check the hidden field but only the tab field. NOTE: The proposed solution restores consistency, but will remove hidden fields from the MARC record. Test plan: [1] Set field 942a to Show all in an authority framework. [2] Open a new record in this framework and verify that you see 942a. [3] Edit an existing record in this framework and verify again. [4] Set field 942a now to Hide all in this framework. [5] Open a new record in this framework and verify that 942a is hidden. [6] Edit an existing record in this framework and verify again. 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 66709 [details] [review] Bug 18811: Add Koha Objects for authority tags and subfields Trivial copy and paste activity. Will be used in a later patch. No test plan needed. 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 66710 [details] [review] Bug 18811: Add a script for checking authority data in hidden fields If you edit an authority record while having data in hidden fields or subfields, that data will be lost now. This script can help you to unhide some fields and prevent data loss. Test plan: [1] Add a PERSO_NAME record. Fill e.g. 100b. [2] Hide 100b in the PERSO_NAME framework. [3] Run auth_show_hidden_data.pl and verify that it reports 100b in the PERSO_NAME framework. 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 66711 [details] [review] Bug 18811: Atomic update to print warning at upgrade time The warning encourages people to run the auth_show_hidden_data script to check for data in hidden fields and adjust their frameworks. 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>
Please provide tests for new modules (based on t/db_dependent/Koha/Cities.t)
Created attachment 66793 [details] [review] Bug 18811: [QA Follow-up] Add tests for Koha::Authority::Subfields/Tags Renaming t/db_dependent/Authorities to Authority Adding Subfields.t and Tags.t Test plan: [1] prove t/db_dependent/Authority Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 66794 [details] [review] Bug 18811: [QA Follow-up] Resolved POD warnings Trivial changes to silence qa tools on POD warnings for Subfields and Tags. Note: Since Subfield and Tag only contain one sub which is considered as private by Pod::Coverage, these modules are listed as unrated (no public symbols defined) and trigger a FAIL on pod coverage in qa tools. This fail can be ignored. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(In reply to Marcel de Rooy from comment #24) > Note: Since Subfield and Tag only contain one sub which is considered as > private by Pod::Coverage, these modules are listed as unrated (no public > symbols defined) and trigger a FAIL on pod coverage in qa tools. This fail > can be ignored. This is true for all 'empty' singular Koha objects. Could be resolved by renaming _type to type, or adding another public symbol. Should be done on another report, if we really feel the need to do so. We can just ignore them too.
Pushed to master for 17.11, thanks to everybody involved!
Pushed to 17.05.x, will be in 17.05.06.
I am not sure about this comment: NOTE: The proposed solution restores consistency, but will remove hidden fields from the MARC record. Nick or Marcel, can you please explain and advise if this should go into 16.11.x?
(In reply to Katrin Fischer from comment #28) > I am not sure about this comment: > > NOTE: The proposed solution restores consistency, but will remove hidden > fields from the MARC record. > > Nick or Marcel, can you please explain and advise if this should go into > 16.11.x? Comment11 addresses this concern. I would recommend to push it in order to keep in line with 17.05 too. Note too that fields are not removed rightaway but could be lost when you save your (authority) record again in such a framework. A warning is printed and a script is provided.
Applying: Bug 18811: [QA Follow-up] Add tests for Koha::Authority::Subfields/Tags fatal: mode change for t/db_dependent/Authorities/MergeRequests.t, which is not in current HEAD
(In reply to Marcel de Rooy from comment #30) > Applying: Bug 18811: [QA Follow-up] Add tests for > Koha::Authority::Subfields/Tags > fatal: mode change for t/db_dependent/Authorities/MergeRequests.t, which is > not in current HEAD This might change the game. It is related to 9988.
This won't get pushed into 16.11.x, closing.
Marcel, this change has terrible side-effects: commit 7e222bb0796196f1e466b73d909fd28d529cab75 Bug 18811: Change visibility checks in authorities.pl next if $tagslib->{$tag}->{$subfield}->{hidden}; I was trying to test the AuthDisplayHierarchy behaviour and notice that the $9 was not filled with the related (broader) authority id. Indeed it is hidden. Can you fix ASAP?
(In reply to Jonathan Druart from comment #33) > Marcel, this change has terrible side-effects: > > commit 7e222bb0796196f1e466b73d909fd28d529cab75 > Bug 18811: Change visibility checks in authorities.pl > > next if $tagslib->{$tag}->{$subfield}->{hidden}; > > I was trying to test the AuthDisplayHierarchy behaviour and notice that the > $9 was not filled with the related (broader) authority id. Indeed it is > hidden. > Can you fix ASAP? I am not really familiar with AuthDisplayHierarchy. The third patch added a script auth_show_hidden_data.pl to detect hidden fields, prompting the user to change them in the frameworks. At the time we considered this to be enough..
We could more or less revert the change but return to the older inconsistent situation..
Make an exception for $9 as a special use case? Not sure if that is a valid option.
(In reply to Katrin Fischer from comment #36) > Make an exception for $9 as a special use case? Not sure if that is a valid > option. Yeah that might not be a bad idea..
Created attachment 70614 [details] [review] Bug 18811: (QA follow-up) Allow hidden $9 subfields Although we could argue that the user should make all $9 subfields visible, this might be a good compromise. It selectively reverts the change from the first patch by accepting $9 subfields without checking the hidden flag in the framework. Test plan: [1] Make a subfield $9 visible for some authority field. [2] Put a value in it somewhere in authorities.pl [3] Make subfield $9 invisible in the auth framework. [4] Open and save the adjusted authority record. [5] Check the marcxml of this record.
Could you test this one, Jonathan ?
(In reply to Marcel de Rooy from comment #39) > Could you test this one, Jonathan ? It fixes the problem, but it sounds like there is something else to fix: On a new DB: MariaDB [koha_kohadev]> select tagfield, tagsubfield, hidden from auth_subfield_structure where tagfield=551 and tagsubfield=9 and authtypecode="GEOGR_NAME"\G *************************** 1. row *************************** tagfield: 551 tagsubfield: 9 hidden: 1 Edit the subfields for GEOGR_NAME /admin/auth_subfields_structure.pl?op=add_form&tagfield=551&authtypecode=GEOGR_NAME Possible value for "Select to display or not"/hidden are -5 and 0, so "Show all" is displayed. Save without modifying anything: hidden is now '0'
(In reply to Marcel de Rooy from comment #38) > Created attachment 70614 [details] [review] [review] > Bug 18811: (QA follow-up) Allow hidden $9 subfields Patch moved to bug 20063
(In reply to Jonathan Druart from comment #40) > It fixes the problem, but it sounds like there is something else to fix: > > On a new DB: > MariaDB [koha_kohadev]> select tagfield, tagsubfield, hidden from > auth_subfield_structure where tagfield=551 and tagsubfield=9 and > authtypecode="GEOGR_NAME"\G > *************************** 1. row *************************** > tagfield: 551 > tagsubfield: 9 > hidden: 1 > > Edit the subfields for GEOGR_NAME > /admin/auth_subfields_structure. > pl?op=add_form&tagfield=551&authtypecode=GEOGR_NAME > > Possible value for "Select to display or not"/hidden are -5 and 0, so "Show > all" is displayed. > Save without modifying anything: hidden is now '0' Moved this to a new report: bug 20074