Bug 23888 - Incorrect vendor id in subscription creation causes internal server error
Summary: Incorrect vendor id in subscription creation causes internal server error
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Serials (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-10-24 10:18 UTC by Joonas Kylmälä
Modified: 2020-04-03 23:07 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00


Attachments
Bug 23888: Do not allow invalid vendor id on creating a subscription (3.39 KB, patch)
2019-11-11 11:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23888: Fix JS error if no vendor defined (1.30 KB, patch)
2019-11-12 11:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23888: Do not allow invalid vendor id on creating a subscription (3.37 KB, patch)
2020-02-14 15:36 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 23888: (follow-up) Wrap English string in translation function (1.84 KB, patch)
2020-02-14 15:36 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 23888: (follow-up) Wrap English string in translation function (1.84 KB, patch)
2020-03-26 13:25 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 23888: Do not allow invalid vendor id on creating a subscription (3.46 KB, patch)
2020-03-27 08:48 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23888: (follow-up) Wrap English string in translation function (2.06 KB, patch)
2020-03-27 08:48 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Joonas Kylmälä 2019-10-24 10:18:51 UTC
When adding a new subscription on the page /cgi-bin/koha/serials/subscription-add.pl and typing in the vendor field a non-existing vendor id "9999999999" and other fields correctly we get the following internal server error when clicking on the second page ”Save subscription”:

DBIx::Class::Storage::DBI::_dbh_execute(): Out of range value for column
'aqbooksellerid' at row 1 at /kohadevbox/koha/Koha/Object.pm line 156 at
/usr/share/perl5/DBIx/Class/Exception.pm line 77

When such incorrect vendor id is typed it should not allow us to go to the next page by clicking ”Next” but instead complain the vendor info is not correct.
Comment 1 Katrin Fischer 2019-11-03 23:02:03 UTC
I can't replicate the error on master - now it just saves and doesn't show a vendor name in the details. We should validate if the vendor id is valid before switching to the next page.
Comment 2 Joonas Kylmälä 2019-11-04 07:31:22 UTC
(In reply to Katrin Fischer from comment #1)
> I can't replicate the error on master - now it just saves and doesn't show a
> vendor name in the details. We should validate if the vendor id is valid
> before switching to the next page.

That happens to me also when I type some other invalid vendor id. If I type exactly "9999999999" then the error happens. Did you type that exactly?
Comment 3 Jonathan Druart 2019-11-11 11:18:08 UTC
Created attachment 95253 [details] [review]
Bug 23888: Do not allow invalid vendor id on creating a subscription

It will avoid crash and invalid data when creating/updating a
subscription.

This could have been done with a AJAX query but seems more convenient
this way.

Test plan:
- Create or update a subscription
- In the "Vendor" input try an empty string, a valid vendor's id, and
invalid one.
=> With an empty string you get the existing alert message
=> With a valid id you do not get any messages
=> With an invalid id you are not allowed to go to page 2
Comment 4 Joonas Kylmälä 2019-11-11 14:17:26 UTC
If I don't have any vendors in Koha then this error comes up to the browser's JavaScript console:

> ReferenceError: BOOKSELLER_IDS is not defined
Comment 5 Jonathan Druart 2019-11-12 11:30:13 UTC
Created attachment 95297 [details] [review]
Bug 23888: Fix JS error if no vendor defined

> ReferenceError: BOOKSELLER_IDS is not defined
Comment 6 Joonas Kylmälä 2019-11-12 14:46:05 UTC
Jonathan, please squash that JS error follow-up to the original patch as both of these patches are yours.
Comment 7 Jonathan Druart 2019-11-12 14:58:48 UTC
Not a problem for testing.
Comment 8 Joonas Kylmälä 2019-11-12 15:01:58 UTC
The meeting notes in https://wiki.koha-community.org/wiki/Development_IRC_meeting_27_September_2017 state that "follow-up patches must be squashed if not signed-off yet, and the history is not needed". It really makes reading the history a lot easier if we don't have unnecessary commits.
Comment 9 Jonathan Druart 2019-11-12 15:26:26 UTC
(In reply to Joonas Kylmälä from comment #8)
> The meeting notes in
> https://wiki.koha-community.org/wiki/
> Development_IRC_meeting_27_September_2017 state that "follow-up patches must
> be squashed if not signed-off yet, and the history is not needed". It really
> makes reading the history a lot easier if we don't have unnecessary commits.

The history is needed for you, to know which changes I made. QA and RM can squash patches if needed. Feel free to squash when signing off this one.
Comment 10 Owen Leonard 2020-02-14 15:36:43 UTC
Created attachment 98963 [details] [review]
Bug 23888: Do not allow invalid vendor id on creating a subscription

It will avoid crash and invalid data when creating/updating a
subscription.

This could have been done with a AJAX query but seems more convenient
this way.

Test plan:
- Create or update a subscription
- In the "Vendor" input try an empty string, a valid vendor's id, and
invalid one.
=> With an empty string you get the existing alert message
=> With a valid id you do not get any messages
=> With an invalid id you are not allowed to go to page 2

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 11 Owen Leonard 2020-02-14 15:36:46 UTC
Created attachment 98964 [details] [review]
Bug 23888: (follow-up) Wrap English string in translation function

This patch makes alert added in subscription-add.js translatable using
the new __() function.

To test, apply the patch and confirm that the alert still works
correctly when submitting the subscription edit form with an invalid
vendor id.

Test that the string can be translated (using fr-Fr for example):

1. cd misc/translator && ./translate update fr-FR
2. Translate strings in misc/tranlator/po/fr-FR-messages-js.po
3. cd misc/translator && ./translate install fr-FR

Switch to the fr-FR translation and confirm the the alert shows your
translated string.
Comment 12 Owen Leonard 2020-02-14 15:40:30 UTC
Notes:

 - I squashed the original 2 patches according to guidelines
 - The QA tool will give a false positive on untranslatable strings because it hasn't been updated along with the new JS-translation features.
Comment 13 Fridolin SOMERS 2020-03-26 12:51:09 UTC
There are 2 patches :
Bug 23888: Do not allow invalid vendor id on creating a subscription
Comment 14 Fridolin SOMERS 2020-03-26 13:25:13 UTC
Created attachment 101826 [details] [review]
Bug 23888: (follow-up) Wrap English string in translation function

This patch makes alert added in subscription-add.js translatable using
the new __() function.

To test, apply the patch and confirm that the alert still works
correctly when submitting the subscription edit form with an invalid
vendor id.

Test that the string can be translated (using fr-Fr for example):

1. cd misc/translator && ./translate update fr-FR
2. Translate strings in misc/tranlator/po/fr-FR-messages-js.po
3. cd misc/translator && ./translate install fr-FR

Switch to the fr-FR translation and confirm the the alert shows your
translated string.
Comment 15 Fridolin SOMERS 2020-03-26 13:27:45 UTC
(In reply to Fridolin SOMERS from comment #13)
> There are 2 patches :
> Bug 23888: Do not allow invalid vendor id on creating a subscription

Ok fixed thanks ;)
Comment 16 Fridolin SOMERS 2020-03-26 13:28:15 UTC
New translation sysptem in JS files is great !
Comment 17 Marcel de Rooy 2020-03-27 08:30:33 UTC
QAing
Comment 18 Marcel de Rooy 2020-03-27 08:38:28 UTC
(In reply to Joonas Kylmälä from comment #8)
> The meeting notes in
> https://wiki.koha-community.org/wiki/
> Development_IRC_meeting_27_September_2017 state that "follow-up patches must
> be squashed if not signed-off yet, and the history is not needed". It really
> makes reading the history a lot easier if we don't have unnecessary commits.

Joonas: This is not day to day experience. When I want to read a larger patch set, I can squash it myself. But quite exceptional, I will submit the squashed patch in QA. Theory and practice?
Comment 19 Marcel de Rooy 2020-03-27 08:47:57 UTC
Fridolin, you forgot signoff lines. No big deal
Comment 20 Marcel de Rooy 2020-03-27 08:48:27 UTC
Created attachment 101967 [details] [review]
Bug 23888: Do not allow invalid vendor id on creating a subscription

It will avoid crash and invalid data when creating/updating a
subscription.

This could have been done with a AJAX query but seems more convenient
this way.

Test plan:
- Create or update a subscription
- In the "Vendor" input try an empty string, a valid vendor's id, and
invalid one.
=> With an empty string you get the existing alert message
=> With a valid id you do not get any messages
=> With an invalid id you are not allowed to go to page 2

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 21 Marcel de Rooy 2020-03-27 08:48:31 UTC
Created attachment 101968 [details] [review]
Bug 23888: (follow-up) Wrap English string in translation function

This patch makes alert added in subscription-add.js translatable using
the new __() function.

To test, apply the patch and confirm that the alert still works
correctly when submitting the subscription edit form with an invalid
vendor id.

Test that the string can be translated (using fr-Fr for example):

1. cd misc/translator && ./translate update fr-FR
2. Translate strings in misc/tranlator/po/fr-FR-messages-js.po
3. cd misc/translator && ./translate install fr-FR

Switch to the fr-FR translation and confirm the the alert shows your
translated string.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Still seeing a false negative in qa tools:
    FAIL Untranslatable string found (The vendor does not exist) - See coding guideline JS5
Comment 22 Marcel de Rooy 2020-03-27 08:50:44 UTC
>    This could have been done with a AJAX query but seems more convenient
>    this way.

Or just lazy programming? :)
Comment 23 Jonathan Druart 2020-03-27 09:18:27 UTC
(In reply to Marcel de Rooy from comment #22)
> >    This could have been done with a AJAX query but seems more convenient
> >    this way.
> 
> Or just lazy programming? :)

Not only, I think it's much more efficient this way.
I did it in AJAX for the biblio, as we do not want to embed an array of all biblionumbers.
Comment 24 Martin Renvoize 2020-03-27 12:23:58 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 25 Joy Nelson 2020-04-03 23:07:31 UTC
QA failure when applying to 19.11.x
not backported


 FAIL   koha-tmpl/intranet-tmpl/prog/js/subscription-add.js
   OK     git manipulation
   FAIL   untranslatable_strings
                Untranslatable string found (The vendor does not exist) - See coding guideline JS5