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

HARMONY-1889: Add support for requesting time or area based averaging #627

Merged
merged 9 commits into from
Sep 26, 2024

Conversation

chris-durbin
Copy link
Contributor

@chris-durbin chris-durbin commented Sep 24, 2024

Jira Issue ID

HARMONY-1889

Description

EDIT: Updated the parameter name from averageType to average to be consistent with other API parameters.

This is to prepare for new time averaging and area averaging services in development by Giovanni. An end user can now use the average parameter to request either 'time' or 'area' averaging. If averaging is requested, the request will be limited to only services that support that type of averaging.

Local Test Steps

Since at the moment no services are configured as supporting time or area you can submit any request with the parameter average=time or average=area and verify that the request is rejected with an appropriate message.

To verify that the capabilities logic will match correctly you can manually modify one of the services in services.yml to add the following to the capabilities section (or some combination of true/false to different services):

averaging:
  time: true
  area: true

Then submit a request to verify the request gets directed to that service. I was testing with harmony-service-example using this request for area averaging:
http://localhost:3000/C1233800302-EEDTEST/ogc-api-coverages/1.0.0/collections/blue_var/coverage/rangeset?subset=lat(20%3A60)&subset=lon(-140%3A-50)&granuleId=G1233800343-EEDTEST&outputCrs=EPSG%3A31975&format=image%2Fpng&average=area

and for time averaging:
http://localhost:3000/C1233800302-EEDTEST/ogc-api-coverages/1.0.0/collections/blue_var/coverage/rangeset?subset=lat(20%3A60)&subset=lon(-140%3A-50)&granuleId=G1233800343-EEDTEST&outputCrs=EPSG%3A31975&format=image%2Fpng&average=area

PR Acceptance Checklist

  • Acceptance criteria met
  • Tests added/updated (if needed) and passing
  • Documentation updated (if needed)

describe('for a collection that can support it', function () {
StubService.hook({ params: { redirect: 'http://example.com' } });
hookRangesetRequest('1.0.0', collection, 'all', { query: { ...averagingTimeQuery } });
// hookRedirect('anonymous');
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: remove commented out code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We will uncomment these lines once there are time and area averaging services and switch the xit to it. I put a comment up above the start of the tests - maybe I should also add another comment directly above each commented out line.

Note the reason they are commented out is because they will break unless the request gets a 3xx redirect back which won't happen until the request gets matched to a service.

describe('for a collection that can support it', function () {
StubService.hook({ params: { redirect: 'http://example.com' } });
hookRangesetRequest('1.0.0', collection, 'all', { query: { ...averagingAreaQuery } });
// hookRedirect('anonymous');
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: remove commented out code

Copy link
Contributor

Choose a reason for hiding this comment

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

I only see coverages requests, no EDR. I'm struggling with writing EDR POST requests for my tests that actually work (GET requests work fine), so if you add some I can copy them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the EDR GET tests. We'll write a separate ticket to address testing with EDR POST.

@vinnyinverso
Copy link
Collaborator

@vinnyinverso
Copy link
Collaborator

vinnyinverso commented Sep 25, 2024

Feel free to ignore if you disagree, but I think average=time/area (vs. averagingType) as the name of the query parameter (not the capability) might be a little bit more consistent with our other API parameters.

@chris-durbin
Copy link
Contributor Author

Feel free to ignore if you disagree, but I think average=time/area (vs. averagingType) as the name of the query parameter (not the capability) might be a little bit more consistent with our other API parameters.

We agreed average was more consistent and updated the name.

@chris-durbin chris-durbin merged commit 3392693 into main Sep 26, 2024
4 of 6 checks passed
@chris-durbin chris-durbin deleted the harmony-1889 branch September 26, 2024 12:15
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.

4 participants