Quick Messages Plugin

Try reproducing the problem using the mobile version of Google Chrome or the mobile version of Firefox (or any other mobile browser).

2 Likes

A cache clear did the trick! Thanks! EDIT: Or not. :frowning:

1 Like

Sorry for the neglect here, I’ve been busy with other work. Will take a look at this plugin again soon.

@DaleKramer @Hifihedgehog If you have a pressing issue let me know and I’ll take a look at it this week.

4 Likes

Actually, after updating the server, the issue returned. It also occurs on all my mobile browsers:

I’ve readded quick messages to thepavilion.io. Can you repro the issue there?

1 Like

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.

1 Like

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.

1 Like

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
require_dependency ‘post_creator’

class ::PostCreator

def valid?

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

4 Likes

That’d be most appreciated. Things are quite busy atm :grimacing:. Thanks!

1 Like

https://github.com/paviliondev/discourse-quick-messages/pull/55

1 Like

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?

1 Like

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 :wink:

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? :thinking:)

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.

https://github.com/paviliondev/discourse-quick-messages/commit/4e36e779875772e356c4befbcd24a2a60b4fa277

@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 :+1:

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.

5 Likes

Thanks @angus. I’m assuming you were able to test this? Also - do I just ask our hosting partner (Communiteq (formerly DiscourseHosting)) to reload the plugin for us?