918648 - single flickr source in image plugin can cause javascript error and project can then be saved

Status: VERIFIED FIXED

(Webmaker Graveyard :: Popcorn Maker, defect)

Description

[Scott [:thecount] Downe]

See this project: https://kjburgam.makes.org/popcorn/1ek4

We need to fix this project so it loads, without needing a republish.

STRs to reproduce this locally.

1. Add an image event.
2. Paste in this as an image source "http://www.flickr.com/photos/44124475522@N01/3186046746/in/photolist-5RxjGs-dx7zDx-56YSWJ-8yNzbJ-89NeM9-dhpEnN-dhpDyF-dhpD1p-7oX4Yn-5PH5Qq-9KEcx1-ac5E8c-8KqBCJ-6m467r-5Tk9nW-9L7n-f1yDVa-f1NVHq-f1NVww-628STi-74C1ch-74C2u9-7ewSLj-6RiUB1-4pdgHz-by3Jwo-6RiU8w-9Nwd55-386kVx-mGvnf-cKRV5d-h199H-h19rW-h19k9-h19gF-h19vj-h19dG-h196V-dj9bnc-8gkU39-7g8xvj-3jAyRy-af8gTA-cFSy91-arTSfd-cFSuZ5-cFSuqo-cFSuDf-cFStPS-cFSu8f-9pQdwh"
3. Save.
4. Refresh.

Expected: page should load.
Actual: it does not.

There is no way for the user to fix this mistake, and now the project and data are lost.

Comment 1

[Scott [:thecount] Downe]

Attachment: https://github.com/mozilla/popcorn.webmaker.org/pull/217

Croppable expects something right away at the moment.

Anything from flickr is inside of a json request.

We need to make croppable work at some async moment, but for now, I am disabling it for flickr single images, it did not work anyway.

I also made sure we didn't error out from the json request not being done.

The changes look like more than they are. I tabbed a big chunk of code.

Another bug found from twitter :D

Comment 2

[Matthew Schranz [:mjschranz]]

Comment on attachment 807600 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/217

You could just dix it for single flickr images with much less code changed.

Wrap https://github.com/mozilla/popcorn.webmaker.org/blob/master/public/templates/assets/plugins/image/popcorn.image.js#L176-L186 in a function that's called either inside https://github.com/mozilla/popcorn.webmaker.org/blob/master/public/templates/assets/plugins/image/popcorn.image.js#L172-L175 or https://github.com/mozilla/popcorn.webmaker.org/blob/master/public/templates/assets/plugins/image/popcorn.image.js#L162-L168 depending on the path taken.

Comment 3

[Matthew Schranz [:mjschranz]]

Which, will fix the project without a republish.

Comment 4

[Scott [:thecount] Downe]

Why make it a function? It currently only needs to be used in one spot.

All I did was move the code to where you're asking I put the function.

That got me to loading the project without a republish, however, it was pretty busted. Loading, but busted.

I'll split this up into other tickets.

Comment 5

[Scott [:thecount] Downe]

Comment on attachment 807600 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/217

Agreed.

Fixed to just fix the loading issues.

We can deal with croppable in another ticket, with clearly defined STRs. Things'll move much smoother that way. At this point, I didn't make it clear what is wrong with croppable, but with just this fix it is still better.

Comment 6

[Matthew Schranz [:mjschranz]]

Comment on attachment 807600 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/217

Why do you not want this for single flickr images? Doing what I suggested allows for both.

Comment 7

[Scott [:thecount] Downe]

Comment on attachment 807600 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/217

I do want this for single images, but, it just doesn't work very well.

If I put https://github.com/mozilla/popcorn.webmaker.org/blob/master/public/templates/assets/plugins/image/popcorn.image.js#L176-L186

Into https://github.com/mozilla/popcorn.webmaker.org/blob/master/public/templates/assets/plugins/image/popcorn.image.js#L162-L168

The cropping ends up showing old data and is pretty janky. This is because the single flickr image comes back async and cropping expects sync. It needs fixing, but I wasn't going to attempt that here.

However, I can adedd the function and we can iterate.

It is more important to me that it lands in a better state than what it is now.

Comment 8

[Matthew Schranz [:mjschranz]]

Comment on attachment 807600 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/217

More broken than I realized, but this will help it in the mean time.

Comment 9

[Scott [:thecount] Downe]

Staged: https://github.com/mozilla/popcorn.webmaker.org/commit/3cdb82571a0991b4ecc6e8f154dcb99e7df0c311

Yeah, it is pretty broken. Going to file a new ticket to clean it up. bug 919560

This stops the crash, though.

Needs verification.