Skip to content

Conversation

@aleksandar-ivic
Copy link
Contributor

Problem

Currently importing annotation or section text properties fails.

Solution

Add support for annotation and section text properties. Expand e2e tests to cover this.

Changelog

Allow annotation and text properties import

@linear
Copy link

linear bot commented Feb 25, 2025

Copy link
Contributor

@JBWilkie JBWilkie left a comment

Choose a reason for hiding this comment

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

I'd add text properties to 1 additional E2E test: test_annotation_classes_are_created_with_properties_on_import

This creates a fresh ontology of classes and properties from local annotation files and an accompanying metadata.json file, then runs the standard import test. This will ensure we can create new text properties from manifest files. To do this, we'll need to:

  • 1: Add text property values to some JSON files in the e2e_tests/data/import/image_new_annotations_with_properties directory
  • 2: Add a text property to the .v7/metadata.json file in the same directory, such as:
      "properties": [
        {
          "name": "ms1",
          "type": "multi_select",
          "description": "",
          "required": true,
          "property_values": [
            {
              "value": "1",
              "color": "rgba(173,255,0,1.0)"
            },
            {
              "value": "2",
              "color": "rgba(143,255,0,1.0)"
            }
          ]
        },
        {
          "name": "my_text_prop",
          "type": "text",
          "description": "Write some text",
          "required": false,
          "property_values": []
        },

Copy link
Collaborator

@umbertoDifa umbertoDifa left a comment

Choose a reason for hiding this comment

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

LGTM a part from the ugly size of what this import has become which isn't the purpose of this PR anyways.

A few tests we should have:

  • Have we tested the scenario when we set a text section-level property to empty?
  • Have we tested the scenario when we set a text annotation-level property to empty?
  • What happens if we have text section level property with the same name of text annotation level property?
  • What happens if the have a text annotation level property and an item level property with the same name?

…e for item and section/annotation level props
@aleksandar-ivic
Copy link
Contributor Author

aleksandar-ivic commented Feb 26, 2025

  • What happens if we have text section level property with the same name of text annotation level property?

LGTM a part from the ugly size of what this import has become which isn't the purpose of this PR anyways.

A few tests we should have:

  • Have we tested the scenario when we set a text section-level property to empty?
  • Have we tested the scenario when we set a text annotation-level property to empty?
  • What happens if we have text section level property with the same name of text annotation level property?
  • What happens if the have a text annotation level property and an item level property with the same name?
  • Empty values failed, I fixed that
  • Section and annotation level properties can't have the same name, this is forbidden on API level.
  • Same name for item and section/annotation level props is allowed, I've added one test case to cover this.

@AndriiKlymchuk
Copy link
Contributor

Can we add some tests to ensure fixes work as expected? Or the are already there and just data were missing?

@aleksandar-ivic
Copy link
Contributor Author

Can we add some tests to ensure fixes work as expected? Or the are already there and just data were missing?

There are tests present, I've just expanded them with new data

@aleksandar-ivic aleksandar-ivic merged commit 67daf74 into master Feb 28, 2025
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants