-
-
Notifications
You must be signed in to change notification settings - Fork 879
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?
Conversation
- Also made changes to lemmy's group apub to be similar to its site, which uses content for the sidebar, and summary for the short description. - Fixes #5078
@@ -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 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)
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.
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.
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.
These match exactly what was done to the site table when we extracted these into two also.
@@ -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 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.
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.
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 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.
@@ -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 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.
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), |
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.
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.
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.
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 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.
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.
Aaahh dang, thx.
description
andsidebar
fields for communities #5078@Nutomic when you get a chance, I could use your help with this peertube issue failing in tests: https://woodpecker.join-lemmy.org/repos/129/pipeline/9615/19#L282
I tried adding
deserialize_skip_error
to all the new fields I added, but it still is getting a parsing error.