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...

I get an different error, "C4::Serials::Numberpattern::AddSubscriptionNumberpattern(): DBI Exception: DBD::mysql::st execute failed: Field 'description' doesn't have a default value"

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?

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

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.

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.

(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?)

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?

(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?

(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.

(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.

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.

(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

Created attachment 140337 [details]
screenshot Modify a pattern

(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.

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)

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>

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>

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.

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.

(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.

I cannot leave description empty: 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

"Display order" should be set to only allow integer inputs.

(In reply to Kyle M Hall from comment #22)
> I cannot leave description empty:
> 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

See the explanation in the patch "Creating numbering pattern: mark mandatory fields required":

[...]
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). 
[...]

Also, I believe this was discussed with Katrin in this bug report as well.

> 
> "Display order" should be set to only allow integer inputs.

It will be a separate patch I believe, would it be possible for someone to add a new patch here or submit it in a separate report? I don't have koha development environment set up anymore unfortunately.

*** Bug 33678 has been marked as a duplicate of this bug. ***

Created attachment 160248 [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>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 160249 [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>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 160250 [details] [review]
Bug 28012: (QA follow-up) Add numeric imput mode to display order field

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

I added a follow-up to use our standard validation with our own error messages, see https://wiki.koha-community.org/wiki/Coding_Guidelines#JS10:_Form_Validation.

You can see the difference in how the error messages display.

But we still have a big issue here on saving:

C4::Serials::Numberpattern::AddSubscriptionNumberpattern(): DBI Exception: DBD::mysql::st execute failed: Incorrect integer value: '' for column `koha_kohadev`.`subscription_numberpatterns`.`add1` at row 1 at /kohadevbox/koha/serials/subscription-numberpatterns.pl line 70
 at /usr/share/perl5/DBIx/Class/Exception.pm line 77

This appears for all 3 columns and all numeric values in the prediction pattern. NONE of these should be required or only be required if you have at least one input field in the column (more complicated, let's go with not exploding for now. Please follow-up here or on a separate bug.

Additionally we explode when display order is left empty. This should be possible as well (compare to adding from the subscription form, where the field is not included).

Forget the last part of the previous comment, only keep:

I added a follow-up to use our standard validation with our own error messages, see https://wiki.koha-community.org/wiki/Coding_Guidelines#JS10:_Form_Validation.

You can see the difference in how the error messages display.

A proper restart_all fixes most issues...

Pushed for 24.05!

Well done everyone, thank you!

Pushed to 23.11.x for 23.11.02

Backported to 23.05.x for upcoming 23.05.08