Bug 18811 - Visibility settings inconsistent between framework and authority editor
Summary: Visibility settings inconsistent between framework and authority editor
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: MARC Authority data support (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Marcel de Rooy
QA Contact: Nick Clemens
URL:
Keywords:
Depends on:
Blocks: 21037 21039 17380 20063 20074
  Show dependency treegraph
 
Reported: 2017-06-15 12:48 UTC by Marcel de Rooy
Modified: 2019-10-14 19:57 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 18811: Change visibility checks in authorities.pl (3.47 KB, patch)
2017-06-15 13:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18811: Add Koha Objects for authority tags and subfields (5.71 KB, patch)
2017-06-22 13:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18811: Add a script for checking authority data in hidden fields (4.61 KB, patch)
2017-06-22 13:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18811: Change visibility checks in authorities.pl (3.53 KB, patch)
2017-06-23 06:01 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18811: Add Koha Objects for authority tags and subfields (5.77 KB, patch)
2017-06-23 06:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18811: Add a script for checking authority data in hidden fields (4.66 KB, patch)
2017-06-23 06:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18811: Atomic update to print warning at upgrade time (1.44 KB, patch)
2017-06-23 06:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 18811: Change visibility checks in authorities.pl (3.56 KB, patch)
2017-08-22 10:20 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 18811: Add Koha Objects for authority tags and subfields (5.80 KB, patch)
2017-08-22 10:20 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 18811: Add a script for checking authority data in hidden fields (4.69 KB, patch)
2017-08-22 10:20 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 18811: Atomic update to print warning at upgrade time (1.47 KB, patch)
2017-08-22 10:20 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 18811: Change visibility checks in authorities.pl (3.60 KB, patch)
2017-09-01 11:19 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18811: Add Koha Objects for authority tags and subfields (5.84 KB, patch)
2017-09-01 11:19 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18811: Add a script for checking authority data in hidden fields (4.73 KB, patch)
2017-09-01 11:19 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18811: Atomic update to print warning at upgrade time (1.51 KB, patch)
2017-09-01 11:19 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 18811: [QA Follow-up] Add tests for Koha::Authority::Subfields/Tags (5.14 KB, patch)
2017-09-04 07:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18811: [QA Follow-up] Resolved POD warnings (1.37 KB, patch)
2017-09-04 08:06 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18811: (QA follow-up) Allow hidden $9 subfields (3.10 KB, patch)
2018-01-17 13:32 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2017-06-15 12:48:40 UTC
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).
Comment 1 Marcel de Rooy 2017-06-15 12:58:41 UTC
In the previous comment 942c must be 942a.
Comment 2 Marcel de Rooy 2017-06-15 13:02:45 UTC Comment hidden (obsolete)
Comment 3 Marcel de Rooy 2017-06-15 13:06:11 UTC Comment hidden (obsolete)
Comment 4 Marcel de Rooy 2017-06-22 09:55:49 UTC Comment hidden (obsolete)
Comment 5 Marcel de Rooy 2017-06-22 13:56:37 UTC Comment hidden (obsolete)
Comment 6 Marcel de Rooy 2017-06-22 13:56:41 UTC Comment hidden (obsolete)
Comment 7 Marcel de Rooy 2017-06-23 06:01:59 UTC
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>
Comment 8 Marcel de Rooy 2017-06-23 06:02:03 UTC
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>
Comment 9 Marcel de Rooy 2017-06-23 06:02:07 UTC
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>
Comment 10 Marcel de Rooy 2017-06-23 06:02:11 UTC
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>
Comment 11 Marcel de Rooy 2017-06-23 06:06:25 UTC
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.
Comment 12 Michael Cabus 2017-06-30 21:20:41 UTC
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.
Comment 13 Marcel de Rooy 2017-07-03 07:20:11 UTC
(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
Comment 14 Josef Moravec 2017-08-22 10:20:11 UTC
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>
Comment 15 Josef Moravec 2017-08-22 10:20:15 UTC
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>
Comment 16 Josef Moravec 2017-08-22 10:20:18 UTC
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>
Comment 17 Josef Moravec 2017-08-22 10:20:22 UTC
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>
Comment 18 Nick Clemens 2017-09-01 11:19:33 UTC
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>
Comment 19 Nick Clemens 2017-09-01 11:19:36 UTC
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>
Comment 20 Nick Clemens 2017-09-01 11:19:40 UTC
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>
Comment 21 Nick Clemens 2017-09-01 11:19:43 UTC
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>
Comment 22 Jonathan Druart 2017-09-01 15:28:21 UTC
Please provide tests for new modules (based on t/db_dependent/Koha/Cities.t)
Comment 23 Marcel de Rooy 2017-09-04 07:21:55 UTC
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>
Comment 24 Marcel de Rooy 2017-09-04 08:06:54 UTC
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>
Comment 25 Marcel de Rooy 2017-09-04 08:09:39 UTC
(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.
Comment 26 Jonathan Druart 2017-09-12 15:12:22 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 27 Fridolin Somers 2017-10-23 13:41:55 UTC
Pushed to 17.05.x, will be in 17.05.06.
Comment 28 Katrin Fischer 2017-10-29 17:37:38 UTC
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?
Comment 29 Marcel de Rooy 2017-10-30 11:11:47 UTC
(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.
Comment 30 Marcel de Rooy 2017-10-30 11:13:31 UTC
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
Comment 31 Marcel de Rooy 2017-10-30 11:20:37 UTC
(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.
Comment 32 Katrin Fischer 2017-10-30 13:19:33 UTC
This won't get pushed into 16.11.x, closing.
Comment 33 Jonathan Druart 2018-01-17 11:09:20 UTC
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?
Comment 34 Marcel de Rooy 2018-01-17 12:26:36 UTC
(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..
Comment 35 Marcel de Rooy 2018-01-17 12:27:23 UTC
We could more or less revert the change but return to the older inconsistent situation..
Comment 36 Katrin Fischer 2018-01-17 13:07:24 UTC
Make an exception for $9 as a special use case? Not sure if that is a valid option.
Comment 37 Marcel de Rooy 2018-01-17 13:08:29 UTC
(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..
Comment 38 Marcel de Rooy 2018-01-17 13:32:24 UTC
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.
Comment 39 Marcel de Rooy 2018-01-17 13:32:57 UTC
Could you test this one, Jonathan ?
Comment 40 Jonathan Druart 2018-01-22 15:51:56 UTC
(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'
Comment 41 Jonathan Druart 2018-01-22 16:45:06 UTC
(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
Comment 42 Marcel de Rooy 2018-01-23 13:24:34 UTC
(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