Replace suggestLoiTaskType with DataCollectionStrategy #2108

gino-m · 2023-12-11T23:01:42Z

Fixes #2097

gino-m · 2023-12-13T01:07:44Z

This is now working, modulo:

Will mark "Ready for review" once those are fixed and/or out for review. @rfontanarosa FYI.

codecov-commenter · 2023-12-13T01:20:56Z

Codecov Report

Attention: 28 lines in your changes are missing coverage. Please review.

Comparison is base (f05948f) 52.01% compared to head (0af709f) 52.11%.

Files	Patch %	Lines
...nd/domain/usecases/submission/SubmitDataUseCase.kt	30.00%	6 Missing and 1 partial ⚠️
...persistence/remote/firebase/schema/JobConverter.kt	0.00%	5 Missing ⚠️
...ersistence/remote/firebase/schema/TaskConverter.kt	0.00%	3 Missing ⚠️
...in/java/com/google/android/ground/model/job/Job.kt	71.42%	2 Missing ⚠️
...istence/remote/firebase/schema/TaskNestedObject.kt	0.00%	2 Missing ⚠️
...ground/persistence/sync/LocalMutationSyncWorker.kt	66.66%	2 Missing ⚠️
...round/ui/datacollection/DataCollectionViewModel.kt	33.33%	0 Missing and 2 partials ⚠️
...d/persistence/local/room/converter/ConverterExt.kt	90.00%	0 Missing and 1 partial ⚠️
...sistence/remote/firebase/schema/JobNestedObject.kt	0.00%	1 Missing ⚠️
...ome/mapcontainer/HomeScreenMapContainerFragment.kt	0.00%	0 Missing and 1 partial ⚠️
... and 2 more

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #2108      +/-   ##
============================================
+ Coverage     52.01%   52.11%   +0.09%     
- Complexity     1213     1215       +2     
============================================
  Files           311      311              
  Lines          6102     6104       +2     
  Branches        645      644       -1     
============================================
+ Hits           3174     3181       +7     
+ Misses         2581     2578       -3     
+ Partials        347      345       -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…o gino-m/2097/1 # Conflicts: # ground/src/main/java/com/google/android/ground/persistence/sync/LocalMutationSyncWorker.kt

gino-m · 2023-12-15T20:08:15Z

@scolsen PTAL

@rfontanarosa FYI

scolsen

couple of small comments but lgtm

scolsen · 2023-12-15T21:05:33Z

ground/src/main/java/com/google/android/ground/model/job/Job.kt

 data class Job(
  val id: String,
  val style: Style? = null,
  val name: String? = null,
  val tasks: Map<String, Task> = mapOf(),
-  val suggestLoiTaskType: Task.Type? = null,
+  val strategy: DataCollectionStrategy = DataCollectionStrategy.MIXED


Should this have a default? Don't we need this to be defined by survey organizers? Shouldn't it be a required argument, and shouldn't we error out if it's not available/provided?

The default value was mainly for tests, but that's probably the wrong reason. Updating now.

Doing this turned out to be more disruptive than I expected; basically we end up needing to defined it in all tests even where it's not relevant. Ideally we'd have a custom constructor with default values for tests, but since we don't have yet that we can apply UNKNOWN as a default based on your next suggestion

scolsen · 2023-12-15T21:06:16Z

ground/src/main/java/com/google/android/ground/model/job/Job.kt

 ) {
+  enum class DataCollectionStrategy {
+    PREDEFINED,


So far, it's been helpful to have UNKNOWN for cases in when the web app evolves before the android app/field names change. Not sure if you want to add one here.

I think UNKNOWN is useful for serialized representations like protos, but for model objects less so; if the strategy is invalid in remote or local data store, by the time to deserialize we should either have chosen a fall-back value, or thrown an error.

scolsen · 2023-12-15T21:09:42Z

ground/src/main/java/com/google/android/ground/persistence/local/room/converter/ConverterExt.kt

@@ -112,7 +113,7 @@ fun JobEntityAndRelations.toModelObject(): Job {
    jobEntity.style?.toModelObject(),
    jobEntity.name,
    taskMap.toPersistentMap(),
-    jobEntity.suggestLoiTaskType?.let { Task.Type.valueOf(it) }
+    jobEntity.strategy.let { DataCollectionStrategy.valueOf(it) }


Why not just DataCollectionStrategy.valueOf(jobEntity.strategy)?

Or. like other entities, I might expect to see jobEntity.strategy.toModelObject()

Why not just DataCollectionStrategy.valueOf(jobEntity.strategy)?

Because strategy might be null, and Enum.valueOf() doesn't accept nulls

Or. like other entities, I might expect to see jobEntity.strategy.toModelObject()

Because in jobEntity strategy is a String, and defining fun String.?toModelObject() would be too broad.

scolsen · 2023-12-15T21:11:32Z

...rc/main/java/com/google/android/ground/persistence/remote/firebase/schema/JobNestedObject.kt

@@ -24,5 +25,5 @@ data class JobNestedObject(
  val defaultStyle: StyleNestedObject? = null,
  val name: String? = null,
  val tasks: Map<String, TaskNestedObject>? = null,
-  val suggestLoiTaskType: String? = null
+  val strategy: String = MIXED.toString()


I worry that this default value and the default value for the Job model object could get out of sync. I think removing a default from the model might be one way to avoid that.

Good idea. Done!

scolsen · 2023-12-15T21:15:54Z

ground/src/main/java/com/google/android/ground/ui/datacollection/DataCollectionViewModel.kt

-    addAll(job.tasksSorted)
-  }
+  // LOI creation task is included only on "new data collection site" flow..
+  val tasks: List<Task> = job.tasksSorted.filter { loiId == null || !it.isAddLoiTask }


I find this filter a little tricky to understand. Seems like we're getting tasks that either have no LOI Id or that are explicitly not add LOI tasks. The latter makes sense given the comment, but I don't understand why we filter for tasks with loiId == null

We're including all tasks when loiId = null. I can rewrite this, lmkwyt.

gino-m and others added 12 commits December 11, 2023 18:01

Replace suggestLoiTaskType with DataCollectionStrategy

6b66109

Minor rename

bf4d256

Serialize new LOIs

22a084b

Minor rename

da834d7

Delete unused code

183ff59

(De)serialize isAddLoiTask

ff78cf0

Remove unused

17ea3c7

ktfmt

8ba3b77

Rename BOTH strategy

409d457

Add debug logging

f05e604

Work around Firestore toObject limitation

9a625cf

Merge branch 'master' into gino-m/2097/1

ff6e479

gino-m mentioned this pull request Dec 13, 2023

"Permission denied" when creating LOI as data collector google/ground-platform#1351

Closed

gino-m and others added 5 commits December 14, 2023 10:51

Tweak logging and error handling

f46f7bc

Reduce log noise

1110cf7

Merge branch 'master' into gino-m/2097/1

c6ead1b

Revert DRAW_AREA rename

b535529

Fix detekt error

bbaf9b9

gino-m marked this pull request as ready for review December 14, 2023 21:36

gino-m added 2 commits December 14, 2023 16:38

Merge branch 'master' of https://github.com/google/ground-android int…

d7c0275

…o gino-m/2097/1 # Conflicts: # ground/src/main/java/com/google/android/ground/persistence/sync/LocalMutationSyncWorker.kt

Remove log noise

9405bcf

gino-m requested a review from scolsen December 15, 2023 20:08

gino-m and others added 2 commits December 15, 2023 15:39

Filter add LOI tasks when appropriate

034c0d8

Merge branch 'master' into gino-m/2097/1

41332a4

scolsen approved these changes Dec 15, 2023

View reviewed changes

gino-m added 2 commits December 15, 2023 16:44

Remove default data collection strategy from Job

fc1c327

Simplify task filter expr

38be4f8

gino-m added 2 commits December 15, 2023 17:21

Restore default value for Job strategy for tests

00f750b

Merge remote-tracking branch 'origin/gino-m/2097/1' into gino-m/2097/1

0af709f

gino-m merged commit 18d518c into master Dec 15, 2023
4 checks passed

gino-m deleted the gino-m/2097/1 branch December 15, 2023 22:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace suggestLoiTaskType with DataCollectionStrategy #2108

Replace suggestLoiTaskType with DataCollectionStrategy #2108

gino-m commented Dec 11, 2023

gino-m commented Dec 13, 2023 •

edited

Loading

codecov-commenter commented Dec 13, 2023 •

edited

Loading

gino-m commented Dec 15, 2023 •

edited

Loading

scolsen left a comment

scolsen Dec 15, 2023

gino-m Dec 15, 2023

gino-m Dec 15, 2023

scolsen Dec 15, 2023

gino-m Dec 15, 2023

scolsen Dec 15, 2023

gino-m Dec 15, 2023

scolsen Dec 15, 2023

gino-m Dec 15, 2023

scolsen Dec 15, 2023

gino-m Dec 15, 2023

Replace suggestLoiTaskType with DataCollectionStrategy #2108

Replace suggestLoiTaskType with DataCollectionStrategy #2108

Conversation

gino-m commented Dec 11, 2023

gino-m commented Dec 13, 2023 • edited Loading

codecov-commenter commented Dec 13, 2023 • edited Loading

Codecov Report

gino-m commented Dec 15, 2023 • edited Loading

scolsen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gino-m commented Dec 13, 2023 •

edited

Loading

codecov-commenter commented Dec 13, 2023 •

edited

Loading

gino-m commented Dec 15, 2023 •

edited

Loading