https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22343 --- Comment #180 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Hi! (In reply to Martin Renvoize from comment #179)
This code looks great and I'm happy to SO as is (with the couple of very minor followups I've added).
Thanks for the extensive review!
I have a few points/questions/suggestions
* You introduce a 'debug' mode you can set at the smtp server level.. I like this, but at present I can't see any references to it anywhere.. it would be nice to see it used to enable/disable additional logging in at least one place with this patchset to give it a purpose and example of use.
The debug flag is actually passed to Email::Sender::Transport::SMTP, and enables verbose logging to STDERR, so I expect logs to show in warnings. I tried it with process_message_queue.pl and it will print the whole SMTP authentication handshake and transaction steps including return codes. It is really helpful for debugging authentication issues. I'd say we should add a 'hint' on the edit page.
* With the above debug mode, I'd like to see that state somehow highlighted in the smtp-servers page table if it's enabled (so it's obvious to an admin when it's turned on) (bold/italic row perhaps, similar to the 'default' hinting in cash registers)
Will try to add a follow-up for this.
* Final note about the debug mode.. it would be awesome as a later followup to expose a 'Send test mail' action into the smpt servers table to allow the immediate send of a test mail from that server with defaults set for from/replyo etc.
I love the idea. Would add it to the edit page as it would be handy to have there for tweaking things in-place if something fails. Definitely a separate bug report.
* It would be nice to see a list of attached branches as one fo the columns in the smtp servers table. * Should deletes be blocked/warned about if the smtp server is being utilised by any libraries? * The 'delete' was slow for me, which meant I could easily double submit the modal form (we should prevent the double submit)
* It would be nice to add the smtp server name as an optional column to display in the libraries table.
I'll file a new report for this as it requires some (incremental) tweaking to the libraries route.
* Another enhancement for the future would be to support library groups
Good idea!
Generally, I really like this and it's a huge improvement.
Thanks! -- You are receiving this mail because: You are watching all bug changes.