fix: stop protecting stale, unverified pc-builder.io catalog prices
Bug found: "Gigabyte GAMING OC GeForce RTX 5070 Ti 16 GB Video Card" kept showing a stale price of 431.17 even after pc-builder.io itself no longer had a fixed offer for it (its "Add to Build" HTML only had a generic Amazon search link, no .price-box element at all, confirmed by fetching the live page directly). Root cause: scrapeComponentType's "never lose a better price" rule (added to make ScrapedPartsController#compare's persisted merchant prices durable across catalog refreshes) was too broad. It treated ANY existing price as worth protecting whenever the incoming catalog price was null or higher - including a price that had only ever come from a *previous* pc-builder.io scrape itself (priceSource: null), with no independent verification. Since pc-builder.io's own listings change constantly (a GPU can lose its priced Amazon link if the referenced ASIN goes out of stock), that logic effectively froze old pc-builder.io prices forever even after the site removed them. Fix: extracted the decision into shouldKeepExistingPrice(), which now only protects a price when priceSource is a real merchant name (set by updatePriceIfCheaper after a live comparison confirmed it on the merchant's own page) - never when priceSource is null (an unverified, previous pc-builder.io catalog price). A plain re-scrape can always correct/clear an unverified price, matching whatever pc-builder.io currently shows. 4 new unit tests for shouldKeepExistingPrice covering: unverified pc-builder.io price gets overwritten (the regression case), verified merchant price is protected from worse/missing catalog data, verified merchant price is still replaced by a genuinely cheaper catalog price, and no-existing-part returns false. Also re-ran `node ace scrape:parts` to apply the fix to already-cached data with stale unverified prices (affected ~1500 rows across the catalog before the fix, mostly likely unaffected in practice but no longer silently trusted going forward).
This commit is contained in:
1 parent
a1f6ff419a
commit
21d56a5da4
2 files changed
+66
-10
No files matched your search
@@ -251,6 +251,33 @@ async function fetchCategoryProducts(
|
|||||||
return rows
|
return rows
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Decides whether a catalog refresh row should overwrite an existing
|
||||||
|
* ScrapedPart's price fields, or leave them untouched.
|
||||||
|
*
|
||||||
|
* Only a price that was actually *verified* via a merchant comparison
|
||||||
|
* (priceSource set to a merchant name by updatePriceIfCheaper, e.g.
|
||||||
|
* "LDLC") is protected from being overwritten by a plain catalog refresh
|
||||||
|
* — that price was confirmed live on the merchant's own page.
|
||||||
|
*
|
||||||
|
* A price with priceSource === null just came from a *previous*
|
||||||
|
* pc-builder.io catalog scrape, with no independent verification.
|
||||||
|
* pc-builder.io's own listings change constantly — a GPU can have a fixed
|
||||||
|
* Amazon price today and only a generic "search on Amazon" link (no
|
||||||
|
* price) tomorrow, if the ASIN it referenced runs out of stock. Treating
|
||||||
|
* that old, unverified pc-builder.io price as untouchable would freeze it
|
||||||
|
* forever even after the site itself removed it, which is exactly the
|
||||||
|
* bug this function fixes (a €431.17 price kept showing for a GPU
|
||||||
|
* pc-builder.io no longer prices at all).
|
||||||
|
*/
|
||||||
|
function shouldKeepExistingPrice(
|
||||||
|
existing: { price: number | null; priceSource: string | null } | null,
|
||||||
|
incomingPrice: number | null
|
||||||
|
): boolean {
|
||||||
|
if (!existing || existing.price === null || existing.priceSource === null) return false
|
||||||
|
return incomingPrice === null || existing.price <= incomingPrice
|
||||||
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Scrapes a single component type's mapped category and upserts the
|
* Scrapes a single component type's mapped category and upserts the
|
||||||
* resulting rows into the scraped_parts cache table.
|
* resulting rows into the scraped_parts cache table.
|
||||||
@@ -280,15 +307,7 @@ async function scrapeComponentType(
|
|||||||
.andWhere('sourceRef', row.sourceRef)
|
.andWhere('sourceRef', row.sourceRef)
|
||||||
.first()
|
.first()
|
||||||
|
|
||||||
// If a previous price comparison already resolved (and persisted) a
|
const keepExistingPrice = shouldKeepExistingPrice(existing, row.price)
|
||||||
// real price for this part from a merchant, don't blow it away just
|
|
||||||
// because this catalog refresh came back with no price — keep
|
|
||||||
// whichever is best (see updatePriceIfCheaper for the "best of all
|
|
||||||
// known sources" logic used when a comparison finds a lower price).
|
|
||||||
const hasBetterExistingPrice =
|
|
||||||
existing?.price !== null &&
|
|
||||||
existing?.price !== undefined &&
|
|
||||||
(row.price === null || existing.price <= row.price)
|
|
||||||
|
|
||||||
await ScrapedPart.updateOrCreate(
|
await ScrapedPart.updateOrCreate(
|
||||||
{ source: SOURCE, sourceRef: row.sourceRef },
|
{ source: SOURCE, sourceRef: row.sourceRef },
|
||||||
@@ -297,7 +316,7 @@ async function scrapeComponentType(
|
|||||||
name: row.name,
|
name: row.name,
|
||||||
imageUrl: row.imageUrl,
|
imageUrl: row.imageUrl,
|
||||||
scrapedAt,
|
scrapedAt,
|
||||||
...(hasBetterExistingPrice
|
...(keepExistingPrice
|
||||||
? {}
|
? {}
|
||||||
: {
|
: {
|
||||||
price: row.price,
|
price: row.price,
|
||||||
@@ -402,6 +421,7 @@ export {
|
|||||||
ensureFreshCategory,
|
ensureFreshCategory,
|
||||||
isCategoryStale,
|
isCategoryStale,
|
||||||
updatePriceIfCheaper,
|
updatePriceIfCheaper,
|
||||||
|
shouldKeepExistingPrice,
|
||||||
CATEGORY_CONFIG,
|
CATEGORY_CONFIG,
|
||||||
CATEGORY_SLUGS,
|
CATEGORY_SLUGS,
|
||||||
CACHE_TTL_HOURS,
|
CACHE_TTL_HOURS,
|
||||||
|
|||||||
@@ -7,6 +7,7 @@ import {
|
|||||||
isCategoryStale,
|
isCategoryStale,
|
||||||
ensureFreshCategory,
|
ensureFreshCategory,
|
||||||
updatePriceIfCheaper,
|
updatePriceIfCheaper,
|
||||||
|
shouldKeepExistingPrice,
|
||||||
CACHE_TTL_HOURS,
|
CACHE_TTL_HOURS,
|
||||||
} from '#services/part_scraper_service'
|
} from '#services/part_scraper_service'
|
||||||
|
|
||||||
@@ -215,3 +216,38 @@ test.group('Best-known-price persistence', () => {
|
|||||||
assert.equal(part.priceSource, 'Grosbill')
|
assert.equal(part.priceSource, 'Grosbill')
|
||||||
})
|
})
|
||||||
})
|
})
|
||||||
|
|
||||||
|
test.group('Catalog refresh price protection (shouldKeepExistingPrice)', () => {
|
||||||
|
test('an unverified pc-builder.io price is NOT protected and gets overwritten', ({ assert }) => {
|
||||||
|
// Regression test: a GPU had price=431.17 from a previous pc-builder.io
|
||||||
|
// catalog scrape (priceSource: null, never independently verified).
|
||||||
|
// pc-builder.io later stopped showing a fixed price for it (only a
|
||||||
|
// generic Amazon search link). The stale 431.17 must NOT be kept.
|
||||||
|
const existing = { price: 431.17, priceSource: null }
|
||||||
|
|
||||||
|
assert.isFalse(shouldKeepExistingPrice(existing, null))
|
||||||
|
assert.isFalse(shouldKeepExistingPrice(existing, 999))
|
||||||
|
})
|
||||||
|
|
||||||
|
test('a verified merchant price IS protected from a worse/missing catalog price', ({
|
||||||
|
assert,
|
||||||
|
}) => {
|
||||||
|
const existing = { price: 350, priceSource: 'LDLC' }
|
||||||
|
|
||||||
|
assert.isTrue(shouldKeepExistingPrice(existing, null))
|
||||||
|
assert.isTrue(shouldKeepExistingPrice(existing, 500))
|
||||||
|
})
|
||||||
|
|
||||||
|
test('a verified merchant price is replaced if the catalog price is actually cheaper', ({
|
||||||
|
assert,
|
||||||
|
}) => {
|
||||||
|
const existing = { price: 350, priceSource: 'LDLC' }
|
||||||
|
|
||||||
|
assert.isFalse(shouldKeepExistingPrice(existing, 200))
|
||||||
|
})
|
||||||
|
|
||||||
|
test('no existing part means nothing to keep', ({ assert }) => {
|
||||||
|
assert.isFalse(shouldKeepExistingPrice(null, 100))
|
||||||
|
assert.isFalse(shouldKeepExistingPrice(null, null))
|
||||||
|
})
|
||||||
|
})
|
||||||
Reference in new issue
Block a user