specify / specify7

Specify 7
https://www.specifysoftware.org/products/specify-7/
GNU General Public License v2.0
60 stars 36 forks source link

Tree search is not scoped properly #5059

Open grantfitzsimmons opened 2 weeks ago

grantfitzsimmons commented 2 weeks ago

Describe the bug When searching in a tree, the results are not scoped.

To Reproduce Steps to reproduce the behavior:

  1. Go to the hujinnhc_2024_07_01 database on the Test Panel
  2. Log into any collection
  3. Go to the Geography tree
  4. Search 'Israel'
  5. See that there are a number of results while only one record exists in this tree

Screenshots

image

https://hujinnhc20240701-edge.test.specifysystems.org/specify/tree/geography/?conformation=~~84501~84503---

Encountering this issue on edge, v7.9.6, v7.9.5, and v7.9.4. Seems to have broken in the v7.9.4 release.

Expected behavior Only nodes from the current tree should appear!

image

Works correctly in v7.9.3.

System Information Specify 7 System Information - 2024-07-01T16_54_20.967Z.txt

Reported By The Hebrew University of Jerusalem

Lately (I think since the last update) every time we look for a place in the geography tree we get multiple instances of the same result:

image

Only one of the options is the "real" result (i.e. the one in the current tree). The other options belong to different disciplines but somehow also appear here.

Additional context This has been encountered internally and an issue may already exist.

melton-jason commented 2 weeks ago

As of #3304, Specify tries to be smart about scoping and prevent Hiearchy Exceptions thrown by the backend (as seen in #3564 and #4989) when trying to fetch a collection of resources.

If the collection fetch is supposed to scoped (by specifying domainfilter: true in the fetchCollection options) but the frontend determines the table can not be scoped, then the domainfilter is removed to the final request sent to the backend.

https://github.com/specify/specify7/blob/080b32ceaec14d747291bbb020d6329f48dcedc0/specifyweb/frontend/js_src/lib/components/DataModel/collection.ts#L105-L111

This results in behavior like in this issue (and an identical issue: https://github.com/specify/specify7/pull/4542#pullrequestreview-1894657939), where we are indicating the request should be scoped, but the frontend is determining the tree table to be 'unscopable', so it omits the domainfilter.

https://github.com/specify/specify7/blob/080b32ceaec14d747291bbb020d6329f48dcedc0/specifyweb/frontend/js_src/lib/components/TreeView/Search.tsx#L71-L96

This assumption that trees are 'unscopable' is fairly reasonable: their scope can not be easily determined dynamically, but domainfilter is allowed in the request because the scope is hardcoded and determinable on the backend:

https://github.com/specify/specify7/blob/080b32ceaec14d747291bbb020d6329f48dcedc0/specifyweb/specify/filter_by_col.py#L27-L34


The preferable solution here would be to handle scoping on the backend and always pass a domainfilter=true if it is specified by the caller of the endpoint on the frontend.

As mentioned in: https://github.com/specify/specify7/pull/5044#pullrequestreview-2140123120

realVinayak commented 2 weeks ago

I somehow think that inverting is better. If we know we can't scope, frontend shouldn't scope If we can't reliably know whether we can scope or not, then frontend shouldn't decide.The ideal implementation in that PR would have done this: check if we absolutely can't scope (or can scope), then only pass in relevant flag. Otherwise, don't act smart. Yes, that would have involved some hard coding, but worth it. Should just move to backend though.