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.
*** Bug 24331 has been marked as a duplicate of this bug. ***
Created attachment 105351 [details] [review] Bug 25449: Set items.itype mandatory by default
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).
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
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.
Should this ticket be "in discussion" or "assigned"(i.e. needs work) If not, what would be a test plan for the current patch?
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.
Created attachment 135571 [details] [review] Bug 25449: Set items.itype mandatory by default
Created attachment 135703 [details] [review] Bug 25449: Set items.itype mandatory by default Signed-off-by: David Nind <david@davidnind.com>
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
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>
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>
Pushed to master for 22.11. Nice work everyone, thanks!
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.
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.
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 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.
Follow-ups pushed to master. Thanks Jonathan and Martin!
Enhancement will not be backported to 22.05.x series unless requested
Manual already said it was required.