search

r3c / custom_from

Plugin for Roundcube webmail, enable virtual email sender input.
https://plugins.roundcube.net/#/packages/r3c/custom-from
Other
20 stars 16 forks source link

Do not CC if reply-all'ing to an email #19

Closed SerialVelocity closed 4 years ago

SerialVelocity commented 4 years ago

Hi Remi,

First of all, thanks for creating this plugin! I just found it and it is saving me so much time by not having to create each identity manually!

I have found one bug though. When reply-all'ing to an email, the identity is left in the CC field even though it is the email I am responding from. Would it be possible to remove the identity from the CC field?

r3c commented 4 years ago

Hello @SerialVelocity, you're very welcome, I'm glad to hear this plugin is useful!

I reproduced your issue locally and will start working on a fix as soon as possible 👍

r3c commented 4 years ago

I just pushed 8e741cb to address the bug you identified. Would you mind telling me if it looks OK to you before I can merge it to master?

SerialVelocity commented 4 years ago

Hi @r3c! It doesn't look like it always fixes the issue.

When responding, my From field will be My Name <my@address.com and the CC field will contain my@address.com without the My Name wrapper.

It seems to work if they are exactly the same though.

r3c commented 4 years ago

Oh I probably didn't get your scenario right then. Could you please tell me what is the contents of the "from" & "cc" headers of the email you're replying to?

SerialVelocity commented 4 years ago

There seems to be two cases. One of them now works.

If I receive an email with these fields it works:

From: some-external-address@gmail.com
To: My Name <test@domain.ccom>

If I receive an email with these fields it doesn't:

From: some-external-address@gmail.com
To: test@domain.ccom

It looks like you need to compare the email address without the name.

r3c commented 4 years ago

Thanks! Indeed it wasn't working with raw addresses as it seems Roundcube is adding a trailing comma there. Hopefully 374e5c5 should handle both cases!

SerialVelocity commented 4 years ago

Hi @r3c! I tried out the new tag and it removes the address from the frontend so it is no longer visible. Unfortunately, when I hit "send", I still get CC'd!

r3c commented 4 years ago

OK I'll need to setup a test smtp server as trying to fix the issue while relying only on the UI clearly doesn't work! Sorry for the back and forth, I'll let you know as soon as I have a verified fix!

r3c commented 4 years ago

Playing with the UI elements from the DOM wasn't a good option, after having a look at Roundcube's Elastic skin code I figured out it's easier to modify the "cc" input value and manually trigger an update of the UI elements. I tested it with a fake smtp server and it seems to be working fine (both updating the UI and effectively removing email from the "cc" field) ; however this remains a hack playing with UI code so I'm not sure it will work fine with other skins than Elastic.

Let me know what you think!

SerialVelocity commented 4 years ago

Do any of the example plugins do anything like this? Maybe they have a way to do this in a less hacky way?

r3c commented 4 years ago

Unfortunately I didn't find anything similar in example plugins. From what I could see by reading the code, implementing this feature in a less hacky way would require adding a new hook in Roundcube to allow reading and modifying recipient fields before the editor is shown.

r3c commented 4 years ago

@SerialVelocity what's your opinion about this change? Should I merge it, or do you prefer to search for better alternatives?

SerialVelocity commented 4 years ago

This change is fine for me as I use the elastic interface. Maybe it is worth filing a feature request on roundcube to add hooks for doing this properly though?

r3c commented 4 years ago

I'm not sure they'll see a benefit in adding a new hook for a single plugin, but let's start the discussion anyway :)

https://github.com/roundcube/roundcubemail/issues/7590

SerialVelocity commented 4 years ago

One thing to note is that I think there currently is a hook to modify the CC before the message is displayed but you can't modify it dynamically from the front-end easily which I think you are currently trying to do so you can add it back?

https://sourcegraph.com/github.com/roundcube/roundcubemail@86849a0ee4c10b3a866b0161a2572aa8d9e7a461/-/blob/program/steps/mail/compose.inc#L350:44

r3c commented 4 years ago

That's the hook I'm using and unless I miss something it allows overriding the "cc" field but it's default value is not available at this point yet. This means I would have to reproduce Roundcube's logic to entirely define the field here, which seems another kind of hack?

r3c commented 4 years ago

So I created issue https://github.com/roundcube/roundcubemail/issues/7590 on Roundcube to start discussion about a new hook to modify "cc" field, then realized it's not exactly straightforward due to how Roundcube is currently building form field values. I'll provide more details in the Roundcube issue thread, but in the meantime I went for another approach here and hacked the feature into "message_compose_body" hook. Still not ideal, but at least it doesn't rely on JavaScript code and should work with all skins now.

SerialVelocity commented 4 years ago

It might be worth either removing the v0.8 compatibility code or make this v0.8 compatible as well btw: https://github.com/r3c/custom_from/blob/2020f447ff44e48fea0264490ee6c63038709e88/custom_from.php#L96-L109

Thanks for continuing to look into this though. I'll try out the new version soon!

r3c commented 4 years ago

I just merged patch to master. Please feel free to reopen if there is anything else you'd like me to change!

SerialVelocity commented 3 years ago

Hey @r3c, sorry about the delay. Your patch to master didn't work.

r3c commented 3 years ago

Hello @SerialVelocity, indeed I was relying on 'message' to be passed through 'message_compose_body' hook as it is in current Roundcube master (link) but this is not the case as of 1.4.11 yet (link). I'm reverting to access global $MESSAGE variable as a temporary workaround, see 13e4cda for details. AFAIK this is not part of the API so it could break in any future release, so I'm also adding an error message so it will be easier to troubleshoot when this happens. Hope this will work better with your setup.