Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mailman support - Update PR after changes in Master #148

Merged
merged 11 commits into from
May 28, 2016

Conversation

janrenz
Copy link
Contributor

@janrenz janrenz commented May 16, 2016

Use latest master

Had to create a new PR cause i has some hassles with Git

@janrenz janrenz changed the title Update PR after changes in Master Mailman support - Update PR after changes in Master May 16, 2016
@scott
Copy link
Member

scott commented May 17, 2016

Hi Jan, thanks for refactoring. One request for the UI:

screen shot 2016-05-17 at 9 50 53 am

<%= f.text_field 'email.imap_server', value: AppSettings['email.imap_server'], label: "IMAP Host" %>
<%= f.text_field 'email.imap_username', value: AppSettings['email.imap_username'], label: "IMAP Login" %>
<%= f.password_field 'email.imap_password', value: AppSettings['email.imap_password'], label: "IMAP Password" %>
</div>
<%= f.form_group 'email.send_email', label: { text: "Enable Outbound (SMTP) Email" }, class: 'send-email-toggle' do %>
Copy link
Member

@scott scott May 18, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we also add a toggle and variable for SSL? I am trying to get it working with gmail and had to make that change since they only allow SSL. I am using IMAP...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. So toggle would set the SSL Flag. What would be the variable? And should SSL be on by default?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, i added a security setting that is either SSL or StartTLS

@scott
Copy link
Member

scott commented May 18, 2016

I must be doing something wrong, I am still having troubles getting this working with gmail. When I run the rake task, I see the message being found, but it never shows up in Helpy. Also there is an error line at the top:

bundle exec rake helpy:mailman
imap config found
#<NameError: undefined local variable or method `message' for #<Mailman::Application:0x007fa9a34731b0>>
I, [2016-05-18T07:13:33.551929 #28278]  INFO -- : Mailman v0.7.3 started
I, [2016-05-18T07:13:33.552043 #28278]  INFO -- : IMAP receiver enabled (inbound.helpy@gmail.com@imap.gmail.com).
I, [2016-05-18T07:13:33.600163 #28278]  INFO -- : Polling enabled. Checking every 60 seconds.
I, [2016-05-18T07:13:35.689596 #28278]  INFO -- : Got new message from 'andy-noreply@google.com' with subject 'Scott, welcome to your new Google Account'.
I, [2016-05-18T07:13:36.067432 #28278]  INFO -- : Got new message from 'inbox+noreply@google.com' with subject 'Welcome to Inbox by Gmail'.
I, [2016-05-18T07:17:41.596793 #28278]  INFO -- : Got new message from 'scott.miller.utah@gmail.com' with subject 'Test of IMAP integration'.

Any ideas? None of the three messages shows up in my local instance of Helpy and the dev log doesn't show any kind of activity on the message which makes me think that error line may have something to do with it?

@janrenz
Copy link
Contributor Author

janrenz commented May 18, 2016

I added a default scope, that fixed one issue. I also changed the lookup of users, so that it is not case sensitive.

@scott
Copy link
Member

scott commented May 18, 2016

Nice! I did notice some artifacts from a merge though:

screen shot 2016-05-18 at 3 39 30 pm

@scott
Copy link
Member

scott commented May 20, 2016

I saw you added a setting for the port, can you also integrated that into the settings UI/tests?

@janrenz
Copy link
Contributor Author

janrenz commented May 23, 2016

What do you prefer? Dropdown with common ports or text field?

@scott
Copy link
Member

scott commented May 23, 2016

Hmm thats a good question, probably stay with a text field just in case.

@scott
Copy link
Member

scott commented Jun 1, 2016

Unfortunately I had to revert this- after a couple days in production on support.helpy.io I noticed a ton of exceptions coming in from the inbound mail handler (we use Sendgrid). On further inspection I noticed the problem was here:

undefined method `[]' for #<Griddler::Email:0x00000009bc8690>
lib/email_processor.rb:74:in `get_email_from_mail'

Can you look into this and resubmit a PR for further testing? It appears there needs to be some kind of switch here depending on whether the message is coming from griddler or not?

@scott
Copy link
Member

scott commented Jun 1, 2016

I can also provide a full rails error transcript output if that would be helpful.

@janrenz
Copy link
Contributor Author

janrenz commented Jun 1, 2016

OK. It seems like my new code expectes an instance of Mail as its provided by mailin. Griddler seems to handle this proprietary. I will take care.

@CGA1123 CGA1123 mentioned this pull request Nov 29, 2016
@scott scott mentioned this pull request Mar 29, 2018
@scott scott mentioned this pull request Oct 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants