-
Notifications
You must be signed in to change notification settings - Fork 11
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
set ExpiresAt in confirmations that support expiration #139
Conversation
5c78307
to
ba4c6ca
Compare
ba4c6ca
to
cab4964
Compare
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, assuming this comes with a migration to update any existing confirmations.
Thanks @darinkrauss! Yes, there's a migration: https://github.com/tidepool-org/tools-private/pull/53 |
cab4964
to
6f56901
Compare
/deploy qa2 |
ewollesen updated values.yaml file in qa2 |
ewollesen updated flux policies file in qa2 |
ewollesen deployed hydrophone eric/invitations-with-expiration branch to qa2 namespace |
e4c1f44
to
900ebad
Compare
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.
Nothing blocking, but some comments for your consideration.
@@ -232,12 +237,10 @@ func (c *Confirmation) ValidateType(expectedType Type, validationErrors *[]error | |||
} | |||
|
|||
func (c *Confirmation) IsExpired() bool { | |||
timeout, ok := Timeouts[c.Type] | |||
if !ok { | |||
if c.ExpiresAt == nil || c.ExpiresAt.IsZero() { |
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.
[nit] Do we have zero time ExpiresAt in the the data currently? If not, then I'd remove the || c.ExpiresAt.IsZero()
clause as not needed.
t.Fatalf("expected nil error, got %s", err) | ||
} | ||
if nonExpiringInviting.IsExpired() { | ||
t.Errorf("expected invitation type %q to never expire", cType) |
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.
Yes, much more self-explanatory than "expected false, got true". 👍
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!
This field will help the care partner mobile app. BACK-2807
d5e51d6
to
aba6454
Compare
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!
/deploy qa5 |
ewollesen updated values.yaml file in qa5 |
ewollesen updated flux policies file in qa5 |
ewollesen deployed hydrophone eric/invitations-with-expiration branch to qa5 namespace |
This field will help the care partner mobile app.
BACK-2807