ome / omero-web

Django-based OMERO.web client
https://www.openmicroscopy.org/omero
16 stars 29 forks source link

Limit number of FileAnnotations retrieved for OMERO.web UI #519

Closed knabar closed 6 months ago

knabar commented 7 months ago

Addresses issue #517

The current "Choose attachment" dialog retrieves a list of every FileAnnotation not currently linked to the selected object(s) for linking, causing poor performance or failures on systems with a large number of FileAnnotations.

The goal of this PR is to address the potential performance impact only; any other improvements e.g. to the UI should be addressed in separate PRs.

Main discussion points:

API changes

  1. getFilesByObject now returns a tuple of the number of total files and a list of FileAnnotationShims; previously the method returned a single list of FileAnnotations.
  2. In case of no objects passed to the controller, getFilesByObject previously returned all FileAnnotations, and will now raise a ValueError. This also means listFileAnnotations is no longer used and could be deprecated.

Testing

There are currently no stated expectations to the exact behavior of the "Choose attachment" dialog or the getFilesByObject method. The UI also does not expose all of the available options of the method. With the large number of permutations of options and object types (and multiple object types, which are not handled), exhaustive testing will be difficult. Current testing is likely minimal.

Potential pitfalls:

Discussion

will-moore commented 7 months ago

With a bunch of FileAnnotations created with https://github.com/will-moore/python-scripts/blob/main/create_file_annotations.py this looks like:

Screenshot 2023-12-11 at 09 47 48

If I add a bunch of FileAnnotations, then these are correctly excluded from the dialog next time I open it and the count goes down accordingly.

Do we want to boost the page size and sort alphabetically as discussed?

knabar commented 7 months ago

Do we want to boost the page size and sort alphabetically as discussed?

@will-moore I'm working on that today.

will-moore commented 6 months ago

I think this PR is failing tests at https://merge-ci.openmicroscopy.org/jenkins/job/OMERO-test-integration/lastCompletedBuild/testReport/OmeroWeb.test.integration.test_annotate/TestFileAnnotations/test_batch_add_fileannotations_2_/

The test is failing to exclude an annotation previously linked to the selected Projects. I tested in merge-ci webclient and found the same thing with FileAnnotations previously created by upload. This may be because they don't have a namespace??

knabar commented 6 months ago

@will-moore @pwalczysko I added the NULL check for the namespace to bring the new query more in line with the old one

will-moore commented 6 months ago

Working fine:

Screenshot 2023-12-13 at 15 37 55

pwalczysko commented 6 months ago

Works as expected on merge-ci. lgtm

imagesc-bot commented 4 months ago

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

https://forum.image.sc/t/unable-to-add-attachment-using-omero-web/84146/22