Fix reported bug about replies to self
ClosedPublic
Actions

Authored by gjditchfield on Sep 17 2019, 8:20 PM.

Details

Reviewers

mlaurent
knauss

Group Reviewers

KDE PIM

Commits

R94:e672edad5849: Fix reported bug about replies to self

Summary

Normally, replies are sent to the sender of the original message.
However, if the KMail user is the sender of the original message, KMail
sends the reply to the recipient of the original message, on the assumption
that the reply is meant to add information to the original.

Bugs 301449, 366356, and 392433 point out that this is not ideal if the
sender and recipient are two different identities of the KMail user. If
the user sends mail from "work.id@example.com" to "home.id@example.org",
the reply should go from "home.id@example.org" to "work.id@example.com".

Bug: 366356
Bug: 392433
Bug: 301449

Diff Detail

Repository

R94 PIM: Message Library

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

gjditchfield created this revision.Sep 17 2019, 8:20 PM

Restricted Application added a project: KDE PIM. · View Herald TranscriptSep 17 2019, 8:20 PM

Restricted Application added a subscriber: kde-pim. · View Herald Transcript

gjditchfield requested review of this revision.Sep 17 2019, 8:20 PM

Harbormaster completed remote builds in B16703: Diff 66334.Sep 17 2019, 8:20 PM

knauss added a subscriber: knauss.Sep 17 2019, 9:12 PM

knauss added inline comments.

messagecomposer/autotests/replystrategytest.cpp
114	this only works on linux and please do not delete the complete `.qttest` dir, only the files that are problematic : QStandardPaths::writableLocation(QStandardPaths::GenericConfigLocation)+QDir::separator() + QStringLiteral("emailidentities");
176	wait you have a original mail from `default -> friend2` and reply smart created a mail `default -> friend1`? Shouldn't it be : << (int)ReplySmart << defaultAddress << only(friend2Address) << nobody;
180	friend1Address vs. friend2Address
252	friend1 vs friend2
256	friend1 vs friend2
messagecomposer/src/helper/messagefactoryng.cpp
158–159	rename the current `toList` into `fromList` and create the variable `toList=m_origMsg->to()->mailboxes()`

knauss added a reviewer: KDE PIM.Sep 17 2019, 9:12 PM

mlaurent requested changes to this revision.Sep 18 2019, 4:49 AM

mlaurent added a subscriber: mlaurent.

mlaurent added inline comments.

messagecomposer/autotests/replystrategytest.cpp
47	const QString &address
52	const QString &...
61	const QString & , const QStringList &
82	const QStringList &
97	coding style QObjet *parent
140	const KMime::Message::Ptr &

This revision now requires changes to proceed.Sep 18 2019, 4:49 AM

In ReplyStrategyTest:

initTestCase() deletes emailidentities and emaildefaults, not .qttest.
uses reference parameters.
corrected coding style.

In messagefactoryng.cpp: better local variable names.

Harbormaster completed remote builds in B16749: Diff 66402.Sep 18 2019, 6:14 PM

@gjditchfield okay my comments about friend1 vs friend2 were wrong - I overseen, that the mails were sent by friend1 to me. So the tests are fine.

gjditchfield marked 12 inline comments as done.Sep 25 2019, 4:22 PM

gjditchfield added inline comments.

messagecomposer/autotests/replystrategytest.cpp
114	OK. Is emaildefaults also problematic?
176	This a "normal mail" case: from friend1, to the user's default identity, and Cc'd to friend2. So the reply goes from the default identity to friend1, with no Cc. (The test case for the bug in question is down at line 196.)
180	Here the original is from friend1, to the non-default identity (and also to friend2), so the reply goes from non-default to friend1 (and friend2 is not added to Cc.)
252	As above.
256	As above.

gjditchfield marked 10 inline comments as done.Sep 27 2019, 3:21 PM

Btw I already accepted your patch, so feel free to push. Or don't you have commit access by now?

messagecomposer/autotests/replystrategytest.cpp
114	Well problematic is only that you used a pure linux specific path in your first version. It is totally fine, that you need to delete two files.

No, I don't have commit access, and given my likely commit rate and weak understanding of your work flow (should I wait for mlaurent?), I'm comfortable with that. Whatever keeps your workload lowest...

This revision was not accepted when it landed; it landed in state Needs Review.Sep 28 2019, 11:03 PM

Closed by commit R94:e672edad5849: Fix reported bug about replies to self (authored by gjditchfield, committed by knauss). · Explain Why

This revision was automatically updated to reflect the committed changes.

In D24035#539341, @gjditchfield wrote:

No, I don't have commit access, and given my likely commit rate and weak understanding of your work flow (should I wait for mlaurent?), I'm comfortable with that. Whatever keeps your workload lowest...

Well our policy is, that everything should be reviewed, but in the end you also commit directly. In your case here all changes by mlaurent were solved and I reviewed them. But I can understand that it is not obvious what is the correct workflow. But this is also because Phabricator is hard to treat...

Well in the end you now have submitted already 10 patches, feel free to ask for commit access (as far as I remember you need two upvotes for this - one from my side).

Revision Contents
Changeset List

		Path
M		messagecomposer/autotests/CMakeLists.txt (1 line)
A	M	messagecomposer/autotests/replystrategytest.h (55 lines)
A	M	messagecomposer/autotests/replystrategytest.cpp (364 lines)
M		messagecomposer/src/helper/messagefactoryng.cpp (22 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	66334	f54a27f		Sep 17 2019, 8:20 PM	★	★
Diff 2	66402	f54a27f	In ReplyStrategyTest:	Sep 18 2019, 6:14 PM	★	★
Diff 3	67011	4c3c89f	R94:e672edad5849bf6d079d3277dda147f8aae1f39d	Sep 28 2019, 11:03 PM	★	★