fix: Double signature in composed Email #16178
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gavindsouza
requested review from
a team and
hasnain2808
and removed request for
a team
gavindsouza
force-pushed
the
email-signature-fix
branch
from
2961bbe
to
3bb374a
Compare
Codecov Report
@@ Coverage Diff @@
## develop #16178 +/- ##
===========================================
+ Coverage 51.86% 56.77% +4.91%
===========================================
Files 744 756 +12
Lines 66193 66847 +654
Branches 5555 5703 +148
===========================================
+ Hits 34329 37954 +3625
+ Misses 27830 25428 -2402
+ Partials 4034 3465 -569
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
Don't know how I missed this in #12878 😓 Looks good. |
Re-do of frappe#12520 Undone by frappe#12878 Changes done to reflect current state of version-13 Co-authored-by: Suraj Shetty <surajshetty3416@gmail.com>
gavindsouza
force-pushed
the
email-signature-fix
branch
2 times, most recently
from
c6fa4e3
to
2d8c9ae
Compare
gavindsouza
force-pushed
the
email-signature-fix
branch
from
2d8c9ae
to
4fa99ed
Compare
This fix adds a signature forcibly if found under the sender's User.email_signature or default outgoing email account's signature field. The previous method of adding a comment into the Email didn't work since Quill would discard comments before setting them. Adding signatures in get_formatted_html didn't seem apt since it's used in QueueBuilder to re-construct the Email before processing the Email Queue. This meant that the email content that was added in the Communication record would not be final. Now, we treat the signature as part of the Communication content.
gavindsouza
force-pushed
the
email-signature-fix
branch
from
4fa99ed
to
0ef99c3
Compare
Seems like this had nothing to do with your PR @sagarvora. At first glance, it did seem so though. The refactor works fine. Sorry about the false alarm. |
This is added to handle purposely removed signature or custom signatures added via the Email Composer or via the email.make API
|
@Mergifyio backport version-13-hotfix |
🟠 Waiting for conditions to match
|
surajshetty3416
requested changes
Mar 7, 2022
✅ Backports have been created
|
mergify bot
added a commit
that referenced
this pull request
Mar 9, 2022
This is a semi-automatic backport of pull request #16178 done by [Mergify](https://mergify.com).
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Quill doesn't deal with HTML for the most part. This means that the comment was being stripped off before it ever reached the backend. I don't know if the
<!-- signature-included -->comment ever worked 🥲.Before
The previous behaviour was to add User's or Email Account's signature via Desk, and if one doesn't exist while processing the Email in the backend add Email Account signature if one exists. Signature added in the backend would not reflect in the Communication or Email Queue records 🥲.
After
The proposed implementation adds a signature (sender's or email account's signature) if one doesn't already exist in the Email content. This behaviour is not triggered when the Email Composer is used via Desk, or via the
email.makeAPI.Re-do of #12520; Undone by #12878