Skip to content

Add ability to disable notifications#2937

Closed
unixfox wants to merge 1 commit intomasterfrom
disable-notifications
Closed

Add ability to disable notifications#2937
unixfox wants to merge 1 commit intomasterfrom
disable-notifications

Conversation

@unixfox
Copy link
Member

@unixfox unixfox commented Feb 25, 2022

This PR adds the ability to disable notifications.


In error:

Showing last frame. Use --error-trace for full trace.

In src/invidious/channels/channels.cr:229:8

 229 | if preferences.notifications && was_insert
          ^----------
Error: undefined local variable or method 'preferences' for top-level

@unixfox unixfox requested review from a team and SamantazFox as code owners February 25, 2022 09:33
@unixfox unixfox force-pushed the disable-notifications branch from cb6619e to 25a1210 Compare February 25, 2022 09:34
@unixfox unixfox force-pushed the disable-notifications branch from 25a1210 to 72f03cd Compare February 25, 2022 09:43
Comment on lines +267 to +268
if preferences.notifications && was_insert
Invidious::Database::Users.add_notification(video)
Copy link
Member

@SamantazFox SamantazFox Feb 25, 2022

Choose a reason for hiding this comment

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

Note that you can't check the preferences in here: this function is called by the refresh job and hence lacks any user context. We need to add a column to the postgres table users so we can do the following:

UPDATE ... 
SET array_append(subscriptions, '<video_id>')
WHERE notifications = true AND '<ucid>' = ANY(subscriptions)

Copy link
Member Author

Choose a reason for hiding this comment

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

We can't do that until we have #2878 merged then?

Copy link
Member Author

@unixfox unixfox Feb 28, 2022

Choose a reason for hiding this comment

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

Mmh the query you are talking about, is it this one: https://github.com/iv-org/invidious/blob/master/src/invidious/database/users.cr#L100?

We could just temporarily convert the preferences column to a json object then check in a where clause which user has notifications enabled.

Copy link
Member

@SamantazFox SamantazFox Feb 28, 2022

Choose a reason for hiding this comment

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

Yep, it's better to wait for 2878 to be merged, so we can start some work on the DB.
And no, the function that updates notifications is this one:

def add_notification(video : ChannelVideo)
request = <<-SQL
UPDATE users
SET notifications = array_append(notifications, $1),
feed_needs_update = true
WHERE $2 = ANY(subscriptions)
SQL
PG_DB.exec(request, video.id, video.ucid)
end
. the one you gave is the function used when a user subscribes to a channel.

As for the temporary solution, Imo it would be preferrable to not clutter the code now.

@SamantazFox
Copy link
Member

@unixfox do you mind if I close this PR?

@unixfox
Copy link
Member Author

unixfox commented Apr 3, 2022

@unixfox do you mind if I close this PR?

I don't know why you really want to close this PR but if you think you have to do it then you can close it.

@SamantazFox
Copy link
Member

Related: #1998 #1999

@unixfox unixfox marked this pull request as draft April 20, 2022 13:08
@github-actions
Copy link

This pull request has been automatically marked as stale and will be closed in 30 days because it has not had recent activity and is much likely abandoned or outdated. If you think this pull request is still relevant and applicable, you just have to post a comment and it will be unmarked.

@github-actions github-actions bot added the stale label Jun 17, 2022
@unixfox unixfox removed the stale label Jun 17, 2022
@github-actions
Copy link

github-actions bot commented Oct 9, 2022

This pull request has been automatically marked as stale and will be closed in 30 days because it has not had recent activity and is much likely abandoned or outdated. If you think this pull request is still relevant and applicable, you just have to post a comment and it will be unmarked.

@github-actions github-actions bot added the stale label Oct 9, 2022
@unixfox unixfox removed the stale label Oct 9, 2022
@github-actions
Copy link

This pull request has been automatically marked as stale and will be closed in 30 days because it has not had recent activity and is much likely abandoned or outdated. If you think this pull request is still relevant and applicable, you just have to post a comment and it will be unmarked.

@github-actions github-actions bot added the stale label Nov 23, 2022
@unixfox unixfox removed the stale label Nov 23, 2022
@github-actions
Copy link

github-actions bot commented Jan 8, 2023

This pull request has been automatically marked as stale and will be closed in 30 days because it has not had recent activity and is much likely abandoned or outdated. If you think this pull request is still relevant and applicable, you just have to post a comment and it will be unmarked.

@github-actions github-actions bot added the stale label Jan 8, 2023
@SamantazFox
Copy link
Member

I guess this can be closed, now that #3473 has been merged?

@unixfox
Copy link
Member Author

unixfox commented Jan 8, 2023

I guess this can be closed, now that #3473 has been merged?

No it's per user here, no globally.

@github-actions github-actions bot removed the stale label Jan 9, 2023
@github-actions
Copy link

This pull request has been automatically marked as stale and will be closed in 30 days because it has not had recent activity and is much likely abandoned or outdated. If you think this pull request is still relevant and applicable, you just have to post a comment and it will be unmarked.

@github-actions github-actions bot added the stale label Feb 23, 2023
@github-actions github-actions bot closed this Mar 25, 2023
@unixfox unixfox deleted the disable-notifications branch May 9, 2025 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants