|
Description
Joonas Kylmälä
2019-10-24 10:18:51 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. (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? 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 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
Created attachment 95297 [details] [review] Bug 23888: Fix JS error if no vendor defined > ReferenceError: BOOKSELLER_IDS is not defined Jonathan, please squash that JS error follow-up to the original patch as both of these patches are yours. Not a problem for testing. 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. (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. 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> 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. 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. There are 2 patches : Bug 23888: Do not allow invalid vendor id on creating a subscription 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. (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 ;) New translation sysptem in JS files is great ! QAing (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? Fridolin, you forgot signoff lines. No big deal 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> 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 > This could have been done with a AJAX query but seems more convenient
> this way.
Or just lazy programming? :)
(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. Nice work everyone! Pushed to master for 20.05 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
|