Issue: Recent Discussions Module doesn't show author

mtschirs

Hi,

I noticed that the recent discussions module (Vanilla 2.2b1) doesn't show the author's name until a first comment has been made. This is due to the $Discussion->LastName being empty for discussions without comments even though $Discussion->LastUserID is always set. This seems to be either inconsistent or erroneous behavior.

Here the relevant member fields for a discussion without comments:

[FirstUserID] => 2
[FirstName] => Admin
[FirstDate] => 2015-07-09 21:12:10

[LastUserID] => 2
[LastName] => 
[LastDate] => 2015-07-09 21:12:10

I didn't create a github issue for this since I first want to hear your feedback. Could this be fixed without touching the Vanilla core?

R_J

It's a great finding and I think that must be fixed in Vanilla, not in the module.

And I also think you should also file a pull request

mtschirs

I tried to fix it - and I really tried hard! - but there are so many inconsistencies... the codebase dealing with discussions and the discussion model seems to have become really messy over the time

First off, a new discussion will have (in the database) a valid DateLastComment field, but LastCommentID, FirstCommentID and LastCommentUserID will be NULL.

Now, what happens when we are querying for a discussion via DiscussionModel::get()?

First off, this method asks for a discussion summary via DiscussionModel::discussionSummaryQuery(). That method queries the InsertUserID as FirstUserID and LastCommentUserID as LastUserID from the database and then tries to join additional user data to the LastUserID such as LastName and LastPhoto, but since LastCommentUserID is NULL the join has no effect.

Then, back in DiscussionModel::get(), it tries - again - to join in additional user data via Gdn::userModel()->joinUsers($Data, array('FirstUserID', 'LastUserID'));. However, since LastCommentUserID = LastUserID = NULL still holds, the join has no effect.

Finally, a call to DiscussionModel::AddDiscussionColumns($Data); causes the undocumented DiscussionModel::Calculate() method to be called which attempts to "fix" the problem of the still missing LastUser ID via the following code:

// Add some legacy calculated columns.
if (!property_exists($Discussion, 'FirstUserID')) {
  $Discussion->FirstUserID = $Discussion->InsertUserID;
  $Discussion->FirstDate = $Discussion->DateInserted;
  $Discussion->LastUserID = $Discussion->LastCommentUserID;
  $Discussion->LastDate = $Discussion->DateLastComment;
}

So, in the end, after two failed tries to join in the name and additional information to the LastCommentUserID / LastUserID (which are NULL), a piece of code officially referred to as "Add some legacy calculated columns" fixes half of the problem (LastUserID and LastDate) while leaving the other half unfixed (LastName etc.).

This results in the user facing bug of showing an empty <a href="/profile/"></a> link instead of linking to the discussion author for uncommented discussions.

In my opinion, all the fields refering to the last comment should be NULL for a newly created discussion.

However, trying to untangle this mess will be very hard for an outsider since such 'fixes' for this inconsistency can be found all over the place, e.g. in the undocumented DiscussionModel::counts() method.

How to proceed?

Bleistivt

In my opinion, all the fields refering to the last comment should be NULL for a newly created discussion.

Sorting by DateLastComment would yield unexpected results if it was null. Which is most likely the reason why it is always set.

Fixing WriteModuleDiscussion to not use these legacy columns anymore seems to be the most reasonable thing to do:

https://github.com/vanilla/vanilla/blob/fd5346/applications/vanilla/views/modules/helper_functions.php#L4-L30

mtschirs

Just created a pull request: https://github.com/vanilla/vanilla/pull/2909

I would be happy if someone else could test it. I completely stripped out the legacy columns and fixed the few views that where still referring to them.