Feature/spec filter by strech345 · Pull Request #276 · orangecoding/fredy

strech345 · 2026-03-08T21:16:52Z

listings can be filtered by specs

specs are: price, size, rooms
by fiilters: maxPrice, minSize, minRooms
will be filtered out for notifications
will be filtered out on overview
for now its only specFilters, but also blacklist and areaFilter should be added here.
can be disabled on overview (in filter section)
add rooms into db because its next to price and size one of the three importen specs
show rooms in overview and detail
update kleinanzeigen config
add rooms for now only on kleinanzeigen but on the other it should also be added

strech345 · 2026-03-08T21:24:22Z

we are using INTEGER as type for the db but saving floating numbers at least for rooms and size.
Interessting that this will be saved as REAL when its an float. So it returns floating numbers.
Im not shure what would be good. For size its ok to reduze to integer, for rooms with 2.5 rooms it would be nice to store, but a bit to much to use REAL for such little numbers.
What we can do, but maybe ugly, multiply rooms by 10 to save and devide by 10 at read, so we can still use INTEGER.
@orangecoding what do you think?

leave it like it is: default is INTERGER, but if the value is float sqlite will automatically store it as REAL
take always REAL
take INTEGER and multiply by 10

orangecoding · 2026-03-09T06:42:24Z

@strech345 why did you close the pr?

strech345 · 2026-03-09T06:49:40Z

@strech345 why did you close the pr?

i really dont know why. I only changed to draft. But dont know why i close it.

orangecoding · 2026-03-09T06:57:04Z

So should I review it now or is this still a draft?

strech345 · 2026-03-09T07:33:39Z

So should I review it now or is this still a draft?

Would be nice if you can read my comments here and give me some Feedback. Also you can Start reviewing it. I would only extend it like explained.

orangecoding · 2026-03-09T12:58:57Z

Would be nice if you can read my comments here and give me some Feedback. Also you can Start reviewing it. I would only extend it like explained.

Tbh, I don't see an issue with this...?

leave it like it is: default is INTERGER, but if the value is float sqlite will automatically store it as REAL

orangecoding · 2026-03-09T13:12:36Z

lib/provider/kleinanzeigen.js


 function normalize(o) {
-  const size = o.size || '--- m²';
+  const parts = (o.tags || '').split('·').map((p) => p.trim());


orangecoding

@strech345 I have not (yet) checked out your fork and ran it on my machine, bur so far I have 4 comments. Let me know when I should check out the whole thing

orangecoding · 2026-03-09T13:16:09Z

lib/FredyPipelineExecutioner.js

-    return listings.map(this._providerConfig.normalize);
+    return listings.map((listing) => {
+      const normalized = this._providerConfig.normalize(listing);
+      // TODO: every provider should return price, size and rooms in type number. This makes it more strong and strict of the provider output. String formats like "m², Zi,..." should not be part and can be added on fe or massages. Move this logic into the provider-specific normalize function.


Can you make sure this is done in the pr?

orangecoding · 2026-03-09T13:44:17Z

lib/FredyPipelineExecutioner.js

+    // like for kleinanzeigen we have tags (includes multiple fields) but will be than extract at normalize, and deleted because its only internal used.
+    // I would suggest that we define a standard list like (id, price, rooms, size, title, link, description, address, image, url)
+    // it might be that some of this props value is null, wich is ok without id, link, title
+    // Also this might be not needed when using typings with typescript. I would suggest to move the whole project to typescript to have save typings.


I don't like typescript for various reasons which would take way to long to explain here, but Fredy won't move to Typescript ;)

thats ok, i now created new type files.
Would be nice to move more to type files so its on one place.

orangecoding · 2026-03-09T13:46:10Z

lib/FredyPipelineExecutioner.js

-    const keys = Object.keys(this._providerConfig.crawlFields);
+    // i removed it because crawlFields might be different than fields which are required.
+    // like for kleinanzeigen we have tags (includes multiple fields) but will be than extract at normalize, and deleted because its only internal used.
+    // I would suggest that we define a standard list like (id, price, rooms, size, title, link, description, address, image, url)


I get this, but I think what we then should do is letting each provider define their list of required params. A standard function that is available in every provider which returns an array of required keys..

Can you please explain me why this is needed? only to understand this.
it still only checks the keys (not the value), so its only about what is added in code.
i changed now the returned obj for every normalize fnc with a new obj like

return { id, link, title: o.title || '', price: extractNumber(o.price), size: extractNumber(o.size), rooms: extractNumber(rooms), address: `${ciity}, ${road}`, image, description: o.description, };

so its more clear what the provider really returns. I also add for all of this functions the type and add a jsonconfig to activate type checks.

orangecoding · 2026-03-09T13:46:24Z

lib/FredyPipelineExecutioner.js

+    // it might be that some of this props value is null, wich is ok without id, link, title
+    // Also this might be not needed when using typings with typescript. I would suggest to move the whole project to typescript to have save typings.
+    //const keys = Object.keys(this._providerConfig.crawlFields);
+    const keys = ['id', 'link', 'title'];


We should not have a fixed list in here.

orangecoding · 2026-03-09T14:39:45Z

@strech345 You probably want to merge Master into your branch as I have just pushed a pretty big change. (There's a new migration as well, so you must increase the number of yours)
See https://github.com/orangecoding/fredy/releases/tag/20.0.0

strech345 · 2026-03-09T14:59:47Z

Ok, will take some time this week is full 😉

strech345 · 2026-03-16T20:15:31Z

@strech345 You probably want to merge Master into your branch as I have just pushed a pretty big change. (There's a new migration as well, so you must increase the number of yours) See https://github.com/orangecoding/fredy/releases/tag/20.0.0

whats about migration 11.add-spatial-filter? that was on master merged before. Now we have two 11s (11.mcp-tokens).

orangecoding · 2026-03-17T07:11:59Z

@strech345 You probably want to merge Master into your branch as I have just pushed a pretty big change. (There's a new migration as well, so you must increase the number of yours) See https://github.com/orangecoding/fredy/releases/tag/20.0.0

whats about migration 11.add-spatial-filter? that was on master merged before. Now we have two 11s (11.mcp-tokens).

Which is why I asked you to increase your migra to 12

strech345 · 2026-03-17T07:27:12Z

@strech345 You probably want to merge Master into your branch as I have just pushed a pretty big change. (There's a new migration as well, so you must increase the number of yours) See https://github.com/orangecoding/fredy/releases/tag/20.0.0

whats about migration 11.add-spatial-filter? that was on master merged before. Now we have two 11s (11.mcp-tokens).

Which is why I asked you to increase your migra to 12

ok i can rename it. Only to make the point clear: Boath migrations are right now on master.
https://github.com/orangecoding/fredy/tree/master/lib/services/storage/migrations/sql
when users uses the current version where boath migrations are 11 and we rename it now, the users might get problems with new version changed migration. But im not shure

orangecoding · 2026-03-17T10:28:24Z

@strech345 You probably want to merge Master into your branch as I have just pushed a pretty big change. (There's a new migration as well, so you must increase the number of yours) See https://github.com/orangecoding/fredy/releases/tag/20.0.0

whats about migration 11.add-spatial-filter? that was on master merged before. Now we have two 11s (11.mcp-tokens).

Which is why I asked you to increase your migra to 12

ok i can rename it. Only to make the point clear: Boath migrations are right now on master. https://github.com/orangecoding/fredy/tree/master/lib/services/storage/migrations/sql when users uses the current version where boath migrations are 11 and we rename it now, the users might get problems with new version changed migration. But im not shure

Fuck you're right. I missed it. Please pull from master

https://github.com/orangecoding/fredy/releases/tag/20.0.5

orangecoding · 2026-03-23T14:51:54Z

Hey @strech345 this pr is already massive. I strongly suggest IF you want to get this merged not to make this any bigger as I also have to review it.

Also make sure you keep the scope ;/)

strech345 · 2026-03-23T14:58:39Z

from my

Hey @strech345 this pr is already massive. I strongly suggest IF you want to get this merged not to make this any bigger as I also have to review it.

Also make sure you keep the scope ;/)

from my side its done and ready for review ;-)

orangecoding · 2026-03-24T07:42:40Z

@claude review

orangecoding

@strech345 I've added a few comments. What concerns me is you removed the delete function from the filter

orangecoding · 2026-03-25T13:34:57Z

lib/services/storage/listingsStorage.js

  }

+  // filterByJobSettings: when true, filter listings by spec_filter in job settings
+  if (filterByJobSettings === true) {


Mhm.. instead of having to filter out this (I assume this has to be done, because you store the data to not scrape it many times), how about scraping it and if it hits something from the filter, simply set it to deleted. This way, it won't show up, you do not have to filter it here and you would save a lot of code...

If you pull master from my branch, you'll see how I did it in e.g. in _filterByArea:

if (filteredIds.length > 0) { deleteListingsById(filteredIds); }

i thought you still want to show all in the fe list. Also it would be possible to change the filter values and the listings would be dynamic filtered on the new values.
But i can change it and mark them as deleted. with this i can remove the fe changes.

lib/types/browser.js

orangecoding · 2026-03-25T13:36:40Z

lib/FredyPipelineExecutioner.js


-    if (filteredIds.length > 0) {
-      deleteListingsById(filteredIds);
+    return filteredListings;


Why did you remove the deleteListings here? That was added on purpose to not scrape things infinite?

might be happend with merge. i dont realize that you did changes here.
i will fix it.

Thanks ❤️

orangecoding · 2026-03-25T13:37:32Z

lib/FredyPipelineExecutioner.js

      throw new NoNewListingsWarning();
    }
-    const sendNotifications = notify.send(this._providerId, newListings, this._notificationConfig, this._jobKey);
+    // TODO: move this to the notification adapter, so it will handle for all providers in same way.


Can you resolve this todo before we proceed?

i dont did it because it means changes on all notification files. Also this feels not correct. i'm not shure where to put it. Maybei can put it into the notify fnc?

ok I get it. leave it as it is for now :)

strech345 · 2026-03-23T14:18:50Z

test/pipeline_filtering.test.js


-  it('should call deleteListingsById when listings are filtered by area', async () => {
+  // TODO: fix this test
+  it.skip('should call deleteListingsById when listings are filtered by area', async () => {


i disabled the test. dont unsterand how area filter should effect deleteListingsById because delete is called before. Would be nice if you can check this.

strech345 · 2026-03-23T14:19:55Z

test/provider/immoscout.test.js

-          expect(notify.link).toContain('https://www.immobilienscout24.de/');
+
+        // check if there is at least one valid notification
+        const hasValidNotification = notificationObj.payload.some((notify) => {


i changed it because some values might missing depending on online listings. Would say other should also work the same to reduce test failures.

strech345 · 2026-03-23T14:21:16Z

lib/FredyPipelineExecutioner.js

+
+    const filteredListings = listings
+      // this should never filter some listings out, because the normalize function should always extract all fields.
+      .filter((item) => requiredKeys.every((key) => key in item))


like you told me i add it, without knowledge for what.

strech345 · 2026-03-23T14:27:12Z

lib/FredyPipelineExecutioner.js

+    // like for kleinanzeigen we have tags (includes multiple fields) but will be than extract at normalize, and deleted because its only internal used.
+    // I would suggest that we define a standard list like (id, price, rooms, size, title, link, description, address, image, url)
+    // it might be that some of this props value is null, wich is ok without id, link, title
+    // Also this might be not needed when using typings with typescript. I would suggest to move the whole project to typescript to have save typings.


thats ok, i now created new type files.
Would be nice to move more to type files so its on one place.

lib/types/browser.js

strech345 · 2026-03-25T20:35:26Z

lib/services/storage/listingsStorage.js

  }

+  // filterByJobSettings: when true, filter listings by spec_filter in job settings
+  if (filterByJobSettings === true) {


i thought you still want to show all in the fe list. Also it would be possible to change the filter values and the listings would be dynamic filtered on the new values.
But i can change it and mark them as deleted. with this i can remove the fe changes.

strech345 · 2026-03-25T20:40:58Z

lib/FredyPipelineExecutioner.js


-    if (filteredIds.length > 0) {
-      deleteListingsById(filteredIds);
+    return filteredListings;


might be happend with merge. i dont realize that you did changes here.
i will fix it.

strech345 · 2026-03-25T20:43:39Z

lib/FredyPipelineExecutioner.js

+   * @returns {ParsedListing[]} Listings considered unique enough to keep.
   */
-  _filterBySimilarListings(listings) {
+  _deleteSimilarListings(listings) {


will than also rename this back. before your changes all filters looks like filters and this not, so i renamed it. now every filter is working with delete so i will rename it back

strech345 · 2026-03-25T21:00:07Z

lib/FredyPipelineExecutioner.js

      throw new NoNewListingsWarning();
    }
-    const sendNotifications = notify.send(this._providerId, newListings, this._notificationConfig, this._jobKey);
+    // TODO: move this to the notification adapter, so it will handle for all providers in same way.


i dont did it because it means changes on all notification files. Also this feels not correct. i'm not shure where to put it. Maybei can put it into the notify fnc?

orangecoding · 2026-03-26T11:12:07Z

@strech345 I loose track of the comments in here. Once you adressed all pr comments can you ping me please to check it one more time? Thanks :)

strech345 added 13 commits February 21, 2026 14:51

feat(): create map component, add area filtering to the job config

bb6b46b

feat(): filter listings by area filter

21db416

chore(): cleanup

2739fab

feat(): solve feedback

ae975d9

feat(): solve most providers

9578192

feat(): solve maybe other providers

a134da3

feat(): add specFilter config, also add rooms to listing

85643fc

feat(): change tests

067682b

feat(): fix kleinanzeigen parser

a7c7273

feat(): add spec filter switch for listing overviiews

82d08d7

feat(): add rooms and size to the overview and detail of a listing

8b368a1

feat(): rem label

18cbe24

Merge remote-tracking branch 'upstream/master' into feature/spec_filter

fdc7a7d

strech345 closed this Mar 8, 2026

strech345 reopened this Mar 9, 2026

orangecoding reviewed Mar 9, 2026

View reviewed changes

orangecoding requested changes Mar 9, 2026

View reviewed changes

strech345 added 2 commits March 16, 2026 21:06

feat(): add types, update providers, they now return specs as numbers

fed77be

Merge branch 'master' into feature/spec_filter

27310b0

feat(): add jsonconfig to enable type checks

153c63c

strech345 added 7 commits March 23, 2026 10:04

Merge branch 'master' into feature/spec_filter

47e3bb9

feat: add type for prividerConfig, add fieldNames per provider

0074053

feat: fix tests, provider, add formatListing

7f9ce14

chore: remov duplicates

2dedcbc

Merge branch 'master' into feature/spec_filter

3a246ff

feat(): fix tests

2bf368d

feat: fix immoscout

58c91f9

strech345 marked this pull request as ready for review March 23, 2026 14:22

Repository owner deleted a comment from claude bot Mar 25, 2026

orangecoding requested changes Mar 25, 2026

View reviewed changes

strech345 commented Mar 26, 2026

View reviewed changes

Uh oh!

Conversation

strech345 commented Mar 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

strech345 commented Mar 8, 2026

Uh oh!

orangecoding commented Mar 9, 2026

Uh oh!

strech345 commented Mar 9, 2026

Uh oh!

orangecoding commented Mar 9, 2026

Uh oh!

strech345 commented Mar 9, 2026

Uh oh!

orangecoding commented Mar 9, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

orangecoding left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

orangecoding commented Mar 9, 2026

Uh oh!

strech345 commented Mar 9, 2026

Uh oh!

strech345 commented Mar 16, 2026

Uh oh!

orangecoding commented Mar 17, 2026

Uh oh!

strech345 commented Mar 17, 2026

Uh oh!

orangecoding commented Mar 17, 2026

Uh oh!

orangecoding commented Mar 23, 2026

Uh oh!

strech345 commented Mar 23, 2026

Uh oh!

orangecoding commented Mar 24, 2026

Uh oh!

orangecoding left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

strech345 commented Mar 8, 2026 •

edited

Loading