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.

Sign up for GitHub

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

Jump to bottom

feat: vordr on posts #2108

Merged
merged 27 commits into from
Aug 13, 2024
Merged

feat: vordr on posts #2108

merged 27 commits into from
Aug 13, 2024

Conversation

omBratteng
Copy link
Member

@omBratteng omBratteng commented Aug 6, 2024 •
edited
Loading

Marking as ready for review, but I'm working on tests for:

  • add vordr check to share post
  • set vordr flag on submissions
  • hide posts that vordr prevented
  • don't send notification if vordr prevented post

idoshamun
idoshamun reviewed Aug 7, 2024
View reviewed changes
Copy link
Member

@idoshamun idoshamun 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, we need to remember the notification as well

@omBratteng
Copy link
Member Author

Looks good, we need to remember the notification as well

Yeah, working on that part

@omBratteng omBratteng mentioned this pull request Aug 8, 2024
Closed
@omBratteng omBratteng marked this pull request as ready for review August 8, 2024 19:11
@omBratteng omBratteng requested a review from a team as a code owner August 8, 2024 19:11
capJavert
capJavert reviewed Aug 9, 2024
View reviewed changes
Copy link
Contributor

@capJavert capJavert left a comment

Choose a reason for hiding this comment

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

Vordr getting more power 💪 Some general comments, also missing few tests here and there.

__tests__/feeds.ts Outdated Show resolved Hide resolved
__tests__/workers/notifications/postAdded.ts Outdated Show resolved Hide resolved
src/common/feedGenerator.ts Show resolved Hide resolved
src/entity/posts/utils.ts Show resolved Hide resolved
src/common/vordr.ts Outdated Show resolved Hide resolved
src/workers/notifications/postAdded.ts Show resolved Hide resolved
rebelchris
rebelchris requested changes Aug 9, 2024
View reviewed changes
Copy link
Contributor

@rebelchris rebelchris left a comment

Choose a reason for hiding this comment

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

Minor comments, good work here!

__tests__/comments.ts Outdated
@@ -978,6 +978,22 @@ describe('mutation commentOnPost', () => {
expect(comment.flags).toEqual({ vordr: false });
});

it('should set correct vordr flags on good user if vordr filter catches it', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe name them explicit for post/comment jest doesn't like same names so much and I actually thought for a second it was duplicated 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

Renamed tests

__tests__/posts.ts Outdated Show resolved Hide resolved
src/common/post.ts
Comment on lines +197 to +198
createdPost.banned = true;
createdPost.showOnFeed = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think @capJavert was right we also need to set it as flags.
Actually Ante, not sure now which one we use, maybe we should only use either raw or the flags?

Copy link
Member Author

Choose a reason for hiding this comment

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

I asked Ido, and he said to use column. But I can just set it both places to be sure

Copy link
Member Author

Choose a reason for hiding this comment

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

I set both column of flags now

rebelchris
rebelchris approved these changes Aug 9, 2024
View reviewed changes
Copy link
Contributor

@rebelchris rebelchris left a comment

Choose a reason for hiding this comment

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

Minor suggestion, but otherwise no blocking issues from my side.
(Just wait for 2 approvals please)

src/common/post.ts Outdated
createdPost.flags.banned = true;

createdPost.showOnFeed = false;
createdPost.flags.showOnFeed = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

You might want to use updateFlagsStatement here for safety :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I get this TS error then

Type '() => string' has no properties in common with type 'Partial<{ sentAnalyticsReport: boolean; banned: boolean; deleted: boolean; private: boolean; visible: boolean; showOnFeed: boolean; promoteToPublic: number; deletedBy: string; vordr: boolean; }>'.ts(2559)

I think it should be fine to not use it, as it hasn't been saved in the database yet, so it's still a js object.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can do this instead

createdPost.flags = {
  ...createdPost.flags,
  banned: true,
  showOnFeed: false,
};

Copy link
Contributor

Choose a reason for hiding this comment

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

CC: @capJavert up to you :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@omBratteng for updateFlagsStatement you have to use insert or update methods since save does not support sql statements, since we are just creating a post we can probably use insert here.

Copy link
Contributor

Choose a reason for hiding this comment

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

But I think we don't have to use updateFlagsStatement here since as @omBratteng said it has not been saved to DB yet.

Copy link
Member Author

Choose a reason for hiding this comment

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

@capJavert went with this as it's before insertion to DB.

createdPost.flags = {
  ...createdPost.flags,
  banned: true,
  showOnFeed: false,
};

src/entity/posts/utils.ts Outdated

if (vordrStatus === true) {
createdPost.banned = true;
createdPost.flags.banned = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here updateFlagsStatement

@omBratteng omBratteng requested a review from idoshamun August 12, 2024 15:50
@omBratteng omBratteng enabled auto-merge (squash) August 13, 2024 10:50
@omBratteng omBratteng merged commit 357151c into main Aug 13, 2024
8 checks passed
@omBratteng omBratteng deleted the AS-463-vordr-posts branch August 13, 2024 10:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rebelchris rebelchris rebelchris approved these changes

@capJavert capJavert capJavert approved these changes

@ilasw ilasw Awaiting requested review from ilasw ilasw is a code owner automatically assigned from dailydotdev/web-team

@sshanzel sshanzel Awaiting requested review from sshanzel sshanzel is a code owner automatically assigned from dailydotdev/web-team

@nensidosari nensidosari Awaiting requested review from nensidosari nensidosari is a code owner automatically assigned from dailydotdev/web-team

@idoshamun idoshamun Awaiting requested review from idoshamun

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@omBratteng @rebelchris @idoshamun @capJavert