Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Serialize draft #8175

Merged
merged 9 commits into from Oct 10, 2019

Conversation

@fzngagan
Copy link
Contributor

commented Oct 9, 2019

This PR adds a function serializeToDraft to the composer model to allow saving topic custom fields to draft and setting them back when the draft is reopened.

Here is the post on meta for this.
https://meta.discourse.org/t/including-composer-custom-fields-on-save-draft/126913/2

@fzngagan

This comment has been minimized.

I think we shouldn't remove all the other keys because they exist in core. I had simply copy-pasted those from core

https://github.com/discourse/discourse/blob/master/app/assets/javascripts/discourse/controllers/composer.js.es6

This comment has been minimized.

Copy link
Collaborator Author

replied Oct 1, 2019

@fzngagan Likewise here. If we're introducing a draft_serializer object we have to use it here as well, otherwise we'll be repeating ourselves.

@fzngagan

This comment has been minimized.

Copy link
Collaborator Author

replied Oct 1, 2019

@fzngagan Fair point, but I think in this case my version is more efficient than core as it uses the serialize() function properly. As we're introducing a draft_serializer object, it wouldn't make sense if we didn't use it here as well.

@discoursebot

This comment has been minimized.

Copy link

commented Oct 9, 2019

You've signed the CLA, fzngagan. Thank you! This pull request is ready for review.

@discoursebot

This comment has been minimized.

Copy link

commented Oct 9, 2019

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/including-composer-custom-fields-on-save-draft/126913/3

Copy link
Member

left a comment

It would be great if there was a test for this. Did you investigate if it could be tested easily?

@fzngagan

This comment has been minimized.

Copy link
Contributor Author

commented Oct 9, 2019

It would be great if there was a test for this. Did you investigate if it could be tested easily?

We did investigate this but noticed that there's no straightforward way of testing it.

Also similar functions serializeToTopic and serializeOnCreate don't have tests.

Copy link
Member

left a comment

Thanks, looks good! I was about to merge but I've noticed there are some linting failures.

@fzngagan

This comment has been minimized.

Copy link
Contributor Author

commented Oct 9, 2019

Thanks, looks good! I was about to merge but I've noticed there are some linting failures.

Sorry about that. It was a weird setting on my IDE which caused that. Its fixed now.

@eviltrout

This comment has been minimized.

Copy link
Member

commented Oct 9, 2019

It's still failing unfortunately. You can see the failures in Travis by clicking Details above.

@fzngagan

This comment has been minimized.

Copy link
Contributor Author

commented Oct 10, 2019

Ok @eviltrout now all the checks are passing. :)

@eviltrout eviltrout merged commit 8fc0cc9 into discourse:master Oct 10, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@eviltrout

This comment has been minimized.

Copy link
Member

commented Oct 10, 2019

Thanks! Merged now.

@fzngagan fzngagan deleted the fzngagan:serialize-draft branch Oct 10, 2019
@fzngagan

This comment has been minimized.

Copy link
Contributor Author

commented Oct 11, 2019

@eviltrout , how about bumping the plugin-api version.

@eviltrout

This comment has been minimized.

Copy link
Member

commented Oct 11, 2019

@fzngagan ah yes, good call. Can you do a follow up PR for that?

@fzngagan

This comment has been minimized.

Copy link
Contributor Author

commented Oct 11, 2019

@eviltrout What version do you want me to bump it to?

@eviltrout

This comment has been minimized.

Copy link
Member

commented Oct 11, 2019

0.8.33 I believe is next. Let's go with that!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.