Added push notifications setting #356
Conversation
There was a problem hiding this comment.
Eventually, we'll need to create a user_settings table, moving all the settings from the users table to the new one. I thought that using this new approach (user_settings table) only for this new setting would actually create complexity instead of reducing it. I think that it would be better to implement user_settings once we can move all the other user attributes that are settings.
Totally agree
| @@ -494,8 +494,7 @@ pt-BR: | |||
| edit: | |||
| edit_user: Trocar usuário | |||
| form: | |||
| admin_warning: Atenção!!! Você está dando poderes a este usuário!! | |||
| superadmin_warning: Atenção!!! Você está dando PODERES DIVINOS a este usuário!! | |||
| notifications: Notificações | |||
| @@ -0,0 +1,5 @@ | |||
| class AddPushNotificationsToUsers < ActiveRecord::Migration | |||
| def change | |||
| add_column :users, :push_notifications, :boolean, default: true | |||
There was a problem hiding this comment.
I would add null: false. If I'm not mistaken, as it is, nothing prevents null values.
mllocs
force-pushed
the
feature/adds-user-push-notification-attribute
branch
from
1a3f5eb
to
72eedbf
Compare
db/schema.rb
Outdated
| t.datetime "created_at" | ||
| t.datetime "updated_at" | ||
| end | ||
|
|
db/schema.rb
Outdated
| end | ||
|
|
||
| add_index "user_settings", ["user_id"], name: "index_user_settings_on_user_id", using: :btree | ||
|
|
| def change | ||
| add_column :users, :push_notifications, :boolean, default: true, null: false | ||
| end | ||
| end |
There was a problem hiding this comment.
I don't see any change in the User model.
There was a problem hiding this comment.
you gotta love Rails 😏
There was a problem hiding this comment.
I guess he meant we still need to validate the presence of push_notifications to get a human-readable error message?
mllocs
force-pushed
the
feature/adds-user-push-notification-attribute
branch
2 times, most recently
from
265cbab
to
5af6e0e
Compare
|
Ready @sauloperez @enricostano |
| @@ -208,6 +208,7 @@ | |||
| t.datetime "locked_at" | |||
| t.string "locale", default: "es" | |||
| t.boolean "notifications", default: true | |||
| t.boolean "push_notifications", default: true, null: false | |||
There was a problem hiding this comment.
Do we need to add a data migration for the existing records?
There was a problem hiding this comment.
I believe the default makes it unnecessary.
There was a problem hiding this comment.
existing records will default to true
| @@ -500,8 +500,7 @@ eu: | |||
| edit: | |||
| edit_user: Erabiltzailea aldatu | |||
| form: | |||
| admin_warning: Kontuz!!! Erabiltzaile honi boterea ematen ari zara. | |||
| superadmin_warning: Kontuz!!! Erabiltzaile honi izugarrizko boterea ematen ari zara. | |||
| notifications: Jakinarazpen | |||
There was a problem hiding this comment.
Should we use locale app for this? cc/ @sseerrggii
| @@ -208,6 +208,7 @@ | |||
| t.datetime "locked_at" | |||
| t.string "locale", default: "es" | |||
| t.boolean "notifications", default: true | |||
| t.boolean "push_notifications", default: true, null: false | |||
There was a problem hiding this comment.
I believe the default makes it unnecessary.
|
This job: https://github.com/coopdevs/timeoverflow/pull/367/files will need to check this setting |
mllocs
force-pushed
the
feature/adds-user-push-notification-attribute
branch
from
5af6e0e
to
c57afda
Compare
|
Rebase please @mllocs |
mllocs
force-pushed
the
feature/adds-user-push-notification-attribute
branch
2 times, most recently
from
b24621c
to
ff463cd
Compare
mllocs
force-pushed
the
feature/adds-user-push-notification-attribute
branch
from
ff463cd
to
6bed3f2
Compare
|
Tested on |
Added a "push notifications" global setting in the users table.
Eventually, we'll need to create a user_settings table, moving all the settings from the users table to the new one. I thought that using this new approach (user_settings table) only for this new setting would actually create complexity instead of reducing it. I think that it would be better to implement user_settings once we can move all the other user attributes that are settings.
I fixed the markup of the form and removed a css hack that is not needed when the markup is correct. I also removed some code and translations that were not being used.