fix(coderd): humanize duration on notifications #14333
Conversation
coderd/util/time/duration.go
Outdated
| func Humanize(d time.Duration) string { | ||
| endTime := time.Now().Add(d) | ||
| return humanize.Time(endTime) |
There was a problem hiding this comment.
Given this is only used in two places, I'm not convinced we need any indirection in front of humanize.Time; we should just use it directly.
If we were using this very broadly, I could see some value is hiding this behind our own interface so we could swap out the underlying implementation, but I don't think we're there yet.
There was a problem hiding this comment.
Happy you noticed this to reduce abstraction!
There was a problem hiding this comment.
100%, I was just too lazy to remove the first abstraction I added. My bad 🙏
| @@ -323,14 +324,15 @@ func (e *Executor) runOnce(t time.Time) Stats { | |||
| } | |||
| } | |||
| if shouldNotifyDormancy { | |||
| dormantTime := time.Now().Add(time.Duration(tmpl.TimeTilDormant)) | |||
There was a problem hiding this comment.
One thing to check: is the dormancy value in the database in UTC? If so, the timezone of the control plane might make things weird and we may want to add a .UTC() call in here too or use dbtime.Now()
There was a problem hiding this comment.
Good to know
| @@ -691,7 +691,7 @@ func TestNotificationTemplatesCanRender(t *testing.T) { | |||
| "reason": "breached the template's threshold for inactivity", | |||
| "initiator": "autobuild", | |||
| "dormancyHours": "24", | |||
| "timeTilDormant": "24h", | |||
| "timeTilDormant": "24 hours", | |||
There was a problem hiding this comment.
A test which includes not such a round number would also be helpful for illustrative purposes.
Close #14325
I added tests for the Humanize function, but we might want to consider adding snapshot tests for the notifications in the near future.
Since these tests would require a database (as the templates are stored there), it can be challenging to update the golden files locally with our current test setup. I’m wondering if we should start considering the use of an embedded database for some tests. 🤔