Skip to content

Interface to configure notification preferences#7001

Merged
tomhughes merged 3 commits intoopenstreetmap:masterfrom
pablobm:notification-preferences
May 6, 2026
Merged

Interface to configure notification preferences#7001
tomhughes merged 3 commits intoopenstreetmap:masterfrom
pablobm:notification-preferences

Conversation

@pablobm
Copy link
Copy Markdown
Contributor

@pablobm pablobm commented Apr 15, 2026

Closes #6897

This introduces a form under Preferences to configure notification preferences. At the moment this just means checkboxes for things like "I want to receive emails when I get changeset comments, but not when I receive diary comments".

The new panel. It's under Preferences, which now has three tabs: Interface, Notifications, and Advanced. The Notifications tab is selected and shows a list of checkboxes, allowing enabling/disabling email for each of the available notifications: changeset comments, diary comments, etc

I am storing preferences under user_preferences, in the format :k => "notification.changeset_comment.email"/:v => "t". The table is there and seems like the right place. I could have gone for an array value instead ("notification.changeset_comment"/["email"]), but my choice simplifies serializing/deserializing and is easier to query.

I have changed the labels in the Preferences section as they read "Preferences" a bit too redundantly. Let me know if you can think of better names, particularly for the first one.

@pablobm
Copy link
Copy Markdown
Contributor Author

pablobm commented Apr 15, 2026

This is currently deployed on the dev server at https://pablobm.apis.dev.openstreetmap.org. There are a couple of users you can use to try out things: mapper and anothermapper, both with password password.

@tomhughes
Copy link
Copy Markdown
Member

From on overall point of view, is it a good idea to let people start turning email notifications off before we have in-site notifications working?

@pablobm
Copy link
Copy Markdown
Contributor Author

pablobm commented Apr 15, 2026

As I understand it, the logic is that people are already ignoring/filtering their notification emails, so this doesn't make a big difference.

@1ec5
Copy link
Copy Markdown
Collaborator

1ec5 commented Apr 15, 2026

The only ones that are fairly critical are changeset comments and note comments. We could perhaps disable those checkboxes for now until the in-site option is available. Direct messages already have an in-site indicator of course. New followers might not matter to some users.

Comment thread app/models/user_notification_preferences.rb Outdated
@pablobm pablobm force-pushed the notification-preferences branch from 32d216b to db272de Compare April 16, 2026 10:42
@pablobm
Copy link
Copy Markdown
Contributor Author

pablobm commented Apr 16, 2026

We could perhaps disable those checkboxes for now until the in-site option is available.

That sounds reasonable to me. What do the maintainers think?

@pablobm
Copy link
Copy Markdown
Contributor Author

pablobm commented Apr 16, 2026

Forgot to mention: this doesn't include preferences for the notifications on GPX import success/failure, as that would require #6939 to be merged first. I think that's ok as they can be added in a later PR.

@pablobm pablobm force-pushed the notification-preferences branch 2 times, most recently from ae49354 to be33840 Compare April 16, 2026 20:37
@pablobm pablobm force-pushed the notification-preferences branch from be33840 to e381651 Compare April 23, 2026 13:42
@1ec5 1ec5 mentioned this pull request Apr 23, 2026
@pablobm pablobm force-pushed the notification-preferences branch from e381651 to 702dcd5 Compare April 29, 2026 12:13
@gravitystorm
Copy link
Copy Markdown
Collaborator

We could perhaps disable those checkboxes for now until the in-site option is available.

That sounds reasonable to me. What do the maintainers think?

I'm happy to let users control whether they receive notification emails. If we forbid them from turning them off, then they are more likely to get frustrated and/or mark the emails as spam.

I realise that we've managed a decade without being able to turn them off, but now that we have the option it would be unexpected to prohibit using it.

@tomhughes
Copy link
Copy Markdown
Member

As #6939 is now merged should we add the GPX notification to this before we merge it?

@pablobm pablobm force-pushed the notification-preferences branch from 702dcd5 to 53f9436 Compare May 5, 2026 06:55
@pablobm pablobm force-pushed the notification-preferences branch 2 times, most recently from f056b75 to 68662cb Compare May 5, 2026 14:54
@tomhughes
Copy link
Copy Markdown
Member

The last commit should probably add tests for the GPX notifiers that you've now added preferences for?

There also seems to be a failing test?

@pablobm
Copy link
Copy Markdown
Contributor Author

pablobm commented May 5, 2026

Yeah, sorry, I had to go in the middle of it. I'll complete tomorrow.

@pablobm pablobm force-pushed the notification-preferences branch from 68662cb to 7887698 Compare May 6, 2026 09:36
@github-actions github-actions Bot added the big-pr label May 6, 2026
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 6, 2026

1 Warning
⚠️ Number of updated lines of code is too large to be in one PR. Perhaps it should be separated into two or more?

Generated by 🚫 Danger

@pablobm pablobm force-pushed the notification-preferences branch from 7887698 to 81c3c70 Compare May 6, 2026 09:51
@pablobm pablobm force-pushed the notification-preferences branch from 81c3c70 to 1f8370e Compare May 6, 2026 09:59
@pablobm pablobm force-pushed the notification-preferences branch from 1f8370e to 8c56c6e Compare May 6, 2026 10:13
@pablobm
Copy link
Copy Markdown
Contributor Author

pablobm commented May 6, 2026

This should be ready now. Gets labelled as "big-pr" particularly because testing each notification type requires ~40 lines of code. Could be DRY'ed up, but I think it would make it more clever than actually maintainable.

@tomhughes
Copy link
Copy Markdown
Member

Thanks for all the work. I think this looks good now.

@tomhughes tomhughes merged commit 2b03312 into openstreetmap:master May 6, 2026
14 checks passed
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.

Page to configure notifications preferences

4 participants