-
Notifications
You must be signed in to change notification settings - Fork 9
feat: added and modified api routes for optimization reports #1079
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Sahil Silare <[email protected]>
Signed-off-by: Sahil Silare <[email protected]>
This PR will trigger a minor release when merged. |
Signed-off-by: Sahil Silare <[email protected]>
Signed-off-by: Sahil Silare <[email protected]>
Signed-off-by: Sahil Silare <[email protected]>
Signed-off-by: Sahil Silare <[email protected]>
…into SITES-33657 Signed-off-by: Sahil Silare <[email protected]>
Signed-off-by: Sahil Silare <[email protected]>
Signed-off-by: Sahil Silare <[email protected]>
Signed-off-by: Sahil Silare <[email protected]>
Signed-off-by: Sahil Silare <[email protected]>
Signed-off-by: Sahil Silare <[email protected]>
src/controllers/reports.js
Outdated
} | ||
|
||
// Validate report period | ||
if (!isNonEmptyObject(data.reportPeriod)) { |
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.
We can create an isValidPeriod(data.reportPeriod, 'reportPeriod') function to reduce duplication.
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.
Done
src/controllers/reports.js
Outdated
|
||
// Check if a report with the same parameters already exists | ||
const existingReports = await Report.allBySiteId(siteId); | ||
const existingReport = existingReports.find((report) => { |
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.
Filter out the failed reports.
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.
Done.
src/controllers/reports.js
Outdated
} | ||
|
||
// Deep comparison of report periods | ||
const periodsMatch = JSON.stringify(reportPeriod) === JSON.stringify(data.reportPeriod); |
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.
add a comparePeriod(period1, period2) function.
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.
Done
// Only generate presigned URLs for successful reports | ||
if (report.getStatus() === 'success') { | ||
try { | ||
const rawReportKey = `${report.getStoragePath()}raw/report.json`; |
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.
we can create getter for this in model. report.getRawPath(), report.getMystiquePath().
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.
This data is not saved in model, it is computed on API call and returned, there's no valid point to save presigned url in dynamo.
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.
Not talking about saving this. Add a getter function to get this path in the model.
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.
okay, adding 👍
src/controllers/reports.js
Outdated
} | ||
|
||
// Delete S3 files if they exist (only for successful reports) | ||
if (report.getStatus() === 'success' && report.getStoragePath()) { |
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.
check if failed processing also creates s3 objects. I can't find implementation of the consumer side.
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.
That will be in the spacecat-reporting-worker
, once the POST SQS message goes to spacecat-reporting-worker
.
Signed-off-by: Sahil Silare <[email protected]>
Signed-off-by: Sahil Silare <[email protected]>
Signed-off-by: Sahil Silare <[email protected]>
Signed-off-by: Sahil Silare <[email protected]>
src/controllers/reports.js
Outdated
// Check if a report with the same parameters already exists | ||
const existingReports = await Report.allBySiteId(siteId); | ||
// Filter out failed reports (only consider successful reports for duplicate checking) | ||
const successfulReports = existingReports.filter((report) => report.getStatus() === 'success'); |
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.
Also consider processing statuses.
…g` status reports in the response. Signed-off-by: Sahil Silare <[email protected]>
Reports API Endpoints
JIRA - SITES-33657
Overview
The Reports API allows you to create, manage, and retrieve report generation jobs for sites. All endpoints require authentication and organization-level access.
Authentication
Include one of the following headers:
Authorization: Bearer <jwt-token>
Authorization: Bearer <ims-token>
x-api-key: <api-key>
Endpoints
POST
/sites/{id}/reports
{reportType, reportPeriod, comparisonPeriod}
{jobId, status}
GET
/sites/{id}/reports
{reports: [...]}
GET
/sites/{id}/reports/{reportId}
{report details + presigned URLs}
DELETE
/sites/{id}/reports/{reportId}
{message}
Request Examples
POST /sites/{siteId}/reports
Request:
Response:
GET /sites/{siteId}/reports
Request:
Response:
GET /sites/{siteId}/reports/{reportId}
Request:
Response:
DELETE /sites/{siteId}/reports/{reportId}
Request:
Response:
Error Responses
400
401
403
404
500
Notes
success
status include presigned URLs for downloadingGET /reports/{reportId}
endpoint only returns successful reports (returns 400 for processing reports)DELETE
endpoint removes both database records and associated S3 files (raw and Mystique analysis)performance
,accessibility
,seo
,security
,conversion
,cwv
,optimization
Please ensure your pull request adheres to the following guidelines:
describe here the problem you're solving.
If the PR is changing the API specification:
yet. Ideally, return a 501 status code with a message explaining the feature is not implemented yet.
If the PR is changing the API implementation or an entity exposed through the API:
If the PR is introducing a new audit type:
Related Issues
This PR implements the Reports API endpoints for managing report generation jobs. The implementation includes:
Thanks for contributing!