Try to catch and log errors during upload #1283
Conversation
kueda
commented
Mar 13, 2024
kueda
commented
- Wraps some upload async methods with try/catch to attempt to log some potential bugs
- Adjusts upload param mappings
- Try to get upload status to more accurately reflect the upload state
* Wraps some upload async methods with try/catch to attempt to log some potential bugs * Adjusts upload param mappings * Try to get upload status to more accurately reflect the upload state
| if ( numUnuploadedObs && numUnuploadedObs !== 0 ) { | ||
| return t( "Upload-x-observations", { count: numUnuploadedObs } ); | ||
| } | ||
|
|
There was a problem hiding this comment.
This might be a bit off spec, but if you upload one of several obs, I think it should say there are still obs to upload when it's done.
There was a problem hiding this comment.
When this branched off the "Upload success" message was sticking around forever, current main is it will disappear after 5s and then the "Upload x observations" state will reappear if there are numUnuploadedObs !== 0 . The way I handled it in my PR is to reset the upload state and therefore set uploadsComplete to false after 5s. With your changes, it will switch back to "Upload x observations" immediately. I am fine with merging this in if it seems more appropriate UX.
There was a problem hiding this comment.
I like your approach better, and I think that's what Abhas wants. I merged from main and tried to get the single-upload case to have that 5s delay as well.
src/realmModels/ObservationPhoto.js
Outdated
| "photo[uuid]": observationPhoto.uuid, | ||
| photo: { | ||
| id: observationPhoto.photo.id | ||
| }, |
There was a problem hiding this comment.
Sending the ObservationPhoto UUID instead of the Photo UUID is dangerous here, because they will be different in server responses. The server doesn't seem to respond with the photo UUID at all, which is a separate problem.
| uuid: mockPhoto.uuid | ||
| }] ) ); | ||
| } ); | ||
|
|
There was a problem hiding this comment.
I'm not sure why this was working, but I'm pretty sure it was not actually integrating the API wrappers and related networking code. This mocks inatjs responses to make sure that when those parts of the app get exercised they get reasonable responses.
There was a problem hiding this comment.
Does it make sense to have those mocks in the inaturalistjs package itself? Would other clients benefit if we have a shared mock?
There was a problem hiding this comment.
Not sure. Here the mock implementation is very specific to the test, like we want it to return very specific values. If inatjs were to have built-in mocks, they'd probably need to return generic mock data, so in this case we'd still need to make our own implementation. There might be other cases where that would be useful, though.
|
@jtklein I haven't had a chance to test this thoroughly under field conditions, but I got a bit into the upload code which I know you worked on recently, so I'd appreciate it if you could take a glance and see if anything seems glaringly wrong. |
There was a problem hiding this comment.
Have started to review but when I upload an observation I am hitting this every time, and the upload does not finish:
LOG 2024-03-13T12:03:18.495Z | INatApiError | ERROR : Error requesting https://stagingapi.inaturalist.org/v2/observation_photos (status: 422):
{"status":"422","errors":[{"errorCode":"422","message":{"errors":"No photo specified"},"from":"externalAPI"}]}
This would need to be fixed before I can continue.
| if ( numUnuploadedObs && numUnuploadedObs !== 0 ) { | ||
| return t( "Upload-x-observations", { count: numUnuploadedObs } ); | ||
| } | ||
|
|
There was a problem hiding this comment.
When this branched off the "Upload success" message was sticking around forever, current main is it will disappear after 5s and then the "Upload x observations" state will reappear if there are numUnuploadedObs !== 0 . The way I handled it in my PR is to reset the upload state and therefore set uploadsComplete to false after 5s. With your changes, it will switch back to "Upload x observations" immediately. I am fine with merging this in if it seems more appropriate UX.
