Improving user feedback for track uploading errors

[SebaDro]

Description
If an error occurs during upload of all local tracks, the user does not get a proper feedback about the reason for the upload failure. The error message just shows "An error occurred during the upload process.". A more meaningful message text would be necessary to assess if the failure is effected by a misuse of the app, an app error or a server failure. In addition, the error is not logged, thus it does not appear within the log report.

Branches
master, develop

How to reproduce
Record a small track with and finish recording. Go to the LocalTrackFragment, turn off internet and click the button for uploading all tracks => the app shows "An error occurred during the upload process."

How to fix
Adjust error handling here:

enviroCar-app/org.envirocar.app/src/org/envirocar/app/views/tracklist/TrackListLocalCardFragment.java

Lines 530 to 536 in 98403d7

	public void onError(Throwable e) {
	if (isDisposed())
	return;
	showSnackbar("An error occurred during the upload process.");
	dialog.dismiss();
	}

Add error logging and enhance the error handling. Maybe, distinguishing the error type such as for single track upload errors would be useful:

enviroCar-app/org.envirocar.app/src/org/envirocar/app/views/tracklist/TrackListLocalCardFragment.java

Lines 390 to 418 in 98403d7

	if (e instanceof TrackUploadException) {
	switch (((TrackUploadException) e).getReason()) {
	case NOT_ENOUGH_MEASUREMENTS:
	showSnackbar(R.string.track_list_upload_error_no_measurements);
	break;
	case TRACK_WITH_NO_VALID_CAR:
	showSnackbar(R.string.track_list_upload_error_no_valid_car);
	break;
	case TRACK_ALREADY_UPLOADED:
	showSnackbar(R.string.track_list_upload_error_already_uploaded);
	break;
	case NO_NETWORK_CONNECTION:
	showSnackbar(R.string.track_list_upload_error_no_network_connection);
	break;
	case NOT_LOGGED_IN:
	showSnackbar(R.string.track_list_upload_error_not_logged_in);
	break;
	case NO_CAR_ASSIGNED:
	showSnackbar(R.string.track_list_upload_error_no_valid_car);
	break;
	case GPS_TRACKS_NOT_ALLOWED:
	showSnackbar(R.string.track_list_upload_error_gps_track);
	break;
	case UNAUTHORIZED:
	showSnackbar(R.string.track_list_upload_error_unauthorized);
	break;
	case UNKNOWN:
	showSnackbar(R.string.track_list_upload_track_general_error);
	break;

[cdhiraj40]

@SebaDro I think its the same issue as #857, but it's better to make the issue alone here from other small fixes like #857

[SebaDro]

Hey @cdhiraj40. Yes, you're right. In parts, the PR adresses this issue. However, it only focusses on the internet connection problem. A proper handling of other error types is still pending. So, I think it's ok to leave both issues open.

[cdhiraj40]

Hey @cdhiraj40. Yes, you're right. In parts, the PR adresses this issue. However, it only focusses on the internet connection problem. A proper handling of other error types is still pending. So, I think it's ok to leave both issues open.

yes I totally agree, I will look into the issue and see why it's not working, thanks :)

[cdhiraj40]

@SebaDro I think its happening cause here

public void onError(Throwable e) {

its throwing NullPointerException
that's why this never gets true

            if (e instanceof TrackUploadException) {

the reason this keeps on getting printed

showSnackbar(R.string.track_list_upload_track_general_error);

detailed version :

java.lang.NullPointerException cannot be cast to org.envirocar.core.exception.TrackUploadException

and the NullPointerException is coming because (not sure)

E/enviroCar: [org.envirocar.app.handler.TrackUploadHandler] Attempt to invoke virtual method 'io.reactivex.Observable io.reactivex.Observable.map(io.reactivex.functions.Function)' on a null object reference:

in the file src/org/envirocar/app/handler/agreement/AgreementManager.java
lines around AgreementManager.java:125

[SebaDro]

Yes, I noticed the NullPointerException, too. If you follow the stacktrace, you will find, that the exception occurs inside the AgreementManager when it comes to check the terms of use:

enviroCar-app/org.envirocar.app/src/org/envirocar/app/handler/agreement/AgreementManager.java

Lines 119 to 139 in 98403d7

	private Observable<TermsOfUse> getRemoteTermsOfUseObservable() {
	LOG.info("getRemoteTermsOfUse() TermsOfUse are null. Try to fetch the last TermsOfUse.");
	return mDAOProvider.getTermsOfUseDAO()
	.getAllTermsOfUseObservable()
	.map(checkNullElseThrowNotConnected())
	.map(termsOfUses -> {
	// Set the list of terms of uses.
	AgreementManager.this.list = termsOfUses;
	try {
	// Get the id of the first terms of use instance and fetch
	// the terms of use
	String id = termsOfUses.get(0).getId();
	TermsOfUse inst = mDAOProvider.getTermsOfUseDAO().getTermsOfUse(id);
	return inst;
	} catch (DataRetrievalFailureException | NotConnectedException e) {
	LOG.warn(e.getMessage(), e);
	throw e;
	}
	});
	}

It seems, there are some implementions missing for the CacheTermsOfUseDAO.

However, I don't think there is a necessity to check the terms of use, if there is no internet. I'd prefer a "fail fast" solution e.g. checking internet connection and showing an error dialog without trying to upload a track.

[cdhiraj40]

Yes, I noticed the NullPointerException, too. If you follow the stacktrace, you will find, that the exception occurs inside the AgreementManager when it comes to check the terms of use:

enviroCar-app/org.envirocar.app/src/org/envirocar/app/handler/agreement/AgreementManager.java

Lines 119 to 139 in 98403d7

	private Observable<TermsOfUse> getRemoteTermsOfUseObservable() {
	LOG.info("getRemoteTermsOfUse() TermsOfUse are null. Try to fetch the last TermsOfUse.");
	return mDAOProvider.getTermsOfUseDAO()
	.getAllTermsOfUseObservable()
	.map(checkNullElseThrowNotConnected())
	.map(termsOfUses -> {
	// Set the list of terms of uses.
	AgreementManager.this.list = termsOfUses;
	try {
	// Get the id of the first terms of use instance and fetch
	// the terms of use
	String id = termsOfUses.get(0).getId();
	TermsOfUse inst = mDAOProvider.getTermsOfUseDAO().getTermsOfUse(id);
	return inst;
	} catch (DataRetrievalFailureException | NotConnectedException e) {
	LOG.warn(e.getMessage(), e);
	throw e;
	}
	});
	}

It seems, there are some implementions missing for the CacheTermsOfUseDAO.

However, I don't think there is a necessity to check the terms of use, if there is no internet. I'd prefer a "fail fast" solution e.g. checking internet connection and showing an error dialog without trying to upload a track.

I think I did try that in #857, maybe we can make functions for particular errors and use it?
It may decrease these errors coming in future, as we already have some functions made for some issues it will become easier to implement them too

[SebaDro]

Have been fixed in parts with #874. However, the app still does not inform the user about the cause for a failing track upload.

[cdhiraj40]

@SebaDro sir is this issue from the server side? if not then we can try handling one by one and check why its not working properly

[SebaDro]

I'm not sure if the server responds with a meaningful error message for each track. First step would be to debug the app and analyze the exceptions.