Bug 20280

Summary: Authorised value categories list in bibliographic frameworks has 2 empty entries
Product: Koha Reporter: Katrin Fischer <katrin.fischer>
Component: System AdministrationAssignee: Bugs List <koha-bugs>
Status: NEW --- QA Contact: Testopia <testopia>
Severity: minor    
Priority: P5 - low CC: gmcharlt, jonathan.druart, nick
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 20280: DB changes - wip

Description Katrin Fischer 2018-02-23 07:39:01 UTC
To test:
- Edit a subfield in any of your bibliographic frameworks
- In the last section, take a look at the authorised value categories list
- Confirm there are 2 empty entries

Tested with sample data.
Comment 1 Jonathan Druart 2018-03-01 15:16:09 UTC
installer/data/mysql/mandatory/auth_val_cat.sql:
  1 INSERT IGNORE INTO authorised_value_categories( category_name )
  2     VALUES
  3     (''),

but installer/data/mysql/updatedatabase.pl:
13164     INSERT IGNORE INTO authorised_value_categories( category_name )
13165     VALUES
13166     ('Asort1'),

"" is missing


Moreover:
  commit 62aa3f4292d83bc9304e296de5f0830e28510492
    Bug 17216: Use Koha::AVC from mss.pl
Added this additional blank entry in the template:
+                                <option value=""></option>

Additional info, in admin/authorised_values.pl we have:
215     my @categories = Koha::AuthorisedValueCategories->search({ category_name => { -not_in => ['', 'branches', 'itemtypes', 'cn_source']}}, { order_by => ['category_name'] } );

So maybe a solution could be to:
- remove all the additional entries in template (if there are others than in admin/marc_subfields_structure.tt)
- the add the AV category "" to existing install (unless it already exists)
Comment 2 Katrin Fischer 2019-02-10 13:23:10 UTC
Sorry Jonathan, not sure I understand. I think there should be no empty entry in our db table, but not sure why it was added in the first place.
Comment 3 Jonathan Druart 2019-02-22 13:48:02 UTC
(In reply to Katrin Fischer from comment #2)
> Sorry Jonathan, not sure I understand. I think there should be no empty
> entry in our db table, but not sure why it was added in the first place.

from auth_val_cat.sql?
Comment 4 Katrin Fischer 2019-02-22 14:42:38 UTC
(In reply to Jonathan Druart from comment #3)
> (In reply to Katrin Fischer from comment #2)
> > Sorry Jonathan, not sure I understand. I think there should be no empty
> > entry in our db table, but not sure why it was added in the first place.
> 
> from auth_val_cat.sql?

I saw that, but why is it there? :)
Comment 5 Jonathan Druart 2019-05-04 16:09:25 UTC
So, weird history.

On bug 17216 (which created the authorised_value_categories table), there is one commit adding the empty option in the template:
+                                <option value=""></option>

(Bug 17216: Use Koha::AVC from mss.pl)

Then one commit hiding the empty value from the DB:
  Bug 17216: Do no display the empty string category name

And finally the last commit (Bug 17216: Move the AV cat list file to the top level) changed the name of .sql installer file and add the empty entry in the DB (nothing in the commit description or the bug report tells something about that change).

Also, another occurrence is in auth_subfields_structure.pl where the empty entry comes from the DB.

Prior to 17216 the empty option was added once from the .pl and once from the .tt (not consistent but at least appeared once and only once (I think!).

The thing that is wrong is that, I guess, the empty row in DB was added to satisfy the FK (marc_subfield_structure.authorised_value), but it must actually be set to NULL instead of "". The update DB entry (16.06.00.033) set it to NULL, but next change to the framework set it to "".

All of that to say that I think we should:
1. uniformize the code to make sure we are using the same code everywhere (also see Koha::AuthorisedValues->new->categories vs Koha::AuthorisedValueCategories->search)
2. Add the empty option in template, but insert NULL when picked
3. In DB, update the '' occurrences with NULL
4. Remove the '' DB row from the .sql installer file

Hope that makes sense!
Comment 6 Jonathan Druart 2019-05-04 16:10:03 UTC
(And so comment 1 was wrong!)
Comment 7 Katrin Fischer 2020-05-23 11:41:03 UTC
> All of that to say that I think we should:
> 1. uniformize the code to make sure we are using the same code everywhere
> (also see Koha::AuthorisedValues->new->categories vs
> Koha::AuthorisedValueCategories->search)
> 2. Add the empty option in template, but insert NULL when picked
> 3. In DB, update the '' occurrences with NULL
> 4. Remove the '' DB row from the .sql installer file
> 
> Hope that makes sense!

It does make sense, but no idea how to start there!
Comment 8 Katrin Fischer 2020-07-01 21:29:55 UTC
*** Bug 25910 has been marked as a duplicate of this bug. ***
Comment 9 Jonathan Druart 2020-07-28 08:40:12 UTC
The tricky part is the installer files. It's a mess and it will be too time consuming to deal with them.
Comment 10 Jonathan Druart 2020-07-28 08:41:50 UTC
Created attachment 107458 [details] [review]
Bug 20280: DB changes - wip
Comment 11 Jonathan Druart 2020-07-28 08:43:17 UTC
Here is a start. I gave up when I see the amount of conscientious work that needs to be done in the .sql files of the different languages.
Comment 12 Katrin Fischer 2020-10-21 00:54:04 UTC
Maybe we could revive that after more installers have been moved to the yml structure? (RESOLVED LATER?)