Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
JSON Content Editor #54
JSON Content Editor #54
Changes from 8 commits
2958e73
cb6f93a
3d4bdfe
8eb3c6c
2321bde
4587141
9263150
efc865d
be3047c
3a9687b
40665db
0cf11d4
df79fd3
3735468
daf2434
e476a59
0f34c5e
9879110
d8c4f83
d943930
c00d589
9e9287f
979afa5
a8424da
25c2106
156e741
7e95d84
79490b9
bbfceff
aaf6574
a4c119e
427b7f9
7fe7193
8061dab
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something similar should happen with this too:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not good. If the content is versionable, this creates a new draft clone of the content item we have no use for. We don't want that, because the new version's content should come from the JSON, not through cloning the existing item.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the content is versionable, we need to edit a new or existing draft, not the latest version, what might be a published one. Unless you want to let people circumvent versioning, what I wouldn't do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are not editing the latest version. We are unpublishing it and the publishing a new content item that's deserialized from the received JSON.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this necessary instead of utilizing Orchard's versioning?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for info and from memory
With
STJ
we did aJConvert
helper to serialize /deserialize a content item as before.Yes the
Version
column is not related to theContentItemVersionId
, it is used when we save an item withcheckConcurrency: true
, it throws if a given record is written concurrently.Yes this is not obvious to call the right content manager methods and in the right order ;)
First I assume that the editor doesn't allow to modify some data that are still managed internally, like
ContentItemId
,ContentItemVersionId
,Latest
,Published
and so on. And I saw that from the editor we only can publish, not save as a draft.Why not if it is an advanced feature intended to act directly on a given version record, but yes here we don't follow the version management pattern, e.g. loading / activating a new version if the type is
Versionable
. Yes, theLatest
may not bePublished
, hmm but if this is the one we are editing it will be, if an item is alreadyPublished
,PublishAsync()
does nothing, that why I think you needed to call_session.Save()
.Okay there is no driver validations but as I can see no updating and validating handlers will be called too. So what I suggest is to follow the pattern used in the
ApiController.Post
of theOC.Contents
module.It requests to load and activate a
DraftRequired
, a new version is built if the type isVersionable
, the result being unpublished, if it doesn't exists here you can returnNotFound
, then it merges this existing item version with the provided one, here can be deserialized from the editor, then it callsUpdateAsync()
andValidateAsync()
that calls validating handlers, finally it doesSaveDraftAsync()
or as herePublishAsync()
(here the item is not already published) that may unpublish a previous version.The whole without having to call any
_session.Save()
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for chiming in, JT. Reopening this convo, then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see it in your branch but it's not in OC proper yet, right? It's a great idea and will make porting easier for everyone.
You can edit the
ContentItemId
, this could be used to clone the current item by publishing it with a different ID.ContentItemVersionId
,Latest
, andPublished
are overwritten to values you'd expect from a Publish type action.You can always turn the content item into a draft using the stock actions menu after it's saved. I almost never use drafts so I had no interest adding this feature at the moment. (my interest is only relevant here because I made this contrib on my own initiative, outside of work) If there is any demand for saving a draft, the editor can be expanded later without breaking changes.
In the current version I just set the
contentItem.Published
to false before callingPublishAshync
to force it, instead of calling_session.Save()
. But that's delving int implementation detail, I think it would be better if there was an additional optional parameter on the method likeIContentManager.PublishAsync(ContentItem contentItem, bool forcePublish = false)
. What do you think? If no problem with it comes to mind, I may submit a PR for that in OC.So would it make more sense to ditch the highlighted code and just pass the content item to
ApiController.Post
directly? That would be a lot of code to copy/reimplement...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried just passing the content item to
ApiController.Post
and it works great! Now that it's trivial to handle drafts by just passing true to the second parameter, I've added that too.The only tricky part was that this action requires
Permissions.AccessContentApi
permission, but it's not so hard to grant that to a temporary claims principal, so I did that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay I understand the context so feel free to do whatever you think is better, these are only suggestions.
We already have a dedicated converter allowing to serialize / deserialize a content item, you are using it here and it will be the same with STJ. Hmm maybe in the controller action you could directly bind to a content item as done in the Api controller action.
Okay I understand, it is more like an advanced feature allowing to update a given version record without having to deal with validations, draft versions and so on. I was not sure but yes in that case my suggestions are less relevant and maybe what you first did was better.
Good idea to use the Api action but I would not recommend this pattern, I think it is better to keep a dedicated controller action that would be easier to tweak.
Otherwise, following my first bad idea, once you have a deserialized content item, could be as simple as the following.