farmOS / field-kit

A modular, offline-first companion app to farmOS.
https://farmOS.org
GNU General Public License v3.0
60 stars 39 forks source link

Birth Records are rejected when trying to sync. #448

Closed jgaehring closed 2 years ago

jgaehring commented 3 years ago

I've realized that creating birth records in Field Kit results in a broken state, b/c we don't have the UI component for editing the mother field, but we're generating a blank birth record log with mother: { id: null }, which is getting rejected by the server.

The farm_birth resource describes the mother field as follows:

{
  "label": "Mother",
  "type": "entityreference",
  "required": 0,
  "data_schema": {
    "id": null
  }
}

As @mstenta has pointed out, the data_schema is the same as for any field of type entityreference, as defined here:

https://github.com/farmOS/farmOS/blob/da504a1be74f085007f294329c72a368a3029358/modules/farm/farm_ui/farm_ui.module#L58

Yet we're not seeing this issue for, say, the asset field of a log, although, perhaps crucially, the asset field accepts an array of objects, as descibed in the same farm_birth resource:

{
  "label": "Children",
  "type": "entityreference",
  "required": 0,
  "data_schema": [
    {
      "id": null
    }
  ]
}

This is a significant bug, but no urgent, imo. So hopefully this documents the issue thoroughly, and if there's an easy fix, great, but otherwise perhaps it depends on farmOS/farmOS#243.

Oh, and for reference, here's where I originally described how the data_schema field should work: https://github.com/farmOS/farmOS-client/issues/346#issuecomment-628300160.

mstenta commented 3 years ago

Spoke with @jgaehring about this today - I'm going to move it to the Field Kit issue queue.

It shouldn't be an issue in farmOS/Field Kit 2.x, so we can probably close is as "wontfix" for 1.x. And maybe we can make a 1.x "known issues" list somewhere for reference. But I'll leave it open for now...

jgaehring commented 3 years ago

To be clear, this primarily becomes an issue where the user tries to create a birth record in Field Kit then sync it, whereat the server rejects it, with a 406 IIRC, b/c it requires that mother field. I'm going to rename this so it's a little more obvious for folks who might encounter this issue in the future

Although come to think of it, this could be an issue for 2.x as well, for the very same reason as the equipment issue (#449). TL;DR, we're currently using a stub for JSON Schema evaluation in farmOS.js 2.x, which means we can only use entities' base fields. The mother field on logs is not a base field, so as long as we're using that stub this issue will probably persist for 2.x as well.

I may expand #449 then to address both those issues for 2.x, but will leave this issue dedicated to the 1.x issue, since 1.x will be stuck with these schema issues no matter what and will require a separate workaround, if we choose to fix this at all.

mstenta commented 3 years ago

Although come to think of it, this could be an issue for 2.x as well ... The mother field on logs is not a base field, so as long as we're using that stub this issue will probably persist for 2.x as well.

Actually this won't be an issue because mother is a base field of birth logs, so that field is guaranteed to exist, and can be hard-coded in Field Kit.

The difference with "Equipment" is that it is added to all logs... and only if the equipment module is enabled.

mstenta commented 3 years ago

Also meant to mention: the mother field will not be required on Birth logs in 2.x like they were in 1.x (I think that was sort of an arbitrary limitation in 1.x) - so pushing a birth log without a mother won't cause an error...

jgaehring commented 2 years ago

As with #449, this has been resolved since the schema stubs were replaced (farmOS/farmOS.js#26), along with the fact that the mother field is no longer required, as Mike mentioned in the previous comment.