https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22343 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |ASSIGNED --- Comment #104 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (In reply to Jonathan Druart from comment #101)
1. If you edit a server without password, there is "****" anyway I tried to fix it, and only display '****' if the password is defined. But then found another bug, you cannot remove the password field once it has been filled already.
I didn't think about deleting the password, good catch.
2. Escaping/unescaping issues. Fixed in a follow-up patch.
Thanks
3. Deletion - Why are not you using the DELETE route? Here you are cheating, I don't think we should GET with "op=delete", the parameters stay in the url
That's because I based it on branches.pl and didn't want to make this bug dependent on an open discussion on how to use the API. I can anyway make the modal call the DELETE route and refresh the datatable.
4. flagsrequired => { parameters => 1 }, Are you sure it's what we want? Not a dedicated subperm?
That's actually a good idea, I will probably add it here. Kyle suggested something along the same lines. parameters => smtp_servers?
5. "SMTP servers Define which SMTP servers to use." is displayed on the admin home page regardless the logged in user has enough permission (click then get "Error: You do not have permission to access this page.")
Oops.
6. - $params->{from} ||= C4::Context->preference('KohaAdminEmailAddress'); + my $from = $params->{from} // C4::Context->preference('KohaAdminEmailAddress');
Any good reasons? I'd stick with || to catch ""
I'll fix it. My opinion is we should (on a separate bug) use Email::Valid against all passed email addresses. But I'd do it on a separate bug because we need to revisit all controller scripts to verify the exception is caught correctly (you guessed, it is not, and there's no eval/try generally).
7. I have tried to cheat and insert twice a default server, on the form edit the dropdown and select value "" (create it) I don't get any errors and see in the log "Cannot add or update a child row: a foreign key constraint fails" for the branch fk I then noticed you used "*" In admin/smtp_servers.pl: 77 catch { 78 if ( blessed $_ and $_->isa('Koha::Exceptions::Object::DuplicateID') ) {
else? We are hiding a potential error/exception. We must log it anyway and make the interface display "something wrong happened"
I'll see what you're talking about and provide a fix.
8. Should not we display the "default config" if no default server is defined?
Would you display above the datatable? Or just as another row?
9. And, finally, is this working? I don't think so.
kohadev-koha@kohadevbox:/kohadevbox/koha$ perl misc/cronjobs/process_message_queue.pl Use of uninitialized value for string at /usr/share/perl5/Email/MIME/Encode.pm line 66. Undefined subroutine &Email::Sender::Simple::sendmail called at /kohadevbox/koha/C4/Letters.pm line 1386. at /kohadevbox/koha/C4/Letters.pm line 1405.
% pmvers Email::Sender::Simple 1.300034
If I replace Email::Sender::Simple::sendmail with sendmail (following the POD of the module), I get: sending email message to patron: 51 at /kohadevbox/koha/C4/Letters.pm line 1007. Use of uninitialized value for string at /usr/share/perl5/Email/MIME/Encode.pm line 66. Don't know how to handle Koha::Email at /usr/share/perl5/Email/Sender/Role/CommonSending.pm line 78. at /kohadevbox/koha/C4/Letters.pm line 1405.
I'll check this. This is probably due to an unadvertised change I made.
Proving tests is not a sufficient test plan.
-- You are receiving this mail because: You are watching all bug changes.