SpringAll
SpringAll copied to clipboard
wuyouzhuguli
Add in-app notification configuration
fix #7686
This PR implements in-app notification configuration feature - ability to turn off in-app notifications per event similar to email. See the comments for screenshots.
Original comment (outdated)
This PR is rather trivial... I don't know if we are okay with moving things in tiny steps. Tell me if we're not.
Our database schema for user notification preferences needs to be extended if we want to support another type of notifications, i.e. in app notifications.
Currently the presence of the user preference record means that the email notification is disabled while absence means it is enabled.
If we want to add in-app notifications and if we want to make them configurable independently from email notifications then we need to support 4 states, not just 2 like now.
Therefore I added 2 new columns -- email_enabled and in_app_enabled.
The default states of the columns correspond current behavior. This is done for backward compatibility. In app notifications are enabled because they are always enabled currently.
Email notifications are disabled because the presence of the record means they are disabled.
Having null: false and the default value set will "back fill" the existing records to correct values in the databases during the migration or lazily depending on the DB type and version.
In the next step (which I imagine to be a separate PR) we can update the existing code to check for email_enabled field value instead of the presence of the record. This will prepare us for the scenarios where the record exists but email_enabled is true.
We can also rename email_type column to notification_type sometime later.
Small question regarding your plan for the next steps: are you planning to insert a true/true entry for every notification type and user, or do you want to still only inserting stuff when the user changes the defaults, but assume true/true if there is no entry?
I was thinking about the latter - assuming true/true when there is no entry
Yes, I also prefer it that way I think. Keeping the table full of true/true for all users would be more complicated (we would also need to insert new entries every time we add a new notification type, for all users), and I think using the table only to store the state for users who changed away from the default makes more sense. And it's also how the table is used at the moment, because we already have entries for all users who changed away from the default :+1:
Maybe add an explanation or a header at the top of the two columns to explain what they mean ... because at the moment somebody could also understand it as they receive a private message instead of a mail? :thinking:
Maybe add an explanation or a header at the top of the two columns to explain what they mean ... because at the moment somebody could also understand it as they receive a private message instead of a mail? thinking
Yes, I thought about that too. I can add a header. Maybe also some kind of tooltip could be helpful?
Yes, a tooltip would also work, but tooltips only work on desktop, not on mobile with touch as you can't hover over things.
@SuperTux88 I think it can be reviewed now. Let me know if I should add some additional specs.
Thanks for the feedback, indeed, I didn't realize that sending email notifications depends on the Notification record presence. I can see how to fix that but it'll require a bit more refactoring. I'll work on it.
@SuperTux88 I'm thinking about moving these self.notify methods out of the models:
https://github.com/diaspora/diaspora/blob/e82690963dc55d10491b8da6b4fc0b6f397bea6d/app/models/notifications/comment_on_post.rb#L15
I'd like to put the code to the "service" layer, possibly creating a new service object for it. There is already NotificationService but it will be too much to put everything there so I'd rather create another one. Could be something like NotifyUserService or SendNotificationService. I like service names to be more like verbs than like nouns. NotificationService#notify method could also be moved to that new service too. Basically NotificationService is doing a few different things right now which don't need to be kept in one class, I think that a service should implement one action, not a few different like index, create and update at once.
How does it sound? Would it be okay?
Sounds good to me so far. I don't mind too much if one service does multiple things about notifications, or if there are different services for it. But if it otherwise would become too big, it's probably better to split them up so it's clearer to read what belong together and also maybe easier to test. As the current notify method doesn't add a lot to the NotificationService at the moment, so it was fine to have it in there, but if it now gets more logic to differ between mail and in-app, it probably makes sense to split stuff up a bit. But I don't know what exactly is needed yet, but I think you'll figure that out as you go :+1:
@SuperTux88 so after some thought I went with the following solution: I moved all self.notify methods from the notification models to respective services which I put to app/services/notifications/*_service.rb (commit). Then I changed the self.notify methods in the services the way that the email sending code wasn't dependent on existing notification record instance (commit).
How do you like that?
I haven't updated the specs yet, thought better to discuss if the refactoring looks okay.
I'm also thinking of moving User#mail method somewhere to the services from the user model.
Thanks for the detailed review @SuperTux88! Gonna work through this :+1: