WBCE / News-with-Images

Extended news module which allows to have multiple images per post
0 stars 3 forks source link

Gallery upload #29

Closed mrbaseman closed 5 years ago

mrbaseman commented 5 years ago

A few minor things to fix:

mrbaseman commented 5 years ago
instantflorian commented 5 years ago

avoid reloading of the whole page, since otherwise all not saved changes are lost... this would be quite important (try the following: enter some text in the input fields, then upload an image... the image is there, but all text fields are empty)

instantflorian commented 5 years ago

...but beside of this, the upload works very well! ::thumb_up::

mrbaseman commented 5 years ago

Avoid reload is a good point. However, newly uploaded pictures don't appear in the gallery then. Maybe it's enough to display a hint about this fact

instantflorian commented 5 years ago

hm... can this be "ajaxed" somehow? maybe it's helpful if you have a look at the oneforall module (https://addons.wbce.org/pages/addons.php?do=item&item=9)? The module uses plupload (https://www.plupload.com/), but there the item image list is updated without reload...

A warning might be a solution, but since saving can be easily forgotten and breaks the workflow (texting > image upload > saving would be more intuitive than texting > saving > image upload) and the risk of a severe loss exists (imagine you worked an hour on your news text, then just want to upload quickly some images at last....and alls is gone! Would you not neither demolish the keyboard or curse the whole CMS then?), I would really prefer a fail safe solution.

mrbaseman commented 5 years ago

hm... can this be "ajaxed" somehow?

sure, everything is possible, it's just a question of the amount of effort you put into it. To be honest, I have uploaded pictures and noticed that they don't appear in the gallery. The automatic reload was a one-liner bound to an event handler of the uploader. Of course I can try to create the entries for the uploaded images in the gallery directly in the DOM with JS... Well, I can't do this for the ID-Keys for the Drag&Drop in the gallery. Hmm... But with respect to the workflow: texting > image upload > then a hint that you have to save the page to make the pictures appear in the gallery would be a way to go i.m.o. (no automatic reload, so no risk to lose the edited text but an explicit encouragement to save the stuff). A popup would probably be disturbing, but upon successful upload of all pictures one can print out the hint in the DOM, maybe in bold to draw some more attention on it.

BTW. wouldn't something like this be a nice improvement for the media management in WBCE 1.4? It was often discussed that it should be modernized or replaced by something more convenient. Sure, the upload fields are just one aspect, but they are something one can replace with a reasonable effort, just like we do here...

instantflorian commented 5 years ago

But with respect to the workflow: texting > image upload > then a hint that you have to save the page to make the pictures appear in the gallery would be a way to go i.m.o. (no automatic reload, so no risk to lose the edited text but an explicit encouragement to save the stuff). A popup would probably be disturbing, but upon successful upload of all pictures one can print out the hint in the DOM, maybe in bold to draw some more attention on it.

Yes, that's a good idea.

wouldn't something like this be a nice improvement for the media management in WBCE 1.4

Oh yes, please!

mrbaseman commented 5 years ago

done with https://github.com/WBCE/News-with-Images/commit/a7008bf18cd6d1471b33c42a03f4d1d5b71c68d3

instantflorian commented 5 years ago

Sorry, there are some issues. Firefox (newest) on Win 10: images are uploaded, but there appears a misleading error message at first: " bild03.jpg - Status: SyntaxError: JSON.parse: unexpected character at line 1 column 1 of the JSON data " appears 2019-05-11_184341

Chrome/Vivaldi Browser on Win 10: When more than one image is selected, only the last one is uploaded, all others are discarded. 2019-05-11_183843

mrbaseman commented 5 years ago

the first one (Syntax Error JSON.parse...) was caused by a NOTICE about an undefined variable when notices are activated in the website output. That is fixed with https://github.com/WBCE/News-with-Images/commit/6f35ede21ee3b28ee55b5cafd5501337269f12c2. The second one (some uploads ignored and only one accepted could have been a subsequent error: the variable was not defined in the response, so the client was missing the feedback about the upload of one of the files (and either due to the undefined variable, or due to confusion caused by the notice, it stopped sending the other ones? I could reproduce a similar behavior with the latest Firefox on Linux, and it seems to be fixed with the latest commit as well. Could you test again with Chrome on Windows, please?

instantflorian commented 5 years ago

issue is fixed, uploads now even work with Edge ;-)

mrbaseman commented 5 years ago

Great, I'm happy that it could be fixed quite easily. So, I can proceed with the remaining open issues and I'm looking forward to make the final release soon.

instantflorian commented 5 years ago

The old error re-appeared :( 2019-05-17_075345

mrbaseman commented 5 years ago

Last time this was caused by a Notice in the output that has messed up the json structure. Do you by chance see any message in the error log of the web server, which would give a hint on the exact cause of the problem?

instantflorian commented 5 years ago

PHP Fatal error: Uncaught RuntimeException: wrong parameter value in /.../modules/news_img/uploader/upload.php:21

mrbaseman commented 5 years ago

Ah, I forgot to resolve the idkey in the uploader, so it is indeed a non-numeric value and therefore triggers the exceptione ... I'll fix this. Btw this shows what would happen if an attacker tried to attach images to posts where he does not own the backend

mrbaseman commented 5 years ago

on my mobile I have overlooked that the idkey check was there, but unfortunately the fourth parameter ajax=true was missing, so the first picture uploaded has invalidated the idkey and for all the other pictures the idkey was not valid anymore., fixed with https://github.com/WBCE/News-with-Images/commit/90272183554d85f3ad03ec3652554b0eb78f591e