mentioned in issue #1360 (closed)

Added 1 commit:

• a428838d - Update mail_room

Can we re-use the for_commit? method in place of the proc?

Find out what we're rescuing from here.

Think about extracting all reply key-related replacing/extracting into its own class.

Note from yourself: Settings.reply_by_email

Added 1 commit:

• 8906caba - Changes and stuff.

Candidate for a filter?

Heh, possibly. I haven't actually read the code so I have no idea. Will check it out tomorrow!

Maybe this filter can be reused.

@razer6 I'll check that out, thanks.

@DouweM Awesome work!

Does it have to be a gmail address? Or is it also possible to use an inhouse imap server? (Maybe a stupid obvious question, but still good to get it answered :D )

I wondered the same when I saw the reply email address example was using plus addressing. That would be a limiting factor in which mail systems could be used.

I used to think the same thing, that plus addressing was Gmail only, but fortunately, it's not! It's supported by Yahoo, iCloud, Outlook.com, as well as in-house Postfix installations: https://en.wikipedia.org/wiki/Email_address#Sub-addressing

One major system that doesn't seem to support it is Exchange. At least AFAICT.

Another option would be to use a catch all email like %{reply_key}@replies.gitlab.example.com. I'm assuming Exchange supports something like that?

Otherwise, the admin would need to install a separate Postfix mail server for this specific purpose.

Great question - I'm not sure.

Added 6 commits:

• 73eef57d - Make MR notification email subject more standard.
• 4f34d363 - Correctly set Message-ID for comment notifications.
• 6b31827a - Include display name with reply to address.
• 01c5eced - Add "Reply to this email directly" to notification footer.
• ee1eb294 - Turn reply-by-email attachments into uploads.
• 170aa3b4 - Update mail_room.

Added 1 commit:

• 76dbafba - Send a rejection email when the incoming email couldn't be processed.

We're pretty much done implementation wise, pending your review. Next up are tests and documentation.

To check: If this feature is disabled, what does this job do? Exit early? Continually poll for nothing?

Looks like it quits early: https://github.com/tpitale/mail_room/blob/master/lib%2Fmail_room%2Fcoordinator.rb#L21

Note from the readme:

It is important to properly configure MailRoom when using -q in conjunction with a process manager like upstart/god/monit/etc, or to configure the manager to not restart a process infinitely. MailRoom will stop running immediately if it has no settings for mailboxes.

I have no idea how this will behave with runit which is used in omnibus.

cc @marin

I commented here: https://github.com/tpitale/mail_room/commit/2de3ffe4af24dee8b6288ff58410dd2e598dc95a#commitcomment-12792653

Added 1 commit:

• 03b54f1f - Fix responding to commit notes

Notify handles these two differently:

  default from: Proc.new { default_sender_address.format }
  default reply_to: Gitlab.config.gitlab.email_reply_to

  private

  # The default email address to send emails from
  def default_sender_address
    address = Mail::Address.new(Gitlab.config.gitlab.email_from)
    address.display_name = Gitlab.config.gitlab.email_display_name
    address
  end

Maybe we should have a base mailer that configures these and has the current_user and can? helpers and whatever else we need to share, that these can both inherit from?

Edit: Nevermind about current_user/can, their implementations are different for these two mailers.

Sounds reasonable. Will do

I'm unsure about this method belonging in Notify. On the one hand, it's great that each individual [whatever]_email method can just call this method directly, but on the other I think it falls outside the scope of the mailer.

Maybe we should make a class method on SentNotification and then the methods can call SentNotification.whatever(...) instead of this method.

Makes sense.

Why the gfm here?

Because the reason will have a list of error messages if the note couldn't be posted because of validation errors.

Added 2 commits:

• a8a861ae - Add BaseMailer to house shared mailer functionality.
• 992dbbd9 - Move sent_notification! out of Notify.

Added 1 commit:

• ed6122d7 - Markdown, not GFM.

Added 1 commit:

• 4e33127b - We'll get the

  for free.

Added 1 commit:

• 0bc94198 - Fix BaseMailer.

Added 3 commits:

• 089aa2de - Add Reply by email feature to admin dashboard.
• f3b57ce6 - Update init scripts.
• 34026c97 - Add documentation.

Added 1 commit:

• c50e5e68 - Fix bin/mail_room.

Added 1 commit:

• 1202875d - Fix lib/support/init.d/gitlab.

Added 1 commit:

• eacdd508 - Fix Basemailer#can?

Added 1 commit:

• 41fdd20c - Test Gitlab::ReplyByEmail.

Added 2 commits:

• 9501495a - Fix init.d script.
• 19a049ec - Make documentation clearer.

Added 1 commit:

• 83081f16 - Start on tests.

Added 3 commits:

• 3ff9d5c6 - Use canonical version of mail_room.
• e9972efc - Extract ReplyParser and AttachmentUploader from Receiver.
• 0b401f2e - Fix a couple of whoopsy daisies.

Added 2 commits:

• 2f78b5e8 - Make error class names more consistent.
• 991c9f6f - Test Email::AttachmentUploader.

Added 1 commit:

• e44936f3 - Test EmailReceiverWorker.

Added 1 commit:

• 8ec5fb13 - Test Gitlab::Email::Receiver.

Added 48 commits:

• 8ec5fb13...8819007c - 47 commits from branch master
• 3d51a6d4 - Merge branch 'master' into reply-by-email

Added 1 commit:

• 2b5a2b8f - Removed unused fixtures.

Added 1 commit:

• e1eb09dd - Remove more unused fixtures.

Added 1 commit:

• ac268674 - Update spec.

Let's add a FixtureHelper module in spec/support/fixtures.rb to house this.

Added 1 commit:

• 54b04f1c - Add fixture_file helper.

@marin Could you see what the impact of this would be to omnibus? We'd need to add the mail_room process if reply by email is enabled, as well as reply_by_email config options (enabled/address, imap host/port/ssl/email/password). You can see the doc for source installations at https://gitlab.com/gitlab-org/gitlab-ce/blob/reply-by-email/doc/reply_by_email/README.md.

Added 1 commit:

• 6110e175 - Use heredocs.

Added 1 commit:

• 48e25a01 - Add stub_reply_by_email_setting helper.

Added 2 commits:

• 123af785 - Add gitlab:reply_by_email:check rake task.
• 02b7be4a - Remove weirdness.

Added 1 commit:

• 41ed60c2 - Fix MailRoom running check.

Added 1 commit:

• f26c2905 - Fix indentation

Added 1 commit:

• 99ef8c81 - Fix markdown specs.

Added 1 commit:

• afb765ad - Fix markdown specs again. Apparently development and test behave differently.

Added 1 commit:

• 3d141d15 - Fix spec by removing global state.

Added 1 commit:

• 35224d5e - Memoize address_regex locally.

@DouweM The impact on omnibus-gitlab is not big(new config and templates but no structure changes), when this is ready for merge open an issue at omnibus-gitlab so I can add in the support.

Added 1 commit:

• 69708dab - Block blocked users from replying to threads by email.

Reassigned to @DouweM

Reassigned to @rspeicher

@rspeicher Ready for the final review? :)

I think anywhere in our documentation where we say what we're about to do and then follow it with a command, the explanatory line should always end in a colon. You did that for the steps above, let's add it to steps 6 and 7.

Agreed

Should we break this out into its own ### Development section below Omnibus?

I'll add a new section. The init.d stuff isn't required in development either.

Added 1 commit:

• 50baa1fd - Add development section to doc.

Added 1 commit:

• 03666553 - Fix check task for development.

Added 1 commit:

• 3abb356d - Add development documentation for real.

Title changed from WIP: Reply by email to Reply by email

Added 1 commit:

• 15fc7bd6 - No HTML-only email please

Status changed to merged

mentioned in commit f0bdf7f8

mentioned in issue omnibus-gitlab#732 (closed)

Great work @DouweM!!

Awesome 📧

@DouweM this is a bad idea. People put customizations in /etc/default/gitlab, we should not be telling them to clobber that file.

When this got deployed to dev last night, we broke Git HTTP access (and hence ci.gitlab.org cc @rspeicher ) because we set the GITLAB_GRACK_AUTH_ONLY variable in that file.

cc @dzaporozhets

@jacobvosmaer Ah, good point. I didn't realize that people may already have that file. I'll update the doc to say they should edit it to add mail_room_enabled=true instead if it already exists.

@jacobvosmaer Done in f00fdc71, part of !1193 (merged).

What happened to the omnibus integration? This got merged with two 'todo checkboxes' unticked.

Edit: ah I see the reference to omnibus-gitlab#732 (closed) now in the fine print above. Never mind :)

@jacobvosmaer Thanks for asking. @DouweM Created an issue in omnibus-gitlab/issues!732 and MR was merged yesterday with this. I pushed a documentation commit to gitlab-ce/merge_requests!1193

@jacobvosmaer Also see !1173 (comment 1935363), which I posted a day before this MR was merged.

Just came across this. Given this is a Sidekiq worker, should this just be logger.warn instead of Rails.logger?

@stanhu Does that make a difference?

mentioned in merge request !1480 (merged)

@DouweM I think it would never reach here because in the case there's an else clause which just returns from the method. Was this expected?

@godfat When e is any of those errors, we don't reach else and return right?

Oh, you're right. I was confused. I didn't use return for a long while. (because functional programming :P)

So personally I'll omit that else clause and write:

EmailRejectionMailer.delay.rejection(reason, raw, can_retry) if reason

@godfat 👍

mentioned in commit godfat/gitlab-ce@cc69bd07

Mentioned in commit ubudzisz/gitlab-ce@cc69bd07

mentioned in commit winniehell/gitlab-ce@4bebdc09

Mentioned in commit winniehell/gitlab-ce@4bebdc09

mentioned in issue #36054 (closed)