shadow forum topics broken by taxonomy revisions

Taxonomy terms are now tied to revisions, looks like shadow forum topics got missed out when that patch went in. I just set one forum module bug to critical, I'll let someone else decide if this one is.

http://drupal.org/node/115667

Promoting to critical as it prevents the forums module working correctly.

catch: so shadown forum topic relations should also be tied to node revisions?

Gabor: well, it seems like overkill, but I can't think of another solution that'd be consistent.

First approach, two things here but I don't know if this is the intended functionality:

a) Correct the summary page to show the count of topics including the shadows.
b) Get all the nodes related with the forum, with shadows, but force distinct to show last revision.

Incorrect posts count.

Corrected posts count on forums page.
Modified forums block to show only revisions.

This patch applies cleanly to HEAD, marking RTBC as it appears to have fixed the issue.

birdmanx35: did you actually test how this works? From the looks, there was a SUM() lost in the patch, which I don't see why, and some DISTINCT got in which would have been good to provide a reason for (I assume there might be different versions for the taxonomy terms, so we need to filter out node duplicates??)

The reasons I have found to modify these querys:

a) The SUM() is lost because, as I understand, node_comment_statistics is already a SUM() of comments to a particular node, so now that term_data is driven by revisions the nid is repeated and the sumatory redundant.
Same reason to adding a COUNT(DISTINC n.nid) the node id is repeated.

b) The DISTINCT keyword on forum_get_topics was added to show shadows joining by n.nid and, at the same time, filtering the results to get only one nid (and not all the revisions) in the active forum topic.

As I said in #7, I don't know if this is the intended functionality, so I'll try to bring some forum savvy people to this issue.

Of course we begin to embark on a very dangerous path here, namely why is not node_comment_statistics revisioned? Are the comments relevant to the new version...? Hmmm, hmmm

Well comments are not versioned at all, so they are supposed to be relevant for all versions, I think.

Anyways that should not derail this patch, test please.

From my testing, the patch works as advertised. Only found one problem. When you create a revision of a shadow copied post, and unset the shadow copy, the shadow copy will still be there. It will also still be counted in the forum topics overview.

The problem I think is that in the queries in forum_get_topics() and forum_get_forums() a inner join on term_node is done on nid, while the shadow copy is 'revision sensitive' and it should in fact be done on vid. I added a patch which basically is the patch of comment #8 by theborg but (hopefully) fixes the shadow copy problem. Test please.

This works as advertised although I'm not convinced that shadow forum topics should be counted in forum_get_forums() at all - to me it's simply a convenient way to find something that's moved and shouldn't affect the counts. It's a real shame adding that DISTINCT to what is already a very expensive query. I'm not going to argue on that though - refactoring those queriesa and/or changing behaviour is a job for D7, so otherwise this looks RTBC.

[12:05pm] goba: drupalborg_w: my possible concern is that IMHO the addition of DISTINCTs makes the queries ignore shadow copies completely
[12:05pm] goba: drupalborg_w: since shadow copies get those duplicates there in the first place, right?

Generally, before the patch, there was no DISTINCT, so shadow copies were counted as separate forum topics if I understand it right. With the patch we join on revision id (which is the bug to fix), so we should still get only one relation per node unless we have a shadow copy as well, right? So the DISTINCT only rules out counting the shadow copy. Or am I misdirected here?

After a conversation with Gábor on #drupal about this issue.

a) Join all querys by vid (version id).
b) Count node revisions and sum all comments by term_id.
c) Remove DISTINCT from forum_topics, the shadows are lost when the node is revisioned.

Since we took the last hour reviewing the patch bit per bit, now it looks fine to me :) Could do with another testing though. To test:

- set up at least two forums
- add forum topics to each of them (at least two to each)
- add a comment on each of them
- edit one or two forum topics with revisions turned on

Before patch, you should see that the topic counts and comment counts are off. After patch, you should see that the topic and comment counts are fine.

This is my one node with lots of revisions between three forums. Nothing else in the DB.

I tested with and without the DISTINCT, (patch applied for convenience). Attached three screenshots including unpatched. As you can see using vid fixes the issue by itself, so it seems redundant to have the DISTINCT there, although it's not making any difference to the listing at all - not hiding the shadow, but not necessary to fix the bug either.

Screenshots don't includ the forums blocks, but I tested those, and again the DISTINCT is redundant - both the forum topic and the shadow appear in both (and multiples appear unpatched).

er, crossposted, forgot to add comments, and more than one forum topic. oops! took out the screenshots and the patch to avoid muddying the issue, will test the last one on here.

OK so with two nodes and some revisions, each with one comment.

#18: both the node and the comments for shadows are counted correctly (i.e. node + comments are counted in the forum topic listing)

#20: the behaviour with and without DISTINCT was the same, only the shadow node was counted, not the comments for the node. We could've happily lost the DISTINCT, but the comment count would still have been off (unless you consider the comments not to be in that forum even thought the shadow forum topic is but meh).

So theborg's patch in #18 looks right, marking RTBC for that.

Retested and works for me on #18. Attached a before/after screenshoot.

catch: from the looks of the patch you submitted, you arrived on the same conclusion as well. So committed #18. Thanks for testing.