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

fix: shared post empty state #2615

Merged
merged 9 commits into from
Jan 27, 2025
Merged

fix: shared post empty state #2615

merged 9 commits into from
Jan 27, 2025

Conversation

rebelchris
Copy link
Contributor

Couple things that happen here:

  1. Added CDC on hard deletion of post to also do all the cleanups (not sure why we didn't have this)
  2. Added a delete post mock (404 post, as we have ghost user)
  3. Update the delete shared post worker
    a. If has commentary we set it to not show on feed
    b. If no commentary we simply set as soft deleted, but add the new DELETED_BY_WORKER flag, this way no reputation is lost.

src/schema/posts.ts Outdated Show resolved Hide resolved
src/schema/posts.ts Outdated Show resolved Hide resolved
@rebelchris rebelchris requested a review from omBratteng January 24, 2025 12:26
@@ -21,6 +22,10 @@ const worker: Worker = {
typeof flags === 'string' ? JSON.parse(flags as string) : flags;
const { deletedBy } = parsedFlags;

if (deletedBy === DELETED_BY_WORKER) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nice

src/workers/postDeletedSharedPostCleanup.ts Outdated Show resolved Hide resolved
@rebelchris rebelchris requested a review from capJavert January 24, 2025 12:53
.createQueryBuilder()
.update()
.where({
sharedPostId: post.id,
Copy link
Member

Choose a reason for hiding this comment

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

Make sure we have index for sharedPostId

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do have one:
IDX_sharedPostId (it's just singular, but for this query probably ok)

Copy link
Member

Choose a reason for hiding this comment

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

Yes should be ok

Copy link
Member

@sshanzel sshanzel left a comment

Choose a reason for hiding this comment

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

Looks amazing. Just the order of the migration queries 🚀

@rebelchris rebelchris requested a review from idoshamun January 27, 2025 07:18
@rebelchris rebelchris merged commit 7430270 into main Jan 27, 2025
7 of 8 checks passed
@rebelchris rebelchris deleted the fix-shared-post-empty branch January 27, 2025 07:37
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.

5 participants