Community post tags (part 2: API methods) #5389
Conversation
| @@ -222,6 +225,9 @@ pub fn config(cfg: &mut ServiceConfig, rate_limit: &RateLimitCell) { | |||
| .route("/icon", delete().to(delete_community_icon)) | |||
| .route("/banner", post().to(upload_community_banner)) | |||
| .route("/banner", delete().to(delete_community_banner)) | |||
| .route("/post_tag", post().to(create_community_tag)) | |||
| .route("/post_tag", put().to(update_community_tag)) | |||
| .route("/post_tag", delete().to(delete_community_tag)) | |||
There was a problem hiding this comment.
I would go with /tags instead, using underscore looks ugly.
There was a problem hiding this comment.
there's also underscore in some other api methods, and here i think it's good to be unambiguous
There was a problem hiding this comment.
IMO these should be at :
- POST
/community/tag(creates) - PUT
/community/tag(updates) - DELETE
/community/tag(deletes)
There was a problem hiding this comment.
The reason I would disagree with this is that it is pretty ambiguous. These are not "community tags" they are "post tags" in a community. Community tags could just as well be tags that admins of an instance add to communities, for example a "technology" tag to all tech communities to group them. This is something that we might want in the future, and adding ambiguity just because an "underscore looks ugly" in a place that no user will ever see seems pretty misguided to me.
There was a problem hiding this comment.
I spose the naming of these could be either where they live / who they belong to, or what they apply to. In that sense these are community tags, that apply to posts. This kind of hints that we're trying to do both at once, when we should adopt one: .route("/**post_tag**", post().to(create_**community**_tag))
Doesn't matter to me too much, as long as we're consistent. I think using the ownership level makes sense tho : these community tags might also apply to comments at some point, in addition to posts.
phiresky
requested review from
dessalines,
dullbananas and
SleeplessOne1917
as code owners
|
Ready for review. the API tests pass locally for me, but require merging LemmyNet/lemmy-js-client#483 |
crates/api/src/community/tag.rs
Outdated
| let tag_form = TagInsertForm { | ||
| name: data.name.clone(), | ||
| community_id: data.community_id, | ||
| ap_id: Url::parse(&format!("{}/tag/{}", community.actor_id, &data.id_slug))?.into(), |
There was a problem hiding this comment.
Could you add this as a function to the community impl? Then you can do community.build_tag_ap_id(...) . Or at least as a helper function somewhere.
There was a problem hiding this comment.
Or better yet a db trigger
crates/api/src/community/tag.rs
Outdated
| .await?; | ||
|
|
||
| // Soft delete the tag | ||
| let tag_form = TagInsertForm { |
There was a problem hiding this comment.
Should be a TagUpdateForm, with the fields as optional. Check out some of the other community/delete.rs for an example.
There was a problem hiding this comment.
Would probably be better to split each action into its own file, like we've done with the other API actions.
crates/api/src/post/tags.rs
Outdated
| ) -> LemmyResult<Json<PostResponse>> { | ||
| let post = Post::read(&mut context.pool(), data.post_id).await?; | ||
|
|
||
| let is_author = local_user_view.person.id == post.creator_id; |
crates/api_common/src/post.rs
Outdated
| #[derive(Debug, Serialize, Deserialize, Clone, Default)] | ||
| #[cfg_attr(feature = "full", derive(TS))] | ||
| #[cfg_attr(feature = "full", ts(export))] | ||
| /// Update tags for a post. |
There was a problem hiding this comment.
Would probably be good to add a comment about how either mods or post creators can do this.
|
Do the re-request review when you want me to have another look. I thought about going through it but I'm guessing its still WIP. |
This is the second part of post tags, implementing the API methods.
Database PR (merged): #4997
Original RFC: LemmyNet/rfcs#4