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

[BIG Refactoring] Add abstraction for search backend client #464

Merged
merged 24 commits into from
Aug 25, 2023

Conversation

fgravin
Copy link
Member

@fgravin fgravin commented Apr 24, 2023

Work in progress for OGC sprint.
Refactor the search client, which is now a full elasticsearch centric service, to be backend agnostic.
Add an abstraction, with clean architecture in mind and first add the gn4 (elasticsearch) implementation. This will prepare the work for adding OGC Records API later on.

We should work with DDD and try to focus on the domain and describe the models and usecases.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 24, 2023

Affected libs: api-metadata-converter, feature-editor, api-repository, feature-catalog, feature-record, feature-router, feature-search, feature-map, feature-dataviz, ui-search, common-domain, common-fixtures, util-shared, ui-elements, ui-catalog, ui-widgets, ui-inputs, ui-map, feature-auth, ui-dataviz, data-access-datafeeder, data-access-gn4, util-app-config, util-i18n, util-data-fetcher, ui-layout,
Affected apps: data-platform, datahub, metadata-converter, metadata-editor, webcomponents, demo, search, map-viewer, datafeeder,

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

@jahow jahow force-pushed the refactor-search-client branch 2 times, most recently from dfe001a to d6cd5a1 Compare May 2, 2023 06:01
@fgravin fgravin changed the title Add abstraction for search backend client [BIG Refactoring] Add abstraction for search backend client Jul 5, 2023
@jahow jahow force-pushed the refactor-search-client branch 3 times, most recently from b025a8b to 493ed7c Compare July 25, 2023 15:25
Copy link
Member Author

@fgravin fgravin left a comment

Choose a reason for hiding this comment

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

Thanks,

Few remarks

  • we should move out gn4 models from the domain
  • atomic.operation should not depend on gn4 model
  • we should only manipulate the pivot format, and remove the old model
  • where will you put the mappers ?
  • we should spread files in folders for the domain
  • shared/links should be moved

It's on a good way though, congratz


export const DEFAULT_PAGE_SIZE = 10

export const FIELDS_SUMMARY: FieldName[] = [
Copy link
Member Author

Choose a reason for hiding this comment

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

This should go in the domain with domain fields

includes: [...ES_SOURCE_BRIEF, 'createDate', 'changeDate'],
})
this.searchFacade.setConfigRequestFields([
...FIELDS_BRIEF,
Copy link
Member Author

Choose a reason for hiding this comment

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

should be pivot format fields

Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs a stronger abstraction and will be done later on

@jahow jahow added this to the 2.0.0 milestone Aug 2, 2023
@jahow jahow force-pushed the refactor-search-client branch 3 times, most recently from b6d9654 to b2e96f8 Compare August 22, 2023 14:14
Copy link
Member Author

@fgravin fgravin left a comment

Choose a reason for hiding this comment

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

Testing

  • clicking on header badges does not update the sortby input

@jahow jahow marked this pull request as ready for review August 22, 2023 16:58
@jahow
Copy link
Collaborator

jahow commented Aug 22, 2023

This is rebased on main and generally finished, we need to do more tests in the various apps to make sure nothing is broken.

I will rework the commit history to make the various steps of the refactoring clearer.

Also note that I've left on the side the handling of favorites and filter geometry in the search state; I think this can be done in another PR because it was just too many changes at once.

@jahow jahow force-pushed the refactor-search-client branch 7 times, most recently from fd97210 to 73a786b Compare August 23, 2023 17:47
@coveralls
Copy link

coveralls commented Aug 23, 2023

Coverage Status

coverage: 83.902% (-2.0%) from 85.859% when pulling 88f0225 on refactor-search-client into 9811dd5 on main.

Copy link
Member Author

@fgravin fgravin left a comment

Choose a reason for hiding this comment

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

Massive work !
Looks good, I noticed a breaking change with the wc elastic configuration. I added a note in the release description.

We can merge and slowly test and fix the side effects.

})
const state = reducerSearch(initialStateSearch, action)
expect(state.config.filters).toEqual({
custom: { any: 'blah', other: 'Some value' },
elastic: {},
Copy link
Member Author

Choose a reason for hiding this comment

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

Will break web components

filter='{"elastic": [{ "term": { "OrgForResource": "Géo2France" } }]}'

@fgravin fgravin merged commit ba8b141 into main Aug 25, 2023
5 checks passed
@fgravin fgravin deleted the refactor-search-client branch August 25, 2023 16:35
@f-necas
Copy link
Collaborator

f-necas commented Aug 25, 2023

🥳

@jahow
Copy link
Collaborator

jahow commented Aug 25, 2023

🎉

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.

4 participants