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

feat(ontology): refactor list of properties in resource class (DSP-1360) #389

Merged
merged 29 commits into from Feb 22, 2021

Conversation

@kilchenmann
Copy link
Collaborator

@kilchenmann kilchenmann commented Feb 17, 2021

resolves DSP-1360 (bug fix by returning resource class and properties in all languages)

incl. refactoring of cache and ontology editor in general.

The ontology editor lists the resource classes with new list of corresponding properties; the design contains:

  • gui order
  • icon = property type (subclass and gui element); has a tooltip with more information
  • label = property label with property comment in tooltip;
    • in case of a link property it shows corresponding resource class
    • in case of a list property it shows corresponding list as a link
  • cardinality = checkbox icons for "can have multiple values" and "value is required"

Screen Shot 2021-02-18 at 07 44 04

@kilchenmann kilchenmann requested review from subotic, flavens and mdelez Feb 17, 2021
Copy link
Collaborator

@flavens flavens left a comment

  • There are some index, e.g. 0) 1), that are doubled or tripled, or even missing, e.g. 0) -> 2)
  • Is it normal behaviour to be able to click on the referred list but not on the linked resource?
  • Maybe only on my local app: For the BEOL ontologies, I have an error message in the console.log: "Cannot read property 'label' of undefined" - property-info.component.ts:95

Loading

@kilchenmann
Copy link
Collaborator Author

@kilchenmann kilchenmann commented Feb 18, 2021

  • There are some index, e.g. 0) 1), that are doubled or tripled, or even missing, e.g. 0) -> 2)

The Gui order is defined in the ontology; it's part of the res class cardinality. In case of missing or doubled numbers it's a mistake in the ontology. The test data are not well done.

  • Is it normal behaviour to be able to click on the referred list but not on the linked resource?

Yes. At the moment we do not have a native route for the linked resource class. I was thinking to highlight the linked class somehow. But this will take more time.

  • Maybe only on my local app: For the BEOL ontologies, I have an error message in the console.log: "Cannot read property 'label' of undefined" - property-info.component.ts:95

I will check this...

Loading

@mdelez
Copy link
Contributor

@mdelez mdelez commented Feb 18, 2021

  • Maybe only on my local app: For the BEOL ontologies, I have an error message in the console.log: "Cannot read property 'label' of undefined" - property-info.component.ts:95

I have the same console error. The BEOL ontology also has a lot of empty resource classes at the bottom for some reason. Maybe that's the issue @kilchenmann?

EDIT: Actually, the Biblio ontology has 36 of these console errors and doesn't contain any empty resource classes,

Screen Shot 2021-02-18 at 11 36 00

Loading

@kilchenmann
Copy link
Collaborator Author

@kilchenmann kilchenmann commented Feb 18, 2021

Yes there's something weird with the ontology test-data in some projects. I will spend some time to do some investigations. Maybe someone of you can help me with that?

Loading

@kilchenmann
Copy link
Collaborator Author

@kilchenmann kilchenmann commented Feb 18, 2021

I already found something: the empty resource classes doesn't have a label. They are all of type "Standoff"-something. Will try to fix it; standoff doesn't have to be displayed

Loading

it('should create', () => {
expect(testHostComponent).toBeTruthy();
});
});
Copy link
Contributor

@mdelez mdelez Feb 18, 2021

Choose a reason for hiding this comment

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

I would add a test here to ensure the component is receiving the correct data for propDef, propCard, and projectcode via the @Inputs

Loading

Copy link
Contributor

@mdelez mdelez Feb 19, 2021

Choose a reason for hiding this comment

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

@kilchenmann please add these assertions and then I will approve :)

Loading

Copy link
Collaborator Author

@kilchenmann kilchenmann Feb 19, 2021

Choose a reason for hiding this comment

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

Will add it this evening.

Loading

Copy link
Collaborator Author

@kilchenmann kilchenmann Feb 20, 2021

Choose a reason for hiding this comment

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

done in 8cf574e

Loading


@Input() propCard: IHasProperty;

@Input() projectcode: string;
Copy link
Contributor

@mdelez mdelez Feb 18, 2021

Choose a reason for hiding this comment

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

projectcode -> projectCode

Loading

Copy link
Collaborator Author

@kilchenmann kilchenmann Feb 18, 2021

Choose a reason for hiding this comment

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

it's the same as in other components. To be consistent, I have to change it everywhere. But I can do it...

Loading

Copy link
Contributor

@mdelez mdelez Feb 18, 2021

Choose a reason for hiding this comment

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

I've noticed it elsewhere as well. Maybe leave it for now and we can open a small PR to rename them everywhere all at once.

Loading

@kilchenmann
Copy link
Collaborator Author

@kilchenmann kilchenmann commented Feb 18, 2021

I have the same console error. The BEOL ontology also has a lot of empty resource classes at the bottom for some reason.

I found the problem. The linked resource is defined in another ontology. The ontology editor is not yet prepared for this case. It will make the things more complex 🙈 ... but fine, I'll fix it.

Loading

@kilchenmann kilchenmann requested review from mdelez and flavens Feb 18, 2021
@kilchenmann
Copy link
Collaborator Author

@kilchenmann kilchenmann commented Feb 18, 2021

It seems to be fixed now. I had to integrate new cache for currentProjectOntologies. Now we have the class information from other ontologies than the current one if in case a link property is connected with a class from another ontology.

Loading

@subotic
Copy link
Contributor

@subotic subotic commented Feb 18, 2021

great, so as soon as you merge this, please make a release and deploy it to test and staging. thanks.

Loading

mdelez
mdelez approved these changes Feb 22, 2021
@kilchenmann kilchenmann changed the title fix(ontology): refactor list of properties in resource class (DSP-1360) feat(ontology): refactor list of properties in resource class (DSP-1360) Feb 22, 2021
@kilchenmann kilchenmann merged commit aa565b3 into main Feb 22, 2021
6 checks passed
Loading
@kilchenmann kilchenmann deleted the wip/dsp-1360-list-of-properties-in-res-class branch Feb 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants