Add support for pulldata with local entities #6451
Conversation
seadowg
force-pushed
the
entities-pulldata
branch
2 times, most recently
from
7eb396e
to
bed7c86
Compare
seadowg
force-pushed
the
entities-pulldata
branch
from
5beb718
to
78b30c6
Compare
| val filterValue = XPathFuncExpr.toString(args[3]) | ||
|
|
||
| return if (instanceAdapter.supportsInstance(instanceId)) { | ||
| instanceAdapter.queryEq(instanceId, filterChild, filterValue)!!.firstOrNull() |
There was a problem hiding this comment.
It looks like queryEq can't return null values so we could update that function by removing ? from the returned type and then get rid of those !!.
| @@ -33,62 +33,82 @@ class LocalEntitiesInstanceAdapter(private val entitiesRepository: EntitiesRepos | |||
| } | |||
| } else { | |||
| entitiesRepository.getEntities(instanceId).map { entity -> | |||
| convertToElement(entity, false) | |||
| convertToElement(entity) | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| fun queryEq(instanceId: String, child: String, value: String): List<TreeElement>? { | |||
| return when { | |||
There was a problem hiding this comment.
You can now simplify this by adding child as a subject of when:
return when (child) {
EntityItemElement.ID ->...
|
|
||
| override fun eval(args: Array<Any>, ec: EvaluationContext): Any { | ||
| val instanceId = XPathFuncExpr.toString(args[0]) | ||
| val child = XPathFuncExpr.toString(args[1]) |
There was a problem hiding this comment.
I think you can move extracting child, filterChild and filterValue down and call only if instanceAdapter.supportsInstance(instanceId) returns true.
| @@ -23,7 +23,7 @@ class LocalEntitiesInstanceAdapter(private val entitiesRepository: EntitiesRepos | |||
|
|
|||
| 0.until(count).map { | |||
| if (it == 0) { | |||
| convertToElement(first, true) | |||
| convertToElement(first) | |||
| } else { | |||
| TreeElement("item", it, true) | |||
There was a problem hiding this comment.
Now I wonder if we need that isPartial flag at all... such elements should have at least label and value right so if they don't have it we could treat them as partial and try to populate.
| @@ -23,7 +23,7 @@ class LocalEntitiesInstanceAdapter(private val entitiesRepository: EntitiesRepos | |||
|
|
|||
| 0.until(count).map { | |||
| if (it == 0) { | |||
| convertToElement(first, true) | |||
There was a problem hiding this comment.
Now I wonder if we need that isPartial flag at all... such elements should have at least label and value right so if they don't have it we could treat them as partial and try to populate.
It is set to true in only one place.
|
|
||
| val instanceAdapter = LocalEntitiesInstanceAdapter(entitiesRepository) | ||
| val results = instanceAdapter.queryEq("things", EntityItemElement.LABEL, "Thing 2") | ||
| assertThat(results!!.size, equalTo(1)) |
There was a problem hiding this comment.
!! is unnecessary here. The same in other tests.
There was a problem hiding this comment.
I've taken a quick look through and discussed briefly with @grzesiek2010 who says his comments could be addressed in follow-up. I'm going to merge this now so it can be part of our next beta to maximize time to get user feedback.
Closes #6443
Blocked by getodk/javarosa#802Why is this the best possible solution? Were any other approaches considered?
I'd initially wanted to implement this by just executing a dynamically built XPath expression, but this proved tricky as JavaRosa currently doesn't attach secondary instances that aren't referenced by an
instanceexpression. This would mean that a form that contained only apulldatareferencing an entity list wouldn't work (as the external instance would never be parsed and attached to the main instance). We'll look to move to the originally intended implementation in the next release as part of #6454.Instead, I've ended up using the
LocalEntityInstanceAdapterin a similar way toLocalEntityFilterStrategywith modifications so that it now supports querying any child of an entity item (likelabelfor example). This new support doesn't add anything significant in the way of optimisation however: queries for children likelabelwork by just loading every entity in the list from the DB and then performing an in-mem filter. This limits the amount of work we do to just supportpulldata.How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
The big risks here will be to all
pulldataforms (including ones that use with entities obviously) and to entity filters in forms in general.One thing to note: I've not extended the instance "normalising" that
pulldatadoes to entity lists. For normal secondary instances,pulldatawould let you use the wrong case for the name or add.csvto the end. We'll discuss if we want to maintain this (currently undocumented) behaviour separately.Before submitting this PR, please make sure you have:
./gradlew connectedAndroidTest(or./gradlew testLab) and confirmed all checks still passDateFormatsTest