Allow update topics for a post #86
Conversation
|
@yurabakhtin Does it really make sense to implement topic handling only at Post endpoint like this? What if we also want to set topics in Polls, Wiki or other content types?. Wouldn't it be better to create a central possibility somewhere at the content endpoint? |
|
@luke- Really I made the changes in base controller |
|
@luke- Same changes for wiki module - humhub/wiki#236 Please note this module already had the topics supporting https://www.humhub.com/en/marketplace/wiki/docs/swagger/wiki.html#tag/Wiki-Page/paths/~1wiki~1container~1{id}/post, but it was available to update topics only by IDs, so I just added new possibility to update topics by names. |
|
@luke- Same changes for tasks module - humhub/tasks#182 |
|
@luke- Changes for calendar module - humhub/calendar#282 |
|
@luke- I have updated all modules, see comments above. |
docs/swagger/post.yaml
Outdated
| @@ -68,7 +68,7 @@ paths: | |||
| - data | |||
| properties: | |||
| data: | |||
| $ref: "#/definitions/Post" | |||
| $ref: "#/definitions/PostForm" | |||
There was a problem hiding this comment.
I don't like the need to have two definitions. Post and PostForm which are almost identical.
This is just only due to Content and ContentForm?
Couldnt we integrate the Topics with in ContentMetadata`?
There was a problem hiding this comment.
@luke- We cannot use the same object/structure for a request data and for a response data.
For example, response data contains id, but we should not allow to send/request this field because it cannot be updated. Also we should not allow to update other fields like guid, created_at which exist in the object ContentMetadata.
So I decided to create the separate object ContentForm in order to keep there only fields which are allowed in request data.
There was a problem hiding this comment.
@yurabakhtin It is possible to flag attributes in Swagger with readOnly or writeOnly.
https://swagger.io/docs/specification/data-models/data-types/
e.g. for id we already use this:
https://github.com/humhub/rest/blob/master/docs/swagger/content.yaml#L212
There was a problem hiding this comment.
@luke- I tested the flags readOnly or writeOnly, but a generated HTML file doesn't hide field depending on these flags in request/response:
But we need this:
I.e. in request and response we have the field with key topics but it has a different format.
There was a problem hiding this comment.
@luke- Please review my changes in the commit 4f3d9d9
I found how to use readOnly and writeOnly flags only for definitions/components but I cannot find how to use these flags for different examples but with same key, as we need for property/key topics.
For request part we need only array on strings:
["First topic", "Second tag"]
For response - array of objects:
[
{ "id": 1, "name": "First topic" },
{ "id": 2, "name": "Second tag" }
]
So in the commit I created a new key topics-array, but in php code we still have topics for request data.
If we stop on this solution we should modify php code and accept topics-array on request side.
I.e. as I wrote above the main reason why I created a separate object/component/definition is that topics has a different schema in request and response parts.
There was a problem hiding this comment.
@yurabakhtin Thanks the commit looks fine. But can't we just use the "Id" in the Topic example as ReadOnly. Or did is missed something here?
For reading:
[
{ "id": 1, "name": "First topic" },
{ "id": 2, "name": "Second tag" } }
]
Writing:
[
{ "name": "First topic" },
{ "name": "Second tag" }
]
I would also like it if the ContentMetadata were not completely readOnly, but you could control the Visibility & Archived & Pinned there.
There was a problem hiding this comment.
@yurabakhtin Thanks for the effort, I really like it that way. One last request:
Can we not use integers here for clearly boolean values (archived, pinned, locked_comments)?
With Visibility it is of course a bit different, because there are different levels.
There was a problem hiding this comment.
Can we not use integers here for clearly boolean values (archived, pinned, locked_comments)?
Commit c8dd1ff
The related PRs have been updated as well:
| protected function updateLockedComments(ContentActiveRecord $activeRecord, array $data): bool | ||
| { | ||
| if (!isset($data['locked_comments'])) { | ||
| return true; | ||
| } | ||
|
|
||
| if (!in_array($data['locked_comments'], [0, 1])) { | ||
| $activeRecord->addError('locked_comments', 'Wrong field "locked_comments" value!'); | ||
| return false; | ||
| } | ||
|
|
||
| if (!$activeRecord->content->canLockComments()) { | ||
| $activeRecord->addError('locked_comments', 'You cannot lock comments of the Content!'); | ||
| return false; | ||
| } | ||
|
|
||
| $activeRecord->content->locked_comments = $data['locked_comments']; | ||
| return $activeRecord->content->save(); | ||
| } |
There was a problem hiding this comment.
@donni106 Yes, partly related, I answered there #88 (comment), thank you for the report!
Issue: #85