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

Add default query mapping path to filter by COT tree def in QB search #6142

Merged
merged 7 commits into from
Jan 24, 2025

Conversation

sharadsw
Copy link
Contributor

Fixes #6098

Checklist

  • Self-review the PR after opening it to make sure the changes look good and
    self-explanatory (or properly documented)
  • Add automated tests
  • Add relevant issue to release milestone
  • Add relevant documentation (Tester - Dev)
  • Add a reverse migration if a migration is present in the PR

Testing instructions

  • Create a new CO and set a COT
  • Click the Search button next to the Determination QCBX
  • Verify there is already a mapping line for filtering by COT tree def

@sharadsw sharadsw added this to the 7.10 milestone Jan 22, 2025
@sharadsw sharadsw marked this pull request as ready for review January 22, 2025 22:27
@CarolineDenis CarolineDenis requested a review from a team January 23, 2025 14:26
Copy link
Contributor

@melton-jason melton-jason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!
Since the queryBuilderFieldPath overrides the field, this can probably be simplified by removing the queryBuilderFieldPath from the QueryComboBoxFilter type and instead allowing the field to be an Array of strings (MappingPath).

e.g., change

readonly field: string & (keyof CommonFields | keyof SCHEMA['fields']);

to

readonly field: MappingPath | (string & (keyof CommonFields | keyof SCHEMA['fields']));

(And handling the case when field is an Array in toQueryFields)

@sharadsw
Copy link
Contributor Author

@melton-jason The text search filter expects field to be a string as it seems to only work with simple fields and not mapping paths. I figured we need queryBuilderFieldPath for cases where the query builder needs a different path than what the search filter uses. (For taxon, the search filter filters by definition while the QB uses definition.id. Let me know if there's a better way to handle both cases at the same time

function testFilter<SCHEMA extends AnySchema>(
resource: SpecifyResource<SCHEMA>,
{
operation,
field,
value,
isNot,
isRelationship,
}: QueryComboBoxFilter<SCHEMA>
): boolean {

Copy link
Collaborator

@emenslin emenslin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Verify there is already a mapping line for filtering by COT tree def

Just want to check this is the correct behavior before approving but all ranks show up even if they are not part of the tree you are querying on. This isn't necessarily a problem but it could just potentially be confusing so I wanted to double check.

01-23-2025_10.07.mp4

@sharadsw
Copy link
Contributor Author

@emenslin Yeah that is expected. The mapping line should ensure Taxon that don't belong to the COT don't show up in the results

@sharadsw sharadsw merged commit 7a67729 into production Jan 24, 2025
12 checks passed
@sharadsw sharadsw deleted the issue-6098 branch January 24, 2025 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

QCBX query builder tool can add a Determination from the wrong tree for the COT
5 participants