-
Notifications
You must be signed in to change notification settings - Fork 83
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jeremyfaller 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 |
b5bc35f
to
563c839
Compare
563c839
to
0b0e351
Compare
/hold Hold on this right now. I don't like the error handling I've put in vercode.go. I'm going to add some more testing. |
6e43da2
to
107f0e1
Compare
107f0e1
to
31dd38d
Compare
/hold Holding AGAIN. while I work out what I don't understand about gorm. |
31dd38d
to
5cb4bc2
Compare
/unhold |
5cb4bc2
to
c4e071c
Compare
c4e071c
to
1cb261f
Compare
Adds per realm stats, and a simple visualization of the codes issued and claimed for the past month.
1cb261f
to
1be8b68
Compare
scope.Log(fmt.Sprintf("failed to update stats: %v", err)) | ||
} | ||
} | ||
|
||
// Update the per-realm stats. | ||
sql := ` |
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 should probably guard this with a sanity:
if v.RealmID != 0 {
can be a follow-up
// per-realm stats. If any of that fails, an error code is returned. | ||
func (v *VerificationCode) claim(db *Database, tx *gorm.DB, verCode string, realmID uint) error { | ||
if expired, err := db.IsCodeExpired(v, verCode); err != nil { | ||
return err |
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.
It'd be nice to wrap this error like fmt.Errorf("failed to claim code: %w")
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.
Can be a follow-up
|
||
// claim checks the VerificationCode, marks as claimed, and updates the | ||
// per-realm stats. If any of that fails, an error code is returned. | ||
func (v *VerificationCode) claim(db *Database, tx *gorm.DB, verCode string, realmID uint) error { |
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.
This API is a little weird to me now, and I'm not sure the comment matches. While this sets Claimed: true
on the Verification Code, it does not save that code in the database. The caller still needs to call db.Save(v)
. That feels subtle and could lead to some bugs. Furthermore, we're updating the realm stats before successfully marking the code as claimed. I'm thinking this flow should be more like:
- Expiration checks
- Claimed check
- Mark as claimed on struct
- Save struct to database (stop if error)
- Update stats
- Return
Alternatively, we need to document this better. What do you think?
Adds per realm stats, and a simple visualization of the codes issued and
claimed for the past month.
Fixes #410
Release Note