Please upgrade here. These earlier versions are no longer being updated and have security issues.
HackerOne users: Testing against this community violates our program's Terms of Service and will result in your bounty being denied.
Options

Broken with mysql strict mode

fcheslackfcheslack New
edited February 2016 in Vanilla 2.0 - 2.8

Vanilla now supports mysql strict mode in 2.2. BetterNotifications does not because it is passing a bool to a tinyint resulting in an error.

Gdn_ErrorException: Incorrect integer value: '' for column 'Notified'

It looks like this was supposed to be addressed in the SetNotified function, by using $FlagValue, but after it is set, it never gets used.
No more error thrown if you change
->Set('Notified', $Notified)
to
->Set('Notified', $FlagValue)

I'd submit a pull request, but https://github.com/jamesinc/BetterNotifications is empty

related: https://github.com/vanilla/vanilla/issues/3507

Comments

  • Options
    hgtonighthgtonight ∞ · New Moderator

    This got caught in the spam filter.

    Sorry.

    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.

  • Options
    jamesincjamesinc Sydney ✭✭

    Hey, I've just seen this. Let me get the github repo working. I'm looking at this plugin anyway as I've noticed a 2.2 compatibility issue.

  • Options
    jamesincjamesinc Sydney ✭✭

    Okay, sorry about the repo not being there. I think I threw this plugin together in a day or two and never pushed it to origin. Unfortunately I have no idea where the original repo was.

    It's now here: https://github.com/jamesinc/better-notifications/

  • Options
    jamesincjamesinc Sydney ✭✭

    @fcheslack BetterNotifications 2.0.3 has now been released, and addresses the issue you raised.

  • Options

    @jamesinc Awesome. Thanks. And thanks for putting the license in there too. I had just put a copy up on github myself and was feeling a little uneasy about doing so without there being a license specified.

    Can I also suggest that the description be updated? It says it's only for bookmarked discussions but (as far as I can tell) it works for any comment notifications. That's exactly the behaviour I was looking for, but I had to check the code to make sure it actually did what I wanted.

  • Options
    jamesincjamesinc Sydney ✭✭

    @fcheslack I can't remember off the top of my head how comment notifications are organised. If I recall, for my use case, users were receiving notifications for bookmarked threads. Any thread that they had started or commented in was bookmarked automatically (using a plugin called AutoBookmark I think).

    But yes, I suppose if you set your notification preferences to maximum verbosity, you'd be receiving notifications for comments in all threads, and the plugin would quash those too.

    I'll update it all over the weekend and make it more release-y.

  • Options

    Yeah, what I meant was all comments you'd normally be notified for, however a user set those preferences.

  • Options

    Compatible with 2.2? Thanks!

Sign In or Register to comment.