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

INSPIRE JRC validation #2444

Merged
merged 3 commits into from
Jan 22, 2018
Merged

Conversation

PascalLike
Copy link
Contributor

@PascalLike PascalLike commented Dec 19, 2017

This pull request introduces an extra validation functionality for ISO19139 metadata, based on ETF web application.

The validation is designed like a remote validation service, by using the API on a configured ETF web application. This ETF endpoint must be configured in settings:

settings

A check on the checkbox and a valid endpoint are both required.

The interaction with this endpoint works in two steps, implemented in two new methods in GeoNetwork API:

api

The first method is used for the submission of a metadata record to the remote service for the validation.
The validation is not in real time, so the second method is needed to check asynchronously the progress.

When the INSPIRE validation is set in GeoNetwork settings, the validation button change in a dropdown with local validation and INSPIRE one:

dropdown

When an INSPIRE validation is completed this panel is shown:

panel

With the exit status of the validation and a link to a report with a detailed report (on the ETF web application):

report

This pull request is made with the intention to improve the integration of INSPIRE directives in GeoNetwork.

@PascalLike PascalLike changed the title Integrate INSPIRE JRC validation INSPIRE JRC validation Dec 19, 2017
@PascalLike
Copy link
Contributor Author

@fxprunayre and @josegar74 please check if it's ok.

@@ -772,6 +772,8 @@
"system/inspire/atomDisabled": "Disabled",
"system/inspire/enable-help": "Enable INSPIRE CSW (ie. language support and INSPIRE GetCapabilities document) and INSPIRE indexing. INSPIRE themes thesaurus MUST be installed to properly index themes and annexes. Note: It does not enable the INSPIRE editor view mode (see iso19139/layout/config-editor.xml).",
"system/inspire/enableSearchPanel-help": "Enable search panel (Ext search only).",
"system/inspire/remotevalidation/url": "INSPIRE remote validation URL",
Copy link
Member

Choose a reason for hiding this comment

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

I would add also a placeholder value for the field with the url of actual service, for now is in a sandbox, but can be updated later when the service is "official". In any case the users can setup the service locally also.

This should help the users with the url format. Placeholder: http://inspire-sandbox.jrc.ec.europa.eu/etf-webapp/

try {
Element md = (Element) ApiUtils.getUserSession(session).getProperty(Geonet.Session.METADATA_EDITING + id);
if (md == null) {
throw new ResourceNotFoundException(String.format("Requested metadata with id '%s' is not available in current session. "
Copy link
Member

Choose a reason for hiding this comment

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

Check to move the message to message bundle files. Not sure if done in other services, but should be good to use them to provide localised error messages.

@@ -0,0 +1,237 @@
package org.fao.geonet.api.records;
Copy link
Member

Choose a reason for hiding this comment

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

Add file header

@@ -0,0 +1,494 @@
package org.fao.geonet.api.records.editing;
Copy link
Member

Choose a reason for hiding this comment

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

Add file header.

Also a unit test for this class can be useful.

@@ -0,0 +1,44 @@
<span>
Copy link
Member

Choose a reason for hiding this comment

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

Check the indentation of the file, afaik we use 2 spaces in html files.

@josegar74 josegar74 added this to the 3.4.2 milestone Dec 20, 2017
+ "This activates an asyncronous process, this method does not return any report. "
+ "This method returns an id to be used to get the report.",
nickname = "submitValidate")
@RequestMapping(value = "/{metadataUuid}/validate/submit",
Copy link
Member

Choose a reason for hiding this comment

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

Would it be better to have the API like

PUT /srv/api/0.1/records/{metadataUuid}/validate/internal (current one XSD + schematron)
PUT /srv/api/0.1/records/{metadataUuid}/validate/inspire

?

So we can add new validator in the long term.

gnPopup.createModal({
class: 'disclaimer-popup',
title: $translate.instant('inspirePopupReportTitle'),
content: '<div>' + $translate.instant('inspirePopupReportText') + status + '</br></br>' + '<a href=\''+url+'\' target=\'_blank\'>'+$translate.instant('inspirePopupReportLink')+'</a></div>'
Copy link
Member

Choose a reason for hiding this comment

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

You should run

cd web-ui
mvn clean install -Pjslint

to clean up JS formatting.

@fxprunayre
Copy link
Member

LGTM, maybe one change to apply to the API ?

In the long term, we should maybe rework the validation to make them more configurable in the DataManager but we should wait for Maria's refactor first.

@fxprunayre
Copy link
Member

This is related to #2380

@PascalLike
Copy link
Contributor Author

Thanks @josegar74 @fxprunayre , fixed 👍

+ "This activates an asyncronous process, this method does not return any report. "
+ "This method returns an id to be used to get the report.",
nickname = "submitValidate")
@RequestMapping(value = "/{metadataUuid}/validate/inspire/submit",
Copy link
Member

Choose a reason for hiding this comment

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

Thanks Antonio for the API layout change. I would even remove /submit and /check as PUT will mean start and GET check ? What do you think ?

Copy link
Member

@fxprunayre fxprunayre left a comment

Choose a reason for hiding this comment

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

Need a rebase too. It would be good to merge this next week.

@PascalLike
Copy link
Contributor Author

@fxprunayre, why it's needed a rebase?
I did requested changes on REST methods signatures.
The checks failed, but seems to be a travis issue.

@PascalLike
Copy link
Contributor Author

I did a rebase (juan told me that there where present conficts on the reviewer interface) and cleaned the commit history

@fxprunayre
Copy link
Member

Testing with http://inspire-sandbox.jrc.ec.europa.eu/etf-webapp, I've quite often 4 times for 5 calls

image

A null test id is returned. Do you have an idea why ? Did you test with local docker install of ETF ?

It's also quite slow around 2min for a successfull or not call.

@PascalLike
Copy link
Contributor Author

I'll investigate how is managed this error scenario

I suggest you to use the docker image. The official etf-webapp is slower and this time I suspect not working
docker run --name etf -d -p 80:8080 -v ~/etf:/etf iide/etf-webapp:latest
(It need some time to be ready after started)

@PascalLike
Copy link
Contributor Author

@fxprunayre , I managed the case where etf returns null but with no HTTP error code. Please let me know if it works now.
The validation task status is checked every 10 seconds, do you think it's too slow?

@Delawen
Copy link
Contributor

Delawen commented Jan 11, 2018

@fxprunayre if you still think this is ok, I will merge. You were the one experiencing weird behaviour.

@@ -421,6 +421,7 @@
isXLinkEnabled: 'system.xlinkResolver.enable',
isSelfRegisterEnabled: 'system.userSelfRegistration.enable',
isFeedbackEnabled: 'system.userFeedback.enable',
isInspireEnabled: 'system.inspireValidation.enable',
Copy link
Member

Choose a reason for hiding this comment

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

This is in a comment section, but the value is not fine, should be the same as in line 440: isInspireEnabled: 'system.inspire.enable',

@fxprunayre fxprunayre merged commit 9e9c543 into geonetwork:3.4.x Jan 22, 2018
@PascalLike PascalLike deleted the etf-remote-validation branch January 23, 2018 08:02
@josegar74
Copy link
Member

@PascalLike something to check also would be the naming of the directive/html template as currently seem handling both the internal and INSPIRE validation, but the naming of the files/directive refers only to INSPIRE and it's confusing users.

Apologies as I didn't notice in the review about this.

@PascalLike
Copy link
Contributor Author

@josegar74 I fixed this here #2507

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

Successfully merging this pull request may close these issues.

None yet

4 participants