Skip to content

Conversation

@themuhammad2
Copy link
Contributor

@themuhammad2 themuhammad2 commented Jun 4, 2025

The aim of this PR is to add API docs related to reporting data-export.

We are documenting below requests handled by app/controllers/api/v3/export/reporting_data_export_controller.rb controller.

  • POST /export/reporting_data/enqueue
  • GET /export/reporting_data/{job_identifier}
  • GET /download/reporting_data/{job_identifier}
  • GET /export/reporting_data/get_datasets

@themuhammad2 themuhammad2 marked this pull request as ready for review June 5, 2025 08:50
@themuhammad2 themuhammad2 force-pushed the muhammad.murtaza/add-docs-related-to-reporting-data-export branch from fccb5ad to 2ddcc8e Compare June 5, 2025 09:03
@themuhammad2 themuhammad2 requested a review from a team June 5, 2025 09:26
"/export/reporting_data/enqueue":
post:
summary: Enqueue a new reporting data export job
tags: [Export]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this tag achieves the grouping rather than needing to nest the resource name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is for grouping.

message: "Export job not found for identifier: job1"
schema:
"$ref": "#/components/schemas/error"
'504':
Copy link
Contributor

Choose a reason for hiding this comment

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

504 is specific to timeouts. Is this a known issue that comes up, and if so could we explain that in the description?

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 have updated the docs after updating the API. Thanks for pointing out.

Comment on lines +1701 to +1694
"/export/reporting_data/get_datasets":
get:
Copy link
Contributor

Choose a reason for hiding this comment

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

Usually wouldn't have get in the path as well as being the HTTP method.

Copy link
Contributor

Choose a reason for hiding this comment

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

But if it matches the behaviour we can update the docs for now. While it's in Unstable though we could change to align with other API endpoints.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@steve-henry yeah that's a good point we shouldn't have get in the name of the resource according to REST conventions. It should have been like

GET /export/reporting_data/datasets

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I think that would be clearer and more consistent with the rest of the APIs

@themuhammad2 themuhammad2 force-pushed the muhammad.murtaza/add-docs-related-to-reporting-data-export branch 2 times, most recently from 63b64dd to a6833b9 Compare June 5, 2025 16:29
@themuhammad2 themuhammad2 force-pushed the muhammad.murtaza/add-docs-related-to-reporting-data-export branch from a6833b9 to d771cd3 Compare June 5, 2025 16:32
@themuhammad2 themuhammad2 merged commit 6a8b8f1 into main Jun 10, 2025
6 checks passed
@themuhammad2 themuhammad2 deleted the muhammad.murtaza/add-docs-related-to-reporting-data-export branch June 10, 2025 06:41
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