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.

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

Adding community description in addition to sidebar, like site. #5120

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
8 changes: 6 additions & 2 deletions crates/api_common/src/community.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,9 @@ pub struct CreateCommunity {
pub name: String,
/// A longer title.
pub title: String,
/// A longer sidebar, or description of your community, in markdown.
/// A sidebar for the community in markdown.
pub sidebar: Option<String>,
/// A shorter, one line description of your community.
pub description: Option<String>,
/// An icon URL.
pub icon: Option<String>,
Expand Down Expand Up @@ -147,7 +149,9 @@ pub struct EditCommunity {
pub community_id: CommunityId,
/// A longer title.
pub title: Option<String>,
/// A longer sidebar, or description of your community, in markdown.
/// A sidebar for the community in markdown.
pub sidebar: Option<String>,
/// A shorter, one line description of your community.
pub description: Option<String>,
/// An icon URL.
pub icon: Option<String>,
Expand Down
1 change: 1 addition & 0 deletions crates/api_common/src/site.rs
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,7 @@ pub struct CreateSite {
/// Edits a site.
pub struct EditSite {
pub name: Option<String>,
/// A sidebar for the site, in markdown.
pub sidebar: Option<String>,
/// A shorter, one line description of your site.
pub description: Option<String>,
Expand Down
21 changes: 14 additions & 7 deletions crates/api_crud/src/community/create.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ use lemmy_utils::{
error::{LemmyErrorExt, LemmyErrorType, LemmyResult},
utils::{
slurs::check_slurs,
validation::{is_valid_actor_name, is_valid_body_field},
validation::{is_valid_actor_name, is_valid_body_field, site_description_length_check},
},
};

Expand All @@ -58,8 +58,18 @@ pub async fn create_community(
let url_blocklist = get_url_blocklist(&context).await?;
check_slurs(&data.name, &slur_regex)?;
check_slurs(&data.title, &slur_regex)?;
let description =
process_markdown_opt(&data.description, &slur_regex, &url_blocklist, &context).await?;
let sidebar = process_markdown_opt(&data.sidebar, &slur_regex, &url_blocklist, &context).await?;

// Ensure that the sidebar has fewer than the max num characters...
if let Some(sidebar) = &sidebar {
is_valid_body_field(sidebar, false)?;
}

let description = data.description.clone();
if let Some(desc) = &description {
site_description_length_check(desc)?;
check_slurs(desc, &slur_regex)?;
}

let icon = diesel_url_create(data.icon.as_deref())?;
let icon = proxy_image_link_api(icon, &context).await?;
Expand All @@ -69,10 +79,6 @@ pub async fn create_community(

is_valid_actor_name(&data.name, local_site.actor_name_max_length as usize)?;

if let Some(desc) = &data.description {
is_valid_body_field(desc, false)?;
}

// Double check for duplicate community actor_ids
let community_actor_id = generate_local_apub_endpoint(
EndpointType::Community,
Expand All @@ -89,6 +95,7 @@ pub async fn create_community(
let keypair = generate_actor_keypair()?;

let community_form = CommunityInsertForm {
sidebar,
description,
icon,
banner,
Expand Down
11 changes: 7 additions & 4 deletions crates/api_crud/src/community/update.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,16 +41,18 @@ pub async fn update_community(
let url_blocklist = get_url_blocklist(&context).await?;
check_slurs_opt(&data.title, &slur_regex)?;

let description = diesel_string_update(
process_markdown_opt(&data.description, &slur_regex, &url_blocklist, &context)
let sidebar = diesel_string_update(
process_markdown_opt(&data.sidebar, &slur_regex, &url_blocklist, &context)
.await?
.as_deref(),
);

if let Some(Some(desc)) = &description {
is_valid_body_field(desc, false)?;
if let Some(Some(sidebar)) = &sidebar {
is_valid_body_field(sidebar, false)?;
}

let description = diesel_string_update(data.description.as_deref());

let old_community = Community::read(&mut context.pool(), data.community_id).await?;

let icon = diesel_url_update(data.icon.as_deref())?;
Expand Down Expand Up @@ -84,6 +86,7 @@ pub async fn update_community(

let community_form = CommunityUpdateForm {
title: data.title.clone(),
sidebar,
description,
icon,
banner,
Expand Down
4 changes: 2 additions & 2 deletions crates/api_crud/src/site/create.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ use lemmy_db_views::structs::{LocalUserView, SiteView};
use lemmy_utils::{
error::{LemmyErrorType, LemmyResult},
utils::{
slurs::{check_slurs, check_slurs_opt},
slurs::check_slurs,
validation::{
build_and_check_regex,
check_site_visibility_valid,
Expand Down Expand Up @@ -168,7 +168,7 @@ fn validate_create_payload(local_site: &LocalSite, create_site: &CreateSite) ->

if let Some(desc) = &create_site.description {
site_description_length_check(desc)?;
check_slurs_opt(&create_site.description, &slur_regex)?;
check_slurs(desc, &slur_regex)?;
}

site_default_post_listing_type_check(&create_site.default_post_listing_type)?;
Expand Down
4 changes: 3 additions & 1 deletion crates/apub/assets/lemmy/objects/group.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,13 @@
"type": "Group",
"preferredUsername": "tenforward",
"name": "Ten Forward",
"summary": "<p>Lounge and recreation facility</p>\n<hr />\n<p>Welcome to the Enterprise!.</p>\n",
"summary": "A description of ten forward.",
"content": "<p>Lounge and recreation facility</p>\n<hr />\n<p>Welcome to the Enterprise!.</p>\n",
"source": {
"content": "Lounge and recreation facility\n\n---\n\nWelcome to the Enterprise!",
"mediaType": "text/markdown"
},
"mediaType": "text/html",
"sensitive": false,
"icon": {
"type": "Image",
Expand Down
25 changes: 17 additions & 8 deletions crates/apub/src/objects/community.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ use crate::{
use activitypub_federation::{
config::Data,
kinds::actor::GroupType,
protocol::values::MediaTypeHtml,
traits::{Actor, Object},
};
use chrono::{DateTime, Utc};
Expand Down Expand Up @@ -107,8 +108,10 @@ impl Object for ApubCommunity {
id: self.id().into(),
preferred_username: self.name.clone(),
name: Some(self.title.clone()),
summary: self.description.as_ref().map(|b| markdown_to_html(b)),
source: self.description.clone().map(Source::new),
content: self.sidebar.as_ref().map(|d| markdown_to_html(d)),
source: self.sidebar.clone().map(Source::new),
summary: self.description.clone(),
media_type: self.sidebar.as_ref().map(|_| MediaTypeHtml::Html),
Copy link
Member

Choose a reason for hiding this comment

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

This renaming will also cause some problems with backwards compat. But it does make more sense with the field definitions, so we probably need to do it this way (also for api).

Copy link
Member Author

Choose a reason for hiding this comment

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

Also they will try to update description with text that is too long. As the field name and type are unchanged, this will also be hard to notice during testing. So for backwards compat it would be better to leave description unchanged, and use a new name for the short description.

Couldn't we truncate to account for that? To me having these fields named different things for instance and community would be a bit scatterbrained, and if we don't make them consistent now, we never will.

Copy link
Member

Choose a reason for hiding this comment

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

Truncating would make sense, see #1375

Copy link
Member Author

Choose a reason for hiding this comment

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

Hrm... we should probably do these all at once then. So for all incoming federation receives, truncate on limited fields before inserting / updating?

Not sure if that should be done in this PR or a separate one.

Copy link
Member

Choose a reason for hiding this comment

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

Exactly, and better in a separate PR.

icon: self.icon.clone().map(ImageObject::new),
image: self.banner.clone().map(ImageObject::new),
sensitive: Some(self.nsfw),
Expand Down Expand Up @@ -146,10 +149,9 @@ impl Object for ApubCommunity {
let local_site = LocalSite::read(&mut context.pool()).await.ok();
let slur_regex = &local_site_opt_to_slur_regex(&local_site);
let url_blocklist = get_url_blocklist(context).await?;
let description = read_from_string_or_source_opt(&group.summary, &None, &group.source);
let description =
process_markdown_opt(&description, slur_regex, &url_blocklist, context).await?;
let description = markdown_rewrite_remote_links_opt(description, context).await;
let sidebar = read_from_string_or_source_opt(&group.content, &None, &group.source);
let sidebar = process_markdown_opt(&sidebar, slur_regex, &url_blocklist, context).await?;
let sidebar = markdown_rewrite_remote_links_opt(sidebar, context).await;
let icon = proxy_image_link_opt_apub(group.icon.map(|i| i.url), context).await?;
let banner = proxy_image_link_opt_apub(group.image.map(|i| i.url), context).await?;

Expand All @@ -163,7 +165,8 @@ impl Object for ApubCommunity {
last_refreshed_at: Some(naive_now()),
icon,
banner,
description,
sidebar,
description: group.summary,
followers_url: group.followers.clone().map(Into::into),
inbox_url: Some(group.inbox.into()),
shared_inbox_url: group.endpoints.map(|e| e.shared_inbox.into()),
Expand Down Expand Up @@ -296,10 +299,16 @@ pub(crate) mod tests {

assert_eq!(community.title, "Ten Forward");
assert!(!community.local);

// Test the sidebar and description
assert_eq!(
community.description.as_ref().map(std::string::String::len),
community.sidebar.as_ref().map(std::string::String::len),
Some(63)
);
assert_eq!(
community.description,
Some("A description of ten forward.".into())
);

Community::delete(&mut context.pool(), community.id).await?;
Site::delete(&mut context.pool(), site.id).await?;
Expand Down
14 changes: 10 additions & 4 deletions crates/apub/src/protocol/objects/group.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use crate::{
community_outbox::ApubCommunityOutbox,
},
local_site_data_cached,
objects::{community::ApubCommunity, read_from_string_or_source_opt},
objects::community::ApubCommunity,
protocol::{
objects::{Endpoints, LanguageTag},
ImageObject,
Expand All @@ -21,6 +21,7 @@ use activitypub_federation::{
protocol::{
helpers::deserialize_skip_error,
public_key::PublicKey,
values::MediaTypeHtml,
verification::verify_domains_match,
},
};
Expand Down Expand Up @@ -50,10 +51,16 @@ pub struct Group {

/// title
pub(crate) name: Option<String>,
pub(crate) summary: Option<String>,
// sidebar
#[serde(deserialize_with = "deserialize_skip_error", default)]
pub(crate) content: Option<String>,
#[serde(deserialize_with = "deserialize_skip_error", default)]
pub(crate) source: Option<Source>,
#[serde(deserialize_with = "deserialize_skip_error", default)]
pub(crate) media_type: Option<MediaTypeHtml>,
// short instance description
#[serde(deserialize_with = "deserialize_skip_error", default)]
pub(crate) summary: Option<String>,
pub(crate) icon: Option<ImageObject>,
Copy link
Member

@Nutomic Nutomic Oct 17, 2024

Choose a reason for hiding this comment

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

You accidentally removed the line #[serde(deserialize_with = "deserialize_skip_error", default)] for icon, thats why tests are failing. Also content and summary dont need deserialize_skip_error, that could hide real problems.

Copy link
Member Author

Choose a reason for hiding this comment

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

Aaahh dang, thx.

/// banner
pub(crate) image: Option<ImageObject>,
Expand Down Expand Up @@ -86,8 +93,7 @@ impl Group {

check_slurs(&self.preferred_username, slur_regex)?;
check_slurs_opt(&self.name, slur_regex)?;
let description = read_from_string_or_source_opt(&self.summary, &None, &self.source);
check_slurs_opt(&description, slur_regex)?;
check_slurs_opt(&self.summary, slur_regex)?;
Ok(())
}
}
2 changes: 1 addition & 1 deletion crates/apub/src/protocol/objects/instance.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,9 @@ pub struct Instance {
pub(crate) content: Option<String>,
#[serde(deserialize_with = "deserialize_skip_error", default)]
pub(crate) source: Option<Source>,
pub(crate) media_type: Option<MediaTypeHtml>,
// short instance description
pub(crate) summary: Option<String>,
pub(crate) media_type: Option<MediaTypeHtml>,
/// instance icon
pub(crate) icon: Option<ImageObject>,
/// instance banner
Expand Down
1 change: 1 addition & 0 deletions crates/db_schema/src/impls/community.rs
Original file line number Diff line number Diff line change
Expand Up @@ -508,6 +508,7 @@ mod tests {
id: inserted_community.id,
name: "TIL".into(),
title: "nada".to_owned(),
sidebar: None,
description: None,
nsfw: false,
removed: false,
Expand Down
4 changes: 3 additions & 1 deletion crates/db_schema/src/schema.rs
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ diesel::table! {
name -> Varchar,
#[max_length = 255]
title -> Varchar,
description -> Nullable<Text>,
sidebar -> Nullable<Text>,
removed -> Bool,
published -> Timestamptz,
updated -> Nullable<Timestamptz>,
Expand Down Expand Up @@ -199,6 +199,8 @@ diesel::table! {
#[max_length = 255]
featured_url -> Nullable<Varchar>,
visibility -> CommunityVisibility,
#[max_length = 150]
description -> Nullable<Varchar>,
}
}

Expand Down
13 changes: 9 additions & 4 deletions crates/db_schema/src/source/community.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ pub struct Community {
pub name: String,
/// A longer title, that can contain other characters, and doesn't have to be unique.
pub title: String,
/// A sidebar / markdown description.
pub description: Option<String>,
/// A sidebar for the community in markdown.
pub sidebar: Option<String>,
/// Whether the community is removed by a mod.
pub removed: bool,
pub published: DateTime<Utc>,
Expand Down Expand Up @@ -68,6 +68,8 @@ pub struct Community {
#[serde(skip)]
pub featured_url: Option<DbUrl>,
pub visibility: CommunityVisibility,
/// A shorter, one-line description of the site.
pub description: Option<String>,
Copy link
Member

Choose a reason for hiding this comment

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

Using the same field name for a different meaning will cause problems for clients which are not updated. They will display the short description in the sidebar, and wont be able to show the full sidebar at all. Also they will try to update description with text that is too long. As the field name and type are unchanged, this will also be hard to notice during testing. So for backwards compat it would be better to leave description unchanged, and use a new name for the short description.

}

#[derive(Debug, Clone, derive_new::new)]
Expand All @@ -79,7 +81,7 @@ pub struct CommunityInsertForm {
pub title: String,
pub public_key: String,
#[new(default)]
pub description: Option<String>,
pub sidebar: Option<String>,
#[new(default)]
pub removed: Option<bool>,
#[new(default)]
Expand Down Expand Up @@ -118,14 +120,16 @@ pub struct CommunityInsertForm {
pub posting_restricted_to_mods: Option<bool>,
#[new(default)]
pub visibility: Option<CommunityVisibility>,
#[new(default)]
pub description: Option<String>,
}

#[derive(Debug, Clone, Default)]
#[cfg_attr(feature = "full", derive(AsChangeset))]
#[cfg_attr(feature = "full", diesel(table_name = community))]
pub struct CommunityUpdateForm {
pub title: Option<String>,
pub description: Option<Option<String>>,
pub sidebar: Option<Option<String>>,
pub removed: Option<bool>,
pub published: Option<DateTime<Utc>>,
pub updated: Option<Option<DateTime<Utc>>>,
Expand All @@ -146,6 +150,7 @@ pub struct CommunityUpdateForm {
pub hidden: Option<bool>,
pub posting_restricted_to_mods: Option<bool>,
pub visibility: Option<CommunityVisibility>,
pub description: Option<Option<String>>,
}

#[derive(PartialEq, Eq, Debug)]
Expand Down
1 change: 1 addition & 0 deletions crates/db_views/src/comment_report_view.rs
Original file line number Diff line number Diff line change
Expand Up @@ -391,6 +391,7 @@ mod tests {
actor_id: inserted_community.actor_id.clone(),
local: true,
title: inserted_community.title,
sidebar: None,
description: None,
updated: None,
banner: None,
Expand Down
1 change: 1 addition & 0 deletions crates/db_views/src/comment_view.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1095,6 +1095,7 @@ mod tests {
actor_id: data.inserted_community.actor_id.clone(),
local: true,
title: "nada".to_owned(),
sidebar: None,
description: None,
updated: None,
banner: None,
Expand Down
1 change: 1 addition & 0 deletions crates/db_views/src/post_view.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1835,6 +1835,7 @@ mod tests {
actor_id: inserted_community.actor_id.clone(),
local: true,
title: "nada".to_owned(),
sidebar: None,
description: None,
updated: None,
banner: None,
Copy link
Member

Choose a reason for hiding this comment

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

These tests would be simpler if you assert only specific fields, instead of building a full dummy object. Then its not necessary to make a change every time some unrelated field changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

There's a lot of these, so we should probably do that later as part of a test cleanup.

Copy link
Member

Choose a reason for hiding this comment

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

Or do one at a time when you come across it.

Expand Down
2 changes: 2 additions & 0 deletions crates/db_views_actor/src/community_view.rs
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,8 @@ fn queries<'a>() -> Queries<
let searcher = fuzzy_search(&search_term);
let name_filter = community::name.ilike(searcher.clone());
let title_filter = community::title.ilike(searcher.clone());
// TODO this currently filters the new short description column, but should it also check the
// sidebar too now?
Copy link
Member Author

@dessalines dessalines Oct 16, 2024

Choose a reason for hiding this comment

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

Not sure what should be done here, when they want to include the description filter. I could do either:

  • Check all 4 (community name, title, description, and sidebar)
  • Check 3 (name, title, description)
  • Check 2 (name, title)

Copy link
Member

Choose a reason for hiding this comment

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

Sidebar has a higher chance for false positives, as it includes all kinds of info about rules, related communities etc. So I would only check name, title and short description.

let description_filter = community::description.ilike(searcher.clone());
query = if options.title_only.unwrap_or_default() {
query.filter(name_filter.or(title_filter))
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
ALTER TABLE community
DROP COLUMN description;

ALTER TABLE community RENAME COLUMN sidebar TO description;

Copy link
Member Author

Choose a reason for hiding this comment

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

These match exactly what was done to the site table when we extracted these into two also.

Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
-- Renaming description to sidebar
ALTER TABLE community RENAME COLUMN description TO sidebar;

-- Adding a short description column
ALTER TABLE community
ADD COLUMN description varchar(150);