FEATURE: add setting to show content of forwarded emails in topics (PR #7935)

You’ve signed the CLA, LeoMcA. Thank you! This pull request is ready for review.

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/enable-forwarded-emails-doesnt-behave-like-conventional-email-forwarding/111501/3

This should be translated.

I wasn’t so sure about keeping this in my commit, it was a “nice hack” I discovered while messing about with ways of formatting the forwarded message.

Localising to the default locale of the server shouldn’t be hard (I’ll add a commit doing that), but it feels like I’ll be opening a whole can of worms trying to localise this locally client side. Happy to remove this and have it as an unnamed quote.

We usually indent the end of the squiggly heredoc comment at the same level as the variable

      EOF

Any way we could avoid duplicating that logic between forwarded_email_create_replies and forwarded_email_quote_forwarded?

I’d be fine with an unnamed quote. It’s less burden on translators. If we find it confusing in the future we can always add it back.

Yep, I had to use a block for creating the embedded user at the correct point in the logic, but it seems pretty clean to me.

Is there a reason to pass in the param via a block vs simply adding a topic_user: nil param?

I did this to keep the current logic around when a staged user is created:

      when :category
        category = destination[:obj]

        return false if user.staged? && !category.email_in_allow_strangers
        return false if !user.has_trust_level?(SiteSetting.email_in_min_trust)

originally the next line was:

embedded_user = find_or_create_user(email, display_name)

now the next line is:

topic_user = block_given? ? yield : user

If we pass the user in with a param, then we create the user prior to checking those conditions, and so I might be unnecessarily creating a staged user. I did try just creating the user and passing it as a param, but it caused a spec to fail.

I see … do we need this level of flexibility here? Maybe just pass email and display_name in optionally? something about this block is not sitting right for me… I think we should probably change the other occurrence as well but this is out of scope for this pr. If we must must have this flexibility I think passing in a lambda is a bit clearer intent wise.

Maybe just pass email and display_name in optionally?

You know, I hadn’t actually thought of this, and jumped straight for the complicated solution. Whoops! Will update the PR.

Actually I opted for a lambda because passing in email and display_name creates hell like this:

topic_user = embedded_user ? find_or_create_user(embedded_user[:email], embedded_user[:name]) : user

Splitting things over multiple lines doesn’t make it much clearer, far less than what we get with a lambda (imo):

topic_user = embedded_user&.call || user
1 Like

I say given we have so many params … let’s name them all:

forwarded_email_create_topic(destination: , user: , raw:, title:, date: nil, embedded_user: nil)

It makes the callers way clearer. I think once we reach about 3 params we should consider strongly naming params.

I say given we have so many params … let’s name them all:

forwarded_email_create_topic(destination: , user: , raw:, title:, date: nil, embedded_user: nil)

It makes the callers way clearer. I think once we reach about 3 params we should consider strongly naming params.

1 Like

I had though that there were too many arguments, and had considered passing them in as a hash, but hadn’t realised this was possible (and so simple!)

I’ve added a commit doing this

2 Likes

LGTM :+1:

1 Like

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/text-of-forwarded-emails-dont-show-up-in-posts/23155/29