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

Community post tags (part 1) #4997

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Community post tags (part 1) #4997

wants to merge 3 commits into from

Conversation

phiresky
Copy link
Collaborator

@phiresky phiresky commented Sep 2, 2024

This is the first part of an implementation of post tags based on LemmyNet/rfcs#4.

It implements the following:

  • The necessary DB tables
  • Fetching the post tags while reading PostView (list and single)
  • Tests for the above

What is missing:

  • New API methods (for fetching tags of a community (public) and for updating post tags (creator+mods)
  • Updated post create method (for adding tags directly when posting
  • Filtering by post tag in post view (I would consider this out of scope for now)
  • Federation of tags and tag updates
  • The frontend (select tags on post creation+edit and show post tags in post listings)

I would like to propose that we work on and merge this (and potentially other larger changes) in multiple parts, as soon as each part is ready and ofc doesn't negatively affect the existing functionality. That way the branches don't live for months and diverge more and more and each chunk stays simple and can be reviewed quickly.

There's some somewhat open questions wrt this code:

  • Table naming: Currently I name the table that contains the data community_post_tag (id, apub_id, name, url) so it's clear this is a tag that belongs to one community and tags posts in that community. This way we can later more tag types that may have different meanings and especially different properties and access control. For example global_post_tag for tags that are the same / interpretable globally (like NSFW). The n:m association table is called post_community_post_tag (post_id, tag_id).
  • The way the tags of a post are fetched. See the comment in the post_view code

published timestamptz NOT NULL DEFAULT now(),
updated timestamptz,
deleted timestamptz
);
Copy link
Member

@Nutomic Nutomic Sep 16, 2024

Choose a reason for hiding this comment

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

How about renaming this to post_tag and later adding a flag global: bool? Then global tags wont need their separate table, query and api endpoints.

Also the name post_community_post_tag could be simplified to post_tag_relation then

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, if we only expect global tags then it makes sense I think. I'm just not sure whether we should plan for more unknowns, because

  • the same tag elements are also incoming from other AP software, but those might have pretty different structure, so can potentially not be handled in the same table
  • there are other types of tags we might want, for example foreign community tags (e.g. fact-checking posts in communities by third parties similar to Community Notes on twitter), or tags with values etc, that are also "post tags" but need to be handled differently as well

Copy link
Member

Choose a reason for hiding this comment

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

the same tag elements are also incoming from other AP software, but those might have pretty different structure, so can potentially not be handled in the same table

Everything that federates needs to fit into our data formats, otherwise it gets ignored completely.

there are other types of tags we might want, for example foreign community tags (e.g. fact-checking posts in communities by third parties similar to Community Notes on twitter), or tags with values etc, that are also "post tags" but need to be handled differently as well

Sounds like an entirely separate feature so I wouldnt worry about it now.

Copy link
Member

@dessalines dessalines Sep 16, 2024

Choose a reason for hiding this comment

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

I would go ahead and create the global tag table, that contains all those columns above (except community_id). We can use that table as the primary store for all kinds of tags in the future, and be the main table that's joined to.

When tagging things later, we can check that its one of the allowed tags for that item. IE we can check that a given tag is in the person_tag table, or the community_post_tag table, before we let the item be tagged via the API.

published timestamptz NOT NULL DEFAULT now(),
updated timestamptz,
deleted timestamptz
);
Copy link
Member

@dessalines dessalines Sep 16, 2024

Choose a reason for hiding this comment

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

I would go ahead and create the global tag table, that contains all those columns above (except community_id). We can use that table as the primary store for all kinds of tags in the future, and be the main table that's joined to.

When tagging things later, we can check that its one of the allowed tags for that item. IE we can check that a given tag is in the person_tag table, or the community_post_tag table, before we let the item be tagged via the API.

@@ -0,0 +1,18 @@
-- a tag for a post, valid in a community. created by mods of a community
CREATE TABLE community_post_tag (
Copy link
Member

Choose a reason for hiding this comment

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

Pry just rename this one tag.

CREATE TABLE community_post_tag (
id serial PRIMARY KEY,
ap_id text NOT NULL UNIQUE,
community_id int NOT NULL REFERENCES community (id) ON UPDATE CASCADE ON DELETE CASCADE,
Copy link
Member

Choose a reason for hiding this comment

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

Remove this linking column, and create a new table called community_post_tag, with a primary key of (community_id, tag_id) . Also add a published column

Check out any of the other bridge tables, like post_read, community_block, etc for examples.

-- an association between a post and a community post tag. created/updated by the post author or mods of a community
CREATE TABLE post_community_post_tag (
post_id int NOT NULL REFERENCES post (id) ON UPDATE CASCADE ON DELETE CASCADE,
community_post_tag_id int NOT NULL REFERENCES community_post_tag (id) ON UPDATE CASCADE ON DELETE CASCADE,
Copy link
Member

@dessalines dessalines Sep 16, 2024

Choose a reason for hiding this comment

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

This can just reference tag_id , and in the API we can double check to make sure that tag exists and is one of the allowed ones in community_post_tag.

And also of course, in the front-ends, it should only show the correct subset of tags anyway (probably via a searchable dropdown or something).

@@ -183,7 +183,10 @@ pub fn config(cfg: &mut web::ServiceConfig, rate_limit: &RateLimitCell) {
.route("/follow", web::post().to(follow_community))
.route("/block", web::post().to(block_community))
.route("/delete", web::post().to(delete_community))
// .route("/post_tags", web::get().to(get_community_post_tags))
Copy link
Member

Choose a reason for hiding this comment

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

Can mark as draft until this is ready.

Copy link
Collaborator Author

@phiresky phiresky Sep 16, 2024

Choose a reason for hiding this comment

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

I would do this in a separate pr after this is merged

CREATE TABLE post_community_post_tag (
post_id int NOT NULL REFERENCES post (id) ON UPDATE CASCADE ON DELETE CASCADE,
community_post_tag_id int NOT NULL REFERENCES community_post_tag (id) ON UPDATE CASCADE ON DELETE CASCADE,
PRIMARY KEY (post_id, community_post_tag_id)
Copy link
Member

Choose a reason for hiding this comment

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

Then this would become (post_id, tag_id)

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