Skip to content

Conversation

@barw4
Copy link
Contributor

@barw4 barw4 commented Jun 24, 2025

🎫 Issue IBX-10125

Description:

Two things:

  • JsonResponse should not succeed with status 200 if it fails.
  • JS improperly handles such a failed request. ThejsonResponse.ErrorMessage call was giving empty results. Shouldn't we rather fallback to full response?

For QA:

Documentation:

@barw4 barw4 self-assigned this Jun 24, 2025
@barw4 barw4 added Bug Something isn't working Ready for review labels Jun 24, 2025
@barw4 barw4 requested review from a team June 24, 2025 11:48
@ezrobot ezrobot requested review from GrabowskiM, Steveb-p, ViniTou, adamwojs, alongosz, ciastektk, mikadamczyk and wiewiurdp and removed request for a team June 24, 2025 11:48
const getErrorMessageObject = (response) => {
const responseErrorMessage = response.json().then((jsonResponse) => {
return jsonResponse.ErrorMessage;
return jsonResponse.ErrorMessage ?? jsonResponse;
Copy link
Contributor

Choose a reason for hiding this comment

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

With HTTP error code other than 2XX, response.ok below will be false.

I would treat jsonResponse.ErrorMessage not being set as an error in this function, and throw an Error. Otherwise you return an object instead of string, and there is a high likelihood that follow up code will fail on that instead.

Copy link
Contributor Author

@barw4 barw4 Jun 25, 2025

Choose a reason for hiding this comment

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

@Steveb-p Actually, it seems that jsonResponse.ErrorMessage was meant to be an object, not a string. This is especially visible here:

const responseErrorMessageObject = await getErrorMessageObject(response);
and also in method's name, hence my fallback to full jsonResponse

@barw4 barw4 force-pushed the ibx-10125-handle-failed-requests branch from c490353 to 37c00d8 Compare June 25, 2025 12:59
@KamilSznajdrowicz KamilSznajdrowicz force-pushed the ibx-10125-handle-failed-requests branch from 37c00d8 to 64f87ea Compare July 11, 2025 09:59
@sonarqubecloud
Copy link

Copy link
Contributor

@KamilSznajdrowicz KamilSznajdrowicz left a comment

Choose a reason for hiding this comment

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

QA Approved
Regression: ibexa/commerce#1396

@adamwojs adamwojs merged commit 2ad411e into 4.6 Jul 12, 2025
32 of 39 checks passed
@adamwojs adamwojs deleted the ibx-10125-handle-failed-requests branch July 12, 2025 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug Something isn't working QA approved

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants