Fix #50 remove unnecessary rewrite_from_domains config

[schellj]

No description provided.

[robnagler]

Looks good. One question. Merge as you see fit. @moellep will need to generate a release. Maybe wait after Schwab is out of beta.

[robnagler]

Something about the above code bothers me. I think it's only necessary for test, but it seems like if the allow_resend_re fails to match, it should fall through to the rewrite lookup. Thoughts?

[schellj]

Well, if $cfg->{allow_resend_from_re} && $old_email !~ $cfg->{allow_resend_from_re}, then the rewrite will occur, but the dmarc check (_rewrite_from_lookup) wouldn't have occurred. Is the lack of the dmarc check in that case what's bothering you?

[robnagler]

Yes, there should always be a dmarc check unless allow_resend_from_re matches. At least that's how I read the code.

[schellj]

You understand the intent of this code better than I, but that would seem to make sense. In that case, 405-407 could be changed to:

    if ($cfg->{allow_resend_from_re} && $old_email =~ $cfg->{allow_resend_from_re}) {
        return 1;

(it won't let me do a multi-line code suggestion here)

[robnagler]

I don't use code suggestions. :) I don't think that's quite right. I think the else has to get removed so that it falls through if the pattern doesn't match.

[schellj]

With the change in my previous comment, the _rewrite_from_lookup dmarc check would get evaluated if the allow_resend_from_re didn't match. I'm not sure what you're saying should change.

[robnagler]

6e5b2c6 is what i mean

[schellj]

ok, that's what I would have figured you meant. I wasn't sure because, with the change to the if clause and return from that block, the code is functionally equivalent whether the else is removed or not. The intent is probably more clear with the else removed, though.

[robnagler]

ah i see what you mean now... sorry.