Skip to content

Commit c8ff6ea

Browse files
committed
docs: more thorough evaluation of failure points in UploadWorker
1 parent a845585 commit c8ff6ea

File tree

1 file changed

+15
-3
lines changed

1 file changed

+15
-3
lines changed

app/src/main/java/fr/free/nrw/commons/upload/worker/UploadWorker.kt

+15-3
Original file line numberDiff line numberDiff line change
@@ -301,6 +301,8 @@ class UploadWorker(var appContext: Context, workerParams: WorkerParameters) :
301301
Timber.e("""upload: ${contribution.media.filename} failed, file path is null""")
302302
}
303303
// FP: this is an obvious genuine failure point
304+
// But the actual return point is not here
305+
// So I guess we can just add a var here which basically says what the string above say
304306

305307
val media = contribution.media
306308
val displayTitle = contribution.media.displayTitle
@@ -379,6 +381,11 @@ class UploadWorker(var appContext: Context, workerParams: WorkerParameters) :
379381

380382
} else {
381383
// FP: Here is a much more ambiguous failure point
384+
// Either uploadResult is null in which case we have a big problem
385+
// or we'll have some form of message in uploadResult.result
386+
// Either way we can probably not confidently say that it is a
387+
// genuine failure, and if we want a more extensive description then
388+
// we have those as well
382389
Timber.e("Stash Upload failed")
383390
showFailedNotification(contribution)
384391
contribution.state = Contribution.STATE_FAILED
@@ -389,7 +396,9 @@ class UploadWorker(var appContext: Context, workerParams: WorkerParameters) :
389396
}catch (exception : Exception){
390397
// FP: Exception in upload from stash.
391398
// Ambiguous in the sense that it is hard to determine from code, but we
392-
// at least have an exception to work with
399+
// at least have an exception to work with. And as we have already
400+
// passed one upload step we can be pretty certain this is not a
401+
// genuine failure.
393402
Timber.e(exception)
394403
Timber.e("Upload from stash failed for contribution : $filename")
395404
showFailedNotification(contribution)
@@ -407,7 +416,9 @@ class UploadWorker(var appContext: Context, workerParams: WorkerParameters) :
407416
}
408417
else -> {
409418
// FP: Error in upload to stash.
410-
// have a stashUploadResult to work with
419+
// This only gives us a failure, nothing more to work with, so can't say it
420+
// is a genuine failure. This is probably where invalid or missing filename
421+
// will fail
411422
Timber.e("""upload file to stash failed with status: ${stashUploadResult.state}""")
412423
showFailedNotification(contribution)
413424
contribution.state = Contribution.STATE_FAILED
@@ -418,7 +429,8 @@ class UploadWorker(var appContext: Context, workerParams: WorkerParameters) :
418429
}catch (exception: Exception){
419430
// FP: Exception in upload to stash.
420431
// Ambiguous in the sense that it is hard to determine from code, but we
421-
// at least have an exception to work with
432+
// at least have an exception to work with. Might also be here invalid filename
433+
// fails
422434
Timber.e(exception)
423435
Timber.e("Stash upload failed for contribution: $filename")
424436
showFailedNotification(contribution)

0 commit comments

Comments
 (0)