Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Try to catch and log errors during upload #1283

Merged
merged 5 commits into from
Mar 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 47 additions & 20 deletions src/components/MyObservations/MyObservationsContainer.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import {
useInfiniteObservationsScroll,
useIsConnected,
useLocalObservations,
useNumUnuploadedObservations,
useObservationsUpdates,
useStoredLayout,
useTranslation
Expand Down Expand Up @@ -144,6 +145,7 @@ const MyObservationsContainer = ( ): Node => {
const { observationList: observations } = useLocalObservations( );
const { layout, writeLayoutToStorage } = useStoredLayout( "myObservationsLayout" );
const { deletionsCompletedAt } = useDeleteObservations( );
const numUnuploadedObservations = useNumUnuploadedObservations( );

const isOnline = useIsConnected( );

Expand Down Expand Up @@ -252,7 +254,10 @@ const MyObservationsContainer = ( ): Node => {
const isOne = state.totalProgressIncrements === 1;
if (
state.singleUpload
&& ( state.uploadProgress[uuid] >= state.totalProgressIncrements || isOne )
&& (
state.uploadProgress[uuid] >= state.totalProgressIncrements
|| isOne
)
) {
if ( isOne ) {
dispatch( {
Expand Down Expand Up @@ -304,19 +309,21 @@ const MyObservationsContainer = ( ): Node => {
}
return e.message;
} ).join( ", " );
} else if ( uploadError.message.match( /Network request failed/ ) ) {
message = "Connection problem. Please try again later.";
} else if ( uploadError.message?.match( /Network request failed/ ) ) {
message = t( "Connection-problem-Please-try-again-later" );
logger.error(
"[MyObservationsContainer.js] upload failed due to network problem: ",
uploadError
`[MyObservationsContainer.js] upload failed due to network problem: ${uploadError}`
);
} else {
logger.error( "[MyObservationsContainer.js] upload failed: ", uploadError );
logger.error( `[MyObservationsContainer.js] upload failed: ${uploadError}` );
throw uploadError;
}
dispatch( { type: "SET_UPLOAD_ERROR", error: message } );
}
}, [realm] );
}, [
realm,
t
] );

const uploadSingleObservation = useCallback( async ( observation, options ) => {
if ( !currentUser ) {
Expand All @@ -336,22 +343,33 @@ const MyObservationsContainer = ( ): Node => {
toggleLoginSheet( );
return;
}
if ( uploadsComplete || uploadInProgress ) {
if ( numUnuploadedObservations === 0 || uploadInProgress ) {
return;
}
dispatch( { type: "START_UPLOAD", singleUpload: uploads.length === 1 } );

await Promise.all( uploads.map( async obsToUpload => {
await uploadObservationAndCatchError( obsToUpload );
dispatch( { type: "START_NEXT_UPLOAD" } );
} ) );
dispatch( { type: "UPLOADS_COMPLETE" } );
}, [currentUser,
uploadsComplete,
uploadInProgress,
uploads,
try {
await Promise.all( uploads.map( async obsToUpload => {
await uploadObservationAndCatchError( obsToUpload );
dispatch( { type: "START_NEXT_UPLOAD" } );
} ) );
dispatch( { type: "UPLOADS_COMPLETE" } );
} catch ( uploadMultipleObservationsError ) {
logger.error( "Failed to uploadMultipleObservations: ", uploadMultipleObservationsError );
dispatch( {
type: "SET_UPLOAD_ERROR",
error: t( "Something-went-wrong" )
} );
}
}, [
currentUser,
numUnuploadedObservations,
t,
toggleLoginSheet,
uploadObservationAndCatchError] );
uploadInProgress,
uploadObservationAndCatchError,
uploads
] );

const stopUploads = useCallback( ( ) => {
dispatch( { type: "STOP_UPLOADS" } );
Expand All @@ -375,13 +393,22 @@ const MyObservationsContainer = ( ): Node => {
const msSinceDeletionsCompleted = ( new Date( ) - deletionsCompletedAt );
if ( msSinceDeletionsCompleted < 5_000 ) {
const naptime = 10_000 - msSinceDeletionsCompleted;
logger.debug(
`[MyObservationsContainer.js] finished deleting recently, waiting ${naptime} ms`
logger.info(
"[MyObservationsContainer.js] downloadRemoteObservationsFromServer finished deleting "
+ `recently deleted, waiting ${naptime} ms`
);
await sleep( naptime );
}
}
logger.info(
"[MyObservationsContainer.js] downloadRemoteObservationsFromServer, fetching observations"
);
const { results } = await searchObservations( searchParams, { api_token: apiToken } );
logger.info(
"[MyObservationsContainer.js] downloadRemoteObservationsFromServer, fetched",
results.length,
"results, upserting..."
);
Observation.upsertRemoteObservations( results, realm );
}, [
currentUser,
Expand Down
13 changes: 9 additions & 4 deletions src/components/MyObservations/ToolbarContainer.js
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ const ToolbarContainer = ( {
if ( uploadInProgress ) {
const translationParams = {
total: numToUpload,
currentUploadCount: numFinishedUploads + 1
currentUploadCount: Math.min( numFinishedUploads + 1, numToUpload )
jtklein marked this conversation as resolved.
Show resolved Hide resolved
};
// iPhone 4 pixel width
if ( screenWidth <= 640 ) {
Expand All @@ -123,12 +123,17 @@ const ToolbarContainer = ( {
}

if ( uploadsComplete ) {
// Note that numToUpload is kind of the number of obs being uploaded in
// the current upload session, so it might be 1 if a single obs is
// being uploaded even though 5 obs need upload
return t( "X-observations-uploaded", { count: numToUpload } );
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return numUnuploadedObs !== 0
? t( "Upload-x-observations", { count: numUnuploadedObs } )
: "";
if ( numUnuploadedObs && numUnuploadedObs !== 0 ) {
return t( "Upload-x-observations", { count: numUnuploadedObs } );
}

return "";
}, [
currentDeleteCount,
deletionsComplete,
Expand Down
2 changes: 1 addition & 1 deletion src/i18n/l10n/en.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -1840,4 +1840,4 @@ Sorry-this-observation-was-deleted = Sorry, this observation was deleted
# Generic confirmation, e.g. button on a warning alert
OK = OK

Connection-problem-please-try-again-later = Connection problem. Please try again later.
Connection-problem-Please-try-again-later = Connection problem. Please try again later.
2 changes: 1 addition & 1 deletion src/i18n/l10n/en.ftl.json
Original file line number Diff line number Diff line change
Expand Up @@ -1345,5 +1345,5 @@
"comment": "Generic confirmation, e.g. button on a warning alert",
"val": "OK"
},
"Connection-problem-please-try-again-later": "Connection problem. Please try again later."
"Connection-problem-Please-try-again-later": "Connection problem. Please try again later."
}
2 changes: 1 addition & 1 deletion src/i18n/strings.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -1840,4 +1840,4 @@ Sorry-this-observation-was-deleted = Sorry, this observation was deleted
# Generic confirmation, e.g. button on a warning alert
OK = OK

Connection-problem-please-try-again-later = Connection problem. Please try again later.
Connection-problem-Please-try-again-later = Connection problem. Please try again later.
19 changes: 14 additions & 5 deletions src/realmModels/Observation.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { Realm } from "@realm/react";
import uuid from "react-native-uuid";
import { createObservedOnStringForUpload } from "sharedHelpers/dateAndTime";
import { log } from "sharedHelpers/logger";
import { readExifFromMultiplePhotos } from "sharedHelpers/parseExif";
import safeRealmWrite from "sharedHelpers/safeRealmWrite";

Expand All @@ -13,6 +14,8 @@ import Taxon from "./Taxon";
import User from "./User";
import Vote from "./Vote";

const logger = log.extend( "Observation" );

// noting that methods like .toJSON( ) are only accessible when the model
// class is extended with Realm.Object per this issue:
// https://github.com/realm/realm-js/issues/3600#issuecomment-785828614
Expand Down Expand Up @@ -120,16 +123,22 @@ class Observation extends Realm.Object {
return observation;
}

static upsertRemoteObservations( observations, realm ) {
if ( observations && observations.length > 0 ) {
const obsToUpsert = observations.filter(
static upsertRemoteObservations( remoteObservations, realm ) {
if ( remoteObservations && remoteObservations.length > 0 ) {
const obsToUpsert = remoteObservations.filter(
obs => !Observation.isUnsyncedObservation( realm, obs )
);
const msg = obsToUpsert.map( remoteObservation => {
const obsPhotoUUIDs = remoteObservation.observation_photos?.map( op => op.uuid );
return `obs ${remoteObservation.uuid}, ops: ${obsPhotoUUIDs}`;
} );
// Trying to debug disappearing photos
logger.info( "upsertRemoteObservations, upserting: ", msg );
safeRealmWrite( realm, ( ) => {
obsToUpsert.forEach( obs => {
obsToUpsert.forEach( remoteObservation => {
realm.create(
"Observation",
Observation.mapApiToRealm( obs, realm ),
Observation.mapApiToRealm( remoteObservation, realm ),
"modified"
);
} );
Expand Down
20 changes: 11 additions & 9 deletions src/realmModels/ObservationPhoto.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,29 +30,31 @@ class ObservationPhoto extends Realm.Object {
return localObsPhoto;
}

static mapPhotoForUpload( observtionID, observationPhoto ) {
static mapPhotoForUpload( observationID, photo ) {
return {
"photo[uuid]": observationPhoto.uuid,
file: new FileUpload( {
uri: observationPhoto.photo.localFilePath,
name: `${observationPhoto.uuid}.jpeg`,
uri: photo.localFilePath,
name: photo.localFilePath,
type: "image/jpeg"
} )
};
}

static mapPhotoForAttachingToObs( id, observationPhoto ) {
static mapPhotoForAttachingToObs( observationID, observationPhoto ) {
return {
"observation_photo[observation_id]": id,
"observation_photo[photo_id]": observationPhoto.id
observation_photo: {
uuid: observationPhoto.uuid,
observation_id: observationID,
photo_id: observationPhoto.photo.id
}
};
}

static mapPhotoForUpdating( id, observationPhoto ) {
static mapPhotoForUpdating( observationID, observationPhoto ) {
return {
id: observationPhoto.uuid,
observation_photo: {
observation_id: id,
observation_id: observationID,
position: observationPhoto.position
}
};
Expand Down
55 changes: 40 additions & 15 deletions src/sharedHelpers/uploadObservation.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,22 @@ import safeRealmWrite from "sharedHelpers/safeRealmWrite";

const UPLOAD_PROGRESS_INCREMENT = 1;

// The reason this doesn't simply accept the record is because we're not being
// strict about using Realm.Objects, so sometimes the thing we just uploaded
// is a Realm.Object and sometimes it's a POJO, but in order to mark it as
// uploaded and add a server-assigned id attribute, we need to find the
// matching Realm.Object
const markRecordUploaded = (
observationUUID: string,
recordUUID: string | null,
type: string,
response: {
results: Array<{id: Number}>
},
realm: Object
realm: Object,
options?: {
record: Object
}
) => {
const { id } = response.results[0];
const observation = realm?.objectForPrimaryKey( "Observation", observationUUID );
Expand All @@ -32,17 +40,30 @@ const markRecordUploaded = (
if ( type === "Observation" ) {
record = observation;
} else if ( type === "ObservationPhoto" ) {
const existingObsPhoto = observation.observationPhotos?.find( p => p.uuid === recordUUID );
const existingObsPhoto = observation.observationPhotos?.find( op => op.uuid === recordUUID );
record = existingObsPhoto;
} else if ( type === "ObservationSound" ) {
const existingObsSound = observation.observationSounds?.find( p => p.uuid === recordUUID );
const existingObsSound = observation.observationSounds?.find( os => os.uuid === recordUUID );
record = existingObsSound;
} else if ( type === "Photo" ) {
// Photos do not have UUIDs, so we pass the Photo itself as an option
record = options?.record;
}

if ( !record ) {
throw new Error(
`Cannot find local Realm object, type: ${type}, recordUUID: ${recordUUID || ""}`
);
}

safeRealmWrite( realm, ( ) => {
// These flow errors don't make any sense b/c if record is undefined, we
// will throw an error above
// $FlowIgnore
record.id = id;
// $FlowIgnore
record._synced_at = new Date( );
}, "marking record uploaded in uploadObservation.js" );
}, `marking record uploaded in uploadObservation.js, type: ${type}` );
};

const uploadEvidence = async (
Expand Down Expand Up @@ -72,16 +93,19 @@ const uploadEvidence = async (
// half one when the obsPhoto/obsSound is attached to the obs
emitUploadProgress( observationUUID, ( UPLOAD_PROGRESS_INCREMENT / 2 ) );
// TODO: can't mark records as uploaded by primary key for ObsPhotos and ObsSound anymore
markRecordUploaded( observationUUID, evidenceUUID, type, response, realm );
markRecordUploaded( observationUUID, evidenceUUID, type, response, realm, {
record: currentEvidence
} );
}

return response;
};

const responses = await Promise.all( evidence.map( item => {
const currentEvidence = item.toJSON( );
let currentEvidence = item;

if ( currentEvidence.photo ) {
currentEvidence = item.toJSON( );
// Remove all null values, b/c the API doesn't seem to like them
const newPhoto = {};
const { photo } = currentEvidence;
Expand Down Expand Up @@ -124,18 +148,19 @@ const uploadObservation = async ( obs: Object, realm: Object ): Object => {

// First upload the photos/sounds (before uploading the observation itself)
const hasPhotos = obs?.observationPhotos?.length > 0;
const unsyncedPhotos = hasPhotos
? obs?.observationPhotos?.filter( item => !item.wasSynced( ) )
const unsyncedObservationPhotos = hasPhotos
? obs?.observationPhotos?.filter( op => !op.wasSynced( ) )
: [];
const modifiedPhotos = hasPhotos
? obs?.observationPhotos?.filter( item => item.wasSynced( ) && item.needsSync( ) )
const unsyncedPhotos = unsyncedObservationPhotos?.map( op => op.photo );
const modifiedObservationPhotos = hasPhotos
? obs?.observationPhotos?.filter( op => op.wasSynced( ) && op.needsSync( ) )
: [];

await Promise.all( [
unsyncedPhotos.length > 0
? await uploadEvidence(
unsyncedPhotos,
"ObservationPhoto",
"Photo",
ObservationPhoto.mapPhotoForUpload,
null,
inatjs.photos.create,
Expand Down Expand Up @@ -196,9 +221,9 @@ const uploadObservation = async ( obs: Object, realm: Object ): Object => {
await Promise.all( [
markRecordUploaded( obs.uuid, null, "Observation", response, realm ),
// Attach the newly uploaded photos/sounds to the uploaded observation
unsyncedPhotos.length > 0
unsyncedObservationPhotos.length > 0
? await uploadEvidence(
unsyncedPhotos,
unsyncedObservationPhotos,
"ObservationPhoto",
ObservationPhoto.mapPhotoForAttachingToObs,
obsUUID,
Expand All @@ -221,9 +246,9 @@ const uploadObservation = async ( obs: Object, realm: Object ): Object => {
)
: null,
// Update any existing modified photos/sounds
modifiedPhotos.length > 0
modifiedObservationPhotos.length > 0
? await uploadEvidence(
modifiedPhotos,
modifiedObservationPhotos,
"ObservationPhoto",
ObservationPhoto.mapPhotoForUpdating,
obsUUID,
Expand Down
Loading
Loading