Adding community description in addition to sidebar, like site. #5120
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
Truncating would make sense, see #1375
There was a problem hiding this comment.
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.
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.
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.
descriptionandsidebarfields 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_errorto all the new fields I added, but it still is getting a parsing error.