Restore original maybe_reject_email API

Restore original maybe_reject_email API

Programs outside this repository expect this method to exist.
maybe_reject_email is truly part of an external API.

Mark new methods as private to make the distinction clear.

From 2b4566ca7b2c41b860862290f50c1128c4ee8934 Mon Sep 17 00:00:00 2001
From: Saj Goonatilleke <saj@discourse.org>
Date: Thu, 29 Nov 2018 19:54:20 +1100
Subject: [PATCH] Restore original maybe_reject_email API

Programs outside this repository expect this method to exist.
maybe_reject_email is truly part of an external API.

Mark new methods as private to make the distinction clear.

diff --git a/lib/mail_receiver/fast_rejection.rb b/lib/mail_receiver/fast_rejection.rb
index 34e7210..8221f22 100644
--- a/lib/mail_receiver/fast_rejection.rb
+++ b/lib/mail_receiver/fast_rejection.rb
@@ -59,41 +59,10 @@ class FastRejection < MailReceiverBase
 		run_filters(args)
 	end
 
-	def run_filters(args)
-		filters = [
-			:maybe_reject_by_sender_domain,
-			:maybe_reject_by_api,
-		]
-
-		for f in filters do
-			action = send(f, args)
-			break unless action == "dunno"
-		end
-		action
-	end
-
-	def maybe_reject_by_sender_domain(args)
-		sender = args['sender']
-
-		return "dunno" if sender.empty?
-
-		domain = domain_from_addrspec(sender)
-		if domain.empty?
-			logger.info("deferred mail with domainless sender #{sender}")
-			return 'defer_if_permit Invalid sender'
-		end
-		if @blacklisted_sender_domains.include? domain
-			logger.info("rejected mail from blacklisted sender domain #{domain} (from #{sender})")
-			return 'reject Invalid sender'
-		end
-
-		return "dunno"
-	end
-
-	def maybe_reject_by_api(args)
+	def maybe_reject_email(from, to)
 		uri = URI.parse(endpoint)
-		fromarg = CGI::escape(args['sender'])
-		toarg = CGI::escape(args['recipient'])
+		fromarg = CGI::escape(from)
+		toarg = CGI::escape(to)
 
 		api_qs = "api_key=#{key}&api_username=#{username}&from=#{fromarg}&to=#{toarg}"
 		if uri.query and !uri.query.empty?
@@ -132,4 +101,41 @@ class FastRejection < MailReceiverBase
 		"#{@env['DISCOURSE_BASE_URL']}/admin/email/smtp_should_reject.json"
 	end
 
+	private
+
+	def run_filters(args)
+		filters = [
+			:maybe_reject_by_sender_domain,
+			:maybe_reject_by_api,
+		]
+
+		for f in filters do
+			action = send(f, args)
+			break unless action == "dunno"
+		end
+		action
+	end
+
+	def maybe_reject_by_sender_domain(args)
+		sender = args['sender']
+
+		return "dunno" if sender.empty?
+
+		domain = domain_from_addrspec(sender)
+		if domain.empty?
+			logger.info("deferred mail with domainless sender #{sender}")
+			return 'defer_if_permit Invalid sender'
+		end
+		if @blacklisted_sender_domains.include? domain
+			logger.info("rejected mail from blacklisted sender domain #{domain} (from #{sender})")
+			return 'reject Invalid sender'
+		end
+
+		return "dunno"
+	end
+
+	def maybe_reject_by_api(args)
+		maybe_reject_email(args['sender'], args['recipient'])
+	end
+
 end

GitHub

You don’t need the return in Ruby since it’ll always return the value of the last statement of a function :wink:

Little distinctions like this are hard to pick up. :slight_smile: I stuck it in there for consistency with the earlier returns in this method, and because the old maybe_reject_email implementation terminated with an explicit return. Should I remove the explicit return from everywhere?

It looks like Rubocop’s Style/RedundantReturn can pick this up.

:+1: for adding this Rubocop’s rule.

The for..in loops in Ruby are a bit weird so we tend to not use them. Instead, we prefer the filters.each do |f| loop :wink:

The for..in loop was used here to avoid introducing a new scope.

filters.each do |f|
  action = send(f, args)
  break unless action == "dunno"
end
action # <-- invalid var ref

Is there a more idiomatic way to do this in Ruby? In this case, it would be preferable to lazily evaluate the filters; i.e.: if the first filter produces a terminal result, do not evaluate the second filter.

(FWIW, the string dunno turns out to be a significant enum of sorts in the Postfix policy delegation protocol.)

1 Like

Good point :+1: I guess we never used it before…

initializing action before the loop or return action if action != "dunno" would work

-> Address Ruby stylistic concerns by saj · Pull Request #3 · discourse/mail-receiver · GitHub

-> Address Ruby stylistic concerns by saj · Pull Request #3 · discourse/mail-receiver · GitHub