Skip to content

Add new feature to change taxon tree type in tree viewer - #5036

Merged
CarolineDenis merged 101 commits into
productionfrom
issue-4829
Aug 1, 2024
Merged

Add new feature to change taxon tree type in tree viewer#5036
CarolineDenis merged 101 commits into
productionfrom
issue-4829

Conversation

@CarolineDenis

@CarolineDenis CarolineDenis commented Jun 24, 2024

Copy link
Copy Markdown
Contributor

Fixes #4829
Fixes #4978
Fixes #4979

Checklist

  • Self-review the PR after opening it to make sure the changes look good
    and self-explanatory (or properly documented)
  • Add relevant issue to release milestone

Testing instructions

==> you need to add a new taxon tree in your db to test this PR

Tree Viewer

  • Open the taxon tree viewer
  • If the db as only one taxon tree, verify that the header is a H2
  • If the db as several taxon trees, verify that the header is a drop down
  • Verify that the header drop down contain all the trees of the db
  • Change the tree type
  • Verify the tree has changed
  • click on a node, add a child
  • verify that the ranks in the drop down are from the correct tree

QB

  • Open the a new Taxon query
  • Verify it is the same case when doing: New CO query > Determination > Taxon
  • Query on any rank from any tree
  • Query on a rank present in 2 or more trees and verify all nodes are doing return

@CarolineDenis CarolineDenis added this to the 7.9.7 milestone Jun 24, 2024
Comment thread specifyweb/frontend/js_src/lib/components/InitialContext/treeRanks.ts Outdated
@melton-jason

Copy link
Copy Markdown
Contributor

@acwhite211 @CarolineDenis

Here is a summary of the major changes I did

Commits:
https://github.com/specify/specify7/pull/5036/files/fc36e4c1b779e172f35010c63c5a9a1c5be11f76..dcc229a65f5af2f1797937136d8d52710c26ec70

Backend

I have gone ahead and implemented the endpoint I suggested in a520069#commitcomment-143518914.

The endpoint returns all of the tree-structure related information for every tree the user has read access to.
That is, for each type of tree (Taxon, Storage, Geography, Lithostrat, Geologictimeperiod), an array of all definitions (TreeDefs) as well as all ranks (TreeDefItems) for each definition sorted by rankid.

The frontend previously was doing a serious of requests to create an object similar to what this endpoint now returns.

export const treeRanksPromise = Promise.all([
// Dynamic imports are used to prevent circular dependencies
import('../Permissions/helpers'),
import('../Permissions').then(async ({ fetchContext }) => fetchContext),
import('../DataModel/tables').then(async ({ fetchContext }) => fetchContext),
fetchDomain,
])
.then(async ([{ hasTreeAccess, hasTablePermission }]) =>
hasTablePermission('Discipline', 'read')
? getDomainResource('discipline')
?.fetch()
.then((discipline) => {
if (!f.has(paleoDiscs, discipline?.get('type')))
disciplineTrees = commonTrees;
})
.then(async () =>
Promise.all(
disciplineTrees
.filter((treeName) => hasTreeAccess(treeName, 'read'))
.map(async (treeName) =>
getDomainResource(getTreeScope(treeName) as 'discipline')
?.rgetPromise(
`${unCapitalize(treeName) as 'geography'}TreeDef`
)
.then(async (treeDefinition) => ({
definition: treeDefinition,
ranks: await fetchRelated(
serializeResource(treeDefinition),
'treeDefItems',
0
).then(({ records }) =>
Array.from(records).sort(
sortFunction(({ rankId }) => rankId)
)
),
}))
.then((ranks) => [treeName, ranks] as const)
)
)
)
: []
)
.then((ranks) => {
// @ts-expect-error
treeDefinitions = Object.fromEntries(ranks.filter(Boolean));
return treeDefinitions;
});

Feel free to share what you think:

def all_tree_information(request):
def has_tree_read_permission(tree: TREE_TABLE) -> bool:
return has_table_permission(
request.specify_collection.id, request.specify_user.id, tree, 'read')
is_paleo_discipline = request.specify_collection.discipline.is_paleo()
accessible_trees = tuple(filter(
has_tree_read_permission, ALL_TRESS if is_paleo_discipline else COMMON_TREES))
result = {}
for tree in accessible_trees:
result[tree] = []
treedef_model = getattr(models, f'{tree.lower().capitalize()}treedef')
tree_defs = treedef_model.objects.all()
for definition in tree_defs:
ranks = definition.treedefitems.order_by('rankid')
result[tree].append({
'definition': obj_to_data(definition),
'ranks': [obj_to_data(rank) for rank in ranks]
})
return HttpResponse(toJson(result), content_type='application/json')

Also did some other small refactoring where I saw fit: primarily improving typing.

Frontend

The frontend work consisted of using the newly defined endpoint, getting the cache for the definition names up and running, and changing the pre-existing code to be compatible with all of the changes we have done to treeRanksPromise (and to lay the foundations for other changes in this PR).

Instead of having a single object in the cache which keeps track of each Tree's definition the user was last using, now we are creating a separate cache entry for each tree. This should make it simpler and easier to both access and set.

https://github.com/specify/specify7/pull/5036/files/fc36e4c1b779e172f35010c63c5a9a1c5be11f76..dcc229a65f5af2f1797937136d8d52710c26ec70#diff-af2e7f4cf2d56aca1781676234d2c93967a908b230cc5a04eb37a1309815e2b2R83-R84

💡 Although I would like to use the definition ids rather than the name internally to keep track of the 'current' tree. If the names of the trees change, this would automatically handle the case.

The code is completely functional right now: if the user has more than one Tree of a specific type created, they can switch which tree they are using. in the TreeView.

Other places which used the treeDefinitions promise and object should now also be compatible with the changes. We will just need to touch these places up when we work out resolving the 'default' Tree (currently it uses the first tree in the array of all trees).

@CarolineDenis

CarolineDenis commented Jun 27, 2024

Copy link
Copy Markdown
Contributor Author

@melton-jason,
Thank you for these changes, the new API makes the frontend code so much simpler

Although I would like to use the definition ids rather than the name internally to keep track of the 'current' tree. If the names of the trees change, this would automatically handle the case.

Yes, I think that would be good

resolving the 'default' Tree (currently it uses the first tree in the array of all trees).

We'll have to define where we set this default

@realVinayak

Copy link
Copy Markdown
Contributor

actually, this'll be in production too.....

@emenslin

Copy link
Copy Markdown
Collaborator

@emenslin , or I guess @specify/ux-testing, clear your cache out before testing

I did clear my cache and it's still happening

@emenslin

Copy link
Copy Markdown
Collaborator

Still not working, I get this error once logged into the db
Screenshot 2024-07-31 101224

Specify 7 Crash Report - 2024-07-31T15_12_25.055Z.txt

@realVinayak

Copy link
Copy Markdown
Contributor

Sorry, I didn't get to test it locally before pushing. Resolved now.

@realVinayak
realVinayak requested a review from emenslin July 31, 2024 15:39
@melton-jason
melton-jason requested review from a team and emenslin and removed request for emenslin July 31, 2024 20:12

@emenslin emenslin left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Testing instructions

==> you need to add a new taxon tree in your db to test this PR

Tree Viewer

  • Open the taxon tree viewer
  • If the db as only one taxon tree, verify that the header is a H2
  • If the db as several taxon trees, verify that the header is a drop down
  • Verify that the header drop down contain all the trees of the db
  • Change the tree type
  • Verify the tree has changed
  • click on a node, add a child
  • verify that the ranks in the drop down are from the correct tree

QB

  • Open the a new Taxon query
  • Verify it is the same case when doing: New CO query > Determination > Taxon
  • Query on any rank from any tree
  • Query on a rank present in 2 or more trees and verify all nodes are doing return

Everything looks good from what I can tell, did not run into any of the previous errors this time

@emenslin
emenslin requested a review from a team August 1, 2024 15:10

@Areyes42 Areyes42 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Testing instructions

==> you need to add a new taxon tree in your db to test this PR

Tree Viewer

  • Open the taxon tree viewer
  • If the db as only one taxon tree, verify that the header is a H2
  • If the db as several taxon trees, verify that the header is a drop down
  • Verify that the header drop down contain all the trees of the db
  • Change the tree type
  • Verify the tree has changed
  • click on a node, add a child
  • verify that the ranks in the drop down are from the correct tree

QB

  • Open the a new Taxon query
  • Verify it is the same case when doing: New CO query > Determination > Taxon
  • Query on any rank from any tree
  • Query on a rank present in 2 or more trees and verify all nodes are doing return

Tree viewer and QB looks good across multiple different DBs that I tested with. Didn't come across any major issues 👍

@lexiclevenger lexiclevenger left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Testing instructions

==> you need to add a new taxon tree in your db to test this PR

Tree Viewer

  • Open the taxon tree viewer
  • If the db as only one taxon tree, verify that the header is a H2
  • If the db as several taxon trees, verify that the header is a drop down
  • Verify that the header drop down contain all the trees of the db
  • Change the tree type
  • Verify the tree has changed
  • click on a node, add a child
  • verify that the ranks in the drop down are from the correct tree

QB

  • Open the a new Taxon query
  • Verify it is the same case when doing: New CO query > Determination > Taxon
  • Query on any rank from any tree
  • Query on a rank present in 2 or more trees and verify all nodes are doing return

Looks good, everything seems to be working as expected. 👍

@CarolineDenis
CarolineDenis dismissed stale reviews from realVinayak, melton-jason, and alesan99 August 1, 2024 17:15

reviewed

@CarolineDenis
CarolineDenis merged commit 822d25e into production Aug 1, 2024
@CarolineDenis
CarolineDenis deleted the issue-4829 branch August 1, 2024 17:15
@realVinayak

Copy link
Copy Markdown
Contributor

@acwhite211 could have at least tried to add unit tests 🤦

@specifysoftware

Copy link
Copy Markdown

This pull request has been mentioned on Specify Community Forum. There might be relevant details there:

https://discourse.specifysoftware.org/t/specify-7-9-7-release-announcement/1979/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: ✅Done