A cache clear did the trick! Thanks! EDIT: Or not.
Sorry for the neglect here, I’ve been busy with other work. Will take a look at this plugin again soon.
I’ve readded quick messages to thepavilion.io. Can you repro the issue there?
Perhaps you could look into this issue I had that seemed to point to this plugin as the culprit…
In newest version quick message are broken, on every message i got this prompt:
Sorry, you can’t create a PM on an existing topic.
True Im getting the same Error: Sorry, you can’t create a PM on an existing topic.
Getting the same “can’t create PM” error. Could it be because of this commit?
Thanks for the reports. I’ll take a look at this asap.
this was the problem. The fix is to overwrite the valid? function without the changes from this commit
so in plugin.rb or something do
rewrite it here minus 4 lines
might do a PR for this if no one else does tomorrow since I was looking at it all morning
That’d be most appreciated. Things are quite busy atm . Thanks!
While I would love to have a fix asap - I’m also concerned about long term usability of this fix. What if the logic inside that core implementation of that function changes?
Yeah, I agree, this is not the way to do it…especially with a
valid? method, this is just waiting for a security vulnerability waiting to happen… and we don’t want Discourse to be the next Wordpress in that respect… but what would be a good alternative?
I’ve been struggling with these kind of things as well lately, where there is a long method full of checks and I want to circumvent a single check only, right in the middle of the method.
You can’t just check if
:create_pm_on_existing_topic was the only error because the code returns immediately when it is set, and it might have failed another check after that as well.
I would at least prepend this module, then check if this is a quick message scenario in the new
valid? method. If it is, run the modified validation code, but it if it not about a quick message, just
return super instead, calling the core code. When the core function changes you will only break the plugin functionality and not the rest of the forum software as well.
I had to compare the two functions line by line to find out that the removed check was
if new_topic? ... else if @topic.present? && @opts[:archetype] == Archetype.private_message errors.add(:base, I18n.t(:create_pm_on_existing_topic)) return false end ...
a comment wouldn’t have hurt
Does anyone know a better way of making sure that these kind of monkey patches stay maintainable?
(Did I actually use “monkey patch” and “maintainable” in a single sentence? )
Actually I’m still not sure why the Quick messages is broken by [FIX: ensures we don’t attempt to create a new PM on an existing topic #9029] ? The 9029 fix works for PMs and Topics - then why is quick messages broken? Is it due to some specific way Quick messages posts a new PM that conflicts with 9029?
Yes, I think that’s it. And #9029 added that specific check.
I’ve just pushed a fix for this plugin. Let me know how that goes.
@Oliver_Walker thanks for the PR. As the others have mentioned, it’s best not to override big methods, but thanks for giving it a shot
At the risk of sounding like a stale fortune cookie in 95% of cases where you’re contemplating overriding part of a big server side method, you’re fighting the logic of the framework (there are maybe a few exceptions to this).
In this case, the first thing to ask is: how do normal PMs get added to existing normal PM topics? Turns out the issue is that the plugin was trying to assign the
private_message archetype to every post, instead of just the first post which creates the topic.
Yup, you can test out the latest version here: https://try.thepavilion.io
Yes. You can also try DiscourseHosting’s nifty “Redeploy Plugins” feature. Go to “Plugins” in the DiscourseHosting control panel and click “Redeploy Plugins” at the bottom.