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.

Sign up for GitHub

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

Community post tags (part 2: API methods) #5389

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Community post tags (part 2: API methods) #5389

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d69ccc8
post tags part 2
phiresky Feb 4, 2025
ee045ad
Merge remote-tracking branch 'origin/main' into post-tags-2
phiresky Feb 4, 2025
1f4c451
post tags part 2 (api methods)
phiresky Feb 4, 2025
55e71b9
re-remove tracing
phiresky Feb 4, 2025
c13feef
api tests
phiresky Feb 7, 2025
5335294
update according to comments
phiresky Feb 11, 2025
475f2fd
update lints and api package
phiresky Feb 11, 2025
f920ca5
Merge remote-tracking branch 'origin/main' into post-tags-2
phiresky Feb 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion crates/api/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ actix-web = { workspace = true }
base64 = { workspace = true }
captcha = { workspace = true }
anyhow = { workspace = true }
tracing = { workspace = true }
chrono = { workspace = true }
url = { workspace = true }
hound = "3.5.1"
Expand Down
1 change: 1 addition & 0 deletions crates/api/src/community/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,5 @@ pub mod follow;
pub mod hide;
pub mod pending_follows;
pub mod random;
pub mod tag;
pub mod transfer;
157 changes: 157 additions & 0 deletions crates/api/src/community/tag.rs
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would probably be better to split each action into its own file, like we've done with the other API actions.

Original file line number Diff line number Diff line change
@@ -0,0 +1,157 @@
use activitypub_federation::config::Data;
use actix_web::web::Json;
use lemmy_api_common::{
community::{
CommunityTagResponse,
CreateCommunityTag,
DeleteCommunityTag,
ListCommunityTags,
ListCommunityTagsResponse,
UpdateCommunityTag,
},
context::LemmyContext,
utils::check_community_mod_action,
};
use lemmy_db_schema::{
source::{
community::Community,
tag::{Tag, TagInsertForm},
},
traits::Crud,
};
use lemmy_db_views::structs::LocalUserView;
use lemmy_utils::{error::LemmyResult, utils::validation::is_valid_tag_slug};
use url::Url;

#[tracing::instrument(skip(context))]
phiresky marked this conversation as resolved.
Show resolved Hide resolved
pub async fn create_community_tag(
data: Json<CreateCommunityTag>,
context: Data<LemmyContext>,
local_user_view: LocalUserView,
) -> LemmyResult<Json<CommunityTagResponse>> {
let community = Community::read(&mut context.pool(), data.community_id).await?;

// Verify that only mods can create tags
check_community_mod_action(
&local_user_view.person,
&community,
false,
&mut context.pool(),
)
.await?;

is_valid_tag_slug(&data.id_slug)?;

// Create the tag
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(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or better yet a db trigger

#5409

published: None, // defaults to now
updated: None,
deleted: false,
};

let tag = Tag::create(&mut context.pool(), &tag_form).await?;

Ok(Json(CommunityTagResponse {
id: tag.id,
name: tag.name,
community_id: tag.community_id,
}))
}

#[tracing::instrument(skip(context))]
pub async fn update_community_tag(
data: Json<UpdateCommunityTag>,
context: Data<LemmyContext>,
local_user_view: LocalUserView,
) -> LemmyResult<Json<CommunityTagResponse>> {
let tag = Tag::read(&mut context.pool(), data.tag_id).await?;
let community = Community::read(&mut context.pool(), tag.community_id).await?;

// Verify that only mods can update tags
check_community_mod_action(
&local_user_view.person,
&community,
false,
&mut context.pool(),
)
.await?;

// Update the tag
let tag_form = TagInsertForm {
name: data.name.clone(),
community_id: tag.community_id,
ap_id: tag.ap_id,
published: None,
updated: Some(chrono::Utc::now()),
deleted: false,
phiresky marked this conversation as resolved.
Show resolved Hide resolved
};

let tag = Tag::update(&mut context.pool(), data.tag_id, &tag_form).await?;

Ok(Json(CommunityTagResponse {
id: tag.id,
name: tag.name,
community_id: tag.community_id,
}))
}

#[tracing::instrument(skip(context))]
pub async fn delete_community_tag(
data: Json<DeleteCommunityTag>,
context: Data<LemmyContext>,
local_user_view: LocalUserView,
) -> LemmyResult<Json<CommunityTagResponse>> {
let tag = Tag::read(&mut context.pool(), data.tag_id).await?;
let community = Community::read(&mut context.pool(), tag.community_id).await?;

// Verify that only mods can delete tags
check_community_mod_action(
&local_user_view.person,
&community,
false,
&mut context.pool(),
)
.await?;

// Soft delete the tag
let tag_form = TagInsertForm {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be a TagUpdateForm, with the fields as optional. Check out some of the other community/delete.rs for an example.

name: tag.name.clone(),
community_id: tag.community_id,
ap_id: tag.ap_id,
published: None,
updated: Some(chrono::Utc::now()),
deleted: true,
};

let tag = Tag::update(&mut context.pool(), data.tag_id, &tag_form).await?;

Ok(Json(CommunityTagResponse {
id: tag.id,
name: tag.name,
community_id: tag.community_id,
}))
}

#[tracing::instrument(skip(context))]
pub async fn list_community_tags(
phiresky marked this conversation as resolved.
Show resolved Hide resolved
data: Json<ListCommunityTags>,
context: Data<LemmyContext>,
) -> LemmyResult<Json<ListCommunityTagsResponse>> {
let tags = Tag::get_by_community(&mut context.pool(), data.community_id).await?;

let tag_responses = tags
.into_iter()
.map(|t| CommunityTagResponse {
id: t.id,
name: t.name,
community_id: t.community_id,
})
.collect();

Ok(Json(ListCommunityTagsResponse {
tags: tag_responses,
}))
}
1 change: 1 addition & 0 deletions crates/api/src/post/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,4 @@ pub mod lock;
pub mod mark_many_read;
pub mod mark_read;
pub mod save;
pub mod tags;
59 changes: 59 additions & 0 deletions crates/api/src/post/tags.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
use activitypub_federation::config::Data;
use actix_web::web::Json;
use lemmy_api_common::{
context::LemmyContext,
post::{UpdatePostTags, UpdatePostTagsResponse},
utils::check_community_mod_action,
};
use lemmy_db_schema::{
source::{community::Community, post::Post, post_tag::PostTag, tag::PostTagInsertForm},
traits::Crud,
};
use lemmy_db_views::structs::{LocalUserView, PostView};
use lemmy_utils::error::LemmyResult;

#[tracing::instrument(skip(context))]
pub async fn update_post_tags(
data: Json<UpdatePostTags>,
context: Data<LemmyContext>,
local_user_view: LocalUserView,
) -> LemmyResult<Json<UpdatePostTagsResponse>> {
let post = Post::read(&mut context.pool(), data.post_id).await?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you fetch the PostView, then you won't need to also fetch the community below.

let community = Community::read(&mut context.pool(), post.community_id).await?;
phiresky marked this conversation as resolved.
Show resolved Hide resolved

let is_author = local_user_view.person.id == post.creator_id;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use Post::is_creator


if !is_author {
// Check if user is either the post author or a community mod
check_community_mod_action(
&local_user_view.person,
&community,
false,
&mut context.pool(),
)
.await?;
}

// Delete existing post tags
PostTag::delete_for_post(&mut context.pool(), data.post_id).await?;

// Create new post tags
for tag_id in &data.tags {
let form = PostTagInsertForm {
post_id: data.post_id,
tag_id: *tag_id,
};
PostTag::create(&mut context.pool(), &form).await?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this need some validation? What if its a tag outside / not allowed by that community. If that's the case then you need to query community tags first and only allow the tag ids if it belongs.

Also diesel lets you insert multiple forms at once, so you could create a new impl function on PostTag, to take in a vector of forms (after you've filtered out the invalid ones)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, but if there is any invalid value just return an error directly.

}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There seems to be two ways now that you're applying post tags, either in the post create itself, or via this method. I'm not sure if we should get rid of one of them.

Seems like it might be cleaner to only keep this one, since it does the additional mod checks, and also clears out the other tags.

Copy link
Member

@Nutomic Nutomic Feb 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the author to change tags on his own post it makes sense to use the existing edit post endpoint. And then a separate endpoint for mods to change specific metadata on posts (eg POST /api/v4/post/metadata). #3566 is related and could be resolved at the same time (even though it was closed its worth reopening).

Copy link
Collaborator Author

@phiresky phiresky Feb 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this request is to replace all tags of a post, while I left the "edit post" request to keep tags unchanged. On the "create post" request I did add tags directly.
I made it a separate request for three reasons:

  1. so the original request does not change its meaning for clients that are not aware of tags (e.g. erasing them unintentionally)
  2. access controls: this request can be done by mods, while the other can't - otherwise the "edit post" request would need to be modified with some kind of complex checks
  3. separation of concerns: there's many options of designing this api in general. should it be a separate "add single tag", "remove single tag" request? or one that replaces all tags? or one that replaces the whole post? hard to tell, especially if you'd want to add more access controls later (e.g. specific tags that can only be set by mods)

Could you both discuss this and give me specific information on if / how you'd like to change this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so the original request does not change its meaning for clients that are not aware of tags (e.g. erasing them unintentionally)

This is not a problem, use an option and if it is None leave the tags unchanged. So for own post the normal edit endpoint can be used.

access controls: this request can be done by mods, while the other can't - otherwise the "edit post" request would need to be modified with some kind of complex checks

Like I said a mod only endpoint to edit some post metadata makes sense, and you can allow changes to nsfw tag at the same time (#3566).

separation of concerns: there's many options of designing this api in general. should it be a separate "add single tag", "remove single tag" request? or one that replaces all tags? or one that replaces the whole post? hard to tell, especially if you'd want to add more access controls later (e.g. specific tags that can only be set by mods)

Replace all tags is fine, otherwise it gets too complicated. Mod only tags can also be done like this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this approach also. As long as the EditPost and CreatePost are doing all the checks necessary.


// Get updated post view
let post_view = PostView::read(
&mut context.pool(),
data.post_id,
Some(&local_user_view.local_user),
false,
)
.await?;

Ok(Json(UpdatePostTagsResponse { post_view }))
phiresky marked this conversation as resolved.
Show resolved Hide resolved
}
59 changes: 58 additions & 1 deletion crates/api_common/src/community.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use lemmy_db_schema::{
newtypes::{CommunityId, LanguageId, PersonId},
newtypes::{CommunityId, LanguageId, PersonId, TagId},
source::site::Site,
CommunityVisibility,
ListingType,
Expand All @@ -16,6 +16,63 @@ use serde_with::skip_serializing_none;
#[cfg(feature = "full")]
use ts_rs::TS;

#[skip_serializing_none]
#[derive(Debug, Serialize, Deserialize, Clone, Default)]
#[cfg_attr(feature = "full", derive(TS))]
#[cfg_attr(feature = "full", ts(export))]
/// Create a tag for a community.
pub struct CreateCommunityTag {
pub community_id: CommunityId,
pub id_slug: String,
pub name: String,
phiresky marked this conversation as resolved.
Show resolved Hide resolved
}

#[derive(Debug, Serialize, Deserialize, Clone)]
#[cfg_attr(feature = "full", derive(TS))]
#[cfg_attr(feature = "full", ts(export))]
pub struct CommunityTagResponse {
pub id: TagId,
pub name: String,
pub community_id: CommunityId,
}

#[skip_serializing_none]
#[derive(Debug, Serialize, Deserialize, Clone, Default)]
#[cfg_attr(feature = "full", derive(TS))]
#[cfg_attr(feature = "full", ts(export))]
/// Update a community tag.
pub struct UpdateCommunityTag {
pub tag_id: TagId,
pub name: String,
}

#[skip_serializing_none]
#[derive(Debug, Serialize, Deserialize, Clone, Default)]
#[cfg_attr(feature = "full", derive(TS))]
#[cfg_attr(feature = "full", ts(export))]
/// Delete a community tag.
pub struct DeleteCommunityTag {
pub tag_id: TagId,
}

#[skip_serializing_none]
#[derive(Debug, Serialize, Deserialize, Clone, Default)]
#[cfg_attr(feature = "full", derive(TS))]
#[cfg_attr(feature = "full", ts(export))]
/// List tags for a community.
pub struct ListCommunityTags {
pub community_id: CommunityId,
}

#[derive(Debug, Serialize, Deserialize, Clone)]
#[cfg_attr(feature = "full", derive(TS))]
#[cfg_attr(feature = "full", ts(export))]
pub struct ListCommunityTagsResponse {
pub tags: Vec<CommunityTagResponse>,
}

// Rest of existing community.rs content...

#[skip_serializing_none]
#[derive(Debug, Serialize, Deserialize, Clone, Default, PartialEq, Eq, Hash)]
#[cfg_attr(feature = "full", derive(TS))]
Expand Down
19 changes: 17 additions & 2 deletions crates/api_common/src/post.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,23 @@ use serde_with::skip_serializing_none;
#[cfg(feature = "full")]
use ts_rs::TS;

#[skip_serializing_none]
#[derive(Debug, Serialize, Deserialize, Clone, Default)]
#[cfg_attr(feature = "full", derive(TS))]
#[cfg_attr(feature = "full", ts(export))]
/// Update tags for a post.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would probably be good to add a comment about how either mods or post creators can do this.

pub struct UpdatePostTags {
pub post_id: PostId,
pub tags: Vec<TagId>,
}

#[derive(Debug, Serialize, Deserialize, Clone)]
#[cfg_attr(feature = "full", derive(TS))]
#[cfg_attr(feature = "full", ts(export))]
pub struct UpdatePostTagsResponse {
pub post_view: PostView,
}
phiresky marked this conversation as resolved.
Show resolved Hide resolved

#[skip_serializing_none]
#[derive(Debug, Serialize, Deserialize, Clone, Default, PartialEq, Eq, Hash)]
#[cfg_attr(feature = "full", derive(TS))]
Expand Down Expand Up @@ -178,8 +195,6 @@ pub struct EditPost {
/// Instead of fetching a thumbnail, use a custom one.
#[cfg_attr(feature = "full", ts(optional))]
pub custom_thumbnail: Option<String>,
#[cfg_attr(feature = "full", ts(optional))]
pub tags: Option<Vec<TagId>>,
/// Time when this post should be scheduled. Null means publish immediately.
#[cfg_attr(feature = "full", ts(optional))]
pub scheduled_publish_time: Option<i64>,
Expand Down
1 change: 1 addition & 0 deletions crates/api_crud/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ actix-web = { workspace = true }
url = { workspace = true }
futures.workspace = true
uuid = { workspace = true }
tracing = { workspace = true }
anyhow.workspace = true
chrono.workspace = true
accept-language = "3.1.0"
Expand Down
12 changes: 12 additions & 0 deletions crates/api_crud/src/post/create.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ use lemmy_db_schema::{
community::Community,
local_site::LocalSite,
post::{Post, PostInsertForm, PostLike, PostLikeForm, PostRead, PostReadForm},
post_tag::PostTag,
tag::PostTagInsertForm,
},
traits::{Crud, Likeable},
utils::diesel_url_create,
Expand Down Expand Up @@ -122,6 +124,16 @@ pub async fn create_post(
let inserted_post = Post::create(&mut context.pool(), &post_form)
.await
.with_lemmy_type(LemmyErrorType::CouldntCreatePost)?;
// Create new post tags
if let Some(tags) = &data.tags {
for tag_id in tags {
let form = PostTagInsertForm {
post_id: inserted_post.id,
tag_id: *tag_id,
};
PostTag::create(&mut context.pool(), &form).await?;
}
}
phiresky marked this conversation as resolved.
Show resolved Hide resolved

let community_id = community.id;
let federate_post = if scheduled_publish_time.is_none() {
Expand Down
1 change: 1 addition & 0 deletions crates/db_schema/src/impls/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ pub mod person_comment_mention;
pub mod person_post_mention;
pub mod post;
pub mod post_report;
pub mod post_tag;
pub mod private_message;
pub mod private_message_report;
pub mod registration_application;
Expand Down
Loading