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

fix(agent): return non-200 status when > 0 failures in bulk op #4520

Closed
wants to merge 1 commit into from

Conversation

ThatOneBro
Copy link
Member

No description provided.

@ThatOneBro ThatOneBro requested a review from a team as a code owner May 6, 2024 17:22
Copy link

vercel bot commented May 6, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
medplum-provider ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 6, 2024 5:29pm
3 Ignored Deployments
Name Status Preview Comments Updated (UTC)
medplum-app ⬜️ Ignored (Inspect) Visit Preview May 6, 2024 5:29pm
medplum-storybook ⬜️ Ignored (Inspect) Visit Preview May 6, 2024 5:29pm
medplum-www ⬜️ Ignored (Inspect) Visit Preview May 6, 2024 5:29pm

Copy link

sonarcloud bot commented May 6, 2024

continue;
}
const [outcome, params] = result.value;
if (!isOk(outcome)) {
failureCount += 1;
Copy link
Member

Choose a reason for hiding this comment

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

I recommend we save the outcome here and use it to determine status.

++?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's possible that you get multiple different types of outcomes, should we just save the first one?

@@ -233,7 +233,7 @@ describe('Agent/$bulk-status', () => {
.get('/fhir/R4/Agent/$bulk-status')
.query({ name: 'Test Agent 2' })
.set('Authorization', 'Bearer ' + accessToken);
expect(res.status).toBe(200);
expect(res.status).toBe(500);
Copy link
Member

Choose a reason for hiding this comment

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

We should never intentionally return a 500 - that should be reserved for unexpected errors.

@@ -132,7 +138,7 @@ export async function handleBulkAgentOperation(
}

return [
allOk,
failureCount === 0 ? allOk : serverError(new Error(`${failureCount} sub-operations failed`)),
Copy link
Member

Choose a reason for hiding this comment

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

Use the saved outcome here rather than serverError

@reshmakh reshmakh added this to the May 31st, 2024 milestone May 10, 2024
@ThatOneBro
Copy link
Member Author

See explanation #4513 (comment)

@ThatOneBro ThatOneBro closed this May 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

None yet

3 participants