[Koha-bugs] [Bug 22343] Add SMTP configuration options to Administration

[bugzilla-daemon at bugs.koha-community.org]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22343

Tomás Cohen Arazi <tomascohen at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|Failed QA                   |ASSIGNED

--- Comment #104 from Tomás Cohen Arazi <tomascohen at 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 at 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.