Bug 9701

Summary: Configure default indicators
Product: Koha Reporter: Anthony Laquerre <anthony.laquerre>
Component: CatalogingAssignee: joachim.ganseman
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: abl, amy, black23, eugenegf, joachim.ganseman, jonathan.druart, josef.moravec, katrin.fischer, m.de.rooy, maksim.sen, marjorie.barry-vila, mtompset, nick, nicole, patrick.robitaille, sandboxes, steven_jeffery, ztajoli
Version: master   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8380
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=2280
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14769
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20547
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
This adds default indicators to bibliographic frameworks. The table marc_tag_structure is adjusted. In order to make effective use of this enhancement, you may want to add values in your MARC frameworks administration.
Version(s) released in:
Bug Depends on:    
Bug Blocks: 21749    
Attachments: Bug 9701 - Configure default indicator
With this second patch, bug 9701 is entirely fixed. The default values are also loaded in the cataloguing form. The database update is now done according to the database update workflow. Previous changes to files that ought not to have changed, done in th
Bug 9701 - fixed typo in atomic update file. Sorry for that!
Bug 9701 - Configure default indicator
With this second patch, bug 9701 is entirely fixed. The default values are also loaded in the cataloguing form. The database update is now done according to the database update workflow. Previous changes to files that ought not to have changed, done in th
Bug 9701 - fixed typo in atomic update file. Sorry for that!
Bug 9701 - Configure default indicator
Bug 9701: QA follow-up - Fix indentation and some strings
Bug 9701: Configure default indicator
Bug 9701: (QA follow-up) Fix indentation and some strings
Bug 9701: (follow-up) Adjust tests
Bug 9701: Configure default indicator
Bug 9701: (QA follow-up) Fix indentation and some strings
Bug 9701: (follow-up) Adjust tests
Bug 9701: Followup to make test pass
Bug 9701: Configure default indicator
Bug 9701: (QA follow-up) Fix indentation and some strings
Bug 9701: (follow-up) Adjust tests
Bug 9701: Followup to make test pass
Bug 9701: Configure default indicator
Bug 9701: (follow-up) Adjust tests
Bug 9701: (QA follow-up) DBIx schema changes

Description Anthony Laquerre 2013-02-22 21:35:25 UTC
Request for enhancement.

Allow to configure default indicators (eg allow us to impose "1" as the first indicator for an individual authority, because it is the one which is adequate most of the time, and right now, it must be added manually each time.
Comment 1 Eugene Espinoza 2015-11-20 14:48:14 UTC
Not only in the first indicator but also in the second indicator, if the system can let the users put default values in the indicators. This can addressed either: 1) In edit field of the MARC Bibliographic framework, there should be another option for the first indicator and second indicator, or 2) In the subfields where a particular subfield affects the indicators (e.g. When 650$2 is filled up, then the indicator should be 7). In no. 2, there should probably be a mechanism of "if and then" much like the MARC modification template we already have in Koha. No. 2 is kind of difficult to implement unlike 1, where you outright assign the indicators. But no. 1 has some problems too, like assigning indicators that are based on the number of characters (e.g. in 245 indicator 2). These are just my thoughts.
Comment 2 joachim.ganseman 2016-06-03 07:49:18 UTC
I guess the easiest way, in order not to complicate the MARC framework even more, would be to add a two columns with default values (or just leave them empty to have no default) in the default MARC framework, then in the MARC Framework editing screen add a possibility to set a default value just as for the regular subfields.

Checking for some impossible or incorrect combinations (like the previously mentioned indicator 7 if 650$2 exists), could maybe be moved to a general "consistency check" routine that happens after editing the record. Or to a plugin?
Comment 3 joachim.ganseman 2016-06-04 13:34:17 UTC Comment hidden (obsolete)
Comment 4 Zeno Tajoli 2016-06-06 08:36:20 UTC
*** Bug 8380 has been marked as a duplicate of this bug. ***
Comment 5 joachim.ganseman 2016-06-06 14:06:12 UTC Comment hidden (obsolete)
Comment 6 Eugene Espinoza 2016-06-07 04:16:30 UTC
I'm testing this in the sandbox and I'm getting this error when I go to the MARC structure: Software error:

Can't use an undefined value as an ARRAY reference at /usr/local/lib/perl/5.14.2/DBI.pm line 2064.
Even the email I received, I got the following:

The sandbox you've requested is now ready.
The patches attached to bugzilla 9701 have been applied, and the result is:

Updatedatabase:
Upgrade to 16.05.00.000 done (Koha 16.05)
Upgrade to 16.06.00.000 done (Koha 16.06 - starting a new dev line at KohaCon16 in Thessaloniki, Greece! Koha is great!)
Upgrade to 16.06.00.001 done (Bug 16200 - 'Hold waiting too long' fee has a translation problem)
DEV atomic update: bug_9701-add_default_indicators.sql
C4::Installer::load_sql returned the following errors while attempting to load /home/koha/src/installer/data/mysql/atomicupdate/bug_9701-add_default_indicators.sql:

So with this, I can't proceed further.
Comment 7 Nick Clemens 2016-06-07 16:02:48 UTC
The atomic update file has 'z' instead of 's' in authorised_value

You don't need to add the updatedabase.pl changes - the RM does that now:
https://wiki.koha-community.org/wiki/Database_updates
Comment 8 joachim.ganseman 2016-06-07 16:07:42 UTC Comment hidden (obsolete)
Comment 9 Eugene Espinoza 2016-06-08 01:21:29 UTC
Looks great now! :) I'm signing this one off. I have a question though but might not be related with this enhancement. When the tag is cloned/repeated and the indicator of this cloned tag is not the default indicator, it's not the difficult indicator that appears instead the indicator in the repeated/cloned tag. Well, that I guess is for another bug. But this enhancement does what original poster requested. Thanks a lot Joachim!
Comment 10 PTFS Europe Sandboxes 2016-06-08 02:03:16 UTC
Patch tested with a sandbox, by Eugene Jose Espinoza <eugenegf@yahoo.com>
Comment 11 PTFS Europe Sandboxes 2016-06-08 02:03:55 UTC Comment hidden (obsolete)
Comment 12 PTFS Europe Sandboxes 2016-06-08 02:03:59 UTC Comment hidden (obsolete)
Comment 13 PTFS Europe Sandboxes 2016-06-08 02:04:03 UTC Comment hidden (obsolete)
Comment 14 joachim.ganseman 2016-06-08 10:17:48 UTC
(In reply to Eugene Espinoza from comment #9)
> Looks great now! :) I'm signing this one off. I have a question though but
> might not be related with this enhancement. When the tag is cloned/repeated
> and the indicator of this cloned tag is not the default indicator, it's not
> the difficult indicator that appears instead the indicator in the
> repeated/cloned tag. Well, that I guess is for another bug. But this
> enhancement does what original poster requested. Thanks a lot Joachim!

I'm not sure, that duplication behavior could also be seen as a feature instead of a bug :) . However, one thing is that this patch only creates default indicator settings for bibliographic records. The same for authority records is still a possible future enhancement. Not sure how urgent that would be though, since in my opinion authority records are usually kept pretty minimalistic in most setups.
Comment 15 Eugene Espinoza 2016-06-08 10:23:44 UTC
I'm not sure, that duplication behavior could also be seen as a feature instead of a bug

-It should be feature enhancement, my bad! :) And that's another enhancement. But still, good job on this! We just have to scour bugzilla about this. Cheers!
Comment 16 Katrin Fischer 2016-07-14 23:52:40 UTC
Hi Joachim,

I have squashed your 3 original patches to avoid conflicts caused by adding and removing the change to updatedatabase.pl in the first 2 patches. Also fixes the problem that only the last patch was signed off by the sandbox script.

I also added a follow-up fixing tabs pointed out by the QA script - please keep in mind that we are using 4 spaces instead of tabs!

The patches work really nicely. We are close to getting this through QA, but please add tests for your change to GetMarcStructure. For changes in the core modules, unit tests are mandatory. Please let us know if you need any help with that!
Comment 17 Katrin Fischer 2016-07-14 23:53:28 UTC
Created attachment 53403 [details] [review]
Bug 9701 - Configure default indicator

This patch adds the possibility to define default indicators in
the MARC frameworks. It adds 2 columns in the marc_tag_structure table in
the database in order to accomplish this. All files that reference this
table have also been updated to reflect these added fields.

Test: Add or edit a MARC framework. In the Field list should be 2 extra
columns. It should be possible to add default indicators (1 character)
    in these fields. Nothing else should have changed in the meantime.
    The default indicator values are not yet visible in the cataloguing module.

The default values are also loaded in the cataloguing form.

Test: Define default values in some MARC framework. Go to cataloguing
and create a new record using this framework. Verify that the defined
defaults are visible when set. Verify the default is empty (as before)
    if no default was set. Verify that if the default is changed, the
    record is saved with the manually changed value. Verify that upon
    changing such a new record, the manually set indicator value is used
    and not the default one from the framework.

Signed-off-by: Eugene Jose Espinoza <eugenegf@yahoo.com>
Comment 18 Katrin Fischer 2016-07-14 23:54:01 UTC
Created attachment 53404 [details] [review]
Bug 9701: QA follow-up - Fix indentation and some strings

- Fixes tabs pointed out by the QA script.
- Changes table headings Indic1 and Indic2 to Ind. 1 and Ind. 2
Comment 19 Josef Moravec 2016-10-25 08:37:11 UTC
What is the status of this bug?
Comment 20 Marcel de Rooy 2017-01-17 08:01:39 UTC
We need a default indicator in the framework too when we merge authorities across the authtype border. See bug 14769.
Can you get this to work again, Joachim ?
Comment 21 Josef Moravec 2018-02-19 10:40:49 UTC
Created attachment 71929 [details] [review]
Bug 9701: Configure default indicator

This patch adds the possibility to define default indicators in
the MARC frameworks. It adds 2 columns in the marc_tag_structure table in
the database in order to accomplish this. All files that reference this
table have also been updated to reflect these added fields.

Test: Add or edit a MARC framework. In the Field list should be 2 extra
columns. It should be possible to add default indicators (1 character)
    in these fields. Nothing else should have changed in the meantime.
    The default indicator values are not yet visible in the cataloguing module.

The default values are also loaded in the cataloguing form.

Test: Define default values in some MARC framework. Go to cataloguing
and create a new record using this framework. Verify that the defined
defaults are visible when set. Verify the default is empty (as before)
    if no default was set. Verify that if the default is changed, the
    record is saved with the manually changed value. Verify that upon
    changing such a new record, the manually set indicator value is used
    and not the default one from the framework.

Don't forget to run database and database schema update

Signed-off-by: Eugene Jose Espinoza <eugenegf@yahoo.com>
Comment 22 Josef Moravec 2018-02-19 10:40:54 UTC
Created attachment 71930 [details] [review]
Bug 9701: (QA follow-up) Fix indentation and some strings

- Fixes tabs pointed out by the QA script.
Comment 23 Josef Moravec 2018-02-19 10:40:59 UTC
Created attachment 71931 [details] [review]
Bug 9701: (follow-up) Adjust tests

Test plan:

prove t/db_dependent/Biblio.t
should be green
Comment 24 Marcel de Rooy 2018-02-20 15:00:44 UTC
My git does not like that second patch..

Applying: Bug 9701: (QA follow-up) Fix indentation and some strings
*** Error in `git': malloc(): memory corruption: 0x0000000002837340 ***
fatal: sha1 information is lacking or useless (C4/Biblio.pm).
Repository lacks necessary blobs to fall back on 3-way merge.
Comment 25 Josef Moravec 2018-02-20 19:22:05 UTC
Probably my bad, I will try to rebase and re-upload patches, but probably not today, and tomorow I am out, so please be patient...
Comment 26 Josef Moravec 2018-02-20 19:41:26 UTC
Created attachment 72010 [details] [review]
Bug 9701: Configure default indicator

This patch adds the possibility to define default indicators in
the MARC frameworks. It adds 2 columns in the marc_tag_structure table in
the database in order to accomplish this. All files that reference this
table have also been updated to reflect these added fields.

Test: Add or edit a MARC framework. In the Field list should be 2 extra
columns. It should be possible to add default indicators (1 character)
    in these fields. Nothing else should have changed in the meantime.
    The default indicator values are not yet visible in the cataloguing module.

The default values are also loaded in the cataloguing form.

Test: Define default values in some MARC framework. Go to cataloguing
and create a new record using this framework. Verify that the defined
defaults are visible when set. Verify the default is empty (as before)
    if no default was set. Verify that if the default is changed, the
    record is saved with the manually changed value. Verify that upon
    changing such a new record, the manually set indicator value is used
    and not the default one from the framework.

Don't forget to run database and database schema update

Signed-off-by: Eugene Jose Espinoza <eugenegf@yahoo.com>
Comment 27 Josef Moravec 2018-02-20 19:41:32 UTC
Created attachment 72011 [details] [review]
Bug 9701: (QA follow-up) Fix indentation and some strings

- Fixes tabs pointed out by the QA script.
Comment 28 Josef Moravec 2018-02-20 19:41:37 UTC
Created attachment 72012 [details] [review]
Bug 9701: (follow-up) Adjust tests

Test plan:

prove t/db_dependent/Biblio.t
should be green
Comment 29 Josef Moravec 2018-02-20 19:42:11 UTC
Marcel, try it now, it applies cleanly on current master for me now.
Comment 30 Marcel de Rooy 2018-02-21 12:50:25 UTC
Applying: Bug 9701: (QA follow-up) Fix indentation and some strings
*** Error in `git': malloc(): memory corruption: 0x0000000000bfc340 ***
fatal: sha1 information is lacking or useless (C4/Biblio.pm).
Comment 31 Josef Moravec 2018-02-21 16:27:33 UTC
(In reply to Marcel de Rooy from comment #30)
> Applying: Bug 9701: (QA follow-up) Fix indentation and some strings
> *** Error in `git': malloc(): memory corruption: 0x0000000000bfc340 ***
> fatal: sha1 information is lacking or useless (C4/Biblio.pm).

I am not understand what is going here - I tried now, and it applied cleanly on current master...
Comment 32 Maksim Sen 2018-03-09 19:32:34 UTC
Hey Josef,

I tried doing this test:
prove t/db_dependent/Biblio.t
should be green


Before applying the patches I had this:
prove t/db_dependent/Biblio.t 
t/db_dependent/Biblio.t .. ok   
All tests successful.
Files=1, Tests=9,  2 wallclock secs ( 0.05 usr  0.01 sys +  1.91 cusr  0.09 csys =  2.06 CPU)
Result: PASS


After applying the patches I had this:
prove t/db_dependent/Biblio.t 
t/db_dependent/Biblio.t .. 7/9 DBD::mysql::st execute failed: Unknown column 'ind1_defaultvalue' in 'field list' [for Statement "SELECT tagfield,liblibrarian,libopac,mandatory,repeatable,ind1_defaultvalue,ind2_defaultvalue
        FROM marc_tag_structure 
        WHERE frameworkcode=? 
        ORDER BY tagfield" with ParamValues: 0=''] at /inlibro/git/koha-master-dev-inlibro/C4/Biblio.pm line 990.
DBD::mysql::st fetchrow failed: fetch() without execute() [for Statement "SELECT tagfield,liblibrarian,libopac,mandatory,repeatable,ind1_defaultvalue,ind2_defaultvalue
        FROM marc_tag_structure 
        WHERE frameworkcode=? 
        ORDER BY tagfield" with ParamValues: 0=''] at /inlibro/git/koha-master-dev-inlibro/C4/Biblio.pm line 993.

    #   Failed test 'expect 6 internals'
    #   at t/db_dependent/Biblio.t line 436.
    #          got: '0'
    #     expected: '6'

    #   Failed test 'check lib'
    #   at t/db_dependent/Biblio.t line 437.
    #          got: '0'
    #     expected: '1'

    #   Failed test 'check tab'
    #   at t/db_dependent/Biblio.t line 438.
    #          got: '0'
    #     expected: '1'

    #   Failed test 'check mandatory'
    #   at t/db_dependent/Biblio.t line 439.
    #          got: '0'
    #     expected: '1'

    #   Failed test 'check repeatable'
    #   at t/db_dependent/Biblio.t line 440.
    #          got: '0'
    #     expected: '1'

    #   Failed test 'check indicator 1 default value'
    #   at t/db_dependent/Biblio.t line 442.
    #          got: '0'
    #     expected: '1'

    #   Failed test 'check indicator 2 default value'
    #   at t/db_dependent/Biblio.t line 443.
    #          got: '0'
    #     expected: '1'
    # Looks like you failed 7 tests of 8.

#   Failed test 'IsMarcStructureInternal'
#   at t/db_dependent/Biblio.t line 444.
# Looks like you failed 1 test of 9.
t/db_dependent/Biblio.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/9 subtests 

Test Summary Report
-------------------
t/db_dependent/Biblio.t (Wstat: 256 Tests: 9 Failed: 1)
  Failed test:  8
  Non-zero exit status: 1
Files=1, Tests=9,  3 wallclock secs ( 0.05 usr  0.00 sys +  2.01 cusr  0.10 csys =  2.16 CPU)
Result: FAIL
Comment 33 Josef Moravec 2018-03-09 19:48:40 UTC
(In reply to Maksim Sen from comment #32)
> 
> After applying the patches I had this:
> prove t/db_dependent/Biblio.t 
> t/db_dependent/Biblio.t .. 7/9 DBD::mysql::st execute failed: Unknown column
> 'ind1_defaultvalue' in 'field list' 

did you perform database update?
Comment 34 Mark Tompsett 2018-03-15 18:54:23 UTC
t/db_dependent/Biblio.t is failing... there is a difference between unsafe and empty call to GetMarcStructure for some reason. Haven't traced it back enough. This is a side issue that is failing this bug.
Comment 35 Mark Tompsett 2018-03-16 01:53:13 UTC
Created attachment 72986 [details] [review]
Bug 9701: Followup to make test pass
Comment 36 Nick Clemens 2018-04-06 13:34:55 UTC
Created attachment 73780 [details] [review]
Bug 9701: Configure default indicator

This patch adds the possibility to define default indicators in
the MARC frameworks. It adds 2 columns in the marc_tag_structure table in
the database in order to accomplish this. All files that reference this
table have also been updated to reflect these added fields.

Test: Add or edit a MARC framework. In the Field list should be 2 extra
columns. It should be possible to add default indicators (1 character)
    in these fields. Nothing else should have changed in the meantime.
    The default indicator values are not yet visible in the cataloguing module.

The default values are also loaded in the cataloguing form.

Test: Define default values in some MARC framework. Go to cataloguing
and create a new record using this framework. Verify that the defined
defaults are visible when set. Verify the default is empty (as before)
    if no default was set. Verify that if the default is changed, the
    record is saved with the manually changed value. Verify that upon
    changing such a new record, the manually set indicator value is used
    and not the default one from the framework.

Don't forget to run database and database schema update

Signed-off-by: Eugene Jose Espinoza <eugenegf@yahoo.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 37 Nick Clemens 2018-04-06 13:35:00 UTC
Created attachment 73781 [details] [review]
Bug 9701: (QA follow-up) Fix indentation and some strings

- Fixes tabs pointed out by the QA script.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 38 Nick Clemens 2018-04-06 13:35:05 UTC
Created attachment 73782 [details] [review]
Bug 9701: (follow-up) Adjust tests

Test plan:

prove t/db_dependent/Biblio.t
should be green

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 39 Nick Clemens 2018-04-06 13:35:09 UTC
Created attachment 73783 [details] [review]
Bug 9701: Followup to make test pass

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 40 Katrin Fischer 2018-04-07 07:56:03 UTC
Hi Marcel, can you please take care of QA here as your patch builds on it?
Comment 41 Marcel de Rooy 2018-04-09 06:40:38 UTC
(In reply to Katrin Fischer from comment #40)
> Hi Marcel, can you please take care of QA here as your patch builds on it?

Yes I will. They are related but do not really depend on each other.
Comment 42 Marcel de Rooy 2018-04-09 07:02:16 UTC
Comment on attachment 73781 [details] [review]
Bug 9701: (QA follow-up) Fix indentation and some strings

This patch continually generates malloc errors and other git problems with me. It seems that we do not need it too. I have no qa warnings about tabs.
Comment 43 Marcel de Rooy 2018-04-09 07:37:03 UTC
(In reply to M. Tompsett from comment #34)
> t/db_dependent/Biblio.t is failing... there is a difference between unsafe
> and empty call to GetMarcStructure for some reason. Haven't traced it back
> enough. This is a side issue that is failing this bug.

I'm having the impression that the third patch changing forlibrarian to 1 is not needed. We only need to flush the cache once ?
Comment 44 Marcel de Rooy 2018-04-09 08:02:35 UTC
Created attachment 73843 [details] [review]
Bug 9701: Configure default indicator

This patch adds the possibility to define default indicators in
the MARC frameworks. It adds 2 columns in the marc_tag_structure table in
the database in order to accomplish this. All files that reference this
table have also been updated to reflect these added fields.

Test: Add or edit a MARC framework. In the Field list should be 2 extra
columns. It should be possible to add default indicators (1 character)
    in these fields. Nothing else should have changed in the meantime.
    The default indicator values are not yet visible in the cataloguing module.

The default values are also loaded in the cataloguing form.

Test: Define default values in some MARC framework. Go to cataloguing
and create a new record using this framework. Verify that the defined
defaults are visible when set. Verify the default is empty (as before)
    if no default was set. Verify that if the default is changed, the
    record is saved with the manually changed value. Verify that upon
    changing such a new record, the manually set indicator value is used
    and not the default one from the framework.

Don't forget to run database and database schema update

Signed-off-by: Eugene Jose Espinoza <eugenegf@yahoo.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 45 Marcel de Rooy 2018-04-09 08:02:42 UTC
Created attachment 73844 [details] [review]
Bug 9701: (follow-up) Adjust tests

Test plan:

prove t/db_dependent/Biblio.t
should be green

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 46 Marcel de Rooy 2018-04-09 08:02:47 UTC
Created attachment 73845 [details] [review]
Bug 9701: (QA follow-up) DBIx schema changes

Changes in MarcTagStructure.pm

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 47 Katrin Fischer 2018-04-09 08:09:23 UTC
(In reply to Marcel de Rooy from comment #42)
> Comment on attachment 73781 [details] [review] [review]
> Bug 9701: (QA follow-up) Fix indentation and some strings
> 
> This patch continually generates malloc errors and other git problems with
> me. It seems that we do not need it too. I have no qa warnings about tabs.

Strange, but I am not attached to it.
Comment 48 Marcel de Rooy 2018-04-09 08:14:46 UTC
(In reply to Katrin Fischer from comment #47)
> (In reply to Marcel de Rooy from comment #42)
> > Comment on attachment 73781 [details] [review] [review] [review]
> > Bug 9701: (QA follow-up) Fix indentation and some strings
> > 
> > This patch continually generates malloc errors and other git problems with
> > me. It seems that we do not need it too. I have no qa warnings about tabs.
> 
> Strange, but I am not attached to it.

Comment24 for instance
Comment 49 Jonathan Druart 2018-04-12 15:05:39 UTC
Patches pushed to master for 18.05.

Congratulations Joachim for your first patch in!
Comment 50 Nick Clemens 2018-05-14 19:34:52 UTC
Enhancement, not backported for 17.11.x