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.

Sign up for GitHub

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

Jump to bottom

Add in-app notification configuration #8375

Open
wants to merge 14 commits into
base: develop
Choose a base branch
from
Open

Add in-app notification configuration #8375

Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
4be89fc
Add schema changes to user preferences to support in notifications
cmrd-senya Jul 21, 2022
048b204
Update code to use email_enabled attribute instead of record presence
cmrd-senya Jul 21, 2022
e55b876
Update specs for #update_user_preferences
cmrd-senya Jul 23, 2022
8abf74c
Use setting for in app notification disabled
cmrd-senya Jul 23, 2022
8e221db
Add frontend for in app notification settings
cmrd-senya Jul 23, 2022
0b0e146
Notification settings: add header
cmrd-senya Jul 23, 2022
1c4a897
Notification settings: update specs
cmrd-senya Jul 23, 2022
dede976
Notification settings: fix cucumber tests
cmrd-senya Jul 23, 2022
04dcbf6
Remove debug comment
cmrd-senya Jul 23, 2022
790117c
Merge branch 'develop' into 7686-db-schema-update
cmrd-senya Jul 31, 2022
f92775b
Notifications: move notify methods to services from models
cmrd-senya Aug 18, 2022
79031b3
Merge remote-tracking branch 'origin/develop' into 7686-db-schema-update
cmrd-senya Aug 18, 2022
80759b7
Notifications: make email sending independend from notification model
cmrd-senya Aug 18, 2022
90678ab
Move mail method from User model to NotificationService
cmrd-senya Aug 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 10 additions & 0 deletions db/migrate/20220721190153_add_email_enabled_to_user_preferences.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
# frozen_string_literal: true

class AddEmailEnabledToUserPreferences < ActiveRecord::Migration[6.1]
cmrd-senya marked this conversation as resolved.
Show resolved Hide resolved
def change
change_table :user_preferences, bulk: true do |t|
t.boolean :email_enabled, null: false, default: false
t.boolean :in_app_enabled, null: false, default: true
end
Comment on lines +5 to +8
Copy link
Member

Choose a reason for hiding this comment

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

I would like to suggest some general changes regarding this whole thing, as this is touched now anyways. To me the naming of this table never made sense, as it's specific a table for notifications and not about "user preferences" ... so I would suggest renaming it to something that explains what this is, for example notification_settings (as you named the service NotificationSettingsService), and then also rename the variables and everything to something regarding notifications. I think that will make the code overall more readable because I was always confused about this.

Also, after thinking about it a bit more, I don't think the _enabled suffix for the new columns is needed, I think it's self-explained anyway, but if you want to keep it, that's fine for me too.

Also, when you are at it, can you rename the existing email_type column to just type, since it's not about emails anymore. I also think that will make this more readable for the future.

end
end