Problem/Motivation

Currently you configure a fetcher and have no idea if it works.

All mail MTA have a "test connection" button that confirms credentials are fine.

Followup from #2379889: Fetch email by IMAP

Proposed resolution

Add a test connection button.

Remaining tasks

User interface changes

API changes

CommentFileSizeAuthor
#9 imap_test_connection-2405045-9-interdiff.txt618 bytesmbovan
#9 imap_test_connection-2405045-9.patch6.68 KBmbovan
#7 imap_test_connection-2405045-7-interdiff.txt6.73 KBmbovan
#7 imap_test_connection-2405045-7.patch6.63 KBmbovan
#7 account_imap.png16.57 KBmbovan
#5 imap_fieldset.png15.91 KBmbovan
#5 imap_test_connection-2405045-5-interdiff.txt3.46 KBmbovan
#5 imap_test_connection-2405045-5.patch5.92 KBmbovan
#3 imap_connect_button.png22.49 KBmbovan
#3 imap_test_connection-2405045-3.patch3.39 KBmbovan

Comments

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Component: User interface » Deliverer
miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

Hint: There's an imap_ping() method to test a connection.

mbovan’s picture

mbovan commented
Status: Active » Needs review
StatusFileSize
new imap_test_connection-2405045-3.patch3.39 KB
new imap_connect_button.png22.49 KB

Added a "Connect" button.

imap_ping() requires opening an IMAP connection, so it seems doImap() call is enough. It fails in case connection cannot be open.

Also, about the tests, I'm only asserting there is a "Connect" button. PHP IMAP extension is needed to test it properly which doesn't seem to be available in tests...

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Needs review » Needs work

We should move all connection credential specific elements into a fieldset and put the Connect button there as last element.

For instance, the Batch size is NOT related to the connection credentials. :-)

Yeah, the IMAP php module isn't that common on server environments... Likely it's missing on testbot.
Testing this would need abstracting it / with mocking capability. Seems to be way too much overhead.

mbovan’s picture

mbovan commented
Status: Needs work » Needs review
StatusFileSize
new imap_test_connection-2405045-5.patch5.92 KB
new imap_test_connection-2405045-5-interdiff.txt3.46 KB
new imap_fieldset.png15.91 KB

Makes sense. :)

Added a fieldset:

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Needs review » Needs work

If i click on "Connect" with wrong credentials, i see no error message. We need one.

The fieldset "IMAP" is repetitive. Let's name it "Account".

I think the button should be labeled "Test connection", because it does not save...

mbovan’s picture

mbovan commented
Status: Needs work » Needs review
StatusFileSize
new account_imap.png16.57 KB
new imap_test_connection-2405045-7.patch6.63 KB
new imap_test_connection-2405045-7-interdiff.txt6.73 KB

Implemented suggestions above.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented 
+++ b/src/Plugin/inmail/Deliverer/FetcherBase.php
@@ -111,4 +111,34 @@ abstract class FetcherBase extends DelivererBase implements FetcherInterface {
+   */
+  public function submitTestConnection(array $form, FormStateInterface $form_state) {
+    // Implement it in the subclass.
+  }

maybe throw an exception here, so implementations see what they need to do if they forget?

mbovan’s picture

mbovan commented
StatusFileSize
new imap_test_connection-2405045-9.patch6.68 KB
new imap_test_connection-2405045-9-interdiff.txt618 bytes

Addressed #8.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Needs review » Fixed

Committed! Thx for adding. :-)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.