Make UI more defensive to null/empty values

[mstenta]

Starting this issue after discussion with @jgaehring in the farmOS dev call 5/12/20.

[jgaehring]

Right now our components are littered with null checks, in a haphazard attempt to defend against potential null and undefined values.

This happens in the template itself:

https://github.com/farmOS/farmOS-client/blob/425bae074da511268dd6684e4839d1b3ca2306f7/src/field-modules/my-logs/components/EditLog.vue#L124-L131

and in computed values:

https://github.com/farmOS/farmOS-client/blob/425bae074da511268dd6684e4839d1b3ca2306f7/src/field-modules/my-logs/components/EditLog.vue#L954-L969

Yet we're still getting reports from users who get into a broken state, where the screen goes blank and they can no longer use the app. So this is clearly not covering all our bases, and it's a major problem that needs to be addressed (before 1.0.0, I believe, so I'm adding this to that milestone).

I think there are two possible ways to remedy this.

The first is to clean up nullish values right before they get passed down to the field modules as props. We're already doing some processing to remove metadata before passing them down, so this could just be included as part of that process:

https://github.com/farmOS/farmOS-client/blob/425bae074da511268dd6684e4839d1b3ca2306f7/src/core/App.vue#L76-L88

https://github.com/farmOS/farmOS-client/blob/425bae074da511268dd6684e4839d1b3ca2306f7/src/core/App.vue#L107-L114

The second possibility, and this seems like the most comprehensive way to approach this, would be to go right to the source, and guarantee that when all logs are created and updated, they receive only valid property values. This means going into farmLog and replacing the default_value's, which are crap, with more reliable default values. We had something like this in the old makeLog implementation, but we had to hard code a lot of default values by hand, which won't suffice now that we're supporting all logs the server supports, and won't know what those are until runtime. Instead we can generate our own defaults based on the each field's type, such as entityreference, text_long, taxonomy_term_reference, etc. Ideally, we would could get this type data from /farm.json, but I think for now it should be easy enough to provide a simple lookup table, something like:

const fieldDefaults = {
  entityreference: [],
  text_long: '',
  taxonomy_term_reference: [],
  // etc
}

Then instead of this:

https://github.com/farmOS/farmOS-client/blob/425bae074da511268dd6684e4839d1b3ca2306f7/src/utils/farmLog.js#L16-L19

we could do something like this:

 const entries = Object.entries(schema).map(([key, { type: fieldType }]) => { 
   const data = props[key] || fieldDefaults[fieldType]; 
   return [key, { changed, data, conflicts: [] }]; 
 });

[mstenta]

Thanks for all this @jgaehring !

I agree we should prioritize this ahead of the v1.0.0. And the "comprehensive approach" sounds like the right way to fix it. Per our conversation, I think we can strike a balance by hard-coding some knowledge about field types rather than individual fields (which Field Kit can't realistically know before connecting to the server).

Using the type information that currently exists in /farm.json will be a good start. And perhaps we can go ahead and remove the default_value property from /farm.json since it has proven to be untrustworthy (as I had feared).

The only potential hurdle I foresee is: some fields that are otherwise the same "type" might still require slightly different default values, depending on whether they are "single value" or "multi value".

I ran into this recently when I was updating the API docs to describe the new ability to auto-create terms (farmOS/farmOS.org@dab40a5).

In that example (a Planting asset being created with Crop and Season terms), both Crop and Season are taxonomy_term_reference type fields, but Crop expects an array of objects (default = []), and Season expects a single object (default = {}).

So... we might need to add something like "multiple": "true" to /farm.json for each field, and some logic to the Field Kit default value code, to handle that bifurcation.

(On the "Season" specifically... I'm actually thinking about making that a multi-value field anyway... but the same consideration applies to other fields as well.)

[tool172]

That solution works. Any thoughts about making a deserizlizer in the class so it doesnt matter when mapping is done?

…

On Wed, May 13, 2020, 06:32 Michael Stenta ***@***.***> wrote: Thanks for all this @jgaehring <https://github.com/jgaehring> ! I agree we should prioritize this ahead of the v1.0.0. And the "comprehensive approach" sounds like the right way to fix it. Per our conversation, I think we can strike a balance by hard-coding some knowledge about field *types* rather than individual fields (which Field Kit can't realistically know before connecting to the server). Using the type information that currently exists in /farm.json will be a good start. And perhaps we can go ahead and remove the default_value property from /farm.json since it has proven to be untrustworthy (as I had feared). The only potential hurdle I foresee is: some fields that are otherwise the same "type" might still require slightly different default values, depending on whether they are "single value" or "multi value". I ran into this recently when I was updating the API docs to describe the new ability to auto-create terms ***@***.*** <farmOS/farmOS.org@dab40a5> ). In that example (a Planting asset being created with Crop and Season terms), both Crop and Season are taxonomy_term_reference type fields, but Crop expects an array of objects (default = []), and Season expects a single object (default = {}). So... we might need to add something like "multiple": "true" to /farm.json for each field, and some logic to the Field Kit default value code, to handle that bifurcation. (On the "Season" specifically... I'm actually thinking about making that a multi-value field anyway... but the same consideration applies to other fields as well.) — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#346 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/APERD7DGMSEAVQDL4CXXLELRRKAOVANCNFSM4M64SQQA> .

[jgaehring]

Hi @tool172, I'm not sure exactly what class you're referring to (we don't use classes in Field Kit), but deserialization is outside the scope of this issue. Can you please open a separate issue here if it concerns Field Kit specifically, or if this is more concerned with the farmOS REST API, you can open an issue here. Please make sure to provide more details so we can address your concern. Thanks!

[mstenta]

I created a new issue for this in the server: https://www.drupal.org/project/farm/issues/3136210

And a first pass at a commit: mstenta/farmOS@1916fd8

@jgaehring is going to look into adapting the Field Kit code to use this new data_schema property instead of default_value.

[mstenta]

@tool172: Maybe this answers your question... we decided to do the "mapping" in the server itself, and provide better default values in /farm.json that Field Kit (and other clients) can consume, so they don't need to have the same mappings hard-coded internally.

[jgaehring]

Following up to document some of the thought process behind PR #347...

Pursuant of our our above discussion, @mstenta and I decided to replace the default_value property on each field with a more purpose-built data_schema property, which could better describe the data structure of each field. The idea is that this may be useful for other means of form validation, but can at the very least be used to derive the default values needed in farmLog, when creating, updating, and syncing logs when the server.

This way, we hope to prevent nullish values from propagating to the UI, as well as prevent invalid field data from being sent to (and rejected by) the server.

Generally speaking, the syntax for these data_schema descriptors represent the "empty set" for their corresponding data type. Specifically:

• Primitive values
  • "" indicates the String type
  • null indicates the Number and Boolean types*
• Collections, which can be nested recursively
  • an Array type is represented by a [ * ], where * represents the primitive or collection value the array contains; for example, [ '' ] for an array of strings, or [ { id: null } ] for an array of objects, respectively
  • The Object type will be represented by an object with all required properties for that data type, where the value of each property is represented by the descriptor for that value's type; for example, { id: null } or { area: [ { id: null } ] }**

There are a lot of other issues we still need to address, and we also need to see how the PR's deploy preview performs, but hopefully this can serve as a starting point for how we pursue this further.

---

* = We've discussed the possibility of disambiguating these values so that Number and Boolean are not both represented by null, but for our initial needs of being able to generate valid default values, this does not seem strictly necessary. If we later chose to expand the use scope of what data_schema can be used for, like more thorough form validation, then we probably want to adapt a standard more inline with JSON schema (see farmOS/farmOS#243). For instance, we will probably want to forgo the concept of the "empty set" descriptor and instead adopt an object syntax, so that

{
  // ...
  name: {
    // ...
    data_schema: "",
  },
  asset: {
    // ...
    data_schema: [ { id: null } ],
  }
}

becomes

{
  // ...
  name: {
    // ...
    data_schema: {
      type: 'string',
    },
  },
  asset: {
    // ...
    data_schema: {
      type: 'array',
      items: {
        type: 'object',
        properties: { id: { type: 'number' } }
      }
    },
  }
}

The latter syntax would make us compatible with the JSON Schema spec, at least on the level of each individual data_schema property. This is one way in which we could progress gradually towards the adoption of that standard, prior to migrating to Drupal 8, which is a prerequisite for the JSON:API-schema module.

---

** = @mstenta, I'm not sure if this last example is consistent with what you've implemented, curious your thoughts on that point.

[mstenta]

The Object type will be represented by an object with all required properties for that data type, where the value of each property is represented by the descriptor for that value's type; for example, { id: null } or { area: [ { id: null } ] }**

** = @mstenta, I'm not sure if this last example is consistent with what you've implemented, curious your thoughts on that point.

Yes, I think that summarizes it.

To clarify the emphasis on "required": it will include the pieces of information that are needed by the server in order to save the data. In the case of entityreference, taxonomy_term_reference, image, file types the only "required" property to send to the server is the id of the entity being referenced (whether that be an asset, log, term, file, etc). If you request the same data back from the server, you'll notice that it also includes uri and resource properties, alongside id. These are not required for sending data to the server.

But, to be be clear, that doesn't mean the overall field is "required" on the record. eg: The "assets" field is optional, but if you want reference an asset you must supply the id in that data schema structure. If that makes sense? :-)

Is that what you meant? Hopefully I understood

[mstenta]

General update on this: we have a few commits on PR #347, and a Netlify deployment for testing it at: https://deploy-preview-347--dreamy-tesla-8069e3.netlify.app

It seems to be working well, so far. I'm excited to get this merged, but also wary of introducing new bugs.

I'm also trying to decide if this should block the release of farmOS 7.x-1.4. I have this one commit that adds the data_schema property, which we are testing against with this PR. But as we've been testing it has required a few small tweaks, so I'm hesitant to merge it and tag 7.x-1.4 in case we then discover something else that needs to be fixed soon after.

[mstenta]

FYI here is the related issue in the farmOS queue: https://www.drupal.org/project/farm/issues/3136210

[jgaehring]

To clarify the emphasis on "required": it will include the pieces of information that are needed by the server in order to save the data. [...] But, to be be clear, that doesn't mean the overall field is "required" on the record. eg: The "assets" field is optional, but if you want reference an asset you must supply the id in that data schema structure. If that makes sense? :-)

Yep, makes sens to me!

Is that what you meant? Hopefully I understood

I guess I was specifically referring to this example: { area: [ { id: null } ] }. I didn't have a chance to check what you had implemented to see if it was consistent with how I described it, but looking at it now, it does appear to be:

https://github.com/farmOS/farmOS-client/blob/54da7439066b117a5f286f952691f5e326e03a4d/src/core/store/defaultLogTypes.js#L10

I'm also trying to decide if this should block the release of farmOS 7.x-1.4. I have this one commit that adds the data_schema property, which we are testing against with this PR. But as we've been testing it has required a few small tweaks, so I'm hesitant to merge it and tag 7.x-1.4 in case we then discover something else that needs to be fixed soon after.

I can't speak to the Drupal side of things, but the FK side seems pretty solid. I think it's to our advantage that the changes are all to the defaults, which will only be used to when no other data is provided. So in essence, the primary case we have to test is also the easiest: create a log, input nothing, sync it. I think the only way to be more thorough is to go through all the log types and run that test, since the data_schema property is tied to the individual fields for every log type.

[mstenta]

I can't speak to the Drupal side of things, but the FK side seems pretty solid. I think it's to our advantage that the changes are all to the defaults, which will only be used to when no other data is provided.

Great! Yea I feel confident in the farmOS side of things too, now that we've tested most of the main pieces. If we find any minor bugs we can fix them as they come up.

So in essence, the primary case we have to test is also the easiest: create a log, input nothing, sync it. I think the only way to be more thorough is to go through all the log types and run that test, since the data_schema property is tied to the individual fields for every log type.

I did test these actions for all log types (I think I did them all), so it feels pretty solid to me too.

Does it make sense for me to reach out to the users who were experiencing the white screen issue and see if I can have them test the Netlify build to see if they still experience it?

I'm also trying to decide if this should block the release of farmOS 7.x-1.4.

With all that in mind, I think I will merge the farmOS commit and move forward with the 7.x-1.4 release. But I will leave the default_value in place for this release and deprecate it to be removed in 7.x-1.5.

[jgaehring]

Does it make sense for me to reach out to the users who were experiencing the white screen issue and see if I can have them test the Netlify build to see if they still experience it?

Yea, I think that makes sense. Seems like a fairly low risk and low time commitment to ask of them: it should be pretty quick to just open the link, login, try hitting sync and see what happens; if it works, great! then we can probably merge into the develop branch and start preparing a release; if it doesn't work, then we go back to the drawing board, and they can just go back to work, without having to worry about uninstalling/reinstalling or anything like that (ah the magic of PWA's!).

Oh, except one thing I just realized: we'll have to make sure their server's farm_access settings are pointing to that Netlify URL, which will also mean, for a short time, anyone using farmos.app with their server will not be able to sync. If you can coordinate it with them, I'm sure you could make that window of time really small, so hopefully it doesn't interfere with normal operations, but yea, will take a little more coordination.