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.
There was a problem hiding this comment.
Yes it all needs to be made more consistent. For the main tags table community_tags makes sense, while the relation tag-post can be represented by a table community_tags_for_post. And the api endpoints should be /community/tag like mentioned above.
There was a problem hiding this comment.
I can adjust it, but I don't think what either of you are saying makes any sense. both pieces of information are important. These tags belong to a community but apply to a post. So the field should be named community/available_tags_for_posts or similar. These are not tags applied to a community.
There was a problem hiding this comment.
The tags are defined by the community so calling them community tags makes sense. And in the future it would also be possible to use the same tags for users.
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 |
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.
There was a problem hiding this comment.
you sure? seems a bit unnecessary
There was a problem hiding this comment.
Doesn't matter to me that much, but it is against the convention of all the other API actions.
|
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. |
phiresky
force-pushed
the
post-tags-2
branch
from
222d18f to
1b3e8b0
Compare
|
I've updated the PR
I'll merge main after reviews are green |
|
Api tests are failing, looks like you need to merge changes from main branch here. |
| let tag_name_lower = tag_name.to_lowercase(); | ||
| let id_slug = VALID_ID_SLUG.replace_all(&tag_name_lower, "-"); | ||
| // limit length to 40 chars | ||
| let id_slug: String = id_slug.chars().take(40).collect(); |
There was a problem hiding this comment.
You should limit length of the tag itself when it is created.
This is the second part of post tags, implementing the API methods.
Database PR (merged): #4997
Original RFC: LemmyNet/rfcs#4