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

[DH] Metadata-Quality widget update #741

Merged
merged 4 commits into from
Dec 28, 2023
Merged

[DH] Metadata-Quality widget update #741

merged 4 commits into from
Dec 28, 2023

Conversation

f-necas
Copy link
Collaborator

@f-necas f-necas commented Dec 22, 2023

Metadata Quality widget simplifier

Removes config parameters for Metadata Quality widget.

Changes

  • Removes SORTABLE,DISPLAY_WIDGET_IN_DETAIL,DISPLAY_WIDGET_IN_SEARCH,DISPLAY_TITLE,DISPLAY_DESCRIPTION,DISPLAY_TOPIC,DISPLAY_KEYWORDS,DISPLAY_LEGAL_CONSTRAINTS,DISPLAY_CONTACT,DISPLAY_UPDATE_FREQUENCY,DISPLAY_ORGANISATION from Toml configuration file
  • Hide widget if pipeline is not installed and display warning in console
  • Update icons

TODO

  • Documentation

@f-necas f-necas requested a review from jahow December 22, 2023 17:07
Copy link
Contributor

github-actions bot commented Dec 22, 2023

Affected libs: feature-search, feature-router, feature-map, feature-dataviz, feature-record, ui-elements, feature-catalog, ui-catalog, ui-search, util-app-config,
Affected apps: datahub, metadata-editor, demo, webcomponents, search, map-viewer,

  • 🚀 Build and deploy storybook and demo on GitHub Pages
  • 📦 Build and push affected docker images

@coveralls
Copy link

coveralls commented Dec 22, 2023

Coverage Status

coverage: 86.601% (+3.6%) from 83.018%
when pulling 3888a57 on update-quality-widget
into ab94f4f on main.

Copy link
Collaborator

@jahow jahow left a comment

Choose a reason for hiding this comment

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

Thanks!! I think the customization options for the quality will be brought back later on but in a more flexible way. For now this should be more than enough.

Do you think it's possible to add an end-to-end test for the quality indicator? There's already a mechanism in the datahub e2e tests to mock a configuration, it could be worthwile to mock a config with the md quality enabled and check whether the widgets show up and the sorting work. This might necessitate running the pipelines on the support services docker compo as well.

This can be done in another PR though.

fix: CI

fix: CI

fix: CI add pipeline

fix: CI add pipeline
@f-necas f-necas force-pushed the update-quality-widget branch from 1130470 to 0192cd9 Compare December 26, 2023 16:49
@f-necas
Copy link
Collaborator Author

f-necas commented Dec 26, 2023

Thanks for your review @jahow. I've added a test but not documentation by now.

Tests are now covering approximatively :

  • Usage of Toml File to enable/disable quality widget display.
  • Records not re-indexed (similar to pipeline not present)
  • Display of quality widget
  • Sorting by quality score (desc)

Copy link
Collaborator

@jahow jahow left a comment

Choose a reason for hiding this comment

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

Added a comment on the docker composition, thanks for adding an e2e test!!

ES_HOST: http://elasticsearch:9200
RECORDS_INDEX: gn-records
depends_on:
init:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you think you could invert the dependency, and make init depend on this? init will do a reindexation of the catalog so that would mean the md quality widget would show up right away without needing a manual indexation


describe('metadata quality widget enabled', () => {
beforeEach(() => {
// this will enable spatial filtering
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// this will enable spatial filtering
// this will enable metadata quality

Comment on lines 486 to 497
it('should reindex records', () => {
cy.login('admin', 'admin', false)
cy.visit(
`http://localhost:8080/geonetwork/srv/eng/admin.console#/tools`
)
cy.intercept({
method: 'PUT',
url: 'http://localhost:8080/geonetwork/srv/api/site/index?reset=false&asynchronous=true',
}).as('indexingRecordsXHR')
cy.get('[class=panel-body]').find('button').first().click()
cy.wait('@indexingRecordsXHR')
})
Copy link
Collaborator

Choose a reason for hiding this comment

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

See my comment on the docker composition; also this check here is unfortunately not enough to guarantee that all records are indeed indexed correctly, which is why I added a more robust logic in the init container. See:

# finally check that the index has records in it
recordsCount=0
while [ "$recordsCount" = '0' ];
do
response=$(
curl -s "http://$host/geonetwork/srv/api/search/records/_search" \
-H 'Accept: application/json, text/plain, */*' \
-H 'Content-Type: application/json;charset=UTF-8' \
-H "Cookie: JSESSIONID=$jsessionid; XSRF-TOKEN=$xsrf_token" \
-H "X-XSRF-TOKEN: $xsrf_token" \
--data-raw '{"size":0}'
)
recordsCount=$(echo $response | sed 's/.*"hits":{"total":{"value":\([0-9]\+\).*/\1/g')
echo "Records found: $recordsCount"
sleep 2
done

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't succeed to make it working that way. The /index curl drops index and re-index by default :
image

If I set a reset=false query param, it doesn't work (400 on datahub). Any idea ?

Copy link
Collaborator

@jahow jahow Dec 27, 2023

Choose a reason for hiding this comment

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

If I set a reset=false query param, it doesn't work (400 on datahub). Any idea ?

Maybe that's because the index wasn't created yet? I'm not sure. You can try using reset=false&asynchronous=true since is what you did in the e2e test.

edit: just tried and indeed, indexing with reset=false doesn't work because then all search requests in the datahub will fail, probably because the index is misconfigured.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I pushed a commit trying to address that, let's see if the CI succeeds

@jahow jahow force-pushed the update-quality-widget branch 5 times, most recently from 9d92082 to f18229c Compare December 28, 2023 13:26
…cords

Also adapts datasets e2e to run without indexing again
@jahow jahow force-pushed the update-quality-widget branch from f18229c to 3888a57 Compare December 28, 2023 13:56
@f-necas f-necas merged commit 6046f20 into main Dec 28, 2023
8 checks passed
@f-necas f-necas deleted the update-quality-widget branch December 28, 2023 14:11
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.

3 participants