Please upgrade here. These earlier versions are no longer being updated and have security issues.

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

rbrahmsonrbrahmson "You may say I'm a dreamer / But I'm not the only one"NY ✭✭✭

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?

Comments

  • rbrahmsonrbrahmson "You may say I'm a dreamer / But I'm not the only one" NY ✭✭✭

    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?

  • hgtonighthgtonight ∞ · New Moderator

    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.

    Search first

    Check out the Documentation! We are always looking for new content and pull requests.

    Click on insightful, awesome, and funny reactions to thank community volunteers for their valuable posts.

    rbrahmson
  • rbrahmsonrbrahmson "You may say I'm a dreamer / But I'm not the only one" NY ✭✭✭
    edited April 2016

    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();
      }
    }
    
  • hgtonighthgtonight ∞ · New Moderator
    edited April 2016

    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.

    Search first

    Check out the Documentation! We are always looking for new content and pull requests.

    Click on insightful, awesome, and funny reactions to thank community volunteers for their valuable posts.

    rbrahmsonR_J
  • rbrahmsonrbrahmson "You may say I'm a dreamer / But I'm not the only one" NY ✭✭✭
    edited April 2016

    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?

    maxyaeger
  • R_JR_J Cheerleader & Troubleshooter Munich Moderator

    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)


  • rbrahmsonrbrahmson "You may say I'm a dreamer / But I'm not the only one" NY ✭✭✭

    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!

  • rbrahmsonrbrahmson "You may say I'm a dreamer / But I'm not the only one" NY ✭✭✭

    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!

  • rbrahmsonrbrahmson "You may say I'm a dreamer / But I'm not the only one" NY ✭✭✭

    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_JR_J Cheerleader & Troubleshooter Munich Moderator

    @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);


    hgtonightrbrahmson
  • rbrahmsonrbrahmson "You may say I'm a dreamer / But I'm not the only one" NY ✭✭✭

    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_JR_J Cheerleader & Troubleshooter Munich Moderator

    $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.


    rbrahmsonhgtonight
  • rbrahmsonrbrahmson "You may say I'm a dreamer / But I'm not the only one" NY ✭✭✭

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

    R_Jhgtonight
Sign In or Register to comment.