Bug 28012 - Error on saving new numbering pattern
Summary: Error on saving new numbering pattern
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Serials (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Joonas Kylmälä
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 17258
  Show dependency treegraph
 
Reported: 2021-03-22 08:25 UTC by Christian Stelzenmüller
Modified: 2022-11-03 17:46 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This fixes the serials new numbering pattern input form so that the name and numbering formula fields are marked as required. Before this, there was no indication that these fields were required and error trace messages were displayed if these were not completed - saving a new pattern or editing an existing pattern would also silently fail. NOTE: Making the description field optional will be fixed in bug 31297. Until this is done, a value needs to be entered into this field - even though it doesn't indicate that it is required.
Version(s) released in:


Attachments
Bug 28012: Use definedness test instead of bool when checking whether to null fields (2.84 KB, patch)
2022-09-03 11:18 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 28012: Creating numbering pattern: mark mandatory fields required (2.79 KB, patch)
2022-09-03 11:18 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 28012: Creating numbering pattern: mark mandatory fields required (2.73 KB, patch)
2022-09-07 16:32 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
screenshot Modify a pattern (6.36 KB, image/png)
2022-09-08 12:49 UTC, Emmanuel Bétemps
Details
Bug 28012: Use definedness test instead of bool when checking whether to null fields (2.89 KB, patch)
2022-10-16 20:57 UTC, David Nind
Details | Diff | Splinter Review
Bug 28012: Creating numbering pattern: mark mandatory fields required (2.78 KB, patch)
2022-10-16 20:57 UTC, David Nind
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Christian Stelzenmüller 2021-03-22 08:25:21 UTC
I got an error after klicking on the Save button for saving a new numbering pattern.

1. go to the serials module
2. click on manage numbering patterns in the left hand navigation
3. add a pattern
4. Fill in the fields
5. Filling in the Numbering formula field leads to an error after saving. 
Leaving the numbering formula field blank, no error occurs, but the new pattern
won't be saved it seems

First lines of error trace:
C4::Serials::Numberpattern::AddSubscriptionNumberpattern(): DBI Exception: DBD::mysql::st execute failed: Incorrect integer value: '' for column `koha_christian8`.`subscription_numberpatterns`.`displayorder` at row 1 [for Statement "INSERT INTO subscription_numberpatterns(label,description,numberingmethod,displayorder,label1,label2,label3,add1,add2,add3,every1,every2,every3,setto1,setto2,setto3,whenmorethan1,whenmorethan2,whenmorethan3,numbering1,numbering2,numbering3) VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?)" with ParamValues: 0="blablub", 1="blablubdescription", 2="{X}, H. {Y} {Z} ", 3="", 4="Jahr", 5="Jahrgang", 6="Heft", 7="1", 8="1", 9="1", 10="12", 11="6", 12="1", 13="1", 14="1", 15="1", 16="9999", 17="9999", 18="6", 19="", 20="", 21=""] at /kohadevbox/koha/serials/subscription-numberpatterns.pl line 62
 at /usr/share/perl5/DBIx/Class/Exception.pm line 77
Comment 1 Jonathan Druart 2021-04-09 09:13:47 UTC
Globally this form is missing data validation.
We could add a default value for displayorder (1?) at DBMS level but then we get another one:
 Incorrect integer value: '' for column 'add1'
and so on...
Comment 2 Owen Leonard 2021-10-21 17:36:18 UTC
I get an different error, "C4::Serials::Numberpattern::AddSubscriptionNumberpattern(): DBI Exception: DBD::mysql::st execute failed: Field 'description' doesn't have a default value"
Comment 3 Katrin Fischer 2022-01-30 12:24:29 UTC
I think it would be nice if the field could remain empty. Changing the display numbering can be tedious and often just alphabetic or chronological ordering might be preferred. 

How is it done when a numbering pattern is added from the subscription form?
Comment 4 Joonas Kylmälä 2022-09-03 11:18:53 UTC
Created attachment 140196 [details] [review]
Bug 28012: Use definedness test instead of bool when checking whether to null fields

Saving a new numbering pattern didn't work without having to fill all
the fields, even those that are not mandatory.

To test:
 1) Go to /cgi-bin/koha/serials/subscription-numberpatterns.pl
    and try creating a new pattern, notice that only Name and
    Description are mandatory after applying this patch
 2) Make sure editing existing numbering patterns still works
Comment 5 Joonas Kylmälä 2022-09-03 11:18:58 UTC
Created attachment 140197 [details] [review]
Bug 28012: Creating numbering pattern: mark mandatory fields required

If any of these fields were missing, notably the Numbering formula
then saving the new numbering pattern silently failed, it didn't give
any error but it didn't save any new pattern either.

To test:
  1) After applying this patch make sure all the 3 fields must be
  filled in
  /cgi-bin/koha/serials/subscription-numberpatterns.pl?op=new
  page and it cannot be saved without those.
Comment 6 Joonas Kylmälä 2022-09-03 11:23:15 UTC
Added a couple patches that make adding a new numbering pattern usable, I don't understand how it can have been, apparently already since bug 7688 / year 2013, this broken and nobody has fixed this.

Further improvement can be done, like I don't think description should be mandatory but will leave this like this to be easily reviewable patch so we can make this feature usable ASAP.

Ready for sign-off.
Comment 7 Katrin Fischer 2022-09-07 10:33:51 UTC
(In reply to Joonas Kylmälä from comment #6)
> Added a couple patches that make adding a new numbering pattern usable, I
> don't understand how it can have been, apparently already since bug 7688 /
> year 2013, this broken and nobody has fixed this.
> 
> Further improvement can be done, like I don't think description should be
> mandatory but will leave this like this to be easily reviewable patch so we
> can make this feature usable ASAP.
> 
> Ready for sign-off.

If this relates to MySQL 5.7 database settings might mitigate the issue (non-strict?)
Comment 8 Katrin Fischer 2022-09-07 10:54:10 UTC
As we have name - which is not a code in this case - we should not make description mandatory. 

I just confirmed that you can save a numbering pattern in a production 20.11 (package installation) without adding a description. Possibly due to DBMS configuration (non-strict?). 

So there should be a lot of patterns out there without description already, that would then throw an error on editing. 

Same fix as for bug 31297?
Comment 9 Joonas Kylmälä 2022-09-07 16:24:04 UTC
(In reply to Katrin Fischer from comment #8)
> As we have name - which is not a code in this case - we should not make
> description mandatory. 
> 
> I just confirmed that you can save a numbering pattern in a production 20.11
> (package installation) without adding a description. Possibly due to DBMS
> configuration (non-strict?). 
> 
> So there should be a lot of patterns out there without description already,
> that would then throw an error on editing. 
> 
> Same fix as for bug 31297?

I was thinking of just focusing here on the UI side since description is mandatory on the DB level. Is it okay for you if we focus on the whether description is mandatory on bug 3129? If so, and bug 3129 gets merged then this patch might have to be adjusted but until it is then I think there's no problem with this patch, if you agree can you change the status back to need sign-off?
Comment 10 Joonas Kylmälä 2022-09-07 16:25:26 UTC
(In reply to Joonas Kylmälä from comment #9)
> I was thinking of just focusing here on the UI side since description is
> mandatory on the DB level. Is it okay for you if we focus on the whether
> description is mandatory on bug 3129? If so, and bug 3129 gets merged then
> this patch might have to be adjusted but until it is then I think there's no
> problem with this patch, if you agree can you change the status back to need
> sign-off?

I meant to type bug 31297. Typo.
Comment 11 Joonas Kylmälä 2022-09-07 16:28:57 UTC
(In reply to Katrin Fischer from comment #8)
> I just confirmed that you can save a numbering pattern in a production 20.11
> (package installation) without adding a description. Possibly due to DBMS
> configuration (non-strict?). 

I missed this. So maybe remove it directly from here if in non-strict mode you can avoid the DB error. Then after that no need to adjust patch depending on bug 31297. Will attach updated patch soon.
Comment 12 Joonas Kylmälä 2022-09-07 16:32:59 UTC
Created attachment 140322 [details] [review]
Bug 28012: Creating numbering pattern: mark mandatory fields required

If any of these fields were missing, notably the Numbering formula
then saving the new numbering pattern silently failed, it didn't give
any error but it didn't save any new pattern either.

Note that there is a plan to make description field optional, so even
if it currently is mandatory on the DB level we leave it optional
here (and due to people using production DB's in non-strict mode which
allows this to be optional).

To test:
  1) After applying this patch make sure all the 3 fields must be
  filled in
  /cgi-bin/koha/serials/subscription-numberpatterns.pl?op=new
  page and it cannot be saved without those.
Comment 13 Emmanuel Bétemps 2022-09-08 12:48:36 UTC
(In reply to Joonas Kylmälä from comment #4)

> To test:
>  1) Go to /cgi-bin/koha/serials/subscription-numberpatterns.pl
>     and try creating a new pattern, notice that only Name and
>     Description are mandatory after applying this patch


Hi,

after applying the patch, when I want to create a new pattern :
- Name is labeled as mandatory
- Description is not labeled as mandatory
- Numbering formula is labeled as mandatory
(see attachment 'Screenshot - Modify pattern.png')

I still can edit existing numbering patterns but if a delete the description, an error occurs when I (try to) save :

C4::Serials::Numberpattern::ModSubscriptionNumberpattern(): DBI Exception: DBD::mysql::st execute failed: Column 'description' cannot be null at /kohadevbox/koha/serials/subscription-numberpatterns.pl line 95
 at /usr/share/perl5/DBIx/Class/Exception.pm line 77



I can't create a new one : 

C4::Serials::Numberpattern::AddSubscriptionNumberpattern(): DBI Exception: DBD::mysql::st execute failed: Incorrect integer value: '' for column `koha_test`.`subscription_numberpatterns`.`add2` at row 1 at /kohadevbox/koha/serials/subscription-numberpatterns.pl line 70
 at /usr/share/perl5/DBIx/Class/Exception.pm line 77
Comment 14 Emmanuel Bétemps 2022-09-08 12:49:21 UTC
Created attachment 140337 [details]
screenshot Modify a pattern
Comment 15 Joonas Kylmälä 2022-09-08 16:01:39 UTC
(In reply to Emmanuel Bétemps from comment #13)
> I still can edit existing numbering patterns but if a delete the
> description, an error occurs when I (try to) save :

It's expected, as explained in the second patch's description. This bug is left to be fixed in bug 31297.

You also quoted in your message the test plan for the first patch btw, in the first patch's test plan I meant with mandatory that it is mandatory to fill those to prevent the error. The second plan then concerns the GUI changes, and it has its own test plan.

> I can't create a new one : 
> 
> C4::Serials::Numberpattern::AddSubscriptionNumberpattern(): DBI Exception:
> DBD::mysql::st execute failed: Incorrect integer value: '' for column
> `koha_test`.`subscription_numberpatterns`.`add2` at row 1 at
> /kohadevbox/koha/serials/subscription-numberpatterns.pl line 70
>  at /usr/share/perl5/DBIx/Class/Exception.pm line 77

Did you remember to run restart_all after applying patches to kohadevbox? This is the exact error the first patch attached solves.
Comment 16 Katrin Fischer 2022-09-08 17:38:17 UTC
Just a note: Patch doesn't apply means you could't test because the patches couldn't be applied to the installation. If you find an error in testing, the status is "Failed QA" (not limited to QA team)
Comment 17 David Nind 2022-10-16 20:57:52 UTC
Created attachment 141947 [details] [review]
Bug 28012: Use definedness test instead of bool when checking whether to null fields

Saving a new numbering pattern didn't work without having to fill all
the fields, even those that are not mandatory.

To test:
 1) Go to /cgi-bin/koha/serials/subscription-numberpatterns.pl
    and try creating a new pattern, notice that only Name and
    Description are mandatory after applying this patch
 2) Make sure editing existing numbering patterns still works

Signed-off-by: David Nind <david@davidnind.com>
Comment 18 David Nind 2022-10-16 20:57:58 UTC
Created attachment 141948 [details] [review]
Bug 28012: Creating numbering pattern: mark mandatory fields required

If any of these fields were missing, notably the Numbering formula
then saving the new numbering pattern silently failed, it didn't give
any error but it didn't save any new pattern either.

Note that there is a plan to make description field optional, so even
if it currently is mandatory on the DB level we leave it optional
here (and due to people using production DB's in non-strict mode which
allows this to be optional).

To test:
  1) After applying this patch make sure all the 3 fields must be
  filled in
  /cgi-bin/koha/serials/subscription-numberpatterns.pl?op=new
  page and it cannot be saved without those.

Signed-off-by: David Nind <david@davidnind.com>
Comment 19 David Nind 2022-10-16 21:10:18 UTC
Testing notes (using koha-testing-docker) - I hope I got this right!

Replicate the issue:
1. Go to Serials > Manage numbering patterns.
2. Add a new numbering pattern, but leave the description field blank (copy an existing numbering pattern such as Number) - note that no fields are marked as required:
   ==> C4::Serials::Numberpattern::AddSubscriptionNumberpattern(): DBI Exception: DBD::mysql::st execute failed: Incorrect integer value: '' for column `koha_kohadev`.`subscription_numberpatterns`.`add2` at row 1 at /kohadevbox/koha/serials/subscription-numberpatterns.pl line 70
 at /usr/share/perl5/DBIx/Class/Exception.pm line 77
3. Repeat step 2 but leave the display order field blank:
   ==> C4::Serials::Numberpattern::AddSubscriptionNumberpattern(): DBI Exception: DBD::mysql::st execute failed: Incorrect integer value: '' for column `koha_kohadev`.`subscription_numberpatterns`.`displayorder` at row 1 at /kohadevbox/koha/serials/subscription-numberpatterns.pl line 70
 at /usr/share/perl5/DBIx/Class/Exception.pm line 77
4. Repeat step 2, leaving name or numbering formula fields blank:
   ==> adding the new numbering pattern silently fails - you are return to the list of numbering patterns and the your new numbering pattern is not saved.
5. Fill in all the fields (name, description, numbering formula, and display order) and save:
   ==> C4::Serials::Numberpattern::AddSubscriptionNumberpattern(): DBI Exception: DBD::mysql::st execute failed: Incorrect integer value: '' for column `koha_kohadev`.`subscription_numberpatterns`.`add2` at row 1 at /kohadevbox/koha/serials/subscription-numberpatterns.pl line 70
 at /usr/share/perl5/DBIx/Class/Exception.pm line 77
6. Edit an existing pattern with the various combinations from steps 2-5 (for example, the existing number pattern) - no fields are marked as required, you get these results:
   . Name: C4::Serials::Numberpattern::ModSubscriptionNumberpattern(): DBI Exception: DBD::mysql::st execute failed: Column 'label' cannot be null at /kohadevbox/koha/serials/subscription-numberpatterns.pl line 95
 at /usr/share/perl5/DBIx/Class/Exception.pm line 77
  . Numbering formula: C4::Serials::Numberpattern::ModSubscriptionNumberpattern(): DBI Exception: DBD::mysql::st execute failed: Column 'numberingmethod' cannot be null at /kohadevbox/koha/serials/subscription-numberpatterns.pl line 95
 at /usr/share/perl5/DBIx/Class/Exception.pm line 77
  . Description: C4::Serials::Numberpattern::ModSubscriptionNumberpattern(): DBI Exception: DBD::mysql::st execute failed: Column 'description' cannot be null at /kohadevbox/koha/serials/subscription-numberpatterns.pl line 95  at /usr/share/perl5/DBIx/Class/Exception.pm line 77
  . Display order: can make blank or renumber

This fixes the new numbering pattern input form so that the name and numbering formula fields are marked as required. Before this, there was no indication that these fields were required and error trace messages were displayed if these were not completed, and saving a new pattern or editing an existing pattern would silently fail.

Notes:
- The description field is not marked as required, but if you leave it blank you will get this error:
C4::Serials::Numberpattern::AddSubscriptionNumberpattern(): DBI Exception: DBD::mysql::st execute failed: Column 'description' cannot be null at /kohadevbox/koha/serials/subscription-numberpatterns.pl line 73
 at /usr/share/perl5/DBIx/Class/Exception.pm line 77
- Making the description field optional will be fixed in bug 31297 (see comment#15).

Test plan:
1. Apply the patches.
2. Go to Serials > Manage numbering patterns.
3. Add a new numbering pattern:
   . Note that the name and numbering formula fields are now marked as required
   . If you try to save without completing any of the required fields, you will be prompted to complete these
   . For testing this bug, put a value in the description field (see notes above).
4. Edit an existing pattern:*
   . Note that the name and numbering formula fields are now marked as required
   . If you try to save without completing any of the required fields, you will be prompted to complete these
   . For testing this bug, put a value in the description field (see notes above).

* No test is included for an existing numbering pattern that doesn't have a description. With the sample data included in KTD a database table update would be required to allow NULL values for the description field.
Comment 20 Katrin Fischer 2022-11-02 19:06:27 UTC
As noted in comments before this one, I don't think that making the description mandatory is the right solution here. 

These features work currently for all libraries as we don't set strict on production systems. So making them required, will be a regression. I confirmed that without the strict, it's possible to save numbering patterns in master with only name and numbering pattern set.

We should instead fix it on database level, perhaps by making the field nullable. 

Note: bug 31297 was supposed to take care of this, but I believe Joonas is no longer around and it's currently in FQA. So we cannot expect a fix there right now.
Comment 21 Joonas Kylmälä 2022-11-03 17:46:24 UTC
(In reply to Katrin Fischer from comment #20)
> As noted in comments before this one, I don't think that making the
> description mandatory is the right solution here. 

I addressed this after the last time you commented on this, please see the patch again, it is not mandatory. Setting the status back to Signed off, let me know if there is something I'm not understanding.