ViewModel is now access based on taskId using Bundle

[anandwana001]

Fixes #2725

@shobhitagarwal1612 @gino-m @sufyanAbbasi PTAL?

[codecov]

Codecov Report

Attention: Patch coverage is 57.14286% with 27 lines in your changes missing coverage. Please review.

Project coverage is 60.86%. Comparing base (876a1b7) to head (146955e).
Report is 3 commits behind head on master.

Files with missing lines	Patch %	Lines
...n/tasks/location/CaptureLocationTaskMapFragment.kt	50.00%	7 Missing and 3 partials ⚠️
...ollection/tasks/polygon/DrawAreaTaskMapFragment.kt	52.63%	7 Missing and 2 partials ⚠️
...tacollection/tasks/point/DropPinTaskMapFragment.kt	55.55%	6 Missing and 2 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #2748      +/-   ##
============================================
- Coverage     60.93%   60.86%   -0.08%     
  Complexity     1158     1158              
============================================
  Files           264      264              
  Lines          6077     6117      +40     
  Branches        840      855      +15     
============================================
+ Hits           3703     3723      +20     
- Misses         1901     1914      +13     
- Partials        473      480       +7     

Files with missing lines	Coverage Δ
...ground/ui/datacollection/DataCollectionFragment.kt	57.97% <100.00%> (ø)
...tion/tasks/location/CaptureLocationTaskFragment.kt	96.00% <100.00%> (-0.30%)	⬇️
.../datacollection/tasks/point/DropPinTaskFragment.kt	54.76% <100.00%> (+1.10%)	⬆️
...tacollection/tasks/polygon/DrawAreaTaskFragment.kt	58.82% <100.00%> (ø)
...tacollection/tasks/point/DropPinTaskMapFragment.kt	52.00% <55.55%> (+2.00%)	⬆️
...ollection/tasks/polygon/DrawAreaTaskMapFragment.kt	41.93% <52.63%> (+8.60%)	⬆️
...n/tasks/location/CaptureLocationTaskMapFragment.kt	56.66% <50.00%> (-10.00%)	⬇️

[gino-m]

@anandwana001 Could you please do the same for other TaskFragments, either in this PR or in a follow-up?

[shobhitagarwal1612]

Please cleanup the debug logs.

[anandwana001]

We do need this, so that we can get in production, right?

[sufyanAbbasi]

Maybe also add the new method, FirebaseCrashLogger().logException(t) here if it's safe to do so, and in the other log areas?

And one more thing, by catching the error without rethrowing it, the app will be in a weird state where no viewModel is set and we skip everything that is in the isInitialized check. But these are critical for the functionality of the app, like updating map pins or capturing the location.

My question is, if the app finds itself in an impossible to recover state like this, should we log the exception and try to continue, or should we go back to the home screen and show and error to the user "An unexpected error has occurred. Please try again or contact your survey organizer"? I'm not sure how to do that off the top of my head, so maybe just navigate to the home screen after logging. Also true for below.

[gino-m]

It's better that the app crashes than remains in an invalid state. But if we know that that can happen, we should try to fix the root cause rather than eating the exception.

[gino-m]

Also, @anandwana001 messages logged with Timber.e() aren't captured anywhere in prod. If you need to communicate critical debug info via Crashlytics, the method @sufyanAbbasi mentioned should work.

[anandwana001]

Isn't it a regular error handling where we pass arguments to a new fragment and use it?
Not in a weird app state because this is something required to do.
yes, we can use FirebaseCrashLogger().logException(t)

[anandwana001]

It's better that the app crashes than remains in an invalid state. But if we know that that can happen, we should try to fix the root cause rather than eating the exception.

This is more of a workaround we can do and user can just do a back and forth and keep continuing the data collection, no more crashes.

Later, as a core solution, we can work on the abstract layer and the ViewModel and support the orientation change to work things better
@gino-m

[gino-m]

Isn't it a regular error handling where we pass arguments to a new fragment and use it?

Not sure what you mean about passing arguments to a new fragment is a regular error handling.. Can you rephrase?

Not in a weird app state because this is something required to do.

It's generally considered bad practice to catch Throwable without managing potential consequences. The app will be in an invalid state if the view model couldn't be set since the view requires it to function, no?

[sufyanAbbasi]

Thank you so much! I'm not Android expert enough to be able to verify the correctness of this, at least not without pulling your PR and giving it a test, but I had a bigger question about error handling here, since swallowing the error means the app is in an even weirder state.

@gino-m, do you have any thoughts about how to handle these irrecoverable states without crashing?

[sufyanAbbasi]

Maybe also add the new method, FirebaseCrashLogger().logException(t) here if it's safe to do so, and in the other log areas?

And one more thing, by catching the error without rethrowing it, the app will be in a weird state where no viewModel is set and we skip everything that is in the isInitialized check. But these are critical for the functionality of the app, like updating map pins or capturing the location.

My question is, if the app finds itself in an impossible to recover state like this, should we log the exception and try to continue, or should we go back to the home screen and show and error to the user "An unexpected error has occurred. Please try again or contact your survey organizer"? I'm not sure how to do that off the top of my head, so maybe just navigate to the home screen after logging. Also true for below.

[gino-m]

Swallowing the invalid state here as well will make the issue impossible to identify in prod and even difficult to find locally.

[anandwana001]

solved in PR #2761 (review)