Events: Save event data when a post is moved to draft

Events: Save event data when a post is moved to draft
0

Feature

Save event data when a post is moved to draft.

Description

When a user writes a post but doesn’t post it, it is moved to drafts. When you reopen the draft, the event data should be still present if it was set while writing the post.

Use Case

It provides a better user experience. Drafts should be supported so the user doesn’t have to re-enter the event info if they plan to publish the post later.

Budget

@fzngagan Yes, this is a bug actually. I’m going to move this to tasks and assign it a high priority. I’ve been meaning to fix this for some time.

1 Like

I’ll take a look and see if I can fix it by today EOD.

Ok, let me know if you get stuck or need any help. I’ll look out for your PR!

1 Like

It turned out a bit trickier than I thought.

I dug into it and here’s is what I’ve found:

The problem I’m facing is that discourse doesn’t seem to have exposed any methods in the composer model to send data to the backend in case of saving/updating a draft.

Whereas, there are methods for adding custom data fields in case of saving the actual post like ‘serializeToTopic’, ‘serializeOnCreate’.

discourse-local-dates plugin also does a similar job but it stores the dates in composer’s editor itself as BBcode.

So how should I go about solving this issue?

I think this is another case where our best option is to actually create a PR into Discourse. We actually need this functionality in multiple places, so writing a workaround that monkey patches the relevant methods is inefficient.

If memory serves a draft’s properties are stored as a simple json object in drafts with a simple CRUD structure on the server, so adding in a custom_fields property to that object should be a relatively straightforward client-side piece.

We’ll tackle it like this:

  1. Think about the simplest “Discourse” implementation of a serializeToDraft function, i.e. a method that adds a key to a static object which is then used to dynamically insert keys into the data being sent down to the server. Use the same structure as serializeOnCreate. Keep it as simple as possible.

    As well as adding the key added by serializeToDraft to the data being sent down to the server, we’ll also need to add it in when the draft is loaded and the properties are set in loadDraft.

  2. I’ll review and help to finalise the approach.

  3. You’ll make the PR into Discourse

  4. You’ll create a topic in #dev on meta explaining the issue and how your PR solves the issue.

1 Like

Thanks for helping me comprehend all of this.

I’ll try to keep it as close to serializeOnCreate as possible.

1 Like

While working on this I realized something. I would like to know what you think about it.

Will there ever be a situation where different data would need to be serialized for draft and create.

I think that the existing methods should work out of the box with drafts. So, isn’t it better that we make the existing methods work with drafts by default instead of creating new methods.

1 Like

Yes, good point. Let’s go with that approach :+1:

1 Like

I’ll start looking into it again.

1 Like

Great, let me know if you hit any issues. Otherwise I’ll look out for a PR!

1 Like

Hey, @angus,
I’ve successfully added the serializer data to draft and saved it to the db. Now I’m working on bringing back the data to composer on loadDraft.

Now I’m facing this issue. For loading the composer with the custom data intact, I need to have access to the _edit_serializer array in the composer controller which lives in composer model file.

Should I expose it via export or there’s a better way to do it.

hm now I’m wondering if it made sense to combine serializeToDraft with serializeOnCreate. Would it make it easier if they were seperate again?

In the sense of solving the problem I mentioned?

Yes, I’m just wondering if seperating out the draft serialization would make this simpler, and not require us to modify existing structures like serializeOnCreate.

One thing I’m thinking about here is that in order to get anything merged into Discourse core, we have to disturb as little of the core code as possible. If our work requires us to move around existing objects it’s less likely to be accepted.

Would having a seperate draft serializer object make it simpler to re-add the data to the composer on loadDraft?

1 Like

I don’t think so. Because, whether its the draft serializer or on create serializer, we’ll need to have that data in loadDraft and actually pass it to Composer.open. So loadDraft will look similar in both the cases.

Update

Now when I think about it, yes a new serializer probably makes the process a bit simpler.

I’ve created this branch :GitHub - fzngagan/discourse at draft-serialize
It gets the events data serialized and saved to the db.

This code throws error on Composer.open at ...serialized in loadDraft method.

I read about this concept of spread in js and it should work in this case. I don’t understand why it doesn’t work.

Please do suggest if that’s the right way of doing what We’re trying to do.

Also, I know the code isn’t clean so I’ll clean it up once it starts to work and before making a PR.

The spread syntax in es6 only applies to iterable objects, i.e. an array. Support for merging objects has only been added in ECMAScript 2018 (es9) which Discourse isn’t using yet. So we’ll need to add the draft serializer keys slightly differently.

You’ve made a good start. The next step is to make this match the existing pattern in Discourse more closely. The way the serializer objects work in Discourse is the objects keys are the properties to be serialized and the values are the key of the property in the class or object being serialized.

So the draft_serializer should look something like this (not exactly, the values will be strings, like the other serializer objects)

export const draft_serializer = {
  draftKey,
  draftSequence,
  action: draft.action,
  title: draft.title,
  categoryId: draft.categoryId || opts.categoryId,
  postId: draft.postId,
  archetypeId: draft.archetypeId,
  reply: draft.reply,
  metaData: draft.metaData,
  usernames: draft.usernames,
  draft: true,
  composerState: Composer.DRAFT,
  composerTime: draft.composerTime,
  typingTime: draft.typingTime,
  whisper: draft.whisper,
  tags: draft.tags,
  noBump: draft.noBump
}

Then, like with the other serializer objects, when this is used the object keys will be iterated and the properties of the object being sent to the server set. In the composer model you can use the existing serialize() method. Maybe you can also export that to the controller to use it there as well.

  serialize(serializer, dest) {
    dest = dest || {};
    Object.keys(serializer).forEach(f => {
      const val = this.get(serializer[f]);
      if (typeof val !== "undefined") {
        Ember.set(dest, f, val);
      }
    });
    return dest;
  },
1 Like

@fzngagan The other thing to keep in mind here is that even if we turn this into a really nice PR, the Discourse team may not accept it. So while we’re doing this also be thinking about how we could do the same thing by overriding methods in a plugin intializer.

At this point, I think it might be worth laying out what we’re trying to do here in a new topic in #dev on meta which you can add to as you go here. The more visibility we have on this the better. If you’d like me to review your post before you make it, happy to do that.

1 Like

Template for meta post on our PR.

Title: Including composer custom fields on save draft.

Recently, I started looking into a bug with the Events plugin where the events data shown on composer is lost if the post is saved to drafts.

I tried to fix the issue and soon found out that there’s no API exposed by Discourse which allows us to do that.

I think this is an important feature from a UX standpoint with many potential use cases other than what I’m faced with.

For that, I am making a PR, which fixes this issue by introducting a new method in the Composer Model called serializeToDraft. This method will add those custom fields while saving the post to draft. Also, those fields will be set to the composer model when the draft is reopened.

@angus is helping me out on this PR by reviewing the code and suggesting the important improvements to be made.

I would like to know the thoughts of the Discourse team on this feature.

1 Like