Better handling with drafts and fixed some additional issues #26
Conversation
There was a problem hiding this comment.
That's quite a massive change, thanks a lot for this contribution! I only looked at the code so far but didn't test it locally. The change looks good to me overall, I just have one minor comment regarding large binary files (.jpg): I believe there is low value in having them versioned so I would rather keep them away from main branch. Maybe I can push them to a dedicated orphan branch (and reference them through a GitHub URL) instead, if that sounds OK to you?
Co-authored-by: Remi Caput <979446+r3c@users.noreply.github.com>
Co-authored-by: Remi Caput <979446+r3c@users.noreply.github.com>
|
I have applied your suggestions and replace a bit images (my mistake was jpg instead of png for screenshots). As for dedicated orphan branch - you decide, I don't have enough rights to create new branches in your repo, but you able to link images even to my repo if you don't want to keep 53k in versioning store. PS. IMHO big binary data starts from 1M, and all less can be stored in git :) Sorry for late response, a bit busy on other projects. |
|
Hi @sfer23, thanks a lot for the updates! I'll check them out locally for testing. However I won't merge the PR as long as it contains binary files. Maybe the easiest would be for you to remove them and I'll prepare a 2nd commit to reference images stored in a separate branch? Otherwise I can rework the branch on my own, but then I would lose your authorship and would prefer to preserve it if possible 🙂 [edit] I pushed change proposal to sfer23#1 ; it can be reviewed commit per commit. |
custom_from.php
Outdated
| if (!$value && $part == 'replyto') { | ||
| $value = $default_identity['reply-to'] ?? ''; | ||
| } | ||
| case self::RECEIVING_EMAIL_OPTION: |
There was a problem hiding this comment.
Is the fallback from previous case to this one intended (seems it is)? If yes and given there are only 2 supported cases on this switch, maybe using simple conditions could be more readable.
custom_from.php
Outdated
| /** | ||
| ** Enable custom "From:" field for drafts. | ||
| */ | ||
| public function compose_draft_from_headers($compose_id, $msg_uid, $reply_from, $attrib) |
There was a problem hiding this comment.
$attrib doesn't seem to be used by this method.
custom_from.php
Outdated
| /** | ||
| * Override headers fields from identity (bcc, reply-to), by plugin settings | ||
| */ | ||
| public function compose_additional_headers($compose_id, $msg_uid, $reply_from, $attrib) |
There was a problem hiding this comment.
$msg_uid doesn't seem to be used by this method.
custom_from.php
Outdated
| default: | ||
| break; |
There was a problem hiding this comment.
The methods compose_additional_headers, compose_draft_from_headers, and compose_from_headers are triggered if there is already a saved or received message.
In the compose_draft_from_headers method, we add the ability to load a custom sender into the text field in drafts.
In the compose_from_headers method, we replace the sender if necessary.
With the option self::RECEIVING_EMAIL_OPTION, we reply from the email to which the message was received.
With the option self::RECEIVING_EMAIL_WITH_IDENTITY_OPTION, we reply from the email to which the message was received and use the IDENTITY settings for the other fields.
With the option self::RECEIVING_EMAIL_WITH_DEFAULT_IDENTITY_OPTION, we reply from the default identity, and we replace the select field to leave only the default identity in the reply field since the Roundcube JS functionality does not allow otherwise.
In the compose_additional_headers method, we clear or replace fields for two options. Roundcube will independently fill in the fields on the frontend for other options. For our cases, we need to use replacements with our own fields so that the JS functionality does not overwrite our values.
Initially, the field value is set to empty and with the self::RECEIVING_EMAIL_OPTION option it is simply cleared and replaced with our html_textarea. With the self::RECEIVING_EMAIL_WITH_DEFAULT_IDENTITY_OPTION option, we also create our own fields so that the Roundcube JS functionality does not overwrite them, and fill the fields with values from the default identity.
custom_from.php
Outdated
| default: | ||
| break; |
- No loose type checks - Add missing `break` statements in switch structures - Remove empty `default` cases
custom_from.php
Outdated
| // Create a textarea field for overriding Roundcube input fields | ||
| $field = new html_textarea(); | ||
|
|
||
| $attrib['content'] = $field->show($value, ['name' => $fname] + $field_attrib); |
There was a problem hiding this comment.
I'm a bit lost on the whole compose_additional_headers / compose_draft_from_headers / compose_from_headers part and the updated README file unfortunately didn't allow me to fully understand how this works.
For example this function is taking this $attrib array (I don't know what its expected contents is, maybe a code comment or pointer to documentation would help) and it seems the only effect it has is this line modifying its 'content' key. When entering the function with $reply_from == self::RECEIVING_EMAIL_OPTION then we would skip part of the switch, for example $field_attrib would not get assigned to any value, yet we try to modify it on line 322, causing a warning (that's a guess, I didn't try it).
Some fragments of the change you sent are a bit obscure to me, and the PR being quite large doesn't really help. I would appreciate if you could clarify to me how these new options are expected to work, then maybe I'll be able to suggest another wording for the README.md file as I guess if I wasn't able to understand it then it's possible other users would face the same issue.
Improvement suggestions for r3c#26
|
|
||
| case self::DEFAULT_IDENTITY_OPTION: | ||
| $default_identity = $rcmail->user->get_identity(); | ||
| $select_from_attrib = array_intersect_key($attrib, array_flip(array('id', 'class', 'style', 'size', 'tabindex'))) + array( |
There was a problem hiding this comment.
I don't understand this part that seems to be trying to extract HTML attributes out of $attrib. tabindex does exist but none of the other. What is the intent here?
| } | ||
|
|
||
| case self::RECEIVING_EMAIL_OPTION: | ||
| $field_attrib = array_intersect_key($attrib, array_flip(array('id', 'class', 'style', 'cols', 'rows', 'tabindex', 'data-recipient-input'))) + array( |
There was a problem hiding this comment.
See above: I don't understand what this extraction of HTML attributes out of $attrib means. When trying locally, $attrib contains these keys only: name, part, id, form, data-recipient-input, content & abort.
|
Hello @sfer23! I see you merged a few minor/style changes I suggested on your PR but I still have several open questions about it, and some parts of the code seem wrong to me. Please let me know if you wish to continue iterating on this PR 🙂 |
|
The methods In the With the option In the Initially, the field value is set to empty and with the |
|
Hello @r3c, of course I want to continue, but I'm on 2 week vacation right now, soon will proceess request faster |
|
Hi @r3c
The first option replies from the receiving email address without adding parameters. Currently, the plugin loses the modified email address and switched back to the user's default identity when saving a draft, closing the editing window and reopening the draft. This should have been fixed as well. |
|
Hi @MiMoHo, thanks for clarifying, it's indeed more clear to me with these details and probably means we can improve the wording of this new option as well. Another part that seem blurry to me at the moment is how the new feature interacts with existing matching rule configuration. The former is a user setting while the later is a global configuration, it would probably make sense to move it to a user setting as well but I can take care of it afterwards. What should be clarified right now however is what replying with "receiving email address" means when the current global configuration changes what headers are taken into account and whether some address is considered matching an existing identity of not. For example I guess the last option ("default identity") completely ignores the global setting, making these two features having an effect one on the other. I would rather try to keep them orthogonal, maybe by having the matching behavior controlled by an option and the "use parts of the identity" flag controlled by another one? (what are these "parts" BTW, signature, real name, anything else?) |
|
Thanks for considering @r3c , but anything coding- or tech-related, @sfer23 would need to address. I can only comment from a user's perspective. What I was told by @sfer23 , there are limitations inside Roundcube that doesn't play nice with the custom_from logic we are extending. Check out related issues here that I commented on. The reply settings shall overrule the default identity taking its parameters more or less into account: Option 3 is most likely the most useful option for private use when having just one identity but you want to make sure that you can reply from each individual email address you gave to the platforms you registered on without having to set up an identity for them that would be needed for option 2, otherwise you'd reply from the receiving email address without additional information such as name and signature. In fact, the first 3 out of 4 options partially ignore the default identity, but this is part of the feature. You could decide which of the four reply settings shall be preset to avoid unintended behavior until the user defines the preference. Personally, I would suggest option 4 (default identity) because: |
|
Thanks for clarifying. I'm wondering about what would be the best way to have these options play together with the existing matching rules configuration option:
Unless I miss something, options 1-4 have their own matching behavior which is different from the previously existing one, and are incompatible with its flags "d" (match by domain) and "e" (match anything). This makes me think the two options may be reworked so they're more consistent one with another. Maybe we could agree on the following:
Let's assume two identities "me@domain1.com" (default) and "myself@domain2.com", and following configuration:
Then here is how it would behave:
Would that make sense to you and answer your needs? |
|
With the four reply options setup, I wanted to take into account all possibilities how users may use the plugin and not just focus on my needs. Let me give an example for each setting: Option 1: Very basic, just making sure that I reply with the same (correct) email address like "github@mydomain.com" and don't reveal my default identity which would be the main mailbox "contact@mydomain.com" to which all emails are forwarded but that I wish to keep private for privacy reasons. Services receiving an email often work with email management solutions that filter primarily by email address and subjects including a "[ticket ID]". They don't reply with a read receipt and will ignore any specification I put in the "reply to" field in my identity, so I'm not using parameters at all. I also don't want to include my signature in emails sent to "github@mydomain.com" as GitHub Support must not know my personal details that I only wish to include when composing a new email from my main mailbox with default identity. Option 2: I need to inform my laywer about the emails with GitHub Support. So even though I usually don't set up identities for each individual email address that I used to register on various platforms, I now do create one for GitHub only in order to automatically include my lawyer in CC (so I don't forget one time). For that, I specify my name, email address and CC value in the identity parameters. All other services shall continue to receive a reply from the receiving email address without any other parameters. Option 3: I set up a default identity with my first and last name and email address contact@example.com. But my main mailbox shall be kept private. I wish to reply to "github@mydomain.com" but make it look neat by including my name specified in the identity. GitHub Support doesn't care about my name preference on the platform. They write to "FirstName SecondName ThirdName Surname ≤github@mydomain.com≥". But I wish to automatically reply from "FirstName Surname ≤github@mydomain.com≥", maybe even include my signature or specify other identity parameters that shall be used in the reply. Option 4: I don't register with individual email addresses as I'm using trusted platforms only. Hence, usually I'm only using my default identity which is my main mailbox. But now, I advertised on the web that I'm looking to meet up with local strangers and wish to preserve my privacy and protect myself from (romance) scam. So I advertised using a custom email meetup@mydomain.com for people to contact me. When replying, I now need the option to manually change my email address from contact to meetup for which I'm using the plugin. When someone is legit, I will simply continue replying using my default identity. These are possible scenarios which can be summarized into four general reply settings as far as I can imagine. I'm not familiar with the technical background of matching rules currently implemented. Again, I'm not coding anything. I cannot even assure our implementation works as it is supposed to be. I can just comment from a user perspective. My webhosting company removed the custom_from plugin due to other customers complaining about the lack of user customization as the reply setting is currently specified universally by the admin which affects all users whether they reply from their default identity or the receiving email address. However, many webhosting companies use the Roundcube webmailer so offering comprehensive reply settings in combination with the possibility adjust the header when composing an email seems crucial to me to allow people to use individual email addresses and protect themselves against spam and on the other hand, they don't struggle with maintaining identities which is a pain when using just the default setup of Roundcube. In the end, it is @r3c 's plugin. When you understand the benefit, please find a proper way how this can be combined with your and the general Roundcube concept 🙏 |
|
Yes I think I understand how the four options you're willing to add were intended to work and the focus on these being configurable per user rather than globally. We agree on this part 🙂 What I was saying is that current implementation causes an inconsistent overlap with existing matching rules configuration (which only exists as a global setting for now, not per user). That's why I was proposing something that would, in my opinion, merge them together. |
|
Yes, I think I understood the reason @r3c , but I do not know what this means for the overall implementation and how to proceed. I hired someone to develop what's needed to introduce the desired features but beyond the current proposal, I am afraid, I cannot contribute with coding-related activity. Unless there are substantial concerns about how this was done in the first place, it would require yourself @r3c to refine the tech. |
|
Then I think we can first focus on improving current contribution, starting with my comment from three weeks ago: #26 (comment) This PR is introducing several separate features so I think it would be best to split it into smaller individual contributions, but if that's too much of an effort then let's keep current PR and iterate until it can be merged. I understand you cannot contribute yourself directly, but the PR we're commenting about requires a fair amount of fixes and improvements before it can be merged, and none of my original feedback points were addressed so far. I'll let you decide what you want to do with this PR and get back to me when you believe it's ready for being reviewed again. |
|
Hi @r3c , @bigclumsypanda made a few changes last week. Did that help in some way? |
It helped a bit, but most of my comments are still not addressed nor answered. Just to be clear, I think this PR is still quite far from a state where it can be merged, and having mostly you @MiMoHo answering but not touching the code doesn't really help making things clearer to me. I'm putting this on hold until some developer answers all previous feedback I already posted and tells me this PR is ready for being read again, as I feel the time spent on this PR is getting to high compared to the reasonably small functional change it was supposed to be. |
|
Hey! We applied some changes and commented on the code. Here’s a summary and questions:
Changed method name and left a comment.
Changed the way of interaction with method. Now it returns value.
Added comment why “break” is not needed.
Moved to a single hook “template_object_composeheaders”
Should we just move settings to User side? |
|
Hi @sfer23, thank you for your answers. I have two concerns about the settings:
|
|
Hi @r3c ,
The user may not want to use all parameters defined in the matched/default identity that could for example include a signature or "copy to" value when replying from an individual email address "someone@domain1.com". For example, when I provide someone with a dedicated email address to make sure I preserve my privacy without revealing the default identity which is identical to the main mailbox, an automatically attached signature would nonetheless be included in a reply from *@domain1.com, but should actually only be sent to my verified contacts that address my main mailbox me@domain1.com.
Assuming I've got a personal and business identity in Roundcube and have configured "John Smith" as my personal identity's name for me@domain1.com while using "John Smith │ CEO of Company" for my secondary business email identity myself@domain2.com. When my business email address receives an email CC, then I would reply with my personal domain's parameters which doesn't include my business role "CEO of Company" in the "from" header value.
As in the second example: assuming, emails sent to my business colleague are sent in CC to me so that I learn about the progress, then – when engaging in the discussion – I would send an email from my personal default identity which is not at all the intended behavior. To conclude: I hope, I understood your logic and you agree on the limitations of a single match behavior which specifies only "one rule per header (To:, Cc:, X-Original-To:, etc.)". I would favor our approach and introduce new matching rules replacing the old logic as I explained with different examples before. |
|
Hi @MiMoHo, thanks for your message. I think the new behavior you want to introduce is clear to me, as you already described in this previous message. I don't think we need to go over it again, I personally think current wording could be more explicit but this is really minor and can be discussed over a PR. The point I was raising is about compatibility with current per-header settings. I understand it doesn't match your need, and I'm open to revisit it. What I was saying however is that it is currently incompatible with your new approach, because both options imply different matching logic:
I would like both options to stay consistent, so if a virtual address is matched against an identity and should be used for replying, then I want profile information from this same matched identity to be used (name, signature, etc.) if enabled. To my understanding this is quite different from what current PR is implementing. Again, I'm open to dropping current per-header approach and keep a single matching configuration, but I would like to make it consistent with the new feature you're introducing, rather than keeping two orthogonal and inconsistent options. Let me try another time to explain how I see this could work:
I think this setup covers the 4 options you were describing:
Other combinations offer more fine-tune control over how identities are matched and whether or not their profile information should be used in outgoing emails. I hope this makes sense to you. |
|
I don't mind a specific technical implementation as long as the functionality is there and it's working flawlessly. When your approach has advantages over ours, it shall prevail 🙂 My goal was to enhance the plugin and make it usable in a webhosting environment where there are different users with different needs using the same setup. |
|
I'm not sure what you expect from me at this point. This PR was opened without any prior technical discussion and therefore expectedly requires a significant rework before it can be merged, as current approach would leave the plugin in an inconsistent state. And we're not talking about dropping unneeded changes here, but rather rewriting most of the contribution from scratch. I understand this was paid work, and am sorry it didn't allow delivering the result you expected. I however can't be expected to provide an equivalent amount of work as a consequence. This topic is on hold on my end. Feel free to resume any time you want, preferrably starting by a discussion rather than sending a PR before we could have a chance to agree on the goal. I'll keep this story in mind as I believe it would be a valuable evolution to bring to the plugin, but spending my own personal time to implement it isn't something I planned to do so far. |
|
Glad to hear this @the0ne 👍 I think the first step would be to split this PR into separate ones changing a single feature each, so we can iterate on each of them and eventually merge them independently instead of blocking the whole thing behind a single large PR. I just fixed the support for drafts in #27, this should hopefully move this one out of the way. Remaining separate features I see in this PR are:
I'll try to rework the configuration part if time allows it. It should bring the plugin in a state closer to what is needed for this PR to be compatible. Then I think the remaining part will be to implement the change of matching behavior. |
|
That sounds fantastic, thank you @r3c 🥳 |
|
I worked on supporting drafts and storing configuration as a user preference, both were merged to master with backward compatibility for globally-stored configuration. This should allow resuming this PR with hopefully much fewer changes. |
|
Et voilà, I just released v1.7.0 with support for drafts, user preference and two new matching options for more fine-tune control. I think the only remaining item to fully address this ticket is the "use signature & name from identity" option. I prepared placeholder code in https://github.com/r3c/custom_from/tree/topic/identity-option with this option added to configuration & preferences ; the implementation is yet to be written but shouldn't require much extra effort and can use this PR as base. Now I'm not super satisfied with current state as I feel I ended up doing the work someone else was paid for, but if we can bring this ticket to a satisfying conclusion then it's for the best. @sfer23 I don't know what deal you had with @MiMoHo but I think it would be fair for you to take care of this last step within the terms of the agreement you already had, so that something gets delivered out of it eventually. Let me know what else I should do to facilitate this resolution. |
|
Hi @MiMoHo ; I didn't hear back from you since I replied to your last e-mail, so answering here for visibility (other people may also be following this thread ; @sfer23 as you still there?) I made some progress regarding the last missing step and merged (#51) a new option to change how name & signature are reused. I hope this will bring the plugin closer to what you wanted to achieve with this topic. Here is how it works now:
Various combinations of these two options should provide roughly the same coverage than what you were initially aiming for. I still have a few things to polish before this gets included in a release, in the meantime let me know if you feel there is still something missing compared to your original specifications. Also mind the implementation is quite different from @sfer23 's one, in my opinion fixing a few issues (for example the "blank signature" approach he took was permanent, and even toggling the plugin off or trying manually insert signature wouldn't work, I don't know if this was expected), but since he doesn't seems to be answering anymore I'm not sure we'll be able to follow-up on this part of the discussion. |
|
Just published v1.8.0. Hopefully it addresses your initial requirements, but from the lack of feedback I understand this was abandonned. Definitely not my best experience at maintaining FOSS, so I'll close the issue at the end of this week and consider it finally done. |
|
Improved in v1.8.0. |
Description of changes:
Comments to the code:
Changed the setting of the arbitrary sender variable in the session to another hook. Previously it was
message_compose, but the hook has been changed totemplate_object_composeheadersto manipulate the message fields, "Bcc" and "Reply-To". Because of this, the name of thecompose_from_headersmethod has changed.A hash change has been added to JavaScript functionality, so that the user, after clicking the plugin button, can save the message.
In the style file for skins, a style has been added that adds a response icon in the settings item.
Additional changes:
Added a few screenshots to README file for better presentation