Fix panic when Daily Summary is nil for a user #199
Conversation
larkox
added
2: Dev Review
Codecov Report
@@ Coverage Diff @@
## master #199 +/- ##
==========================================
- Coverage 23.05% 22.97% -0.08%
==========================================
Files 64 64
Lines 2316 2324 +8
==========================================
Hits 534 534
- Misses 1703 1709 +6
- Partials 79 81 +2
Continue to review full report at Codecov.
|
server/mscalendar/daily_summary.go
Outdated
| @@ -75,6 +83,14 @@ func (m *mscalendar) SetDailySummaryEnabled(user *User, enable bool) (*store.Dai | |||
| return nil, err | |||
| } | |||
|
|
|||
| if user.Settings.DailySummary == nil { | |||
| user.Settings.DailySummary = &store.DailySummaryUserSettings{ | |||
There was a problem hiding this comment.
LGTM. Wondering if it would be worth returning this object from a small function since it is used 3 times?
There was a problem hiding this comment.
I thought so, but since the object is being used in the store and in the main package, I thought it would create circular dependencies unless we store it in the store. And the store does not seem the best place for this.
There was a problem hiding this comment.
1/5 I agree with Jason to make it into a function. If the store is the only place it can go, then I suppose that's where it has to go.
server/mscalendar/daily_summary.go
Outdated
| @@ -75,6 +83,14 @@ func (m *mscalendar) SetDailySummaryEnabled(user *User, enable bool) (*store.Dai | |||
| return nil, err | |||
| } | |||
|
|
|||
| if user.Settings.DailySummary == nil { | |||
| user.Settings.DailySummary = &store.DailySummaryUserSettings{ | |||
There was a problem hiding this comment.
1/5 I agree with Jason to make it into a function. If the store is the only place it can go, then I suppose that's where it has to go.
DHaussermann
added
4: Reviews Complete
Summary
There seems to be a panic in master that happens when the daily summary settings are set to nil. This should never happen, but maybe a weird status in the system is triggering this.
This PR makes sure the daily summary setting is set by adding a default in every point it is present.
Another solution could be remove the indirection and trust the go defaults.
Ticket Link
None