Follow Plugin 👨

OK, I’ve gone ahead and built the required logic to give users more discretion on what notifications they receive.

Also, the global setting now switches all follow notifications on or off for all users.


(default: ON)

If this is switched ON, user gets discretion:


(all default: ON).

It’s currently a PR to myself:

https://github.com/paviliondev/discourse-follow/pull/10

I’m currently testing it on one of my sites.

You can test it if you are brave :sweat_smile: :

- git clone https://github.com/paviliondev/discourse-follow -b add_granular_notifcation_settings

This should result in a lot less notification spam for people (so long as you tell them they can switch it off).

@buildthomas with these new additions I’ve switched things around a little so settings are ‘positive’ rather than ‘negative’ (aka enable versus disable). The global setting in admin now impacts all follow notifications. Once this is merged, be sure to let your users know to update their settings as I don’t intend to provide a migration.

8 Likes

FYI This was merged recently after I had some Production time with it. Let me know if you have any issues however.

4 Likes

@merefield idk if you’ve encountered this, but after installing the version with the optional notifications, my forum’s users started having problems with old notifications repeating. They do register that you’ve clicked them at first, but then after a while, those same notifications come back and this cycle then repeats endlessly. I’ve uninstalled and reinstalled a few times now using both the traditional method and using the ProCourse plugin installer, and it only occurs when this plugin is installed.

4 Likes

Thanks for the report. I’ll investigate.

3 Likes

Thanks a million, it’s really unfortunate because otherwise, this plugin is straight up fantastic.

3 Likes

OK I found a bug that caused some PostAlert jobs to fail, causing perpetual retries and I believe the observed user experience of intermittent spam.

That probably caused notifications to pop up with every retry. I could see dozens of attempts for each failed post notification in Production. The problem was, some work was getting done anyway but the Job was getting marked as failed.

https://github.com/paviliondev/discourse-follow/commit/d20a90663465d13ecab1c338206bc115fbb3181f

Please upgrade this plugin asap (please do not peform a full rebuild if possible if using other Pavilion plugins in case of compatibility issues with other plugins and core, as we are outside the support period).

Once you’ve applied the update the failed jobs should get retried and they will now leave the retry queue, hopefully never to return! :gun: :cowboy_hat_face:

As the retried jobs tick out successfully, there will be a last round of notification spam.

@b481 hopefully this addresses your issue fully

6 Likes

Has this been fixed or this is still a bug in this plugin, if this is fixed i can try this, else i will not install it as it will create orphan notifications and break my users UI if i uninstall the plugin, @angus

2 Likes

This is not a bug. If you uninstall a plugin there is no code to run. Use the script provided for time being.

We may provide admin UI to delete the notifications within the plugin some time in the future so you can do that before uninstalling the plugin.

Btw please do not @ people, especially at the weekend. That’s bad etiquette.

3 Likes

This plugins has many bugs.

  • when disable the plugin all profile pages show 404 page.
  • after delete disable, notifications not showing.
    • Javascript gives this error in console: Uncaught TypeError: Cannot read property ‘dasherize’ of undefined
  • when plugin is active old notificatians appears again and again.

The code and the discourse system are at last version.

No, it does not.

This bug was fixed as notified above. Please update to latest tests-passed and the latest plugin version and provide detailed steps to reproduce.

This is noted in OP and discussed in the Topic. There is a script to run if you remove or disable the plugin. Please read this before posting.

So the only unresolved one I wasn’t aware of is this:

I’ll take a look

2 Likes

https://github.com/paviliondev/discourse-follow/commit/0290c428210199e35972e17746fefa8c67f24070

Please let me know if that resolves it for you.

2 Likes

I think this fix introduced some new issue or did not fully fix the nil-indexing issue.

We’re seeing tens of thousands of errors related to this on our 400k-user Discourse forum:

Job exception: undefined method `push' for nil:NilClass

We’re also seeing this one thrown much less often, also by this plugin:

Job exception: Couldn't find User with 'id'=xxx

image

We have this version of the plugin installed (the commit before your disabled setting profile fix): https://github.com/paviliondev/discourse-follow/tree/d20a90663465d13ecab1c338206bc115fbb3181f

As far as I see, the latest version (the 1 newer commit) doesn’t change anything about the code that is causing errors below.

Stack trace of “undefined method push” error:

/var/www/discourse/plugins/discourse-follow/plugin.rb:165:in `block in author_replied_followers'
/var/www/discourse/plugins/discourse-follow/plugin.rb:161:in `each'
/var/www/discourse/plugins/discourse-follow/plugin.rb:161:in `reduce'
/var/www/discourse/plugins/discourse-follow/plugin.rb:161:in `author_replied_followers'
/var/www/discourse/plugins/discourse-follow/plugin.rb:148:in `after_save_post'
/var/www/discourse/plugins/post-approval/plugin.rb:162:in `after_save_post'
/var/www/discourse/app/jobs/regular/post_alert.rb:11:in `execute'
/var/www/discourse/app/jobs/base.rb:232:in `block (2 levels) in perform'
rails_multisite-2.5.0/lib/rails_multisite/connection_management.rb:76:in `with_connection'
/var/www/discourse/app/jobs/base.rb:221:in `block in perform'
/var/www/discourse/app/jobs/base.rb:217:in `each'
/var/www/discourse/app/jobs/base.rb:217:in `perform'
sidekiq-6.1.2/lib/sidekiq/processor.rb:196:in `execute_job'
sidekiq-6.1.2/lib/sidekiq/processor.rb:164:in `block (2 levels) in process'
sidekiq-6.1.2/lib/sidekiq/middleware/chain.rb:138:in `block in invoke'
/var/www/discourse/lib/sidekiq/pausable.rb:138:in `call'
sidekiq-6.1.2/lib/sidekiq/middleware/chain.rb:140:in `block in invoke'
sidekiq-6.1.2/lib/sidekiq/middleware/chain.rb:143:in `invoke'
sidekiq-6.1.2/lib/sidekiq/processor.rb:163:in `block in process'
sidekiq-6.1.2/lib/sidekiq/processor.rb:136:in `block (6 levels) in dispatch'
sidekiq-6.1.2/lib/sidekiq/job_retry.rb:111:in `local'
sidekiq-6.1.2/lib/sidekiq/processor.rb:135:in `block (5 levels) in dispatch'
sidekiq-6.1.2/lib/sidekiq.rb:38:in `block in <module:Sidekiq>'
sidekiq-6.1.2/lib/sidekiq/processor.rb:131:in `block (4 levels) in dispatch'
sidekiq-6.1.2/lib/sidekiq/processor.rb:257:in `stats'
sidekiq-6.1.2/lib/sidekiq/processor.rb:126:in `block (3 levels) in dispatch'
sidekiq-6.1.2/lib/sidekiq/job_logger.rb:13:in `call'
sidekiq-6.1.2/lib/sidekiq/processor.rb:125:in `block (2 levels) in dispatch'
sidekiq-6.1.2/lib/sidekiq/job_retry.rb:78:in `global'
sidekiq-6.1.2/lib/sidekiq/processor.rb:124:in `block in dispatch'
sidekiq-6.1.2/lib/sidekiq/logger.rb:10:in `with'
sidekiq-6.1.2/lib/sidekiq/job_logger.rb:33:in `prepare'
sidekiq-6.1.2/lib/sidekiq/processor.rb:123:in `dispatch'
sidekiq-6.1.2/lib/sidekiq/processor.rb:162:in `process'
sidekiq-6.1.2/lib/sidekiq/processor.rb:78:in `process_one'
sidekiq-6.1.2/lib/sidekiq/processor.rb:68:in `run'
sidekiq-6.1.2/lib/sidekiq/util.rb:15:in `watchdog'
sidekiq-6.1.2/lib/sidekiq/util.rb:24:in `block in safe_thread'

Stack trace of “couldn’t find user” error:

activerecord-6.0.3.3/lib/active_record/core.rb:177:in `find'
/var/www/discourse/plugins/discourse-follow/plugin.rb:162:in `block in author_replied_followers'
/var/www/discourse/plugins/discourse-follow/plugin.rb:161:in `each'
/var/www/discourse/plugins/discourse-follow/plugin.rb:161:in `reduce'
/var/www/discourse/plugins/discourse-follow/plugin.rb:161:in `author_replied_followers'
/var/www/discourse/plugins/discourse-follow/plugin.rb:148:in `after_save_post'
/var/www/discourse/plugins/post-approval/plugin.rb:162:in `after_save_post'
/var/www/discourse/app/jobs/regular/post_alert.rb:11:in `execute'
/var/www/discourse/app/jobs/base.rb:232:in `block (2 levels) in perform'
rails_multisite-2.5.0/lib/rails_multisite/connection_management.rb:76:in `with_connection'
/var/www/discourse/app/jobs/base.rb:221:in `block in perform'
/var/www/discourse/app/jobs/base.rb:217:in `each'
/var/www/discourse/app/jobs/base.rb:217:in `perform'
sidekiq-6.1.2/lib/sidekiq/processor.rb:196:in `execute_job'
sidekiq-6.1.2/lib/sidekiq/processor.rb:164:in `block (2 levels) in process'
sidekiq-6.1.2/lib/sidekiq/middleware/chain.rb:138:in `block in invoke'
/var/www/discourse/lib/sidekiq/pausable.rb:138:in `call'
sidekiq-6.1.2/lib/sidekiq/middleware/chain.rb:140:in `block in invoke'
sidekiq-6.1.2/lib/sidekiq/middleware/chain.rb:143:in `invoke'
sidekiq-6.1.2/lib/sidekiq/processor.rb:163:in `block in process'
sidekiq-6.1.2/lib/sidekiq/processor.rb:136:in `block (6 levels) in dispatch'
sidekiq-6.1.2/lib/sidekiq/job_retry.rb:111:in `local'
sidekiq-6.1.2/lib/sidekiq/processor.rb:135:in `block (5 levels) in dispatch'
sidekiq-6.1.2/lib/sidekiq.rb:38:in `block in <module:Sidekiq>'
sidekiq-6.1.2/lib/sidekiq/processor.rb:131:in `block (4 levels) in dispatch'
sidekiq-6.1.2/lib/sidekiq/processor.rb:257:in `stats'
sidekiq-6.1.2/lib/sidekiq/processor.rb:126:in `block (3 levels) in dispatch'
sidekiq-6.1.2/lib/sidekiq/job_logger.rb:13:in `call'
sidekiq-6.1.2/lib/sidekiq/processor.rb:125:in `block (2 levels) in dispatch'
sidekiq-6.1.2/lib/sidekiq/job_retry.rb:78:in `global'
sidekiq-6.1.2/lib/sidekiq/processor.rb:124:in `block in dispatch'
sidekiq-6.1.2/lib/sidekiq/logger.rb:10:in `with'
sidekiq-6.1.2/lib/sidekiq/job_logger.rb:33:in `prepare'
sidekiq-6.1.2/lib/sidekiq/processor.rb:123:in `dispatch'
sidekiq-6.1.2/lib/sidekiq/processor.rb:162:in `process'
sidekiq-6.1.2/lib/sidekiq/processor.rb:78:in `process_one'
sidekiq-6.1.2/lib/sidekiq/processor.rb:68:in `run'
sidekiq-6.1.2/lib/sidekiq/util.rb:15:in `watchdog'
sidekiq-6.1.2/lib/sidekiq/util.rb:24:in `block in safe_thread'

This is causing significant disruption of our distributed forum deployment because it is retrying PostAlert jobs all the time and sending many duplicate notifications to users (because the job only partially fails, some users keep getting re-notified for the same posts).

2 Likes

I can’t support older commits :smiley:

Please update to latest and give it a whirl.

I’m not seeing any such issue.

That said, something might be going on here:

    def author_replied_followers(post)
      User.find(post.user_id).followers.reduce([]) do |users, user_id|
        user = User.find(user_id).notify_me_when_followed_replies ? User.find(user_id) : nil
        following = user ? user.following.select { |data| data[0] == post.user_id } : nil
        if following && following.last.to_i == Follow::Notification.levels[:watching]
          users.push(user)
        end
      end
    end
1 Like

My bad – just checked and we are running the latest version since a week, and the issues are still occurring (with either version, which makes sense because the latest commit doesn’t touch this code and only adds some enabled checks).

So the issue occurs with latest.

2 Likes

Cool, I’ll take a look.

It’s clear to me already user can be set to nil yet later on this is used without a check for nil, which doesn’t look right.

EDIT: no, its not that - I jumped the gun - looks like users is nil …

2 Likes

From a quick inspection it looks like the reduce isn’t set up properly – it is meant to return the updated value, but right now it only returns non-nil when it adds a user (passes all the checks/if).

If it doesn’t pass that if statement it will set nil as the reduced value, which subsequently errors on the next iteration of the run.

This demonstrates what happens:

[1, 2, 3].reduce(0) { |sum, n|
    if n == 3
        sum + n
    end
}

iterations:

  1. sum = 0, n = 1, returns 1
  2. sum = 1, n = 2, returns 3
  3. sum = 3, n = 3, doesn’t pass the if so returns nil
  4. sum = nil, n = 4, throws
undefined method `+' for nil:NilClass (NoMethodError)

I think the fix is something like:

    def author_replied_followers(post)
      User.find(post.user_id).followers.reduce([]) do |users, user_id|
        user = User.find(user_id).notify_me_when_followed_replies ? User.find(user_id) : nil
        following = user ? user.following.select { |data| data[0] == post.user_id } : nil
        if following && following.last.to_i == Follow::Notification.levels[:watching]
+          return users.push(user)
        end
+       users
      end
    end

(or with an else that returns users)

Please sanity check of course :slightly_smiling_face:

1 Like

I’m not sure.

What angus was doing here was starting with an empty set, and pushing objects to it. That should be legal?

The users object is initialised as [ ], so is NOT nil.

1 Like

The lambda you pass to reduce has to return the updated value. The current one returns nothing if the if-statement isn’t passed. (users.push will return the updated array, it is not actually updating the users value)

2 Likes

Yep, that looks right, thanks. The initial value is irrelevant if it is lost somewhere during a single iteration.

2 Likes