Bug 25449

Summary: Make itemtype mandatory by default
Product: Koha Reporter: Katrin Fischer <katrin.fischer>
Component: MARC Bibliographic data supportAssignee: Jonathan Druart <jonathan.druart+koha>
Status: Needs documenting --- QA Contact: Katrin Fischer <katrin.fischer>
Severity: enhancement    
Priority: P5 - low CC: david, dcook, jonathan.druart+koha, liz, lucas, martin.renvoize, nick, phil, tomascohen, victor
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24331
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00
Attachments: Bug 25449: Set items.itype mandatory by default
Bug 25449: Set items.itype mandatory by default
Bug 25449: Set items.itype mandatory by default
Bug 25449: Set items.itype mandatory by default
Bug 25449: (QA follow-up) Set items.itype to mandatory by default (UNIMARC)
Bug 25449: Fix basic_workflow.t
Bug 25449: (QA follow-up) Fix basic_workflow.t

Description Katrin Fischer 2020-05-10 12:19:37 UTC
Items without homebranch, holding banch and/or itemtype can cause errors in Koha that are not all caught (and probably never will). 

As we ship Koha with item-level_itypes = item and this is the most common setting, I suggest to make the itemtype mandatory in our default and additional sample frameworks to help avoid problems.
Comment 1 Jonathan Druart 2020-05-20 15:10:54 UTC
*** Bug 24331 has been marked as a duplicate of this bug. ***
Comment 2 Jonathan Druart 2020-05-26 10:08:54 UTC
Created attachment 105351 [details] [review]
Bug 25449: Set items.itype mandatory by default
Comment 3 Jonathan Druart 2020-05-26 10:12:10 UTC
Should we:
1. Modify the mandatory flag for all the frameworks if item-level_itypes is modified (and add a note in the syspref's description)?
2. If yes, do the same with biblioitems.itemtype?
3. Create a way to define a "default" item type that will be preselected?
4. Or should we make biblioitems.itypetype mandatory as well (and it will be the default when we create we new item).
Comment 4 Jonathan Druart 2020-11-11 08:08:08 UTC
From a dev meeting:

https://meetings.koha-community.org/2020/development_irc_meeting_20_may_2020.2020-05-20-14.00.log.html

14:39:39 <Joubu> #action Joubu add a fix for 25449
14:39:51 <Joubu> #action cait signoff on 25449
14:39:57 <Joubu> #action kidclamp QA 25449
Comment 5 Phil Ringnalda 2021-01-13 22:36:11 UTC
942$c, biblioitems.itemtype, (but not 942 itself) is already mandatory in every framework except ACQ, which makes it easy to see how toothless mandatory framework subfields are.

Add a record in the default framework with the basic editor, you are required to have a 942$c to save the record.

Add a record in the default framework with the advanced editor, you are only required to have a 942$c if you add a 942, but you aren't required to add a 942.

Add a record in the default framework with batch import, you aren't required to have a 942$c even if you make 942 mandatory (and yes, that does also mean that making items.itype mandatory in the framework has no effect on items created by batch import).

So this bug needs to depend on a bug to change batch import, either by respecting mandatory fields/subfields in frameworks (which would be a huge behavior change, requiring users to learn how to use MarcEdit to batch change records before importing them) or by adding a select list to the actual import step to choose a "Default itemtype if 942$c or 952$y is missing", and for mandatory biblioitems.itemtype to actually work with the advanced editor, the 942 field needs to also be mandatory.

To me, that makes taking the patch from bug 24331 to stop throwing an ISE when a patron places a hold seem like a pretty good idea.
Comment 6 Victor Grousset/tuxayo 2021-04-30 23:36:33 UTC
Should this ticket be "in discussion" or "assigned"(i.e. needs work)

If not, what would be a test plan for the current patch?
Comment 7 Martin Renvoize 2021-05-05 14:51:51 UTC
I think the work here is a solid first step.

Whilst I think I understand Phil's concerns, I think those should be addressed as their own bugs after this one... improving adherence of the advanced editor and adding defaults to bulk imports deserve a bug each.
Comment 8 Jonathan Druart 2022-06-02 06:21:18 UTC
Created attachment 135571 [details] [review]
Bug 25449: Set items.itype mandatory by default
Comment 9 David Nind 2022-06-06 03:37:13 UTC
Created attachment 135703 [details] [review]
Bug 25449: Set items.itype mandatory by default

Signed-off-by: David Nind <david@davidnind.com>
Comment 10 David Nind 2022-06-06 03:44:55 UTC
Tested using koha-testing-docker:

1. Reset KTD so that the web installer is used to setup Koha (see 'Start the web installer' for steps on how to do this).
2. Walk through the setup:
   . When setting up the default settings, don't add any of the optional data except item types
3. Once setup and logged in, check the two frameworks (default and ACQ) and note that $952$y is not mandatory.
4. Apply the patch.
5. Repeat steps 1-2.
6. Once setup and logged in, check the two frameworks (default and ACQ) and note that $952$y is now mandatory.

Start the web installer
~~~~~~~~~~~~~~~~~~~~~~~

Sometimes you need to test the web installer instead of using the default setup and sample data created by KTD:

1. Access the database server[1]: mysql -uroot -ppassword -hkoha_db_1
2. Drop the koha_kohadev database: drop database koha_kohadev;
3. Create the database: create database koha_kohadev;
4. Add privileges (for a real installation this would be limited): grant all on koha_kohadev.* to koha_kohadev;
5. Restart everything (may not be required, but just to be safe): flush_memcached + restart_all
6. Access the web installer: go to 127.0.0.1:8081
7. Use the database user name and password: get from /etc/koha/sites/kohadev/koha-conf.xml (default: koha_kohadev, password)
8. Continue through the installation process
Comment 11 Katrin Fischer 2022-06-12 12:34:50 UTC
Created attachment 135972 [details] [review]
Bug 25449: Set items.itype mandatory by default

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 12 Katrin Fischer 2022-06-12 12:34:54 UTC
Created attachment 135973 [details] [review]
Bug 25449: (QA follow-up) Set items.itype to mandatory by default (UNIMARC)

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 13 Tomás Cohen Arazi 2022-07-18 13:31:17 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 14 Jonathan Druart 2022-07-20 15:41:06 UTC
Created attachment 137937 [details] [review]
Bug 25449: Fix basic_workflow.t

We need to create an itemtype for the biblio, then the item form will
have 942$c prefilled by default

STRACE: /usr/share/perl5/Try/Tiny.pm:123 in Selenium::Remote::Driver::catch {...}
        /usr/share/perl5/Selenium/Remote/Driver.pm:361 in Try::Tiny::try
        (eval 1726):1 in Selenium::Remote::Driver::__ANON__
        (eval 1728):2 in Selenium::Remote::Driver::__ANON__
        /usr/share/perl5/Selenium/Remote/Driver.pm:953 in Selenium::Remote::Driver::_execute_command
        t/db_dependent/selenium/basic_workflow.t:224 in Selenium::Remote::Driver::get_title

Error while executing command: unexpected alert open: Dismissed user prompt dialog: Form not submitted because of the following problem(s)
------------------------------------------------------------------------------------

- 1 mandatory fields empty (highlighted) at /usr/share/perl5/Selenium/Remote/Driver.pm line 411.
Comment 15 Martin Renvoize 2022-07-21 06:51:13 UTC
Created attachment 137953 [details] [review]
Bug 25449: (QA follow-up) Fix basic_workflow.t

Now that we have made item.itype a required field, we need to ensure we
set it for the corresponding selenium test.. this wasn't entirely easy
as the html select is hidden by select2. I had to find the select2 span,
trigger a click event to open it and then find and click an option.
Comment 16 Martin Renvoize 2022-07-21 06:53:21 UTC
Oooh.. you beat me to it Jonathan.

I think Jonathans fix is more elegant here Tomas, so I'd grab his.. I'll leave mine here temporarily as an example of how to mess with select2 if needed.. it also gets the test passing but it's rather messy and likely fragile.
Comment 17 Martin Renvoize 2022-07-21 07:12:00 UTC
Comment on attachment 137937 [details] [review]
Bug 25449: Fix basic_workflow.t

Review of attachment 137937 [details] [review]:
-----------------------------------------------------------------

::: t/db_dependent/selenium/basic_workflow.t
@@ +206,4 @@
>                  $v = strftime("%Y-%m-%d", localtime);
>                  $effective_input = $driver->find_element('//div[@id="subfield952w"]/input[@class="input_marceditor flatpickr-input"]');
>              }
> +            elsif (

As discussed on irc.. we don't actually need this condition.. the select is hidden by select2 so we already filter it out with the selector.. it won't appear in this loop.
Comment 18 Tomás Cohen Arazi 2022-07-21 12:50:48 UTC
Follow-ups pushed to master. Thanks Jonathan and Martin!
Comment 19 Lucas Gass 2022-08-23 16:06:57 UTC
Enhancement will not be backported to 22.05.x series unless requested