Event gets cleared if topic is reviewed

Event gets cleared if topic is reviewed
0

Steps to Reproduce

Not sure if anyone else has run into this, but we have our events setup on a moderated category. When they come in for review, it doesn’t show the date/times and location on that view. But then, even if the topic is approved, it seems to somehow strip out the time/date and location fields. They don’t make it to the actual post after moderation. We tried it twice and ran into it both times.

Example

N/A

Logs

1 Like

@angus, At it. I’ll first try and reproduce it on my local.

1 Like

@angus, I looked into it. The problem is, when the topic is approved by mod, it is created programatically, and custom fields are not set. Its probably related to PostCreator class.

1 Like

I see. I can’t review the code at the moment (on my phone), but this could be a case for a PR into core Discourse as custom fields should always be included in topic creation.

See if you can reproduce the issue with a plugin produced by CDCK (discourse.org) that has the same issue. Perhaps the discourse-voting plugin? If it doesn’t have the same issue, see if you can figure out why.

1 Like

Thanks for that lead. I’ll try to repro with the voting plugin.

I tested the voting plugin by creating a vote enabled category which required moderation. It works. I’ll see how they’re handling that. Also, I’ll give you an update on the events plugin description.

1 Like

I tried to understand the magic that allowed the voting plugin to maintain the cusom fields after the review.

I think I’ve found it. They’re using an event :topic_approved and explicitly saving the custom fields on that event(probably).

Update:

There’s this NewPostManager class which is used by the polls plugin to validate/add the custom fields and save them to the db.

I think this api is exposed by discourse to handle how the programatically created posts should be saved.

Update 2

I spent the whole day trying to figure this out. I agree with you that saving custom fields for reviewable should be provided out of the box.

I studied the poll plugin, which uses the :post_approved event to save the custom fields.

One thing I’m yet to try is to add a custom field to the ReviewableQueuedPost class. I’m not sure whether it’ll solve the problem but I’ll give it a go and see. I’ve also posted on meta in order to get some clarity on this.

1 Like

Great investigative work :+1:

One strategy I use to get the CDCK team’s attention on a particular piece of the codebase is too look at who last was involved in work on that part of the codebase and @mention them.

So the strategy here from our perspective is:

  1. Adopt the approach used by the polls plugin to fix the immediate issue in the events plugin. We have a clear path to a solution, so let’s use it. Make a PR with this approach and I’ll review.

  2. Start a conversation with the relevant member of the Discourse team to improve how this is handled in core Discourse

  3. Depending on how 2 goes, either Discourse themselves, or we (via a PR), will update the way custom_fields get handled in post approval.

1 Like

@angus. I might need some help from you to resolve this. Can you help me out?

Sure! Lay out the issue you’re having here and I’ll take a closer look.

1 Like

Here’s a snippet from polls plugin

on(:approved_post) do |queued_post, created_post|
    if queued_post.payload["is_poll"]
      created_post.validate_polls(true)
    end
  end

We need to do the same. Only change is we need to update the custom fields of thetopic of the post with the fields present in the post.

Hence, I need to bring the event data to the queued_post's payload object.

For that, this is what polls plugin (seems to be) doing

NewPostManager.add_handler(1) do |manager|
    post = Post.new(raw: manager.args[:raw])

    if !DiscoursePoll::PollsValidator.new(post).validate_polls
      result = NewPostResult.new(:poll, false)

      post.errors.full_messages.each do |message|
        result.errors[:base] << message
      end

      result
    else
      manager.args["is_poll"] = true
      nil
    end
  end

Now, I did the a similar thing. But still, my data doesn’t get set in queued_post's payload.

The thing is regular posts and under review posts are being saved in different manner.

Update:

Core guys have started looking into it and it seems that it is a problem with Discourse itself.

The core guys have fixed the issue.

Now, I can get back to work again.

Wow that was quick! Great work :+1:

Does this entirely solve this issue in the events plugin?

1 Like

@angus, I’ll confirm today.

Its very likely though that the issue will be solved by this.

Update

I confirmed this. The fix works as promised. :wink: . I’ll be submitting a PR in couple of hours.

1 Like

@angus, I’ve submitted a PR. The only thing that concerns me is this fix will work only with master branch of discourse. Should we ask the core guys to backport the fix?

1 Like

Will it break sites not on the latest master, or will it just not work if you’re not on the latest master?

If it will just not work unless you’re on the latest master that’s fine. Asking plugin users to update to take advantage of a fix is fine. It’s not feasible to support all versions of Discourse. The codebase changes too much.

As long as it doesn’t break other functionality for users who haven’t updated.

1 Like

It won’t break sites. I’ve added a respond_to? condition for the new method so that it’ll gracefully fail and not break anything.

1 Like

Great, I’ve merged it.

Now you need to communicate the fix to the user(s) who reported the issue and ask them to update their Discourse if they want to take advantage of it. Ask them to report back on how it goes.

2 Likes

Thanks. Sure. I’ll tell them to update the plugin and discourse.

2 Likes