Message sent to oneself breaks new/#replies data loading

OK, the second way of fixing this is rather simple: In _privatemsg_send() we gather the list of {pm_index} records into an array first, so that duplicates collapse into single records nicely. The author is added first, so whenever he is one of recipients too, he gets the record as unread message (that's what he surely expects, when sending to himself).

Note, that incorrect number of replies on listing also occured, when the recipient was specified twice - although privatemsg UI seems to avoid that, it's still possible to pass a recipients list with duplicates for example to privatemsg_new_thread() [I tested, it really triggered the wrong-number-of-replies problem on recipient's list]. So, this patch also makes _privatemsg_send() more robust regarding duplicates.

Two patches on this thread: Both fix the problem. The one attached to this comment avoids saving duplicates to {pm_index}, while the previous one deals with the consequences, if there were such duplicates already. We might want to commit one or the other, or both for a really fool-proof fix.

On second look, I would vote for committing BOTH these fixes, as one fix the consequences of having duplicates (which is good to have, for existing users, but not only), and the other fix the case where duplicates are created (which is not nice regardless the immediate outcome). Leaving to others, too.

Except that this is rolled against current 1.x-dev (clicked a wrong row first time) ;-)

I can't see how having two *identical* [after setting as already read] rows is supposed to make sense - I guess it's more of previously untested/unseen edge case. Just thinking out loud, I wasn't there when that code got written ;-)

Anyway, from my angle of view, I see no need to discuss this at lengths (feel free to disagree). Both the approaches are basically fine for me, so if there are concerns about the duplicates-removal change, we can just fix the queries per my initial post. I would be happy to see this fixed either way, and I have absolutely no intention to make this a lengthy/depressing thread. :-)

That change fixed the second part of the bug, which is: Duplicate records in the index cause more replies to be shown in overview, i.e. a completely new thread (no replies) sent to oneself is shown as having 1 reply. That's similar to "forever unrerad" part, as it's caused by the same problem (duplicate entries), and results in broken display on overview screen.

The query didn't count threads as such - it's a query grouped by threads to retrieve single rows for the overview [threads] list, retrieving message counts inside these groups [inside overview table rows, that is]. Previously, it only just counted [message-oriented] rows returned [grouped] by the query; instead of COUNT(pmi.thread_id), we might use just any of the columns retrieved by that query, with pretty much the same effect. But however, when we have duplicate records for some messages, we need to eliminate these - and that's COUNT(DISTINCT pmi.mid), which only just removes duplicates from message count, without really changing anything else. COUNT(DISTINCT pmi.thread_id) won't work, since the thread ID is the same for all records in each group, so we'll get always a count of 1.

If we can't use the query fix for some reason (which I believe is not the case), we need to go with #1 instead.

Or nbz (most probably) meant the patch in the initial post to be used - the one in #1 is the other of my two approaches (although it works for me too, only in a different way).

I tested #15 on my setup:

- Patch is rolled with Windows CR/LF line endings, and so completely fails to apply on my environment (Linux command-line patch utility). But however, I've re-written all the line endings in patch file manually in a text editor, and proceeded to further testing.
- Patch then applied to latest (a few minutes ago downloaded) privatemsg-6.x-1.x-dev.tar.gz with offset -22 lines. No big deal, I guess.
- Code looks good, and works fine for me.

So I would say this is RTBC as far as I'm concerned, except that it was my own patch initially and I had problems to make it apply - so others should decide on that part.

My end, too:

$ patch -v
patch 2.5.9

I tried --ignore-whitespace switch (which is really the only one shown by --help, that seems even remotely relevant, but no luck). Weird, but not really a problem for the actual code in the patch.

No time to actually apply&test the patch today (I'll be back on Tuesday), but from quick reading - I guess this will work, but it feels weird to me to have a mixture of both the approaches (fix one part in loading query, and other part at saving time). That's just a feeling, though.

Otherwise, I see two points to discuss:
- As #4 mentions, this won't fix existing data (already existing "forever unread" messages)
- Why insert a row with "new", and then update it - shouldn't we just insert the correct one first time?

Also, if I send a message to myself (as a new user exploring features, or just wanting to create a "reminder to myself" message), I most probably expect it to show as unread, until I really view it. Although I certainly did read the message while typing/submitting it, having it shown as "read" without visiting the thread might be a bit unexpected behavior.

cross-posted, sorry...

#22: I meant a bit different thing: Inserting all the recipients rows as 'unread', and then immediately update one of these to 'read' is unneeded extra query (although it's not run frequently, I admit). So I thought it might be better to add a condition inside the loop, so that the 'recipient' row for the author is inserted without the new flag initially.

See also #23, though.

I would say yes, mark as unread. It would be more of an expected behavior, and thinking of it, having BOTH the rows unread should avoid the problem all the same. It's also simpler code, just changing the status on the extra row for the author.

But however, I smell a risk about the unread messages count - I'm not sure, but having TWO unread rows might show two unread messages in the title? (I didn't test)

I'm back from my short holidays, so I tested the new dev version. I have a few concerns about this patch:

- The update function 6003 lacks { } around table names (three instances), so it fails horribly on prefixed tables.

- The code comment on adding the extra index record for the author shouldn't mention "set column new to 0" anymore, as it's not true now. BTW, there's also a typo ("tha author") and double-space on that line, and the new comment above should have a dot at the end.

I have no time to roll a patch now (my deadline is too close, so switching to "do only what's absolutely necessary for the site" mode), but still I consider this "needs work" because of the update function failure.

Otherwise, everything works fine on my test install, so I confirm the bug being fixed, except the above.