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(ontologies-endpoint): add new cardinalities check route (DEV-1704) #499

Merged
merged 6 commits into from
Feb 28, 2023

Conversation

mdelez
Copy link
Contributor

@mdelez mdelez commented Feb 15, 2023

resolves DEV-1704

@mdelez mdelez added the enhancement Improve existing code or new feature label Feb 15, 2023
@mdelez mdelez self-assigned this Feb 15, 2023
@linear
Copy link

linear bot commented Feb 15, 2023

DEV-1704 Teach JS-Lib new routes

In the course of the "Changing of Cardinalities" project a new route to dsp-api was added, which allows for a pre-update check of whether a specific cardinality can be set on a class/property combination:

HTTP GET to http://host/v2/ontologies/canreplacecardinalities/CLASS_IRI?propertyIri=PROPERTY_IRI&newCardinality=[0-1|1|1-n|0-n]

The request

HTTP POST to http://host/v2/ontologies/cardinalities

if it fails now also contains the reasons for the failure as it is using the same pre-update check as above.

Both features are available in the latest release. However, the documentation did not make it, but you can find the documentation in the following PR:

https://github.com/dasch-swiss/dsp-api/pull/2420/files

Especially take a look at chapter "Replacing the Cardinalities of a Class" and its paragraph "Pre-Update Checks" where the new endpoint and new response are described.

@mdelez mdelez requested a review from mpro7 February 16, 2023 14:58
Copy link
Collaborator

@mpro7 mpro7 left a comment

Choose a reason for hiding this comment

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

Looks good. If both question I asked below are no === merge ;)

Comment on lines 27 to 39
export const getCardinalityString = (cardinality: Cardinality): string => {
switch(cardinality) {
case Cardinality._0_1:
return "0-1"
case Cardinality._0_n:
return "0-n"
case Cardinality._1:
return "1"
case Cardinality._1_n:
return "1-n"
}
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Wouldn't be nicer to make a Map<Cardinality, string> a const and then .get the value or for...of the keys, just instead of 2 switches.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doing this would make it more flexible but I'm not sure if it's really needed. I have the second switch in the app component because I don't have access to Cardinality in the template to pass it into the method. I'm not sure if this would ever actually be an issue in the DSP-APP. Having said that, I can try it out and see how it works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in 8b2406c

Comment on lines -779 to -780
const canDelete = page.getEle('div button.can-replace-card-for-res-card');
canDelete.click();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just to confirm this is not needed anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was incorrectly named probably due to copy-pasting

@mdelez mdelez merged commit fef23cf into main Feb 28, 2023
@mdelez mdelez deleted the wip/dev-1704-new-cardinality-route branch February 28, 2023 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improve existing code or new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants