Wp-calypso: (5P) Media Library: generic error appears when uploading an item that exceeds the site storage limit

[designsimply]

Steps to reproduce:

1. For a WordPress.com Simple site, go to My Sites > Site > Media.
2. Add items until you run out of space.
3. Upload a new item that exceeds the site storage limit.
4. Observe the error message that appears.

Result: I expected to see a warning about going over storage capacity, but I see a generic error instead. (11m58s)

1 file could not be uploaded because an error occurred while uploading.

^{Tested using Safari 13.1 macOS 10.15.4 on WordPress.com Free site gotravelrewards.blog logged in as testuser0313 unproxied.}

This may be a regression, see https://github.com/Automattic/wp-calypso/pull/4172#issuecomment-199299144 and #4333.

Please also be aware of localization, see https://github.com/Automattic/wp-calypso/pull/42921.

[rachelmcr]

I tested and confirmed that I get the same generic error, regardless of my account's UI language. With the account UI language set to German, I get this generic error:

1 Datei konnte nicht hochgeladen werden, weil dabei ein Fehler aufgetreten ist.

That's definitely a regression since the localization fix made in https://github.com/Automattic/wp-calypso/pull/42921.

[rachelmcr]

@saramarcondes Could the changes to the media reducer in https://github.com/Automattic/wp-calypso/pull/42999 have introduced this regression?

[saramarcondes]

@rachelmcr It certainly looks like something that could have been caused by our media reduxification work, but I don't think it is that PR in particular. I'm taking a look to see if I can identify when the issue was introduced. Right now every error appears to be being understood as a generic server error meaning that the error code being returned didn't match anything in this part of our error handling: https://github.com/Automattic/wp-calypso/blob/master/client/state/media/reducer.js#L59:L91

Update: I've confirmed the issue. The errors reducer is not correctly parsing the error body. Here is an example response from a french site:

{
  "code": 400,
  "headers": [
    {
      "name": "Content-Type",
      "value": "application\\/json"
    }
  ],
  "body": {
    "error": "upload_error",
    "message": "rest_upload_limited_space|Pas assez d\\u2019espace pour t\\u00e9l\\u00e9verser. Il y a besoin de 229 KB. Retour"
  }
}

Note that in our errors reducer we read the error property of the error: https://github.com/Automattic/wp-calypso/blob/master/client/state/media/reducer.js#L71

In this case it corresponds to upload_error. In this case we return SERVER_ERROR resulting in the generic message: https://github.com/Automattic/wp-calypso/blob/master/client/state/media/reducer.js#L81

However, we'll notice that message starts with an error code, rest_upload_limited_space in this case. This matches one of the cases that we expected to see from error.error. I think there's simply a misunderstanding here where we need to change the error reducer logic here to nest like so:

switch ( error.error ) {
    // ... all the other cases
    case 'upload_error': {
        const [ messageType ] = error.message.split('|');
        switch ( messageType ) {
            case 'rest_upload_limited_space':
                return MediaValidationErrors.NOT_ENOUGH_SPACE;
            case 'rest_upload_file_too_big':
                return MediaValidationErrors.EXCEEDS_MAX_UPLOAD_SIZE;
            case 'rest_upload_user_quota_exceeded':
                return MediaValidationErrors.EXCEEDS_PLAN_STORAGE_LIMIT;
            default:
                return MediaValidationErrors.SERVER_ERROR
        }
    }
}

I'm not sure when this was introduced. It seems like there might have also been changes on the WordPress.com side of things because the code from before the diff I linked above still wouldn't work as it depended on the message starting with actual English text, which of course is not the case any longer.

In any case, I think there's a clear fix for this for whomever picks it up.

[davemart-in]

~Note: For Business and eCommerce customers who signed up prior to August 2019 we need to make it so that the 200GB upload quota does not apply and they do not see any upload error messages. Let's chat about this once this issue gets picked up.~

Edit by @marekhrabe: separated into #44930, not part of this task anymore

[sfougnier]

When a user has reached their storage capacity and attempts to upload a new file, this error will occur with updated copy:

[cpapazoglou]

@rachelmcr It certainly looks like something that could have been caused by our media reduxification work, but I don't think it is that PR in particular. I'm taking a look to see if I can identify when the issue was introduced. Right now every error appears to be being understood as a generic server error meaning that the error code being returned didn't match anything in this part of our error handling: https://github.com/Automattic/wp-calypso/blob/master/client/state/media/reducer.js#L59:L91

Update: I've confirmed the issue. The errors reducer is not correctly parsing the error body. Here is an example response from a french site:

{
  "code": 400,
  "headers": [
    {
      "name": "Content-Type",
      "value": "application\\/json"
    }
  ],
  "body": {
    "error": "upload_error",
    "message": "rest_upload_limited_space|Pas assez d\\u2019espace pour t\\u00e9l\\u00e9verser. Il y a besoin de 229 KB. Retour"
  }
}

Note that in our errors reducer we read the error property of the error: https://github.com/Automattic/wp-calypso/blob/master/client/state/media/reducer.js#L71

In this case it corresponds to upload_error. In this case we return SERVER_ERROR resulting in the generic message: https://github.com/Automattic/wp-calypso/blob/master/client/state/media/reducer.js#L81

However, we'll notice that message starts with an error code, rest_upload_limited_space in this case. This matches one of the cases that we expected to see from error.error. I think there's simply a misunderstanding here where we need to change the error reducer logic here to nest like so:

switch ( error.error ) {
  // ... all the other cases
  case 'upload_error': {
      const [ messageType ] = error.message.split('|');
      switch ( messageType ) {
          case 'rest_upload_limited_space':
              return MediaValidationErrors.NOT_ENOUGH_SPACE;
          case 'rest_upload_file_too_big':
              return MediaValidationErrors.EXCEEDS_MAX_UPLOAD_SIZE;
          case 'rest_upload_user_quota_exceeded':
              return MediaValidationErrors.EXCEEDS_PLAN_STORAGE_LIMIT;
          default:
              return MediaValidationErrors.SERVER_ERROR
      }
  }
}

I'm not sure when this was introduced. It seems like there might have also been changes on the WordPress.com side of things because the code from before the diff I linked above still wouldn't work as it depended on the message starting with actual English text, which of course is not the case any longer.

In any case, I think there's a clear fix for this for whomever picks it up.

👋 @rachelmcr! Thanks for providing such an intel! By investigating further the root cause, I can see that the current issue should have been resolved by https://github.com/Automattic/wp-calypso/pull/42921 which as it seems it correctly returns the server response only if I comment out

if ( '0' != $file['error'] && ! isset( $_POST['html-upload'] ) && ! wp_doing_ajax() ) {
  wp_die( $file['error'] . ' <a href="javascript:history.go(-1)">' . __( 'Back', 'default' ) . '</a>' );
}

in D44299-code. The rewrite_generic_upload_error() in the trunk does exactly what you are trying to do in the reducer! So it comes down to why wp_doing_ajax() returns false when I actually do an AJAX request ;-)!

Does anyone have any prior knowledge if there was a define( 'DOING_AJAX', true ) which is now missing? @yuliyan, @akirk do you have any clues why this may be happening? Also, the original WP_REST_Attachments_Controller::check_upload_size() doesn't have that conditional, are there some edge cases we are trying to cover?

[obenland]

@cpapazoglou asked me to double-check his findings and I come to the same conclusion. Commenting out the wp_die() call fixes that bug for me.

@yuliyan, @akirk, can we just remove that or does it serve a deeper purpose?

Does anyone have any prior knowledge if there was a define( 'DOING_AJAX', true ) which is now missing?

I suspect it was just copied over verbatim from check_upload_size(). DOING_AJAX usually is only set for requests to /wp-admin/admin-ajax.php and /wp-admin/async-upload.php

[yuliyan]

IIRC, we have indeed taken the code from check_upload_size so that if statement was most likely kept mainly to keep the code closer to the original rather than handling a known case for the media upload endpoint. Therefore, it should be safe to remove it.

Looking at the code again, I started to doubt if we have incorrectly tested the patch and it never actually worked properly. But if that's the case, I'm sorry for causing troubles.

[cpapazoglou]

I have come to the same conclusion

I suspect it was just copied over verbatim from check_upload_size(). DOING_AJAX usually is only set for requests to /wp-admin/admin-ajax.php and /wp-admin/async-upload.php

cause wp_doing_ajax() is only referenced in the same manner in wp-admin/includes/ms.php.

So, I will remove this offensive conditional and it will still resemble more to check_upload_size() which seems correct.