-
-
Notifications
You must be signed in to change notification settings - Fork 876
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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, | ||
|
@@ -21,6 +21,7 @@ use activitypub_federation::{ | |
protocol::{ | ||
helpers::deserialize_skip_error, | ||
public_key::PublicKey, | ||
values::MediaTypeHtml, | ||
verification::verify_domains_match, | ||
}, | ||
}; | ||
|
@@ -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>, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You accidentally removed the line There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Aaahh dang, thx. |
||
/// banner | ||
pub(crate) image: Option<ImageObject>, | ||
|
@@ -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(()) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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>, | ||
|
@@ -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>, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)] | ||
|
@@ -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)] | ||
|
@@ -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>>>, | ||
|
@@ -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)] | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or do one at a time when you come across it. |
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)) | ||
|
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; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
|
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.