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 local_subscribers field to CommunityAggregates. Fixes #4144 #4166

Conversation

ismailkarsli
Copy link
Contributor

I gave it a try.

@ismailkarsli ismailkarsli changed the title Add local_subscribers field to CommunityAggregates struct and schema. Fixes #4144 Add local_subscribers field to CommunityAggregates. Fixes #4144 Nov 15, 2023
@ismailkarsli
Copy link
Contributor Author

I couldn't get the local_subscriber tests right. If you have any suggestions where to do it, I can try.

@ismailkarsli
Copy link
Contributor Author

Pls some feedbacks lol

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.

See above. LMK if you need any help with this.

@ismailkarsli
Copy link
Contributor Author

I won't be able to complete this right now. Closing it to not keep stale.

@ismailkarsli
Copy link
Contributor Author

Add this trigger

CREATE FUNCTION delete_follow_before_person ()
    RETURNS TRIGGER
    LANGUAGE plpgsql
    AS $$
BEGIN
    DELETE FROM community_follower AS c
    WHERE c.person_id = OLD.id;
    RETURN OLD;
END;
$$;

CREATE TRIGGER delete_follow
    BEFORE DELETE ON person
    FOR EACH ROW
    EXECUTE FUNCTION delete_follow_before_person ();

Wait I've read this more carefully and it looks brilliant! It deletes follows before deleting person, which is better than duplicating same trigger. Absolutely better than my first solution idea 👌

@ismailkarsli
Copy link
Contributor Author

ismailkarsli commented Jan 2, 2024

Federation tests work when I test them locally with yalc, so there is no problem with the tests right now.

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.

I've deployed this now too, so you can add it to the package.json :

lemmy-js-client: 0.19.2-alpha.2

@ismailkarsli
Copy link
Contributor Author

Even though follow tests passed, federation tests gave the error below. It could be temporary or maybe about new lemmy-js-client. I'll check tomorrow.

/woodpecker/src/github.com/LemmyNet/lemmy/api_tests/node_modules/node-fetch/lib/index.js:1501
			reject(new FetchError(`request to ${request.url} failed, reason: ${err.message}`, 'system', err));
			       ^
FetchError {
  message: 'request to http://127.0.0.1:8541/api/v3/site? failed, reason: socket hang up',
  type: 'system',
  errno: 'ECONNRESET',
  code: 'ECONNRESET'
}

@ismailkarsli
Copy link
Contributor Author

ismailkarsli commented Jan 5, 2024

And yeah, it passed today without any changes.

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.

LGTM, thx!

Lets have @phiresky take a look also.

@dessalines dessalines merged commit 8670403 into LemmyNet:main Jan 24, 2024
1 check passed
@ismailkarsli ismailkarsli deleted the add_local_subscribers_to_community_aggregates branch January 29, 2024 07:52
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.

None yet

4 participants