-
Notifications
You must be signed in to change notification settings - Fork 17
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
fix: Find third party ids query #433
Conversation
Pull Request Test Coverage Report for Build 1970387670
💛 - Coveralls |
src/Item/Item.model.ts
Outdated
WHERE collections.third_party_id = ANY(${thirdPartyIds})`) | ||
SELECT ${itemsTable}.* | ||
FROM ${itemsTable} | ||
JOIN ${collectionsTable} ON collections.id = ${itemsTable}.collection_id |
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.
missing a ${collectionsTable} here right? (collections.id
)
Question: it's a good idea to do this in general to avoid typos, but at the same time it makes the query really difficult to read. Can we strike a balance perhaps? shorter names but using the string in the query?
(we might need tests for queries heh)
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.
Ups, added it!
What makes me doubt is that we have the table name used as a variable for what I think is to avoid typos (or be reactive when the table changes), but we're only using it for the table once.
migrations/migrate.ts
Outdated
@@ -17,6 +17,7 @@ export function migrate( | |||
} | |||
|
|||
const spawnArgs = [ | |||
'--no-check-order', |
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 PR fixes an issue where we were using
item
instead ofitems
when naming a table.The aliases were removed, as the names of the collections are sufficient enough for this query and the table names were replaced by their table variable.