Skip to content
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

Replace toasts - be more optimistic - prepaid and postpaid #1184

Closed
wants to merge 46 commits into from
Closed
Show file tree
Hide file tree
Changes from 42 commits
Commits
Show all changes
46 commits
Select commit Hold shift + click to select a range
4d052dd
Replace useInvoiceable with usePayment hook
ekzyis Apr 15, 2024
8b71200
Show WebLnError in QR code fallback
ekzyis May 8, 2024
4d959c6
Fix missing removal of old zap undo code
ekzyis May 9, 2024
d9bf839
Fix payment timeout message
ekzyis May 10, 2024
eca90ac
Fix unused arg in super()
ekzyis May 10, 2024
a7391a1
Also bail if invoice expired
ekzyis May 10, 2024
23b6103
Fix revert on reply error
ekzyis May 10, 2024
e3aa20a
Use JIT_INVOICE_TIMEOUT_MS constant
ekzyis May 10, 2024
a118dd6
Remove unnecessary PaymentContext
ekzyis May 11, 2024
c15658f
Fix me as a dependency in FeeButtonContext
ekzyis May 15, 2024
5b40288
Fix anon sats added before act success
ekzyis May 16, 2024
73eec20
Optimistic updates for zaps
ekzyis May 16, 2024
bfbcc74
Fix modal not closed after custom zap
ekzyis May 16, 2024
a47c291
Optimistic update for custom zaps
ekzyis May 16, 2024
b78e8d8
Optimistic update for bounty payments
ekzyis May 16, 2024
e11561a
Consistent error handling for zaps and bounty payments
ekzyis May 16, 2024
eeee7e8
Optimistic update for poll votes
ekzyis May 16, 2024
4a4daeb
Use var balance in payment.request
ekzyis May 18, 2024
ac9bcd6
Allow postpaid replies
ekzyis May 18, 2024
5775245
Don't show payments for pending items in notifications
ekzyis May 18, 2024
6740e6a
Add and fix comments
ekzyis May 20, 2024
f747f7d
Rethrow error early
ekzyis May 20, 2024
29c41d7
Move var declaration closer to usage
ekzyis May 20, 2024
7fc3d01
Remove unused me from serialize parameters
ekzyis May 20, 2024
4295ac1
Replace interactive tx with serialize call
ekzyis May 20, 2024
f1a3ce7
Log cancelled invoices
ekzyis May 20, 2024
4afa840
Use DEFAULT_INVOICE_TIMEOUT_MS for invoice expiry
ekzyis May 20, 2024
e64a5b3
Set item status to FAILED if payment failed
ekzyis May 20, 2024
dc75a27
Client notifications
ekzyis May 21, 2024
02df708
Fix missing postpaid support for discussions etc.
ekzyis May 22, 2024
3e30af6
Filter pending and failed items
ekzyis May 21, 2024
572387f
Add notifications for failed items
ekzyis May 22, 2024
600f697
Run finalizeAction sooner
ekzyis May 23, 2024
76ac89f
Exclude failed items from item_spam
ekzyis May 23, 2024
d90c119
Move action functions into worker/action.js
ekzyis May 23, 2024
fb43c48
Check invoice status in finalizeAction
ekzyis May 23, 2024
afc4864
Handle side effects for pending items
ekzyis May 23, 2024
6741fab
Remove unnecessary passing of act
ekzyis May 23, 2024
711ed36
Use AbortController for zap undos
ekzyis May 23, 2024
0949b08
Prune query params by type
ekzyis May 23, 2024
dcd59fd
Prevent links to failed items to be clickable
ekzyis May 23, 2024
1b6b02a
Merge branch 'master' into wallet-ux-2
huumn May 23, 2024
669ad67
Fix reply stays prefilled after submit
ekzyis May 24, 2024
5aadd98
Handle action in same tx as confirm_invoice
ekzyis May 24, 2024
bbdbb62
Only handle error in finalizeAction
ekzyis May 25, 2024
aff8f5b
Fix missing await
ekzyis May 25, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
122 changes: 101 additions & 21 deletions api/resolvers/item.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { GraphQLError } from 'graphql'
import { ensureProtocol, removeTracking, stripTrailingSlash } from '@/lib/url'
import serialize from './serial'
import serialize, { InsufficientFundsError } from './serial'
import { decodeCursor, LIMIT, nextCursorEncoded } from '@/lib/cursor'
import { getMetadata, metadataRuleSets } from 'page-metadata-parser'
import { ruleSet as publicationDateRuleSet } from '@/lib/timedate-scraper'
Expand All @@ -9,7 +9,7 @@ import {
ITEM_SPAM_INTERVAL, ITEM_FILTER_THRESHOLD,
COMMENT_DEPTH_LIMIT, COMMENT_TYPE_QUERY,
ANON_USER_ID, ANON_ITEM_SPAM_INTERVAL, POLL_COST,
ITEM_ALLOW_EDITS, GLOBAL_SEED, ANON_FEE_MULTIPLIER, NOFOLLOW_LIMIT, UNKNOWN_LINK_REL
ITEM_ALLOW_EDITS, GLOBAL_SEED, ANON_FEE_MULTIPLIER, NOFOLLOW_LIMIT, UNKNOWN_LINK_REL, ANON_INV_PENDING_LIMIT, ANON_BALANCE_LIMIT_MSATS, INV_PENDING_LIMIT, USER_IDS_BALANCE_NO_LIMIT, BALANCE_LIMIT_MSATS, DEFAULT_INVOICE_TIMEOUT_MS
} from '@/lib/constants'
import { msatsToSats } from '@/lib/format'
import { parse } from 'tldts'
Expand All @@ -21,6 +21,9 @@ import { datePivot, whenRange } from '@/lib/time'
import { imageFeesInfo, uploadIdsFromText } from './image'
import assertGofacYourself from './ofac'
import assertApiKeyNotPermitted from './apiKey'
import { createInvoice } from 'ln-service'
import { Prisma } from '@prisma/client'
import { createHmac } from './wallet'
Comment on lines +24 to +26
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tip

Codebase Verification

Ensure proper error handling for external calls to createInvoice.

The search results indicate multiple instances of createInvoice calls across various files. However, not all instances show explicit error handling. Specifically, in api/resolvers/item.js, there is no clear indication of error handling around the createInvoice call.

  • api/resolvers/item.js: Ensure that the createInvoice call is wrapped in a try-catch block or has appropriate error handling mechanisms.
Analysis chain

Ensure proper error handling for external calls to createInvoice.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify proper error handling for external calls to `createInvoice`.

# Test: Search for error handling patterns around `createInvoice` calls.
rg --type python 'createInvoice' --context 5

Length of output: 75



Script:

#!/bin/bash
# Description: Verify proper error handling for external calls to `createInvoice`.

# Test: Search for error handling patterns around `createInvoice` calls.
rg --type js 'createInvoice' --context 5

Length of output: 13248


function commentsOrderByClause (me, models, sort) {
if (sort === 'recent') {
Expand Down Expand Up @@ -51,26 +54,29 @@ function commentsOrderByClause (me, models, sort) {
async function comments (me, models, id, sort) {
const orderBy = commentsOrderByClause(me, models, sort)

const filter = '' // empty filter as we filter clientside now
let filter = 'AND "Item"."status" <> \'FAILED\''
if (me) {
filter += `AND ("Item"."userId" = ${me.id} OR "Item"."status" <> 'PENDING')`
const [{ item_comments_zaprank_with_me: comments }] = await models.$queryRawUnsafe(
'SELECT item_comments_zaprank_with_me($1::INTEGER, $2::INTEGER, $3::INTEGER, $4::INTEGER, $5, $6)', Number(id), GLOBAL_SEED, Number(me.id), COMMENT_DEPTH_LIMIT, filter, orderBy)
return comments
}

filter += 'AND "Item"."status" <> \'PENDING\''
Comment on lines +57 to +65
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optimize the SQL query to prevent potential performance issues with large datasets.

Consider using more efficient SQL constructs or adding appropriate indexes to improve the performance of this query.

const [{ item_comments: comments }] = await models.$queryRawUnsafe(
'SELECT item_comments($1::INTEGER, $2::INTEGER, $3, $4)', Number(id), COMMENT_DEPTH_LIMIT, filter, orderBy)
return comments
}

export async function getItem (parent, { id }, { me, models }) {
export async function getItem (parent, { id, status = true }, { me, models }) {
const [item] = await itemQueryWithMeta({
me,
models,
query: `
${SELECT}
FROM "Item"
WHERE id = $1`
WHERE id = $1
${status ? `AND ${statusClause(me)}` : ''}`
Comment on lines +71 to +79
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tip

Codebase Verification

The search results indicate multiple instances where the status parameter is used, but there is no clear evidence of validation for the status parameter in the context of preventing SQL injection or logical errors. The status parameter is directly interpolated into SQL queries, which can be risky.

To ensure the status parameter is validated properly, it would be prudent to add explicit validation logic before using it in SQL queries. This can be done by checking if the status parameter is one of the expected values (e.g., 'ACTIVE', 'PENDING', etc.).

Action Required:

  • Add validation for the status parameter to ensure it only contains expected values before using it in SQL queries.

Suggested Code Change:

export async function getItem (parent, { id, status = true }, { me, models }) {
  // Validate the status parameter
  const validStatuses = ['ACTIVE', 'PENDING', 'FAILED', 'STOPPED'];
  if (status && !validStatuses.includes(status)) {
    throw new Error('Invalid status parameter');
  }

  const [item] = await itemQueryWithMeta({
    me,
    models,
    query: `
      ${SELECT}
      FROM "Item"
      WHERE id = $1
      ${status ? `AND ${statusClause(me)}` : ''}`
  }, Number(id));
  return item;
}

Locations to Update:

  • api/resolvers/item.js (lines 71-79)
Analysis chain

Validate the status parameter to prevent SQL injection or logical errors.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Check for validation of `status` parameter in other parts of the codebase.

# Test: Search for validation patterns for `status`.
rg --type python 'status' --context 5

Length of output: 68



Script:

#!/bin/bash
# Description: Check for validation of `status` parameter in other parts of the codebase.

# Test: Search for validation patterns for `status` in JavaScript files.
rg --type js 'status' --context 5

Length of output: 139868

}, Number(id))
return item
}
Expand Down Expand Up @@ -251,6 +257,18 @@ export async function filterClause (me, models, type) {
return [freebieClause, outlawClause]
}

export function statusClause (me) {
let filter = '"Item"."status" <> \'FAILED\' AND '

if (me) {
filter += `("Item"."userId" = ${me.id} OR "Item"."status" <> 'PENDING')`
} else {
filter += '"Item"."status" <> \'PENDING\''
}

return filter
}
Comment on lines +260 to +270
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactor the statusClause function to improve readability and maintainability.

Consider breaking down the function into smaller, more manageable parts or using helper functions to handle different conditions.


function typeClause (type) {
switch (type) {
case 'links':
Expand Down Expand Up @@ -344,7 +362,8 @@ export default {
await filterClause(me, models, type),
nsfwClause(showNsfw),
typeClause(type),
whenClause(when || 'forever', table))}
whenClause(when || 'forever', table),
statusClause(me))}
${orderByClause(by, me, models, type)}
OFFSET $4
LIMIT $5`,
Expand All @@ -364,7 +383,8 @@ export default {
activeOrMine(me),
await filterClause(me, models, type),
typeClause(type),
muteClause(me)
muteClause(me),
statusClause(me)
)}
ORDER BY "Item".created_at DESC
OFFSET $2
Expand All @@ -388,7 +408,8 @@ export default {
typeClause(type),
whenClause(when, 'Item'),
await filterClause(me, models, type),
muteClause(me))}
muteClause(me),
statusClause(me))}
ORDER BY rank DESC
OFFSET $3
LIMIT $4`,
Expand All @@ -407,7 +428,8 @@ export default {
typeClause(type),
whenClause(when, 'Item'),
await filterClause(me, models, type),
muteClause(me))}
muteClause(me),
statusClause(me))}
${orderByClause(by || 'zaprank', me, models, type)}
OFFSET $3
LIMIT $4`,
Expand Down Expand Up @@ -463,7 +485,8 @@ export default {
'"Item"."parentId" IS NULL',
'"Item".bio = false',
subClause(sub, 3, 'Item', me, showNsfw),
muteClause(me))}
muteClause(me),
statusClause(me))}
ORDER BY rank DESC
OFFSET $1
LIMIT $2`,
Expand All @@ -482,6 +505,7 @@ export default {
${whereClause(
subClause(sub, 3, 'Item', me, showNsfw),
muteClause(me),
statusClause(me),
// in "home" (sub undefined), we want to show pinned items (but without the pin icon)
sub ? '"Item"."pinId" IS NULL' : '',
'"Item"."deletedAt" IS NULL',
Expand Down Expand Up @@ -514,7 +538,8 @@ export default {
'"pinId" IS NOT NULL',
'"parentId" IS NULL',
sub ? '"subName" = $1' : '"subName" IS NULL',
muteClause(me))}
muteClause(me),
statusClause(me))}
) rank_filter WHERE RANK = 1
ORDER BY position ASC`,
orderBy: 'ORDER BY position ASC'
Expand Down Expand Up @@ -899,7 +924,7 @@ export default {
WHERE act IN ('TIP', 'FEE')
AND "itemId" = ${Number(id)}::INTEGER
AND "userId" = ${me.id}::INTEGER)::INTEGER)`,
{ models }
{ models, lnd, hash, hmac }
)
} else {
await serialize(
Expand Down Expand Up @@ -1353,26 +1378,81 @@ export const createItem = async (parent, { forward, options, ...item }, { me, mo
fee = sub.baseCost * ANON_FEE_MULTIPLIER + (item.boost || 0)
}
}
fee += imgFees;
fee += imgFees

([item] = await serialize(
models.$queryRawUnsafe(
try {
([item] = await serialize(
models.$queryRawUnsafe(
`${SELECT} FROM create_item($1::JSONB, $2::JSONB, $3::JSONB, '${spamInterval}'::INTERVAL, $4::INTEGER[]) AS "Item"`,
JSON.stringify(item), JSON.stringify(fwdUsers), JSON.stringify(options), uploadIds),
{ models, lnd, me, hash, hmac, fee }
))
{ models, lnd, me, hash, hmac, fee }
))
} catch (err) {
// post payment flow is only allowed for stackers, not anons since we can't show pending items to anons
if (!me || !(err instanceof InsufficientFundsError)) {
throw err
}

await createMentions(item, models)
// create invoice and insert as pending item
const invLimit = me ? INV_PENDING_LIMIT : ANON_INV_PENDING_LIMIT
const balanceLimit = USER_IDS_BALANCE_NO_LIMIT.includes(Number(me?.id)) ? 0 : me ? BALANCE_LIMIT_MSATS : ANON_BALANCE_LIMIT_MSATS
const description = 'Creating item on stacker.news'
// TODO: allow users to configure expiration
const expiresAt = datePivot(new Date(), { milliseconds: DEFAULT_INVOICE_TIMEOUT_MS })
// we don't do partial payments yet, users need to pay full amount via invoice
const mtokens = err.cost

const lndInv = await createInvoice({
description: me.hideInvoiceDesc ? undefined : description,
lnd,
mtokens,
expires_at: expiresAt
})

await enqueueDeletionJob(item, models)
let invoice
// need to use interactive tx here to set invoice.actionId = item.id
await models.$transaction(
async (tx) => {
// insert pending item
([item] = await tx.$queryRawUnsafe(
`${SELECT} FROM create_item($1::JSONB, $2::JSONB, $3::JSONB, '${spamInterval}'::INTERVAL, $4::INTEGER[]) AS "Item"`,
JSON.stringify({ ...item, status: 'PENDING' }), JSON.stringify(fwdUsers), JSON.stringify(options), uploadIds))

// set required invoice data to update item on payment
const actionData = { cost: err.cost, credits: err.balance };
Copy link
Member

@huumn huumn May 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If these fields need to be saved, might we do better storing them as columns with types and naming them in the db layer?

([invoice] = await tx.$queryRaw`
SELECT * FROM create_invoice(${lndInv.id}, NULL, ${lndInv.request},
${expiresAt}::timestamp, ${mtokens}, ${item.userId}::INTEGER, ${description}, NULL, NULL,
${invLimit}::INTEGER, ${balanceLimit}, 'ITEM'::"ActionType", ${item.id}::INTEGER, ${JSON.stringify(actionData)}::JSONB)`)

// since SubscribeToInvoices does not trigger if an invoice expired, we need a job that handles expired invoices
await tx.$queryRaw`
INSERT INTO pgboss.job (name, data, retrylimit, retrybackoff, startafter)
VALUES ('finalizeAction', jsonb_build_object('hash', ${invoice.hash}::TEXT), 21, true, ${expiresAt})`
}, { isolationLevel: Prisma.TransactionIsolationLevel.Serializable })

// hmac is not required to submit action again but to allow user to cancel payment
invoice.hmac = createHmac(invoice.hash)

item.invoice = invoice
}

item.comments = []

// we need to insert these even for pending items to show toasts
// we will delete the jobs if payment failed
await enqueueDeletionJob(item, models)
await createReminderAndJob({ me, item, models })

notifyUserSubscribers({ models, item })
const isPending = !!item.invoice
if (isPending) {
return item
}

await createMentions(item, models)
notifyUserSubscribers({ models, item })
notifyTerritorySubscribers({ models, item })

item.comments = []
return item
}

Expand Down
19 changes: 17 additions & 2 deletions api/resolvers/notifications.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { GraphQLError } from 'graphql'
import { decodeCursor, LIMIT, nextNoteCursorEncoded } from '@/lib/cursor'
import { getItem, filterClause, whereClause, muteClause } from './item'
import { getItem, filterClause, whereClause, muteClause, statusClause } from './item'
import { getInvoice, getWithdrawl } from './wallet'
import { pushSubscriptionSchema, ssValidate } from '@/lib/validate'
import { replyToSubscription } from '@/lib/webPush'
Expand Down Expand Up @@ -151,7 +151,8 @@ export default {
${whereClause(
'"Item".created_at < $2',
await filterClause(me, models),
muteClause(me))}
muteClause(me),
statusClause(me))}
ORDER BY id ASC, CASE
WHEN type = 'Mention' THEN 1
WHEN type = 'Reply' THEN 2
Expand Down Expand Up @@ -216,6 +217,7 @@ export default {
WHERE "Invoice"."userId" = $1
AND "confirmedAt" IS NOT NULL
AND "isHeld" IS NULL
AND "actionType" IS NULL
AND created_at < $2
ORDER BY "sortTime" DESC
LIMIT ${LIMIT})`
Expand Down Expand Up @@ -313,6 +315,16 @@ export default {
LIMIT ${LIMIT})`
)

queries.push(
`(SELECT "Item".id::text, "Item"."created_at" AS "sortTime", NULL as "earnedSats", 'FailedItem' AS type
FROM "Item"
WHERE "Item"."userId" = $1
AND "Item"."created_at" < $2
AND "Item"."status" = 'FAILED'
ORDER BY "sortTime" DESC
LIMIT ${LIMIT})`
)

const notifications = await models.$queryRawUnsafe(
`SELECT id, "sortTime", "earnedSats", type,
"sortTime" AS "minSortTime"
Expand Down Expand Up @@ -396,6 +408,9 @@ export default {
return await getItem(n, { id: itemId }, { models, me })
}
},
FailedItem: {
item: async (n, args, { models, me }) => getItem(n, { id: n.id, status: false }, { models, me })
},
Comment on lines +412 to +413
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review the handling of failed items in resolvers.

The resolver for FailedItem uses a potentially incorrect query condition (status: false). Verify this condition and adjust it to correctly reflect the intended logic.

- getItem(n, { id: n.id, status: false }, { models, me })
+ // Correct the status condition based on intended logic

Committable suggestion was skipped due low confidence.

TerritoryTransfer: {
sub: async (n, args, { models, me }) => {
const transfer = await models.territoryTransfer.findUnique({ where: { id: Number(n.id) }, include: { sub: true } })
Expand Down
15 changes: 13 additions & 2 deletions api/resolvers/serial.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,15 @@ import { createHmac } from './wallet'
import { msatsToSats, numWithUnits } from '@/lib/format'
import { BALANCE_LIMIT_MSATS } from '@/lib/constants'

export default async function serialize (trx, { models, lnd, me, hash, hmac, fee }) {
export class InsufficientFundsError extends GraphQLError {
constructor (balance, cost) {
super('insufficient funds', { extensions: { code: 'BAD_INPUT' } })
this.balance = Number(balance)
this.cost = Number(cost)
}
}

export default async function serialize (trx, { models, lnd, hash, hmac, fee }) {
// wrap first argument in array if not array already
const isArray = Array.isArray(trx)
if (!isArray) trx = [trx]
Expand Down Expand Up @@ -39,8 +47,11 @@ export default async function serialize (trx, { models, lnd, me, hash, hmac, fee
// XXX prisma does not provide a way to distinguish these cases so we
// have to check the error message
if (error.message.includes('SN_INSUFFICIENT_FUNDS') ||
// TODO: does '\\"' match '"' in error message or is this a bug?
error.message.includes('\\"users\\" violates check constraint \\"msats_positive\\"')) {
bail(new GraphQLError('insufficient funds', { extensions: { code: 'BAD_INPUT' } }))
const balance = /user_msats=(\d+)/.exec(error.message)?.[1]
const cost = /cost_msats=(\d+)/.exec(error.message)?.[1]
bail(new InsufficientFundsError(balance, cost))
}
if (error.message.includes('SN_NOT_SERIALIZABLE')) {
bail(new Error('wallet balance transaction is not serializable'))
Expand Down
23 changes: 20 additions & 3 deletions api/resolvers/user.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { GraphQLError } from 'graphql'
import { decodeCursor, LIMIT, nextCursorEncoded } from '@/lib/cursor'
import { msatsToSats } from '@/lib/format'
import { bioSchema, emailSchema, settingsSchema, ssValidate, userSchema } from '@/lib/validate'
import { getItem, updateItem, filterClause, createItem, whereClause, muteClause } from './item'
import { getItem, updateItem, filterClause, createItem, whereClause, muteClause, statusClause } from './item'
import { ANON_USER_ID, DELETE_USER_ID, RESERVED_MAX_USER_ID, SN_NO_REWARDS_IDS } from '@/lib/constants'
import { viewGroup } from './growth'
import { timeUnitForRange, whenRange } from '@/lib/time'
Expand Down Expand Up @@ -285,6 +285,7 @@ export default {
'r.created_at >= "ThreadSubscription".created_at',
await filterClause(me, models),
muteClause(me),
statusClause(me),
...(user.noteAllDescendants ? [] : ['r.level = 1'])
)})`, me.id, lastChecked)
if (newThreadSubReply.exists) {
Expand All @@ -305,7 +306,8 @@ export default {
OR ("Item"."parentId" IS NOT NULL AND "UserSubscription"."commentsSubscribedAt" IS NOT NULL AND "Item".created_at >= "UserSubscription"."commentsSubscribedAt")
)`,
await filterClause(me, models),
muteClause(me))})`, me.id, lastChecked)
muteClause(me),
statusClause(me))})`, me.id, lastChecked)
if (newUserSubs.exists) {
foundNotes()
return true
Expand Down Expand Up @@ -339,7 +341,8 @@ export default {
'"Mention".created_at > $2',
'"Item"."userId" <> $1',
await filterClause(me, models),
muteClause(me)
muteClause(me),
statusClause(me)
)})`, me.id, lastChecked)
if (newMentions.exists) {
foundNotes()
Expand Down Expand Up @@ -503,6 +506,20 @@ export default {
return true
}

const newFailedItems = await models.item.findFirst({
where: {
userId: me.id,
status: 'FAILED',
updatedAt: {
gt: lastChecked
}
}
})
if (newFailedItems) {
foundNotes()
return true
}
Comment on lines +509 to +521
Copy link
Contributor

@coderabbitai coderabbitai bot May 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider optimizing the query for checking new failed items.

The current implementation performs a query for each type of notification. This could potentially be optimized by combining some of these queries into a single query, especially if they are checking similar conditions or can be grouped logically.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment reminds me that I probably need to manually update updatedAt in raw queries since it's not based on a trigger.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ekzyis, you're absolutely right about the need to manually update the updatedAt field in raw queries, especially since it's not managed by a database trigger in this context. Manually managing this field can ensure that your data remains consistent and accurate, which is crucial for features that rely on the timeliness of data updates.

If you're updating multiple records or have multiple places where this needs to be handled, you might consider abstracting this logic into a helper function or middleware to keep your code DRY and maintainable. Here’s a quick example of how you might implement such a function in JavaScript:

const updateWithTimestamp = async (model, where, data) => {
  const updatedAt = new Date();
  const updatedData = { ...data, updatedAt };
  return await model.update({ where, data: updatedData });
};

This function takes the model, the conditions (where), and the data to be updated, and it automatically appends the current timestamp to the updatedAt field before executing the update.

Let me know if you need further assistance with this or any other part of your implementation!

Copy link
Member Author

@ekzyis ekzyis May 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mhh, even though I don't see a trigger that does this, updatedAt gets updated after a raw query updates an item 🤔

These are the existing item triggers:

Triggers:
    index_item AFTER INSERT OR UPDATE ON "Item" FOR EACH ROW EXECUTE FUNCTION index_item()
    ncomments_after_comment_trigger AFTER INSERT ON "Item" FOR EACH ROW EXECUTE FUNCTION ncomments_after_comment()
    path_tgr BEFORE INSERT OR UPDATE ON "Item" FOR EACH ROW EXECUTE FUNCTION update_item_path()
    timestamp_item_on_insert AFTER INSERT ON "Item" FOR EACH ROW EXECUTE FUNCTION timestamp_item_on_insert()

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ekzyis, it's interesting to see that the updatedAt field is being updated automatically, even though there isn't a specific trigger listed for this action. This behavior might be due to a couple of reasons:

  1. Database Abstraction or ORM Behavior: If the project uses an ORM (Object-Relational Mapping) or any database abstraction layer, it's common for these tools to manage updatedAt fields automatically. They often do this during any update operation without needing explicit triggers in the database.

  2. Existing Trigger Handling More Than Its Name Suggests: The trigger timestamp_item_on_insert might be slightly misleading by its name. It's possible that this trigger is configured to handle updates as well as inserts, despite what the name suggests. This would allow it to update the updatedAt field on any modification to the row.

If you need to confirm or inspect this behavior further, you might consider checking the definition of the timestamp_item_on_insert trigger to see if it indeed handles updates. Alternatively, reviewing the ORM or database layer's configuration or documentation could provide insights into this automatic handling of the updatedAt field.

Let me know if you need more detailed information on this or any other queries!


// update checkedNotesAt to prevent rechecking same time period
models.user.update({
where: { id: me.id },
Expand Down
Loading
Loading