CUMULUS-3859-1: Remove es from local api code and update active collection search #3861
Conversation
There was a problem hiding this comment.
Looks great @jennyhliu! I had a couple questions but mostly just looking for context.
| @@ -79,7 +79,7 @@ | |||
| "@babel/eslint-parser": "^7.24.1", | |||
| "@babel/preset-env": "^7.24.4", | |||
| "@docusaurus/eslint-plugin": "^2.3.0", | |||
| "@octokit/graphql": "2.1.1", | |||
| "@octokit/graphql": "^2.3.0", | |||
There was a problem hiding this comment.
This addresses GHSA-3xgq-45jj-v275 ?
There was a problem hiding this comment.
Could we have that added to the CL?
There was a problem hiding this comment.
This should be added to master branch, whoever does that should add CL.
| @@ -39,6 +48,13 @@ export class CollectionSearch extends BaseSearch { | |||
| super({ queryStringParameters }, 'collection'); | |||
| this.active = (active === 'true'); | |||
| this.includeStats = (includeStats === 'true'); | |||
|
|
|||
| // for active collection search, omit the fields which are for searching granules | |||
| if (this.active) { | |||
There was a problem hiding this comment.
This appears to be separate from CUMULUS-3859? Would you mind briefly explaining the use case here? We're searching for active collections and removing querystring params that correspond to granule properties instead of collections?
It's likely replacing some previous logic but I'm just not sure how it's used. Any context would be appreciated.
There was a problem hiding this comment.
From the dashboard, user can filter active collection with the provider name which is granule field. Dashboard integration test is broken without the updates:
https://github.com/nasa/cumulus-dashboard/blob/v12.2.0/cypress/integration/collections_spec.js#L93-L94
There was a problem hiding this comment.
These are the fields exist in both granules and collections. With /collections/active endpoint, we want to filter these fields on granules table, not on collections table. The previous implementation overrides buildRangeQuery and applies range parameters on granules, and only filters collections with granule updated in the time frame.
There was a problem hiding this comment.
Ok, I think I understand. We have a set of queryparams on the /collections/active endpoint that correspond to granules, not collections. In this case we omit the granule params to fetch a collection.
It makes me wonder if there's a better way to structure this endpoint such that we don't send queryparams unrelated to the records it's returning but that's way out of scope and debatable.
Thanks for the clarification!
There was a problem hiding this comment.
This looks good @jennyhliu. I think given that this is a separate endpoint (/collections/active) it's ok that it's a bit complex and concerns both granules and collections.
@etcart was also reviewing so I'll approve as my questions are answered but don't want to step on toes.
|
awesome, and this has answered my concerns as well, thanks |
Summary: Summary of changes
Addresses [CUMULUS-3859: cumulus-dashboard: Update @cumulus packages for cypress tests
Changes
PR Checklist