-
Notifications
You must be signed in to change notification settings - Fork 0
Prod - SFDC fixes #36
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
| SELECT | ||
| fp.billing_account AS "billingAccountId", | ||
| TO_CHAR(DATE_TRUNC('month', fp.created_at), 'YYYY-MM') AS "month", | ||
| TO_CHAR(DATE_TRUNC('month', fp.created_at AT TIME ZONE 'America/New_York'), 'YYYY-MM') AS "month", |
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.
[correctness]
The addition of AT TIME ZONE 'America/New_York' to fp.created_at ensures that the date is correctly adjusted to the specified timezone. However, ensure that all relevant parts of the system are aware of this timezone change to avoid inconsistencies, especially if other parts of the system assume UTC or another timezone.
| COUNT(fp.payment_id) AS "paymentCount", | ||
| MIN(fp.created_at)::date AS "earliestPaymentDate", | ||
| MAX(fp.created_at)::date AS "latestPaymentDate", | ||
| MIN(fp.created_at AT TIME ZONE 'America/New_York')::date AS "earliestPaymentDate", |
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.
[correctness]
The use of AT TIME ZONE 'America/New_York' for MIN(fp.created_at) and MAX(fp.created_at) ensures that the earliest and latest payment dates are correctly adjusted to the specified timezone. Verify that this change aligns with business requirements and that all downstream systems are prepared to handle this timezone adjustment.
| GROUP BY | ||
| fp.billing_account, | ||
| DATE_TRUNC('month', fp.created_at), | ||
| DATE_TRUNC('month', fp.created_at AT TIME ZONE 'America/New_York'), |
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.
[correctness]
The addition of AT TIME ZONE 'America/New_York' to DATE_TRUNC('month', fp.created_at) ensures that the month is calculated based on the specified timezone. Confirm that this change is consistent with how other date calculations are performed across the system to prevent potential discrepancies.
| c.status as "challengeStatus", | ||
| c."endDate" as "completeDate", | ||
| p.created_at as "paymentDate", | ||
| c."endDate" AT TIME ZONE 'America/New_York' as "completeDate", |
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.
[❗❗ correctness]
The use of AT TIME ZONE 'America/New_York' for c."endDate" assumes that the input timestamp is in UTC. Ensure that the source data is indeed in UTC to avoid incorrect time conversions.
| c."endDate" as "completeDate", | ||
| p.created_at as "paymentDate", | ||
| c."endDate" AT TIME ZONE 'America/New_York' as "completeDate", | ||
| p.created_at AT TIME ZONE 'America/New_York' as "paymentDate", |
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.
[❗❗ correctness]
The use of AT TIME ZONE 'America/New_York' for p.created_at assumes that the input timestamp is in UTC. Ensure that the source data is indeed in UTC to avoid incorrect time conversions.
| AND ($9::text[] IS NULL OR m.handle = ANY($9::text[])) | ||
| AND ($10::numeric IS NULL OR (p.total_amount + p.challenge_fee) >= $10::numeric) | ||
| AND ($11::numeric IS NULL OR (p.total_amount + p.challenge_fee) <= $11::numeric) | ||
| ORDER BY c."endDate" DESC, p.created_at DESC |
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.
[performance]
The removal of the LIMIT 1000 clause could lead to performance issues if the result set is large. Consider adding a limit or pagination to manage large datasets efficiently.
| SELECT | ||
| p.payment_id as "paymentId", | ||
| p.created_at as "paymentDate", | ||
| p.created_at AT TIME ZONE 'America/New_York' as "paymentDate", |
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.
[❗❗ correctness]
The use of AT TIME ZONE 'America/New_York' changes the timezone of created_at. Ensure that this change aligns with the business requirements, as it will affect how paymentDate is interpreted and displayed. If the application logic or downstream systems expect UTC or another timezone, this could lead to inconsistencies.
| j.currency, | ||
| j.created_at AS "createdAt", | ||
| j.updated_at AS "updatedAt" | ||
| j.created_at AT TIME ZONE 'America/New_York' AS "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.
[correctness]
Using AT TIME ZONE 'America/New_York' directly in the query can lead to unexpected results during daylight saving time transitions. Consider using a more robust method to handle time zone conversions, such as using a function that accounts for daylight saving time changes.
| j.created_at AS "createdAt", | ||
| j.updated_at AS "updatedAt" | ||
| j.created_at AT TIME ZONE 'America/New_York' AS "createdAt", | ||
| j.updated_at AT TIME ZONE 'America/New_York' AS "updatedAt" |
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.
[correctness]
Similar to the previous line, ensure that the time zone conversion handles daylight saving time changes correctly to avoid potential issues with timestamp accuracy.
| rb.billing_account_id AS "billingAccountId", | ||
| rb.created_at AS "createdAt", | ||
| rb.updated_at AS "updatedAt" | ||
| rb.created_at AT TIME ZONE 'America/New_York' AS "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.
[correctness]
Using AT TIME ZONE 'America/New_York' directly in the query can lead to unexpected results during daylight saving time transitions. Consider using a timezone-aware function or storing timestamps in UTC and converting them in the application layer for consistency.
| rb.created_at AS "createdAt", | ||
| rb.updated_at AS "updatedAt" | ||
| rb.created_at AT TIME ZONE 'America/New_York' AS "createdAt", | ||
| rb.updated_at AT TIME ZONE 'America/New_York' AS "updatedAt" |
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.
[correctness]
Similar to the previous line, applying AT TIME ZONE 'America/New_York' directly in the query may cause issues during daylight saving time changes. It's safer to handle timezone conversions in the application layer or use a timezone-aware function.
| description, | ||
| payment_status_desc AS "paymentStatus", | ||
| created_at AS "paymentDate" | ||
| created_at AT TIME ZONE 'America/New_York' AS "paymentDate" |
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.
[correctness]
Using AT TIME ZONE 'America/New_York' directly in the query can lead to unexpected results during daylight saving time changes. Consider using a timezone-aware function or storing timestamps in UTC and converting them in the application layer to ensure consistent behavior.
No description provided.