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 option to be notified when a followed user posts #13546

Merged
merged 6 commits into from
Sep 18, 2020
Merged

Conversation

Gargron
Copy link
Member

@Gargron Gargron commented Apr 25, 2020

Equivalent of YouTube's "bell" button.

Fix #4890

To support changing only notify or only show_reblogs settings for follows, without requiring the client to remember and send along the other value every time, I've changed how defaults for those values are set. The import from CSV service was relying on old defaults behaviour because a CSV import where columns are omitted means we want to reset the omitted values back to defaults, so I had to change that service.

@Gargron Gargron force-pushed the feature-bell branch 2 times, most recently from dab9c8b to 093028d Compare April 26, 2020 19:04
@Gargron Gargron changed the title Add bell button Add option to be notified when a followed user posts Apr 26, 2020
@mstdn
Copy link

mstdn commented Apr 27, 2020

This would be a amazing feature

@brawaru
Copy link
Contributor

brawaru commented Jul 16, 2020

@Gargron any chance to revive this after 3.2.0 is released? :meowowo:

@Gargron Gargron force-pushed the feature-bell branch 2 times, most recently from 9dbf09d to 096cbee Compare September 14, 2020 21:12
@Gargron Gargron marked this pull request as ready for review September 14, 2020 21:13
app/models/notification.rb Outdated Show resolved Hide resolved
app/workers/feed_insert_worker.rb Outdated Show resolved Hide resolved
app/workers/local_notification_worker.rb Outdated Show resolved Hide resolved
db/schema.rb Outdated Show resolved Hide resolved
db/schema.rb Outdated Show resolved Hide resolved
spec/services/notify_service_spec.rb Outdated Show resolved Hide resolved
@Gargron Gargron force-pushed the feature-bell branch 2 times, most recently from b781412 to 9f5affb Compare September 17, 2020 19:46
@ClearlyClaire
Copy link
Contributor

Now, the schema.rb file is somehow missing the new columns…

@Gargron
Copy link
Member Author

Gargron commented Sep 17, 2020

Hi @takayamaki, I have a question. I am creating a new index on notifications (account_id, type) for this PR. Would it make sense to add the order clause to it and would that make the (account_id, id) index redundant? When I try to EXPLAIN a typical notifications query, it seems to (currently, before this PR) use the account_activity index (account_id, activity_id, activity_type), I'm not sure when (account_id, id) is used?

@Gargron Gargron merged commit 974b1b7 into master Sep 18, 2020
@Gargron Gargron deleted the feature-bell branch September 18, 2020 15:26
thenameisnigel-old pushed a commit to ChatterlyOSE/Chatterly that referenced this pull request Sep 18, 2020
* Add bell button

Fix mastodon#4890

* Remove duplicate type from post-deployment migration

* Fix legacy class type mappings

* Improve query performance with better index

* Fix validation

* Remove redundant index from notifications
@takayamaki
Copy link
Contributor

Would it make sense to add the order clause to it?

When no order specified, sorting are not cause. The records will probably be implicitly sorted by ID.

When specified created_at, it may cause sort. However it may same as liner scan(created_at may are same as id usually).

Other cases, insert order condition column after account_id.

would that make the (account_id, id) index redundant?

May be yes.
All indexes have primary key as last column implicitly.
In this case, primary key is id.
So, difference is that id are asc or desc.
How effect this difference to performance, I seem little.

When I try to EXPLAIN a typical notifications query, it seems to (currently, before this PR) use the account_activity index (account_id, activity_id, activity_type), I'm not sure when (account_id, id) is used?

fmm…
index (account_id, activity_id, activity_type) covers (account_id, id). The former will usually be used.

It may be used when the index becomes very very very large and the postgres planner wants to reduce the reading of the index.

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.

Feature Request: notifications for all toots of particular users
6 participants