Skip to content

Add notification policies and notification requests#29366

Merged
Gargron merged 1 commit intomainfrom
feature-notifications-filter
Mar 7, 2024
Merged

Add notification policies and notification requests#29366
Gargron merged 1 commit intomainfrom
feature-notifications-filter

Conversation

@Gargron
Copy link
Member

@Gargron Gargron commented Feb 22, 2024

Allow users to set a policy for who they want to see notifications from. The rest appear as notification requests that can be perused and accepted separately.

Instead of silently dropping messages from limited accounts, filter them and allow users to peruse them as notification requests, even if the user has no policy to filter notifications set.

REST API:

  • GET /api/v1/notifications/requests
  • POST /api/v1/notifications/requests/:id/accept
  • Post /api/v1/notifications/requests/:id/dismiss
  • GET|PUT /api/v1/notifications/policy

Fixes MAS-39, fixes #12181, fixes #28429, fixes #24117

@Gargron Gargron added the api REST API, Streaming API, Web Push API label Feb 22, 2024
@Gargron Gargron force-pushed the feature-notifications-filter branch 3 times, most recently from 5e628d8 to 183d00b Compare February 22, 2024 22:18
@Gargron Gargron changed the title Notification policies and notification requests Add notification policies and notification requests Feb 22, 2024
@Gargron Gargron force-pushed the feature-notifications-filter branch 6 times, most recently from 728af2b to 436c814 Compare February 23, 2024 04:02
@Gargron Gargron force-pushed the feature-notifications-filter branch 3 times, most recently from 35a5501 to 0b486d8 Compare February 26, 2024 07:29
@Gargron Gargron marked this pull request as ready for review February 26, 2024 07:29
Copy link
Contributor

@ClearlyClaire ClearlyClaire left a comment

Choose a reason for hiding this comment

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

I really like the idea but I'm very worried that must_be_following_dm is gone with no equivalent. I really do think Private Mentions warrant their own filter condition.

Otherwise, this looks good, but I have left a few inline comments.

@Gargron Gargron force-pushed the feature-notifications-filter branch 6 times, most recently from 5dbb26a to cfd1f2b Compare February 27, 2024 19:20
Copy link
Contributor

@ClearlyClaire ClearlyClaire left a comment

Choose a reason for hiding this comment

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

This looks good overall, and I'm happy the Private Mention filter is back in some form!

I have a few concerns still:

  • I think we should preserve the current settings for existing users as much as possible, since changes will be unexpected and possibly disruptive, especially for people who use apps that won't immediately handle the filtered notification settings
  • is dismissing notification requests intended to dismiss them forever? If so, we should probably clean up and avoid creating new notifications for dismissed requests, as well as provide a way to undo the action, as is currently possible with mutes. If not, that needs to be changed.

@Gargron Gargron force-pushed the feature-notifications-filter branch from 86e01b0 to 5db7ade Compare March 5, 2024 16:10
@Gargron
Copy link
Member Author

Gargron commented Mar 5, 2024

I added a way to retrieve dismissed notification requests in the API. The accept method does not require the request to not be dismissed, so dismissed requests could still be accepted by the user if they change their mind.

@Gargron Gargron force-pushed the feature-notifications-filter branch from 5db7ade to cf6a8e3 Compare March 6, 2024 00:51
@Gargron Gargron enabled auto-merge March 6, 2024 02:10
@Gargron Gargron requested a review from ClearlyClaire March 6, 2024 02:10
@Gargron Gargron force-pushed the feature-notifications-filter branch from cf6a8e3 to 41d99af Compare March 6, 2024 14:01
Copy link
Member

@renchap renchap left a comment

Choose a reason for hiding this comment

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

I did not review this, but all the previous comments have been addressed.

Let's merge this, and another PR if some things needs to be adjusted.

@Gargron Gargron added this pull request to the merge queue Mar 7, 2024
Merged via the queue into main with commit 50b17f7 Mar 7, 2024
@Gargron Gargron deleted the feature-notifications-filter branch March 7, 2024 14:58
@ShadowJonathan
Copy link
Contributor

I see that this does not edit any UI elements, is there a PR somewhere which will add these? 👀

@renchap
Copy link
Member

renchap commented Mar 7, 2024

Yes, #29433

lutoma pushed a commit to ohaisocial/mastodon that referenced this pull request Mar 19, 2024
@mcclure
Copy link

mcclure commented Mar 21, 2024

Hello, I contribute to the Tusky project. I do not find (or am I just missing it?) the four new endpoints in the API page https://docs.joinmastodon.org/api/ , and the notes above are not sufficient to implement support for this feature (what payload does /api/v1/notifications/policy receive?).

When can we expect a full documentation of this feature? If content is being hidden from third party clients unless they implement a feature which is only documented as Ruby source, that is problematic.

@renchap
Copy link
Member

renchap commented Mar 21, 2024

Yes, we have an internal ticket to add API documentation for this.

ClearlyClaire added a commit that referenced this pull request Jun 5, 2024
In #29366, it was intended that notifications from limited non-followed users
would end up in filtered notifications instead of being outright dropped.

However, those notifications were still dropped in an earlier check.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api REST API, Streaming API, Web Push API

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Spam/Junk inbox, "Quarantine" Notifications from limited accounts do not arrive for admins/mods Account quality filter for notifications

6 participants