Add Date/Time button missing on latest discourse

Steps to Reproduce

After updating to the latest version of discourse, the Add Date/Time button disappears. Events cannot be added.

Example

Logs

The locations plugin is also affected.

@fzngagan It looks like the reason this is happening is because in the last few weeks the Discourse team have been making changes to explicitly tie the plugin enabled setting and associated functionality with the loading of plugin assets.

This plugin needs to properly use the “plugin_enabled” system. Here’s what I suggest we do:

  1. The events_enabled setting is currently used to toggle site-wide event use. That setting should be renamed to events_enabled_sitewide, i.e. a direct find and replace.

  2. Add a new setting events_enabled to enable / disable the plugin

  3. Identify this as the plugin enabled setting in the plugin.rb using enabled_site_setting.

Make sense? I’m going to do the same for the locations plugin.

3 Likes

Thanks for clarifying.

So there are two settings, one for using with code and other to tell discourse that plugin is enabled.

Is that what you’re saying?

1 Like

Yes. Part of Discourse’s plugin system is support for a “plugin enabled” site setting. This setting is intended to be a setting which turns the entire plugin on and off. A little bit like the activated / deactivated state of plugins in Wordpress.

The Discourse plugin system has had minor support for automating the on / off functionality of the enabled setting in the past, however you had to set up a fair bit of the toggling yourself, e.g. by adding a

if (!Discourse.SiteSettings.events_enabled) return

At the top of an intializer that was making modifications to client-side classes. See further: Plugin development: site settings "enabled" checkbox not working - dev - Discourse Meta

There has also been support for use of the setting in the server-side API, for example take a look at the add_to_seriializer method in lib/plugins/instance.rb.

It seems the CDCK team have been doing more work on the plugin system to automate more use of the enabled setting system. So we need to be sure the plugin is usiing that system properly.

2 Likes

I use to wonder that for many plugins, their UI would stick around even if the plugin was disabled. I wished for that feature to be there and the Core guys have surprised us with that feature :wink: .

I’ll quickly make the change and make a PR after testing it.

2 Likes

I’m actually still trying to figure out the full extent of the changes. I haven’t fixed this myself for the locations plugin production yet (it’s working in development). It won’t hurt for us to properly support that settinig in the events plugin, so go ahead with what I’ve laid out above, but we still need to dig inito this a bit more.

3 Likes

I followed the steps you mentioned. I couldn’t get the plugin to work on my local.
Currently, I’m looking at the js side to see if something is wrong there.

Ok, the basic issue is that the add_to_serializer plugin method is not working the same way (or not working?) so the various custom_fields which the events and locations plugins namespace on various serializers are not being serialized to the client properly.

After removing this namespacing

the locations plugin now shows the “Add Location” button in production: Sandbox - Pavilion.

Could you test that approach on the events plugin?

We’ll potentially need to remove the namespacing for all properties being serialized to the client.

2 Likes

I’ll do the same thing with events and check.

Update

I’ve gotten some success at debugging this. I was able to get back the Add Date/Time button on my dev.

2 Likes

@fzngagan FYI we’ve got the same problem on TLP, i’m looking at that at moment.

1 Like

Do share the important findings about the change.

@angus, why do you call this namespacing?

1 Like

Well that’s meant to add the property to the serializer. The reason I called in ‘namespacing’ is because that’s what it’s effectively doing in the basic_category_serializer. Category custom fields are automatically added to the basic_category_serializer, in the custom_fields property. So adding another property to the serializer to access the custom_field directly is essentially namespacing.

But the larger point is that for whatever reason add_to_serializer is not working anymore

In the case of the basic_category_serializer, back when I wrote this code custom_fields were not automatically added, so it was necessary to add them in, besides having them accessible at category.location, instead of category.custom_fields.location.

Where custom_fields are automatically added to serializers, like basic_category, it’s better if we just use that serialization for now to avoid whatever issue is occurring with add_to_serializer.

So it just means you need to change instances of category.location on the client to category.custom_fields.location or equivalent.

1 Like

I get it, so when you define a custom field now on an object, it should automatically feed that new property without amending the serializer

1 Like

On the serializers in which custom_fields are already there. For each serializer your plugin is using you’ll need to check if it serializes the custom_fields of the relevant object.

Take a look at basic_category_serializer.rb to see what I mean

None of the serializers used to do that. But most seem to now

1 Like

ah so there might be gaps where they’ve still to implement that?

1 Like

Possibly

1 Like

so this won’t work if the serializers don’t include custom fields, this will remain broken and we might have to escalate to and wait for core fix?

OK, summary of issue, please correct me where wrong and I’ll post this on Meta:

There seems to be a change/change of approach in Core wrt to serialization of Custom Fields.
Previously we needed (or had the option ?) to add_to_serializer to include a custom field on a serialized object. Now this method doesn’t seem to work. Instead in some cases we can rely on the existing serializer including the custom fields we create. However, there may be some objects where this support has yet to be added?

@angus Is that a reasonable issue characterisation?

1 Like