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

feat: add block mutations #2377

Merged
merged 5 commits into from
Nov 5, 2024
Merged

feat: add block mutations #2377

merged 5 commits into from
Nov 5, 2024

Conversation

capJavert
Copy link
Contributor

@capJavert capJavert commented Oct 31, 2024

AS-651

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.

Nothing blocking

src/common/contentPreference.ts Show resolved Hide resolved
_: true,
};
},
block: async (
Copy link
Contributor

Choose a reason for hiding this comment

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

Not 100% sure, but do we also need to introduce the other side backward compatibility?
So the current block flow to set contentPreference?

(Maybe you did this in other PR already, can't recall)

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 did add it, blockKeyword also saves to Feedtag with blocked: true and blockSource saves to FeedSource.

Do we do anything additional with blocking?

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 good to me. One question though, why does blocking and unblocking looks similar in terms of notificationPreference. They both have 0 entry on that entity, shouldn't we add an entry with blocked = true so that even default notifications won't be sent as they are blocked, if I understand correctly.

@capJavert
Copy link
Contributor Author

@sshanzel in both cases we wont to remove all notification preferences so user is not subscribed to anything, does that make sense?

They both have 0 entry on that entity, shouldn't we add an entry with blocked = true so that even default notifications won't be sent as they are blocked, if I understand correctly.

We do add blocked: true for FeedTag and FeedSource for backward compatibility.

@sshanzel
Copy link
Member

sshanzel commented Nov 4, 2024

@sshanzel in both cases we wont to remove all notification preferences so user is not subscribed to anything, does that make sense?

They both have 0 entry on that entity, shouldn't we add an entry with blocked = true so that even default notifications won't be sent as they are blocked, if I understand correctly.

We do add blocked: true for FeedTag and FeedSource for backward compatibility.

Thank you for the clarity, yeah, I was just wondering if it should also be the case for User entity. This is so, for example, when you are mentioned by the blocked user, you won't get notified, since your preference is blocked.

@capJavert
Copy link
Contributor Author

@sshanzel yeah, currently you can't block an user (at least from UI) so it will be a silent feature, but for sure good idea for when we want to introduce it.

Comment on lines -17 to -18
@Column({ type: 'text', default: SourceMemberRoles.Member })
role: SourceMemberRoles;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Leaving role in db for now so I can copy data

@capJavert capJavert merged commit e5eb193 into main Nov 5, 2024
8 checks passed
@capJavert capJavert deleted the AS-651-block-mutations branch November 5, 2024 11:31
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