Improve feedback on invalid image upload #308

2020-05-08T07:50:49Z

RoukePouw commented

2020-05-08 07:50:49 +00:00

(Migrated from github.com)

No description provided.

matthijskooijman commented

2020-05-08 09:42:04 +00:00

(Migrated from github.com)

Looks good at first glance. However, I do wonder: Where does the upload form get handled normally? Isn't there some error handling there already?

I believe that the upload plugin we're using now is (mostly) unmodified from the wymeditor version, we just plugged in a custom backend to handle the uploads. So maybe there is already working error handling, and a better (but not more complicated) fix would be to change the backend to return errors in a way the frontend code expects?

Looks good at first glance. However, I do wonder: Where does the upload form get handled normally? Isn't there some error handling there already? I believe that the upload plugin we're using now is (mostly) unmodified from the wymeditor version, we just plugged in a custom backend to handle the uploads. So maybe there is already working error handling, and a better (but not more complicated) fix would be to change the backend to return errors in a way the frontend code expects?

RoukePouw commented

2020-05-08 10:07:50 +00:00

(Migrated from github.com)

this gets passed null while it seems to expect an array. Will need to look further to see why it ends up with null here and not with backend response <script>...uploadReponse('~error')...

jquery.wymeditor.image-upload.js:41
complete: function(response){
				response = response[0];
				jQuery(".wym_src", doc).val(response.thumbUrl);
				jQuery(".wym_alt", doc).val(response.original_filename);
				jQuery("form#image_upload_form .submit", doc).val(oldSubmitLabel);
			}

this gets passed `null` while it seems to expect an array. Will need to look further to see why it ends up with null here and not with backend response `<script>...uploadReponse('~error')...` ``` jquery.wymeditor.image-upload.js:41 complete: function(response){ response = response[0]; jQuery(".wym_src", doc).val(response.thumbUrl); jQuery(".wym_alt", doc).val(response.original_filename); jQuery("form#image_upload_form .submit", doc).val(oldSubmitLabel); } ```

RoukePouw commented

2020-05-08 10:14:33 +00:00

(Migrated from github.com)

This is the response on a good upload:

 [{original_filename: "quest.png", downloadUrl: "http://localhost:8888/images/5eb53066f3abe.png", thumbUrl: "http://localhost:8888/images/5eb53066f3abe.png"}]

That comes from

pages.php:360
if ($return !== null) {
						echo json_encode($return);
					} else {
					    echo '<script language="javascript" type="text/javascript">window.top.window.uploadResponse(\''.$response.'\');</script>';
					}

I can replace the script with json but I haven't found the attribute to indicate an error (perhaps through http statuscode? (How can I make hypha return a 500?)

This is the response on a good upload: ``` [{original_filename: "quest.png", downloadUrl: "http://localhost:8888/images/5eb53066f3abe.png", thumbUrl: "http://localhost:8888/images/5eb53066f3abe.png"}] ``` That comes from ``` pages.php:360 if ($return !== null) { echo json_encode($return); } else { echo '<script language="javascript" type="text/javascript">window.top.window.uploadResponse(\''.$response.'\');</script>'; } ``` I can replace the script with json but I haven't found the attribute to indicate an error (perhaps through http statuscode? (How can I make hypha return a 500?)

matthijskooijman commented

2020-05-08 14:08:27 +00:00

(Migrated from github.com)

this gets passed null while it seems to expect an array. Will need to look further to see why it ends up with null here and not with backend response

I suspect it gets passed null, because the string is invalid json (and jquery is told to expect JSON).

That looks like returning the error message in JSON is actually a good idea, then the success and error response are both JSON which is nice and consistent. However, it does seem that the original plugin code has no way to handle errors, which is a bit sad...

So, I would suggest we just add the error handling (probably just with an alert as before) in the image upload plugin, e.g. in here. It would be elegant to use a HTTP status code and have some kind of error handler, but it actually seems that the "iframe post form plugin" does not really support this (not sure if it could, using this iframe approach). So, just returning json with an errror field and a message, check for that, and do an alert if error is present is probably a clean approach?

(I am wondering if we could maybe ditch the entire "iframe" approach somehow with newer web APIs or jquery plugins, but maybe we should not make this bigger than needed for now...)

> this gets passed null while it seems to expect an array. Will need to look further to see why it ends up with null here and not with backend response I suspect it gets passed null, because the string is invalid json (and jquery [is told to expect JSON](https://github.com/PlanBCode/hypha/blob/c04fc82159b0c3355d72029a72ac876e93d5805b/system/wymeditor/plugins/image_upload/jquery.wymeditor.image_upload.js#L37)). That looks like returning the error message in JSON is actually a good idea, then the success and error response are both JSON which is nice and consistent. However, it *does* seem that the original plugin code has no way to handle errors, which is a bit sad... So, I would suggest we just add the error handling (probably just with an alert as before) in the image upload plugin, e.g. [in here](https://github.com/PlanBCode/hypha/blob/c04fc82159b0c3355d72029a72ac876e93d5805b/system/wymeditor/plugins/image_upload/jquery.wymeditor.image_upload.js#L41-L46). It would be elegant to use a HTTP status code and have some kind of error handler, but it actually seems that the ["iframe post form plugin"](https://github.com/PlanBCode/hypha/blob/master/system/wymeditor/plugins/jquery.iframe-post-form.js) does not really support this (not sure if it could, using this iframe approach). So, just returning json with an `errror` field and a message, check for that, and do an alert if `error` is present is probably a clean approach? (I am wondering if we could maybe ditch the entire "iframe" approach somehow with newer web APIs or jquery plugins, but maybe we should not make this bigger than needed for now...)

RoukePouw commented

2020-05-08 14:57:57 +00:00

(Migrated from github.com)

So, I would suggest we just add the error handling (probably just with an alert as before) in the image upload plugin,

Done :-)

Can we now also remove the ~ in front of the error messages or is that this used someplace else?

but maybe we should not make this bigger than needed for now...

I'm very much in favor of baby steps ;-) Though I fully share the view that lossing the iframe would be a good thing :-)

> So, I would suggest we just add the error handling (probably just with an alert as before) in the image upload plugin, Done :-) Can we now also remove the ~ in front of the error messages or is that this used someplace else? > but maybe we should not make this bigger than needed for now... I'm very much in favor of baby steps ;-) Though I fully share the view that lossing the iframe would be a good thing :-)

matthijskooijman (Migrated from github.com) reviewed

2020-05-08 15:13:19 +00:00

matthijskooijman (Migrated from github.com) left a comment

Yeah, looks good!

Can we now also remove the ~ in front of the error messages or is that this used someplace else?

Yes, let's just do that right away. I have not checked, but I suspect these strings are not used elsewhere (but that would be easy to check).

Yeah, looks good! > Can we now also remove the ~ in front of the error messages or is that this used someplace else? Yes, let's just do that right away. I have not checked, but I suspect these strings are not used elsewhere (but that would be easy to check).

system/core/pages.php

					
				@ -357,11 +357,9 @@ EOF;

											}

										}

									}

This could run into problems with escaping (e.g. when there is a " in $response). I would suggest:

if ($return === null)
    $return = [['error' => $response]];

And then let the existing json_encode handle (dropping the if before it).

There is some additional refactoring that could maybe be done here, but some of this code might be replaced by Laurens' backend changes anyway.

This could run into problems with escaping (e.g. when there is a `"` in `$response`). I would suggest: if ($return === null) $return = [['error' => $response]]; And then let the existing `json_encode` handle (dropping the if before it). There is some additional refactoring that could maybe be done here, but some of this code might be replaced by Laurens' backend changes anyway.

system/wymeditor/plugins/image_upload/jquery.wymeditor.image_upload.js

On error, you now do

document.getElementById('upload-image').value = oldSubmitLabel;

and on success

                jQuery("form#image_upload_form .submit", doc).val(oldSubmitLabel);

Those look like they might be doing the same, so maybe this can be one line outside of the if? Of is there a difference?

On error, you now do document.getElementById('upload-image').value = oldSubmitLabel; and on success jQuery("form#image_upload_form .submit", doc).val(oldSubmitLabel); Those look like they might be doing the same, so maybe this can be one line outside of the if? Of is there a difference?

                jQuery("form#image_upload_form .submit", doc).val(oldSubmitLabel);

jQuery("form#image_upload_form .submit", doc).val(oldSubmitLabel);

RoukePouw (Migrated from github.com) reviewed

2020-05-08 16:36:27 +00:00

system/wymeditor/plugins/image_upload/jquery.wymeditor.image_upload.js

Ah, missed that one, dragging stuff around. Thanks!

RoukePouw (Migrated from github.com) reviewed

2020-05-08 16:36:38 +00:00

system/core/pages.php

					
				@ -357,11 +357,9 @@ EOF;

											}

										}

									}

Will do :)

matthijskooijman (Migrated from github.com) reviewed

2020-05-11 12:43:25 +00:00

matthijskooijman (Migrated from github.com) left a comment

PR looks better now. I still left some minor nitpicks inline, but I'll go ahead and just fix them in a minute myself.

system/core/editor.php

This seems to add a newline for no apparent reason? I guess this part of the diff could be removed.

system/wymeditor/plugins/image_upload/jquery.wymeditor.image_upload.js

Hmm, seems my comment here got lost. The problem was (and still is) that the indentation is wrong here. I suspect some tabs vs spaces setting in your editor. The same happens elsewhere as well.

system/wymeditor/plugins/image_upload/jquery.wymeditor.image_upload.js

I think this extra id is no longer needed.

matthijskooijman (Migrated from github.com) reviewed

2020-05-11 13:01:36 +00:00

system/wymeditor/plugins/image_upload/jquery.wymeditor.image_upload.js

Looking at the file contents, the original file uses tabs for indentation, but your inserted lines use 4 spaces. Might be good to fix in your editor.

matthijskooijman (Migrated from github.com) reviewed

2020-05-11 13:25:12 +00:00

system/wymeditor/plugins/image_upload/jquery.wymeditor.image_upload.js

Fixed.

matthijskooijman (Migrated from github.com) reviewed

2020-05-11 13:25:18 +00:00

system/core/editor.php

Fixed.

matthijskooijman (Migrated from github.com) reviewed

2020-05-11 13:25:28 +00:00

system/wymeditor/plugins/image_upload/jquery.wymeditor.image_upload.js

Fixed.

matthijskooijman commented

2020-05-11 13:26:42 +00:00

(Migrated from github.com)

I fixed the issues I pointed out, squashed everything into one commit with a longer commit message (also adding a reference to the issue being fixed to allow auto-closing it) and rebased on top of master. I think this can be merged as-is now. Maybe @RoukePouw or @laurensmartina want to give this one last double-check?

matthijskooijman (Migrated from github.com) approved these changes

2020-05-11 13:26:57 +00:00

RoukePouw commented

2020-05-11 14:11:15 +00:00

(Migrated from github.com)

Maybe @RoukePouw or @laurensmartina want to give this one last double-check?

Double checked, looks good! Thanks for the final fixes!

> Maybe @RoukePouw or @laurensmartina want to give this one last double-check? Double checked, looks good! Thanks for the final fixes!

matthijskooijman commented

2020-05-12 14:45:35 +00:00

(Migrated from github.com)

I fixed one more minor whitespace issue in the javascript, I'll merge this now.

Sign in to join this conversation.

No reviewers

matthijskooijman

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

harmen/hypha!308

Rows
Columns