v5 repeated values compatibility with brapi field sync

[chaneylc]

fix #548
fix #682
added repeated values to brapi sync feature

Description

Provide a summary of your changes including motivation and context.
If these changes fix a bug or resolves a feature request, be sure to link to that issue.

Type of change

What type of changes does your code introduce? Put an x in boxes that apply.

• CHANGE (fix or feature that would cause existing functionality to not work as expected)
• FEATURE (non-breaking change which adds functionality)
• BUGFIX (non-breaking change which fixes an issue)
• ENHANCEMENT (non-breaking change which improves existing functionality)
• NONE (if none of the other choices apply. Example, tooling, build system, CI, docs, etc.)

Checklist:

• I have performed a self-review of my code
• I have commented my code, particularly in hard-to-understand areas
• I have updated relevant documentation

Changelog entry

Please add a one-line changelog entry below. This will be copied to the changelog file during the release process.

[bellerbrock]

Hey @chaneylc, I've done some testing with this branch and a BIMS field with repeated measurements. The repeats are imported now via brapi sync, but the rep numbering doesn't work for me.

Thinking about this a bit more, does it even make sense to store rep number explicitly when we have timestamp? There are a lot of ways it could end up wrong if values are deleted or collected across multiple devices. I think it'd be simpler, cleaner, and mroe robust just to get rid of the rep column and instead make sure we store timestamp with all synced observations. Then rep can just be calculated on export after sorting by timestamp. What do you think?

Another observation (maybe out of scope here though) is that repeated measures aren't visible on the collect screen when the feature is disabled. But are visible in the datagrid and the field detail count. That gets kind of confusing, what about leaving them visible on the collect screen too and just disabling /hiding the button to add additional repeats?

[bellerbrock]

@chaneylc looking at the getExportDbData function the export uses, should be straightforward to calculate rep in the SQL. Just adding RANK() OVER (PARTITION BY obs.observation_unit_id, obs.observation_variable_name ORDER BY obs.observation_time_stamp) AS calculated_rep to the select clause should do the trick.

[chaneylc]

@chaneylc looking at the getExportDbData function the export uses, should be straightforward to calculate rep in the SQL. Just adding RANK() OVER (PARTITION BY obs.observation_unit_id, obs.observation_variable_name ORDER BY obs.observation_time_stamp) AS calculated_rep to the select clause should do the trick.

That is definitely a good use of window functions but we can't do it this way. API 30 is when sqlite 3.28 was introduced which was the first version with window functions, we still have a minimum of 21. I'm guessing even android studio's sqlite version is below this for API 26 compatibility.

[bellerbrock]

@chaneylc Gotcha, that would explain some of the syntax errors I kept getting when trying the window function approach in the observationDao (though for some reason it works fine in the observationUnitPropertyDao).

Its not as concise, but a subquery should also do the trick. But rather than repeat it everywhere the observations table is accessed, what do you think about making an ObservationsDetail View? I see a TODO here suggestiing one here, I could add it and have rep calculated using a subquery.

[chaneylc]

@chaneylc Gotcha, that would explain some of the syntax errors I kept getting when trying the window function approach in the observationDao (though for some reason it works fine in the observationUnitPropertyDao).

Its not as concise, but a subquery should also do the trick. But rather than repeat it everywhere the observations table is accessed, what do you think about making an ObservationsDetail View? I see a TODO here suggestiing one here, I could add it and have rep calculated using a subquery.

Yeah that sounds ok, it would be nice to combine that query and getHostImageObservations too. These were originally meant to return brapi observations, so be careful there with the data source null check to make this robust enough for local repeated measures.

[bellerbrock]

@chaneylc Ok, working on the view approach now. Looks like the existing views are defined in Migrator.kt then its imported into the DataHelper and Daos for view creation and view name retrieval, respectively. But the Migrator seems written to only be used for v8 to v9 db upgrade. Do you think I should refactor it so it can be used for handling additional views, or would you suggest a different approach?

[chaneylc]

@chaneylc Ok, working on the view approach now. Looks like the existing views are defined in Migrator.kt then its imported into the DataHelper and Daos for view creation and view name retrieval, respectively. But the Migrator seems written to only be used for v8 to v9 db upgrade. Do you think I should refactor it so it can be used for handling additional views, or would you suggest a different approach?

Instead of refactoring I think it would be best to encapsulate your view definition, and any functions necessary to maintain it in its own DAO class.

[bellerbrock]

@chaneylc There were a lot of messy downstream effects when using the view, so I've thought better of it, reverted those commits, and have gone with calculating rep for the brapi obs based on the existing obs during the sync save process. Let me know what you think of this solution.