Bug 20074 - Auth_subfield_structure changes hidden attribute
Summary: Auth_subfield_structure changes hidden attribute
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: MARC Authority data support (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 18811
Blocks: 20075
  Show dependency treegraph
 
Reported: 2018-01-23 13:23 UTC by Marcel de Rooy
Modified: 2018-06-04 20:15 UTC (History)
2 users (show)

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


Attachments
Bug 20074: Fix hidden value in auth_subfields_structure (2.06 KB, patch)
2018-01-23 14:24 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20074: Database revision to convert hidden to -5 (1.31 KB, patch)
2018-01-23 14:24 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20074: (Follow-up) Remove another remainder of biblio hidden logic (1.14 KB, patch)
2018-01-23 15:08 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20074: Fix hidden value in auth_subfields_structure (2.02 KB, patch)
2018-02-19 09:41 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20074: Database revision to convert hidden to -5 (1.33 KB, patch)
2018-02-19 09:41 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20074: (Follow-up) Remove another remainder of biblio hidden logic (1.16 KB, patch)
2018-02-19 09:41 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20074: Fix hidden value in auth_subfields_structure (2.08 KB, patch)
2018-02-25 22:54 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 20074: Database revision to convert hidden to -5 (1.38 KB, patch)
2018-02-25 22:54 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 20074: (Follow-up) Remove another remainder of biblio hidden logic (1.22 KB, patch)
2018-02-25 22:54 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 20074: (Follow-up) Replace hidden value -5 by 1 in data and interface (2.68 KB, patch)
2018-02-26 14:13 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 2018-01-23 13:23:19 UTC
From bug 18811 comment40:

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 1 Marcel de Rooy 2018-01-23 14:24:20 UTC
Created attachment 70842 [details] [review]
Bug 20074: Fix hidden value in auth_subfields_structure

The template included an else branch where a value not equal to 0 or -5
was interpreted as 0 (Show all).
This effectively converted hidden values 1 and 8 to 0.
This patch removes that else branch.

Test plan:
Toggle a authority field between Hide all and Show all. Check what happens.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 2 Marcel de Rooy 2018-01-23 14:24:24 UTC
Created attachment 70843 [details] [review]
Bug 20074: Database revision to convert hidden to -5

The hidden value is used as a boolean in authority frameworks (in contrast
with biblio framework).
It is consistent to apply the same value (-5) to all non-zero values.

Test plan:
Check if you a have 0, 1 and 8 value in hidden somewhere.
Run the dbrev.
Verify that 1 and 8 are converted to -5.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 3 Marcel de Rooy 2018-01-23 14:28:14 UTC
Another improvement would be to correct the sql installer files.
See bug 20075
Comment 4 Marcel de Rooy 2018-01-23 15:08:50 UTC
Created attachment 70847 [details] [review]
Bug 20074: (Follow-up) Remove another remainder of biblio hidden logic

Script authorities.pl still contains a module 2 calculation on the hidden
attribute, although it is used as a boolean.
Since -5 mod 2 == 1, it does no harm, but we better remove it.

Test plan:
Edit and save an authority. Verify that everything still works as expected.
Comment 5 Josef Moravec 2018-02-19 09:41:25 UTC
Created attachment 71926 [details] [review]
Bug 20074: Fix hidden value in auth_subfields_structure

The template included an else branch where a value not equal to 0 or -5
was interpreted as 0 (Show all).
This effectively converted hidden values 1 and 8 to 0.
This patch removes that else branch.

Test plan:
Toggle a authority field between Hide all and Show all. Check what happens.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 6 Josef Moravec 2018-02-19 09:41:28 UTC
Created attachment 71927 [details] [review]
Bug 20074: Database revision to convert hidden to -5

The hidden value is used as a boolean in authority frameworks (in contrast
with biblio framework).
It is consistent to apply the same value (-5) to all non-zero values.

Test plan:
Check if you a have 0, 1 and 8 value in hidden somewhere.
Run the dbrev.
Verify that 1 and 8 are converted to -5.

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

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 7 Josef Moravec 2018-02-19 09:41:30 UTC
Created attachment 71928 [details] [review]
Bug 20074: (Follow-up) Remove another remainder of biblio hidden logic

Script authorities.pl still contains a module 2 calculation on the hidden
attribute, although it is used as a boolean.
Since -5 mod 2 == 1, it does no harm, but we better remove it.

Test plan:
Edit and save an authority. Verify that everything still works as expected.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 8 Katrin Fischer 2018-02-25 22:54:28 UTC
Created attachment 72191 [details] [review]
Bug 20074: Fix hidden value in auth_subfields_structure

The template included an else branch where a value not equal to 0 or -5
was interpreted as 0 (Show all).
This effectively converted hidden values 1 and 8 to 0.
This patch removes that else branch.

Test plan:
Toggle a authority field between Hide all and Show all. Check what happens.

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

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 9 Katrin Fischer 2018-02-25 22:54:34 UTC
Created attachment 72192 [details] [review]
Bug 20074: Database revision to convert hidden to -5

The hidden value is used as a boolean in authority frameworks (in contrast
with biblio framework).
It is consistent to apply the same value (-5) to all non-zero values.

Test plan:
Check if you a have 0, 1 and 8 value in hidden somewhere.
Run the dbrev.
Verify that 1 and 8 are converted to -5.

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

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

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 10 Katrin Fischer 2018-02-25 22:54:38 UTC
Created attachment 72193 [details] [review]
Bug 20074: (Follow-up) Remove another remainder of biblio hidden logic

Script authorities.pl still contains a module 2 calculation on the hidden
attribute, although it is used as a boolean.
Since -5 mod 2 == 1, it does no harm, but we better remove it.

Test plan:
Edit and save an authority. Verify that everything still works as expected.

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

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 11 Katrin Fischer 2018-02-25 22:55:55 UTC
The database update will take care of the problem for existing installations, but we will reintroduce the problem in every new installation as long as bug 20075 is not fixed. Maybe we should hold off the database update here or perform it a second time with the fix for the frameworks?
Comment 12 Marcel de Rooy 2018-02-26 07:41:10 UTC
(In reply to Katrin Fischer from comment #11)
> The database update will take care of the problem for existing
> installations, but we will reintroduce the problem in every new installation
> as long as bug 20075 is not fixed. Maybe we should hold off the database
> update here or perform it a second time with the fix for the frameworks?

Thx Katrin.
The problem we solve here, is not reintroduced since we improved the logic. I guess it is still possible to get 20075 also into 18.05, but we could repeat the db update on 20075 too altough is has only theoretical value for purists.
Comment 13 Katrin Fischer 2018-02-26 08:24:17 UTC
(In reply to Marcel de Rooy from comment #12)
> (In reply to Katrin Fischer from comment #11)
> > The database update will take care of the problem for existing
> > installations, but we will reintroduce the problem in every new installation
> > as long as bug 20075 is not fixed. Maybe we should hold off the database
> > update here or perform it a second time with the fix for the frameworks?
> 
> Thx Katrin.
> The problem we solve here, is not reintroduced since we improved the logic.
> I guess it is still possible to get 20075 also into 18.05, but we could
> repeat the db update on 20075 too altough is has only theoretical value for
> purists.

Poor choice of words - "reintroduce unclean data" would have been better :)
Comment 14 Marcel de Rooy 2018-02-26 10:59:46 UTC
(In reply to Katrin Fischer from comment #13)
> (In reply to Marcel de Rooy from comment #12)
> > (In reply to Katrin Fischer from comment #11)
> > > The database update will take care of the problem for existing
> > > installations, but we will reintroduce the problem in every new installation
> > > as long as bug 20075 is not fixed. Maybe we should hold off the database
> > > update here or perform it a second time with the fix for the frameworks?
> > 
> > Thx Katrin.
> > The problem we solve here, is not reintroduced since we improved the logic.
> > I guess it is still possible to get 20075 also into 18.05, but we could
> > repeat the db update on 20075 too altough is has only theoretical value for
> > purists.
> 
> Poor choice of words - "reintroduce unclean data" would have been better :)

OK. I submitted a few patches on 20075 to obtain clean data.
Comment 15 Marcel de Rooy 2018-02-26 14:13:44 UTC
Created attachment 72209 [details] [review]
Bug 20074: (Follow-up) Replace hidden value -5 by 1 in data and interface

[ Moved from 20075 to 20074 ]

Since hidden is used as a boolean, it makes more sense to save the values
0 and 1 instead of 0 and -5.

Test plan:
Test toggling between Show all and Hide all in the auth frameworks.
Run the db rev.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 16 Marcel de Rooy 2018-02-26 14:14:23 UTC
If you agree, Jonathan, take the last patch along coming from 20075. It fits better here..
Comment 17 Jonathan Druart 2018-02-26 16:28:58 UTC
Pushed to master for 18.05, thanks to everybody involved!