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

Add support for Featured Posts #2585

Merged
merged 3 commits into from
Dec 12, 2022
Merged

Conversation

makotech222
Copy link
Contributor

@makotech222 makotech222 commented Nov 24, 2022

This is the backend changes for adding Featured Posts (Local and Community) (#2067). Summary of changes below:

  • General rename of Sticky -> Feature, where appropriate
  • Add featured_community and featured_local database fields to Post/PostAggregates
  • Update 'FeaturePost' endpoint to differentiate between local/community featuring.
  • Update PostQuery to order correctly for featured posts in Local/Community view

Only current uncertainty is around changes in apub, please give guidance on whether I did it properly or not.

Once this PR is approved, I will make the PRs for the frontend changes, and merge all at once.

Thanks!

Child PRS:

@@ -181,7 +181,7 @@ pub fn config(cfg: &mut web::ServiceConfig, rate_limit: &RateLimitCell) {
web::post().to(route_post::<MarkPostAsRead>),
)
.route("/lock", web::post().to(route_post::<LockPost>))
.route("/sticky", web::post().to(route_post::<StickyPost>))
.route("/feature_post", web::post().to(route_post::<FeaturePost>))
Copy link
Member

Choose a reason for hiding this comment

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

This is a breaking change, might be better to leave it for later.

Copy link
Member

Choose a reason for hiding this comment

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

The data structure is also breaking, we can just wait to merge until after 0.17.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So leave the route as "/sticky"?

published: agg.published,
newest_comment_time_necro: inserted_post.published,
newest_comment_time: inserted_post.published,
featured_community: false,
featured_local: false,
Copy link
Member

Choose a reason for hiding this comment

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

You can probably do ..Default::default() and remove explicit values like featured_local: false, score: 0 etc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll leave that for future work, as none of the other structs are doing this currently it seems like.

@@ -82,7 +82,7 @@ pub enum ModlogActionType {
All,
ModRemovePost,
ModLockPost,
ModStickyPost,
ModFeaturePost,
Copy link
Member

Choose a reason for hiding this comment

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

You will also have to add a mod log entry for AdminFeaturePost.

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 existing table is pry better, then you don't have to do the views again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can always add another column to differentiate between local/community feature?

crates/api_common/src/post.rs Outdated Show resolved Hide resolved
crates/apub/src/protocol/objects/page.rs Outdated Show resolved Hide resolved
crates/db_schema/src/impls/post.rs Outdated Show resolved Hide resolved
@@ -181,7 +181,7 @@ pub fn config(cfg: &mut web::ServiceConfig, rate_limit: &RateLimitCell) {
web::post().to(route_post::<MarkPostAsRead>),
)
.route("/lock", web::post().to(route_post::<LockPost>))
.route("/sticky", web::post().to(route_post::<StickyPost>))
.route("/feature_post", web::post().to(route_post::<FeaturePost>))
Copy link
Member

Choose a reason for hiding this comment

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

The data structure is also breaking, we can just wait to merge until after 0.17.0

src/api_routes.rs Outdated Show resolved Hide resolved
@@ -82,7 +82,7 @@ pub enum ModlogActionType {
All,
ModRemovePost,
ModLockPost,
ModStickyPost,
ModFeaturePost,
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 existing table is pry better, then you don't have to do the views again.

migrations/2022-11-20-032430_sticky_local/up.sql Outdated Show resolved Hide resolved
migrations/2022-11-20-032430_sticky_local/up.sql Outdated Show resolved Hide resolved
alter table post_aggregates DROP COLUMN stickied;

alter table mod_sticky_post
rename column stickied TO featured;
Copy link
Member

Choose a reason for hiding this comment

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

This still needs some column to differentiate between local and community. Can be a boolean, or an enum, your choice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed with 'is_featured_community' bool on table

Copy link
Member

@dessalines dessalines left a comment

Choose a reason for hiding this comment

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

Looks good.

id -> Int4,
mod_person_id -> Int4,
post_id -> Int4,
stickied -> Nullable<Bool>,
is_featured_community -> Bool,
Copy link
Member

Choose a reason for hiding this comment

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

Hope these are in the correct order, not sure if compile would fail if they're not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

haha it compiled, but when reading from the database the bools were in the wrong place. I just ran into the bug while testing today lol

@dessalines
Copy link
Member

I'll make sure @Nutomic approve too before merging.

embed_title,
embed_description,
embed_video_url,
thumbnail_url,
ap_id: Some(page.id.clone().into()),
local: Some(false),
language_id,
featured_community: page.stickied,
featured_local: Some(false),
Copy link
Member

Choose a reason for hiding this comment

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

Its possible that a local admin features a remote post. In case the post creator edits the post, this would overwrite the value for featured. You should just set None here so it keeps the existing value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay fixed

@@ -227,7 +228,8 @@ impl ApubObject for ApubPost {
.community_id(community.id)
.ap_id(Some(page.id.clone().into()))
.locked(page.comments_enabled.map(|e| !e))
.stickied(page.stickied)
.featured_community(page.stickied)
.featured_local(Some(false))
Copy link
Member

Choose a reason for hiding this comment

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

Same here (you can just remove the line).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay fixed

@Nutomic Nutomic enabled auto-merge (squash) December 3, 2022 20:00
@Nutomic Nutomic disabled auto-merge December 5, 2022 10:56
@Nutomic
Copy link
Member

Nutomic commented Dec 5, 2022

CI wasnt working properly for this repo, hopefully its fixed now. Anyway there is a clippy error that needs to be fixed:

error: you are deriving `PartialEq` and can implement `Eq`
--> crates/db_schema/src/lib.rs:100:84
|
100 | #[derive(EnumString, Display, Debug, Serialize, Deserialize, Clone, Copy, Default, PartialEq)]
|                                                                                    ^^^^^^^^^ help: consider deriving `Eq` as well: `PartialEq, Eq`
|
= note: `-D clippy::derive-partial-eq-without-eq` implied by `-D clippy::style`
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#derive_partial_eq_without_eq

@Nutomic Nutomic enabled auto-merge (squash) December 5, 2022 12:55
auto-merge was automatically disabled December 5, 2022 15:28

Head branch was pushed to by a user without write access

@Nutomic Nutomic enabled auto-merge (squash) December 5, 2022 15:36
auto-merge was automatically disabled December 5, 2022 21:25

Head branch was pushed to by a user without write access

@makotech222
Copy link
Contributor Author

Looks like the federation tests have failed. Looks like this will have to wait until the lemmy-js-client is updated.

@Nutomic Nutomic merged commit 9dfd819 into LemmyNet:main Dec 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants