-
Notifications
You must be signed in to change notification settings - Fork 31
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
Calculate expires_at For Order Adding One Month #1870
Conversation
@@ -142,6 +142,22 @@ func (r *Order) GetTimeBounds(ctx context.Context, dbi sqlx.QueryerContext, id u | |||
return result, nil | |||
} | |||
|
|||
// GetExpiresAtP1M returns expires_at that is last_paid_at (or now()) plus 1 month. | |||
func (r *Order) GetExpiresAtP1M(ctx context.Context, dbi sqlx.QueryerContext, id uuid.UUID) (time.Time, error) { | |||
const q = `SELECT (SELECT COALESCE(last_paid_at, now()) AS last_paid_at) + interval '1 month' AS expires_at FROM orders WHERE id = $1` |
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.
Could we use the (max) value in the order item list for valid_for_iso instead of having a per interval function to make it more generic?
max(order_items.valid_for_iso)::interval
??
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.
Please have a look at #1874.
A note on calculation – unless I am wrong, type casting has to happen before we MAX
, otherwise we'd just compare text. I think we want to compare intervals. Hence, MAX(order_items.valid_for_iso::interval)
.
@@ -1435,10 +1435,10 @@ func (pg *Postgres) recordOrderPayment(ctx context.Context, dbi sqlx.ExecerConte | |||
} | |||
|
|||
func (pg *Postgres) updateOrderExpiresAt(ctx context.Context, dbi sqlx.ExtContext, orderID uuid.UUID) error { | |||
orderTimeBounds, err := pg.orderRepo.GetTimeBounds(ctx, dbi, orderID) | |||
expiresAt, err := pg.orderRepo.GetExpiresAtP1M(ctx, dbi, orderID) |
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.
Lets keep it generic GetTimeBounds
see next comment.
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.
Please have a look at #1874. A note on naming: since the method no longer returns bounds, strictly speaking, the old name no longer makes sense. It returns a ready-to-use value for expires_at
, and the name in the new PR reflects that – GetExpiresAtAfterISOPeriod
.
Closing this in favour of #1874. |
Summary
This PR is aiming at addressing #1864.
It does so by delegating calculation to the database – it already has access to
last_paid_at
. It takes that ornow()
, and adds an interval of 1 month to it.Additionally, the PR contains a few test cases for the new code.
The approach taken in #1865 could work, had the functionality in
libs/time
been trusted. At present, there are doubts that that code works the way it is expected to work. That issue will be considered, and if necessary – addressed, separately.For now, this fix would suffice.
Type of Change
Tested Environments
Before Submitting