Repository navigation
feat(jobs): add per-job location exclusions - #493
andrii-kovpak-dev wants to merge 1 commit into
Conversation
|
Hi! Just wanted to follow up on this PR when you have a chance. I’d appreciate any feedback — whether the implementation fits the direction of the project, whether you’d like something changed, or whether this kind of contribution isn’t something you’re interested in merging. No problem either way — I’d just like to understand the project’s direction and whether further contributions from my side would be useful. Thanks! |
|
Just to add some context on why I implemented this feature: I’m currently actively searching for an apartment myself, and this was one of the things I was missing most. In many apps, you can choose locations you want to include, but there is often no simple way to exclude locations you definitely don’t want. In practice, I find exclusions much more convenient: usually there are only a few districts that are too far away, inconvenient for work or study, or simply not relevant for personal reasons. I’m already actively using this feature with my own updates, and it saves me a noticeable amount of time. I no longer get notifications for listings in areas I would immediately ignore anyway, so the notifications stay much more relevant and don’t distract me with apartments I’m not interested in. The exclusion list can also naturally grow over time as you learn which areas don’t work for you. In many cases, it’s much easier to exclude a few unsuitable areas than to manually include every area that could potentially work. That’s the main reason I thought this could be useful not only for me, but as a general feature for other users as well. |
|
Hey. Im having a cold atm. Will get back to any of the pr's, emails and contacts about Fredy when I recovered |
|
Oops! Get well soon! |
orangecoding
left a comment
orangecoding
left a comment
There was a problem hiding this comment.
Thanks for this, and for explaining the use case. The feature makes sense to me, and it's wired through storage, MCP and the job form the same way the blacklist is. Three things before I can merge it, see inline: the matching drops listings that aren't in the excluded district, the first filter pass sits in front of the price change handling, and the migration number clashes with master.
| // Several providers use this placeholder when the portal withheld the address. | ||
| if (normalizedAddress === 'no address found') return false; | ||
| return normalizeExcludedLocations(excludedLocations).some((location) => | ||
| normalizedAddress.includes(location.toLowerCase()), |
There was a problem hiding this comment.
Plain substring matching drops listings that have nothing to do with the excluded district. The address contains the street, and lots of streets are named after other districts. Excluding Spandau (which is what the placeholder suggests) also removes Spandauer Damm 12, 14059 Berlin, Charlottenburg and Spandauer Straße 4, 10178 Berlin, Mitte. Same with Köpenick vs Köpenicker Straße in Kreuzberg, Mitte vs Mittenwalder Straße, Buch vs Buchholzer Str.. I ran these through this function and all of them return true. The user never sees those listings and has no way to find out why.
Please match whole words only, e.g. (?<![\p{L}\p{N}])term(?![\p{L}\p{N}]) with the u flag (\b is ASCII only and breaks on umlauts). Build the patterns once in the constructor instead of normalizing the list again for every listing, and return early when the list is empty, like _filterBySpecs does.
| : this._getListings(url), | ||
| ) | ||
| .then(this._normalize.bind(this)) | ||
| .then(this._filterByExcludedLocations.bind(this)) |
There was a problem hiding this comment.
This runs before _findNew and _splitOffPriceChanges, so a listing that is already stored and sits in an excluded area never reaches _splitOffPriceChanges anymore. For those rows the scraped price isn't recorded (recordPriceChange writes the history and the new price), setListingHash isn't updated, and markListingsPriceChecked is skipped, so the price tracker fetches them again on its own. That contradicts the comment in notifyPriceChanges and the README: through the tracker the history gets written, through the scrape it doesn't.
notifyPriceChanges already filters the alert, so this pass can move behind _splitOffPriceChanges, next to the first _filterBySpecs. It still runs before the detail fetch and the geocode, which is the point of it.
| * @param {import('better-sqlite3').Database} db | ||
| * @returns {void} | ||
| */ | ||
| export function up(db) { |
There was a problem hiding this comment.
52 is already taken on master (52.listing-added-manually.js). The runner copes because it sorts by name on a tie, but precheckRestore uses the highest number as the schema version, so it can't tell a backup with this column from one without it. Please rebase and rename to 53.job-excluded-locations.js.
7d92cc3 to
2c4387f
Compare
|
Thanks for the review. I’ve updated the existing commit with all three requested changes and rebased the branch onto the latest Changes applied:
I also updated the tests covering whole-word location matching, pipeline filtering, price-change handling, price history, and excluded-location storage. The full offline suite passes: 4,969 passed / 33 skipped. Lint and formatting pass as well. The updated branch has been pushed to the existing PR. |
2c4387f to
b0b4355
Compare
|
One additional update: after rebasing onto the latest The existing commit was amended and the PR branch updated. |
b0b4355 to
40ee90b
Compare
What does this PR do?
Adds per-job excluded locations so users can filter out listings from unwanted districts or locations before they reach notifications.
Related issue
none
AI disclosure (required)
ai:none- I wrote this myself. No AI generated code, text or commit messages.ai:assisted- AI helped with parts of it (autocomplete, refactoring, tests, docs). I reviewed and understand every line.ai:generated- AI produced most or all of this PR. I reviewed it, but it is largely machine written.Which AI: OpenAI Codex, GPT-6.1 Sol
How much: Most of the implementation, tests, migration, UI integration, translations and documentation changes were generated by AI and then reviewed and tested locally.
Why: To analyze the existing Fredy architecture, implement the feature consistently across storage, pipeline, UI and notifications, and add comprehensive test coverage.
Checklist
yarn test:offlinepasses (oryarn testif the change touches a live provider)yarn lintandyarn format:checkpass