-
Notifications
You must be signed in to change notification settings - Fork 32
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
Reactivate filtering by favorites & geometry #623
Conversation
Affected libs:
|
5be83c4
to
4d739a5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this recovery @jahow ! Code LGTM and features worked fine when testing them.
Regarding our comment in the default.toml
"...all records which do not intersect with the geometry will be excluded", we should maybe change "excluded" to something like "ranked lower" as the results still show up in the search, shouldn't we?
beforeEach(() => { | ||
// this will enable spatial filtering | ||
cy.intercept('GET', '/assets/configuration/default.toml', { | ||
fixture: 'config-with-geometry.toml', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really handy!
@@ -29,6 +30,48 @@ Cypress.Commands.add('login', () => { | |||
cy.get('[name="gnSigninForm"]').submit() | |||
}) | |||
|
|||
Cypress.Commands.add('clearFavorites', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, also really handy! I did not know this.
map((action: SearchActions) => new RequestNewResults(action.id)) | ||
this.actionsWithNewResults$.pipe( | ||
// this will aggregate actions until the debounceTime ticks | ||
buffer(this.actionsWithNewResults$.pipe(debounceTime(0))), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this buffer/debounceTime
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the idea here is that if you call for instance searchFacade.orderBy('createDate').setPageSize(10)
etc., each call to the facade will trigger an action that will eventually trigger a RequestNewResults
action. So you end up having many of these, many search requests and potential conflicts.
But because there are several search ids, the grouping is actually quite tricky indeed... I've tried to make this as clear as possible 😬
// once we have a list of actions emitted since last time, we can split them by search id | ||
const requestNewResults = actions | ||
.map((action) => action.id) | ||
.filter((value, index, array) => array.indexOf(value) === index) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Waoh, pretty tricky we need to think of handling this.
Without this change, actions would be grouped together inc. with different search id, and that means this could break concurrent searches with different ids
4d739a5
to
d055be3
Compare
@tkohr thanks for the review! I adjusted the doc in the config file and also improved the clearFavorites command which was failing in headless mode (Cypress is still very mysterious sometimes...) |
These features were disabled in #464 .
Filtering by favorites and geometry was reimplemented in a very similar way, with a few structural changes:
@geonetwork-ui/api/repository/gn4
libfeature/auth
lib that simply provides the services for showing avatars; we might put other things there in the future so I didn't get rid of itFixes #590
Added E2E tests for those two filtering options.
Also fixes a bug where, because now we're "grouping" actions before triggering a new search, actions targeting different search ids could end up being list.
Also fixes a bug where icons in buttons were not correctly sized (missed from 4d7654a)