publiclab / mapknitter

Upload your own aerial images, position (rubbersheet) them in a web interface over existing map data, and share via web or composite and export for print.
http://mapknitter.org
GNU General Public License v3.0
267 stars 207 forks source link

Issue with image position saving on stable server #1041

Closed jywarren closed 5 years ago

jywarren commented 5 years ago

(This is on the main branch, so priority!)

I uploaded a couple images to the stable testing server, and they can be placed and manipulated. But they are not getting saved... I'm not sure why. @ViditChitkara would you mind taking a quick look at the errors? There are some about websocket connections, and I wonder if that's blocking them being saved?

If that's correct, could we redesign the ordering so that even if websockets aren't connecting, it doesn't interrupt the individual saving of images?

image

http://mapknitter-stable.laboratoriopublico.org/maps/test6/edit

ViditChitkara commented 5 years ago

Having a look!

On Thu, Sep 19, 2019 at 11:29 PM Jeffrey Warren notifications@github.com wrote:

(This is on the main branch, so priority!)

I uploaded a couple images to the stable testing server, and they can be placed and manipulated. But they are not getting saved... I'm not sure why. @ViditChitkara https://github.com/ViditChitkara would you mind taking a quick look at the errors? There are some about websocket connections, and I wonder if that's blocking them being saved?

If that's correct, could we redesign the ordering so that even if websockets aren't connecting, it doesn't interrupt the individual saving of images?

[image: image] https://user-images.githubusercontent.com/24359/65268442-26830f00-dae5-11e9-90c4-71979c9ed5b4.png

http://mapknitter-stable.laboratoriopublico.org/maps/test6/edit

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/publiclab/mapknitter/issues/1041?email_source=notifications&email_token=AFMRPUM3NKLWAZZFCGYXBODQKO4W3A5CNFSM4IYOIOYKYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4HMPRFNA, or mute the thread https://github.com/notifications/unsubscribe-auth/AFMRPUJZPDZURTVUPTEBKK3QKO4W3ANCNFSM4IYOIOYA .

ViditChitkara commented 5 years ago

I think this is very similar to https://github.com/publiclab/plots2/pull/5744#issuecomment-504156582 Maybe we have to check out https://stackoverflow.com/a/51048929 and adjust nginx config here. @jywarren , @icarito any ideas?

Screenshot 2019-09-20 at 1 03 07 AM
jywarren commented 5 years ago

Thank you! This is, i think, the last thing before we publish this all to the live site! ❤️

jywarren commented 5 years ago

Is there a way to rewire this so that the image position saving isn't interrupted by whether actioncable is running?

jywarren commented 5 years ago

Just to decouple the errors?

ViditChitkara commented 5 years ago

Yes!! This is great!! Really excited about this one!

ViditChitkara commented 5 years ago

Is there a way to rewire this so that the image position saving isn't interrupted by whether actioncable is running?

Will have to check that. Doing that.

jywarren commented 5 years ago

Hm, also cc'ing @sashadev-sky just because one of the errors I'm seeing is that it's not finding any corners - one of the errors is on getCorners()[0] --

image

I also noticed that in the images tab it's not showing the second image as having uploaded properly, though it did work for the first. This could all be cascading from an initial issue with placing the first image getting interrupted by the ActionCable stuff, but it could also be a distinct bug.

jywarren commented 5 years ago

Thanks Vidit!

jywarren commented 5 years ago

Aha - one issue is that it's finding the map slug from the page URL, here:

https://github.com/publiclab/mapknitter/blob/cee7479cf4f0323a262b931a37cc72215ca48273/app/assets/javascripts/channels/concurrent_editing.js#L28

The page URL has just changed to /maps/SLUG/edit instead of /maps/SLUG, so this needs to be tweaked. I'll make that change in a PR but I think we should still be sure that the whole system works even if actioncable/websockets is not working.

jywarren commented 5 years ago

OK, merged that, now let's let it build on Jenkins and let's test the stable server again.

ViditChitkara commented 5 years ago

Let's keep a close look there..

ViditChitkara commented 5 years ago

Also, I think the nginx configuration might still be needed for the websockets.

jywarren commented 5 years ago

Can you link to or explain exactly what the nginx config changes will have to be? Thanks.

jywarren commented 5 years ago

Ah, i see you did above - https://github.com/publiclab/mapknitter/issues/1041#issuecomment-533276283

Great. However we are oddly seeing that this is correctly configured on plots2 but not working there either, any ideas on that? https://github.com/publiclab/plots2/issues/6138

sashadev-sky commented 5 years ago

@jywarren I vaguely remember all of those exact errors. I see places where these errors would be caused.

for ex - this is how I use the image load event: https://github.com/publiclab/mapknitter/blob/bd7e1cf77c88a5ef850970f1ff1d15b094b60d75/app/assets/javascripts/mapknitter/Map.js#L114-L119 https://github.com/publiclab/mapknitter/blob/bd7e1cf77c88a5ef850970f1ff1d15b094b60d75/app/assets/javascripts/mapknitter/Map.js#L156-L169

and this is how it is used in synchronous editing method: https://github.com/publiclab/mapknitter/blob/bd7e1cf77c88a5ef850970f1ff1d15b094b60d75/app/assets/javascripts/mapknitter/Map.js#L487-L491

the load event of an image doesn't have a layer saved on it. so the hasTool part will throw an error like you showed

ViditChitkara commented 5 years ago

Ah, i see you did above - #1041 (comment) Great. However we are oddly seeing that this is correctly configured on plots2 but not working there either, any ideas on that? publiclab/plots2#6138

Will have to dig in a bit deeper for this.

ViditChitkara commented 5 years ago

Doing

sashadev-sky commented 5 years ago

done this way so that it can share methods with this listener: https://github.com/publiclab/mapknitter/blob/bd7e1cf77c88a5ef850970f1ff1d15b094b60d75/app/assets/javascripts/mapknitter/Map.js#L629-L633

I didn't have those errors when my PRs were merged though, but I think either a) were using the wrong version of LDI on stable? or b) Fixing some parts of synchronous editing results in new bugs c) maps in readOnly are not editable https://github.com/publiclab/mapknitter/blob/bd7e1cf77c88a5ef850970f1ff1d15b094b60d75/app/assets/javascripts/mapknitter/Map.js#L615-L618

meaning if you call something like img.editing.hasTool() without first enabling editing, you will get the error

leaflet.distortableimage.js:2104 Uncaught TypeError: Cannot read property 'some' of undefined
    at NewClass.hasTool (leaflet.distortableimage.js:2104)
    at <anonymous>:1:13
hasTool @ leaflet.distortableimage.js:2104

Do any of those sound like a reason you might be getting the error?

jywarren commented 5 years ago

Yes, so due to the asset management location change referenced in #999 stable had been running on 0.4.3, so that may have been it! Thanks all, and no worries, this is why we have the stable server, to catch last minute integration issues like this, and we have both your projects plus @cesswairimu's work on the new edit URLs all coming together in one place, so it's not too surprising!

sashadev-sky commented 5 years ago

Ok! I will open a PR to refactor action cable code in Map.js when the connection part starts working! Keep me updated

Sasha Boginsky

On Sep 20, 2019, at 3:12 PM, Jeffrey Warren notifications@github.com wrote:

Yes, so due to the asset management location change referenced in #999 stable had been running on 0.4.3, so that may have been it! Thanks all, and no worries, this is why we have the stable server, to catch last minute integration issues like this, and we have both your projects plus @cesswairimu's work on the new edit URLs all coming together in one place, so it's not too surprising!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

jywarren commented 5 years ago

OK, so now we are seeing a 500 error on the PATCH submission when an image tries to save:

application-7eb6d4804d4a33b083091d5a6b2fe0b411ee0820b52f5a84a935216e784da23b.js:34 PATCH http://mapknitter-stable.laboratoriopublico.org/images/1 500 (Internal Server Error)
send @ application-7eb6d4804d4a33b083091d5a6b2fe0b411ee0820b52f5a84a935216e784da23b.js:34
ajax @ application-7eb6d4804d4a33b083091d5a6b2fe0b411ee0820b52f5a84a935216e784da23b.js:33
saveImage @ application-7eb6d4804d4a33b083091d5a6b2fe0b411ee0820b52f5a84a935216e784da23b.js:277
saveImageIfChanged @ application-7eb6d4804d4a33b083091d5a6b2fe0b411ee0820b52f5a84a935216e784da23b.js:277
s @ application-7eb6d4804d4a33b083091d5a6b2fe0b411ee0820b52f5a84a935216e784da23b.js:116
F @ application-7eb6d4804d4a33b083091d5a6b2fe0b411ee0820b52f5a84a935216e784da23b.js:116
n @ application-7eb6d4804d4a33b083091d5a6b2fe0b411ee0820b52f5a84a935216e784da23b.js:116

This should be locally reproducible. Could a routes change have affected this? The payload was:

{
"warpable_id": 1,
"locked": "false",
"points": "138.70246767997745,35.362115359652016:138.73585581779483,
35.36813477748592:138.73800158500674,35.34811497800847:138.70246767997745,35.34811497800847"
}
jywarren commented 5 years ago
I, [2019-09-23T16:08:05.223386 #6088]  INFO -- : [5ce8066d-811f-44d3-b460-2579cd52aa2e] Completed 500 Internal Server Error in 1084ms (ActiveRecord: 63
3.6ms)
I, [2019-09-23T16:08:05.471680 #6088]  INFO -- : [5ce8066d-811f-44d3-b460-2579cd52aa2e] Sending event 871170401b1647a0b9dbc7550f125eb1 to Sentry
D, [2019-09-23T16:08:05.485997 #6088] DEBUG -- : [5ce8066d-811f-44d3-b460-2579cd52aa2e] Raven HTTP Transport connecting to https://sentry.io
F, [2019-09-23T16:08:05.768351 #6088] FATAL -- : [5ce8066d-811f-44d3-b460-2579cd52aa2e]   
F, [2019-09-23T16:08:05.768941 #6088] FATAL -- : [5ce8066d-811f-44d3-b460-2579cd52aa2e] NameError (uninitialized constant #<Class:0x000055e68782ad70>::Cartagen):
F, [2019-09-23T16:08:05.769048 #6088] FATAL -- : [5ce8066d-811f-44d3-b460-2579cd52aa2e]   
F, [2019-09-23T16:08:05.769174 #6088] FATAL -- : [5ce8066d-811f-44d3-b460-2579cd52aa2e] app/models/warpable.rb:101:in `get_cm_per_pixel'
[5ce8066d-811f-44d3-b460-2579cd52aa2e] app/controllers/images_controller.rb:93:in `update'

OK! uninitialized constant #<Class:0x000055e68782ad70>::Cartagen

jywarren commented 5 years ago

OK, adding require 'cartagen' in warpable.rb: https://github.com/publiclab/mapknitter/pull/1046

jywarren commented 5 years ago

OK! Saving is fixed! image

And we're on LDI v0.7.7 -- but, not seeing the multiple image export feature yet. Closing this, though!

jywarren commented 5 years ago

Looks like we still need nginx config for the websockets to work, but at least saving is working now!