Number of bookmarks (CountBookmarks) in 2.2 set to one or zero?

rbrahmson

While testing the move to 2.2 I noticed that the number of bookmarks for a discussion is either one or zero in the discussion table. We use an internally developed plugin that displays that value (and validated that through the stats box plugin, myadmin sql as well).

Is this a change from 2.1 or is there an issue on my test 2.2.

Unfortunately both of my servers are inside intranet firewall so I can offer outside access.

Also, is there a specific post about 2.2 changes for plugin developers?

R_J

The example you have found uses parameters: it is flexible to accept different categories.
The update you need does not need any variable parameters. So you do not need to do any search&replace.

$prefix = Gdn::database()->DatabasePrefix;
$sql = 'UPDATE '.$prefix.'Discussion d, 
(SELECT ud.DiscussionID, SUM(ud.Bookmarked) AS 'CountBookmarked' 
FROM '.$prefix.'UserDiscussion ud
GROUP BY ud.DiscussionID
) AS ud
SET d.CountBookmarks = ud.CountBookmarked
WHERE d.DiscussionID = ud.DiscussionID

The query builder cannot be used.

R_J

@rbrahmson said:
Hi @R_J : I gave it shot. On phpadmin and the in the code. In phpmyadmin it worked just fine, in the code it did nothing.
It's the first time I am trying sql outside the query builder, so perhaps I am missing something.

For sure. You only build a string and that wouldn't have any effect at all. You need to query the db: $result = Gdn::sql()->query($sql);

R_J

$whatever = ... is just the php language construct for assigning a string to a variable. There are some conventions in Vanilla that you should follow so that your code stays readable. But you could as well call the sql $statement or whatever you like. When I've said that the query builder can not be used, I wanted to say that you cannot build the query. You can use the sql class for querying all the time. There are some sql constructs that cannot be created with the function calls (select(), from(), where(), etc) that you already have seen. But as soon as you have a raw (valid!) sql string, you can use Vanillas classes again to run that sql against the db and use the result like you are used to.

rbrahmson

I can further attest that the UserDiscussion table does contain indicators of bookmarks. In other words, the count of bookmarks in UserDiscussions is correct but in the Discussions Table is either zero or one.

My look at the Vanilla code at github did not help.
@hgtonight -this affects your stats box plugin (on my test 2.2). Any insights?

hgtonight

Looking through the codebase, there is a single function that will update the denormalized column "CountBookmarks" on the discussion table (DiscussionModel::bookmarkDiscussion()). It doesn't appear to be called anywhere!

I am guessing that column is deprecated.

The good news: there is an event you can hook into immediately after a bookmark has been set/cleared and update the column yourself.

public function discussionModel_afterBookmark_handler($sender) {
  $discussionID = $sender->EventArguments['DiscussionID'];
  $count = $sender->BookmarkCount($discussionID);
  $sender->SQL->update('Discussion')
              ->set('CountBookmarks', $count)
              ->where('DiscussionID', $discussionID)
              ->put();
}

There is probably an issue that needs to be filed about this duplicate functionality needing to be removed.

rbrahmson

I actually do see that is is being called (line 2593):

https://github.com/vanilla/vanilla/blob/19075201a695e161576437d93c391e355821403c/applications/vanilla/models/class.discussionmodel.php#L2592-L2619

so it is a mystery.

I did code the hook for that event and inserted a "Gdn::InformMessage($Msg);" where $Msg shows the discussion id, the status, and the count.

What I see is that the numbers are correct on 2.1, but no inform message shows on 2.2. I wonder whether that part of the code is executed.

I will try the suggested code with a small mod:

  $discussionID = $sender->EventArguments['DiscussionID'];
  $count = $sender->BookmarkCount($discussionID);
  if ($count != $discussionID->CountBookmarks)
    $sender->SQL->update('Discussion')
              ->set('CountBookmarks', $count)
              ->where('DiscussionID', $discussionID)
              ->put();
  }
}

hgtonight

@rbrahmson said:
I actually do see that is is being called (line 2593):

https://github.com/vanilla/vanilla/blob/19075201a695e161576437d93c391e355821403c/applications/vanilla/models/class.discussionmodel.php#L2592-L2619

Find me a place in the code where DiscussionModel::bookmarkDiscussion() is called. I couldn't find it, which is what I was referring to.

I will try the suggested code with a small mod:

  $discussionID = $sender->EventArguments['DiscussionID'];
  $count = $sender->BookmarkCount($discussionID);
  if ($count != $discussionID->CountBookmarks)
    $sender->SQL->update('Discussion')
              ->set('CountBookmarks', $count)
              ->where('DiscussionID', $discussionID)
              ->put();
  }
}

Good idea. Use brackets lest Lord Brackos is displeased.

rbrahmson

Hi @hgtonight - You wrote

"Find me a place in the code where DiscussionModel::bookmarkDiscussion() is called."

Reminds me of Archimedes "Give me a place to stand, and I shall move the world." But you are right, it is not called. Which explains why the "AfterBookmarkDiscussion" fireevent doesn't work in 2.2 but works in 2.1(albeit unnecessary for my purpose).

So instead I changed to the "AfterBookmark" event and it works on 2.2. Tested it with your Stats Box plugin as well. All is fine.

Another observation - the if test to see whether the counts need an update is always triggered. I assume that the $Discussion->CountBookmarks does not reflect the SQL update that precedes the Fireevent...

A one-time update of all the discussions in the discussion table is required. I assume this could be done in the "public function Setup()" function of the plugin.

I'm not that proficient in SQL, but I'm pretty sure there is a single SQL statement that could do the update (which should be based on the UserDiscussion table). Is this a correct assumption? Do you know what SQL to use for that?

R_J

That would be the Sql, but I doubt that you can build it with Vanillas query builder

ˋˋˋˋ
Update GDN_Discussion d,
(Select ud.DiscussionID, sum(ud.Bookmarked) as 'CountBookmarked'
From GDN_UserDiscussion ud
Group by ud.DiscussionID
) as ud
Set d.CountBookmarks = ud.CountBookmarked
Where d.DiscussionID = ud.DiscussionID
ˋˋˋ

You should but that in structure(), not in setup() (but of course make the call from setup)

rbrahmson

Hi @R_J - thanks for that SQL. You wrote:

but I doubt that you can build it with Vanillas query builder

But here's what I found on the Github source:

// Update all categories.
            $Sql = "update :_Category c
            left join (
              select
                d.CategoryID,
                coalesce(count(d.DiscussionID), 0) as CountDiscussions,
                coalesce(sum(d.CountComments), 0) as CountComments
              from :_Discussion d
              $Where
              group by d.CategoryID
            ) d
              on c.CategoryID = d.CategoryID
            set
               c.CountDiscussions = coalesce(d.CountDiscussions, 0),
               c.CountComments = coalesce(d.CountComments, 0)";
            $Sql = str_replace(':_', $this->Database->DatabasePrefix, $Sql);
            $this->Database->query($Sql, $Params, 'DiscussionModel_UpdateDiscussionCount');

While it deals with categories it leads me to try to do this on the discussion table using the above technique. GTG right now, will try later. Thanks!

R_J

The example you have found uses parameters: it is flexible to accept different categories.
The update you need does not need any variable parameters. So you do not need to do any search&replace.

$prefix = Gdn::database()->DatabasePrefix;
$sql = 'UPDATE '.$prefix.'Discussion d, 
(SELECT ud.DiscussionID, SUM(ud.Bookmarked) AS 'CountBookmarked' 
FROM '.$prefix.'UserDiscussion ud
GROUP BY ud.DiscussionID
) AS ud
SET d.CountBookmarks = ud.CountBookmarked
WHERE d.DiscussionID = ud.DiscussionID

The query builder cannot be used.

rbrahmson

Thanks! Will try this later this week. I've been working on removing my intranet dependencies from my plugin (which works for 2.1)and once I incorporate and test this I'll publish it and thanks to your and @hgtonight it will be my first 2.2 plugin (the others have only been tested on 2.1). So thank you guys!

rbrahmson

Hi @R_J : I gave it shot. On phpadmin and the in the code. In phpmyadmin it worked just fine, in the code it did nothing.
It's the first time I am trying sql outside the query builder, so perhaps I am missing something. Here is the code snippet:

$prefix = Gdn::database()->DatabasePrefix;
$Statement = "UPDATE ".$prefix."Discussion d, 
                    (SELECT ud.DiscussionID, SUM(ud.Bookmarked) AS 'CountBookmarked' 
                    FROM ".$prefix."UserDiscussion ud
                    GROUP BY ud.DiscussionID
                    ) AS ud
                    SET d.CountBookmarks = ud.CountBookmarked
                    WHERE d.DiscussionID = ud.DiscussionID";
$sql = $Statement;
echo $Statement;//This is the echoed statement I ran successfully in phpmyadmn.

R_J

@rbrahmson said:
Hi @R_J : I gave it shot. On phpadmin and the in the code. In phpmyadmin it worked just fine, in the code it did nothing.
It's the first time I am trying sql outside the query builder, so perhaps I am missing something.

For sure. You only build a string and that wouldn't have any effect at all. You need to query the db: $result = Gdn::sql()->query($sql);

rbrahmson

I looked at some sample code and thought that $sql was a reserved way to issue sql and didn't even think about using Gdn::sql()->query($sql); because of the previous "The query builder cannot be used." comment.
My embarrassing bad...

So thank you and it works now!

R_J

$whatever = ... is just the php language construct for assigning a string to a variable. There are some conventions in Vanilla that you should follow so that your code stays readable. But you could as well call the sql $statement or whatever you like. When I've said that the query builder can not be used, I wanted to say that you cannot build the query. You can use the sql class for querying all the time. There are some sql constructs that cannot be created with the function calls (select(), from(), where(), etc) that you already have seen. But as soon as you have a raw (valid!) sql string, you can use Vanillas classes again to run that sql against the db and use the result like you are used to.

rbrahmson

Thanks! In my last testing phase before I upload the plugin.