Remember user's SMS template preference #1379
Conversation
googlebot
added
the
cla: yes
pkg/database/migrations.go
Outdated
| `UPDATE users SET remember_last_used_sms_template = TRUE WHERE remember_last_used_sms_template IS NULL`, | ||
| `ALTER TABLE users ALTER COLUMN remember_last_used_sms_template SET DEFAULT TRUE`, | ||
| `ALTER TABLE users ALTER COLUMN remember_last_used_sms_template SET NOT NULL`, | ||
| `ALTER TABLE users ADD COLUMN IF NOT EXISTS default_sms_template_label VARCHAR(255)`, |
There was a problem hiding this comment.
We need to add some validation or a database constraint here. Either:
- Changing an SMS label should fail if a user is depending on it
- Changing an SMS label clears the default for any users
pkg/database/user.go
Outdated
| @@ -43,6 +43,9 @@ type User struct { | |||
| Name string `gorm:"type:varchar(100)"` | |||
| SystemAdmin bool `gorm:"column:system_admin; default:false;"` | |||
|
|
|||
| RememberLastUsedSMSTemplate bool `gorm:"type:boolean; not null; default:true;"` | |||
| DefaultSMSTemplateLabel string `gorm:"type:varchar(255);"` | |||
There was a problem hiding this comment.
A few things here:
- A user can be a member of multiple realms (with different templates), so this might be better as a property on the
Membershipinstead of the user - There needs to be some validation that the template is one of the keys in the hstore for the realm
- There needs to be some validation/changes when the hstore column is updated to cascade here
There was a problem hiding this comment.
I don't think we need to keep the membership preferred template in sync with the existing ones. When the page loads and the template doesn't exist, that selectedIf doesn't match anything and the user goes back to the default template in the selector.
There was a problem hiding this comment.
how does this work for users that in in more than 1 realm?
There was a problem hiding this comment.
This uses Seth's suggestion now - it's attached to Membership
There was a problem hiding this comment.
so I have to join a club?
|
/hold this needs some tests too |
|
/unhold |
| var audits []*AuditEntry | ||
|
|
||
| var existing Membership | ||
| if err := tx. |
There was a problem hiding this comment.
should we be reading this for update?
There was a problem hiding this comment.
We do that to diff existing and add audits. This is the pattern we're using in SaveRealm, SaveUser, etc
There was a problem hiding this comment.
It's all in a tx, so we don't need to read for update as that's implied.
| var audits []*AuditEntry | ||
|
|
||
| var existing Membership | ||
| if err := tx. |
There was a problem hiding this comment.
It's all in a tx, so we don't need to read for update as that's implied.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sethvargo, whaught The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Issue #1320
Proposed Changes
Release Note