Univention Bugzilla – Bug 33902
Contact / NotificationEmail must not be an invalid email address
Last modified: 2023-03-25 06:51:43 CET
NotificationEmail must not be an invalid email address. Example: NotificationEmail= means notification_email = '' which is not a valid email address. (To achieve the fallback to Contact the field must not be set at all which leads to notification_email = None) Contact likewise has to contain a valid email address.
r55580: * 20_appcenter/11_check_apps_ini_field_values: check that options in ini files have values assigned (Bug #33902).
Looks good. 'UseShop': ('optional', 'is_bool',), is in there twice. 'LicenseFile' is not really a field. I do not know why it is in some ini files. Speaking of that: Could you please validate that there is nothing else in the ini file? I would somehow prefer this test failing if an unknown (useless) variable is in the ini file. It is not so much the cluttering of the file, but possible typos. Something like VisbleInAppCatalogue or something. Downside is that we would need to update this test each time we add more meaningful variables in the ini file, but I think this is okay. Please run the test once and report if too many ini files would fail.
Sorry, I looked in the wrong test (12* instead of 11*).
"contact@" does not fail. This is not very critical as it fails in 12_check_apps_ini_all_fields... which makes me wonder why 11_check_apps_ini_field_values even exists. On the other side, 12_check_apps_ini_all_fields lets an empty value pass. I would remove 11* and change the checks in 12* a little bit. However. Strictly speaking 11_check_apps_ini_field_values is not correct.
(In reply to Dirk Wiesenthal from comment #2) > Speaking of that: Could you please validate that there is nothing else in > the ini file? I would somehow prefer this test failing if an unknown > (useless) variable is in the ini file. I'm not sure if this is a good idea. This will generate a lot of false positives while we develop new features in the App Center platform and we have to adjust always the test case. Dirk, if it is important to you, please file a new bug with a detailed description which values should be checked.
(In reply to Dirk Wiesenthal from comment #4) > "contact@" does not fail. > > This is not very critical as it fails in 12_check_apps_ini_all_fields... > which makes me wonder why 11_check_apps_ini_field_values even exists. > > On the other side, 12_check_apps_ini_all_fields lets an empty value pass. I > would remove 11* and change the checks in 12* a little bit. > > However. Strictly speaking 11_check_apps_ini_field_values is not correct. Two tests appeared since a week after current Bug #33902 (test 11_*) was resolved, another Bug #36730 (test 12_*) was assigned. If it was the other way around -> I would have only added 12_check_apps_ini_all_fields, as it basically covers all the cases.
(In reply to Dirk Wiesenthal from comment #4) > "contact@" does not fail. > On the other side, 12_check_apps_ini_all_fields lets an empty value pass. I > would remove 11* and change the checks in 12* a little bit. I cannot reproduce that. The test 12_check_apps_ini_all_fields performs the checks only for the Apps that are installed, as Bug #36730 (Comment 2) requires. While test 11_check_apps_ini_field_values performs simple check if .ini sections are there (with @ inside) for all .ini files found. I've checked number of cases with test 12_*: 1. Completely missing 'Contact' field (see http://hutten.knut.univention.de/pastebin/m1eea53e1) 2. Field Contact= with no value assigned (see http://hutten.knut.univention.de/pastebin/m689d54f7) 3. Incorrect e-mail address value assigned, like 'Contact=sales@' (see http://hutten.knut.univention.de/pastebin/m4352b21a) In all cases, test '12_check_apps_ini_all_fields' failed (see pastebin). There is an regex check for value and a check that value/field exists. Please let me know via Bug #36730 if you can reproduce test pass with incorrect .ini settings. And if we agree, I can remove the test 11_check_apps_ini_field_values completely.
Code Review: Works. Maybe it would be better to organize these tests in one file, but the effort is not worth it. Better do this along with something more important than the NotificationEmail check