Skip to content
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

Tree search is not scoped properly #5059

Closed
grantfitzsimmons opened this issue Jul 1, 2024 · 6 comments
Closed

Tree search is not scoped properly #5059

grantfitzsimmons opened this issue Jul 1, 2024 · 6 comments
Assignees
Labels
1 - Bug Incorrect behavior of the product 2 - Trees Issues that are related to the tree system and related functionalities. todo:verify Needs Verification
Milestone

Comments

@grantfitzsimmons
Copy link
Member

grantfitzsimmons commented Jul 1, 2024

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.

@grantfitzsimmons grantfitzsimmons added 1 - Bug Incorrect behavior of the product 2 - Trees Issues that are related to the tree system and related functionalities. labels Jul 1, 2024
@grantfitzsimmons grantfitzsimmons added this to the 7.9.8 milestone Jul 1, 2024
@melton-jason
Copy link
Contributor

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.

else if (key === 'domainFilter') {
// GetScope() returns undefined for tables with only collectionmemberid.
const scopingField = tables[tableName].getScope();
return value === true &&
(tableName === 'Attachment' || typeof scopingField === 'object')
? 'true'
: undefined;

This results in behavior like in this issue (and an identical issue: #4542 (review)), 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.

fetchCollection(
// eslint-disable-next-line @typescript-eslint/no-unnecessary-type-assertion
tableName as AnyTree['tableName'],
{
limit: DEFAULT_FETCH_LIMIT,
orderBy: 'name',
domainFilter: true,
},
{
[resolvedSearchField]: searchCaseSensitive
? value
: value.toLowerCase(),
}
).then(({ records }) =>
records.map((node) => {
const rankDefinition = treeDefinitionItems.find(
({ rankId }) => rankId === node.rankId
);
const rankName = rankDefinition?.title || rankDefinition?.name;
return {
label: node.fullName ?? node.name,
subLabel: rankName,
data: node as SerializedResource<SCHEMA>,
};
})
)

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:

if queryset.model in (Geography, Geologictimeperiod, Lithostrat):
return queryset.filter(definition__disciplines=collection.discipline)
if queryset.model is Taxon:
return queryset.filter(definition__discipline=collection.discipline)
if queryset.model is Storage:
return queryset.filter(definition__institutions=collection.discipline.division.institution.id)


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: #5044 (review)

@realVinayak
Copy link
Contributor

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.

@emenslin
Copy link
Collaborator

Can recreate in edge (7.9.6)

@grantfitzsimmons
Copy link
Member Author

This manifests in worse behavior than just search–

Pycreus is a named taxon in both the 'Vascular Plants' and 'Lichens' collections in this database.

There is only one in each tree:

image image

Yet in the WorkBench I need to disambiguate it between the two:

image

From the disambiguation dialog, I see the following:

  • Taxon ID 8078 (current discipline, VP)
  • Taxon ID 15920 (other discipline, Lichens)

I can select the node from the other discipline
image

@CarolineDenis
Copy link
Contributor

@grantfitzsimmons, I cannot reproduce this on Edge

@CarolineDenis CarolineDenis self-assigned this Sep 4, 2024
@CarolineDenis CarolineDenis added the todo:verify Needs Verification label Sep 4, 2024
@emenslin
Copy link
Collaborator

emenslin commented Oct 8, 2024

Cannot reproduce

@emenslin emenslin closed this as not planned Won't fix, can't repro, duplicate, stale Oct 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1 - Bug Incorrect behavior of the product 2 - Trees Issues that are related to the tree system and related functionalities. todo:verify Needs Verification
Projects
None yet
Development

No branches or pull requests

5 participants