Bug 20074

Summary:	Auth_subfield_structure changes hidden attribute
Product:	Koha	Reporter:	Marcel de Rooy <m.de.rooy>
Component:	MARC Authority data support	Assignee:	Marcel de Rooy <m.de.rooy>
Status:	CLOSED FIXED	QA Contact:	Testopia <testopia>
Severity:	major
Priority:	P5 - low	CC:	jonathan.druart, thibaud.guillot
Version:	Main
Hardware:	All
OS:	All
Change sponsored?:	---	Patch complexity:	Small patch
Documentation contact:		Documentation submission:
Text to go in the release notes:		Version(s) released in:
Circulation function:
Bug Depends on:	18811
Bug Blocks:	20075
Attachments:	Bug 20074: Fix hidden value in auth_subfields_structure Bug 20074: Database revision to convert hidden to -5 Bug 20074: (Follow-up) Remove another remainder of biblio hidden logic Bug 20074: Fix hidden value in auth_subfields_structure Bug 20074: Database revision to convert hidden to -5 Bug 20074: (Follow-up) Remove another remainder of biblio hidden logic Bug 20074: Fix hidden value in auth_subfields_structure Bug 20074: Database revision to convert hidden to -5 Bug 20074: (Follow-up) Remove another remainder of biblio hidden logic Bug 20074: (Follow-up) Replace hidden value -5 by 1 in data and interface Add syspref to deal with overdues notification

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!

Comment 18 Thibaud Guillot (thibaud_g) 2021-12-10 15:17:37 UTC Comment hidden (obsolete)

Created attachment 128400 [details]
Add syspref to deal with overdues notification

I took the same test plan as victor but I added the system preference to manage the case more easily, especially for users who do not have access to the koha server.

Test plan
1. Check the size of the message queue
     With the following SQL query (using an SQL report if you want)
     SELECT COUNT(*) FROM message_queue;
2. Run misc/cronjobs/overdue_notices.pl
3. Check the size of the message queue
     To ensure that no other overdues will create noise in this test plan.
     Or you can take them into account.
4. Choose a patron with no email address
5. Create an overdue (checkout an item and unfold "Checkout settings"
   and set a date in the past which is compatible with what you find in
   staff:/cgi-bin/koha/tools/overduerules.pl
6. Run misc/cronjobs/overdue_notices.pl
7. Check that you have two new messages in the queue
8. Inspect these two messages
   SELECT * FROM message_queue ORDER BY time_queued DESC LIMIT 2 \G
   1. One has the type "print" and the borrowernumber matching the patron.
   2. The other has
        subject: Overdue Notices
        borrowernumber: NULL
        message_transport_type: email
        and contains "These messages were not sent directly to the patrons."
        This is the one we don't want anymore.
        Because it's now obsolete due to the first message.
9. Apply this patch
10. Change syspref 'EmailOverduesNoEmail' to "Don't send"
11. Delete data from message_queue (if you have access) for a cleaner view
12. Run again misc/cronjobs/overdue_notices.pl
13. Check that only the print message is now generated and not the
      "Overdue Notices" one.

Comment 19 Thibaud Guillot (thibaud_g) 2021-12-10 15:21:39 UTC Comment hidden (obsolete)

Sorry for disturbing I miss my last bug attachement with git bz :/