-
Notifications
You must be signed in to change notification settings - Fork 24
Fix/challenge timestamp issue #784
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.
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
Conversation
Codecov Report
@@ Coverage Diff @@
## staging #784 +/- ##
========================================
Coverage 24.64% 24.64%
========================================
Files 77 77
Lines 7871 7871
========================================
Hits 1940 1940
Misses 5663 5663
Partials 268 268
Flags with carried forward coverage won't be shown. Click here to find out more. Help us with your feedback. Take ten seconds to tell us how you rate us. |
|
||
if err := db.Transaction(func(tx *gorm.DB) error { | ||
return c.SaveWith(tx) | ||
}); err != nil { | ||
logging.Logger.Error("[challenge]add: ", | ||
zap.String("challenge_id", c.ChallengeID), | ||
zap.Time("created", c.CreatedAt), | ||
zap.Time("created", common.ToTime(c.CreatedAt)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'd better call common.ToTime()
one time, and use the result to avoid redundant time conversions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
if time.Since(common.ToTime(cr.CreatedAt)) > config.Configuration.ChallengeCompletionTime { | ||
cr.Status = Cancelled | ||
cr.StatusMessage = "challenge completion time expired" | ||
cr.UpdatedAt = time.Now().UTC() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to call cr.Save()
to update the changes to db.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Fixes
This PR removes ambiguity in CreatedAt field.
Tests
Tasks to complete before merging PR:
Associated PRs (Link as appropriate):