Get log types from /farm.json

[jgaehring]

Right now we have log types hardcoded (in a couple places, actually), which is far from ideal. The log types are already available from the server at the /farm.json endpoint. So I've implemented a simple way of retrieving them from the server and peristing them in localStorage: 8e8c73a.

However, the main hindrance to merging this in is that certain log types have restrictions and requirements for certain log fields, like quantities, and we don't yet have logic for enforcing all of those. The result is that logs of those types get rejected with a 406 error from the server.

Ideally we would be able to get metadata for each log type regarding its fields. For instance whether the field was required or not, whether it was a string, int or other type, and possibly a default value. Ultimately, such metadata could be used for form validation in Field Kit; that would be a really nice solution, because it would guarantee that FK's form validation logic was always in agreement with what the server required.

Although it shouldn't be too hard, this will still take a little more work in both the server and client to make it possible.

@mstenta , my question for you is, do we want to wait until we can fully implement this before merging those initial changes in? Or do we simply want to allow for server errors to be thrown and presented to the user if there was a conflict? I guess I'm leaning towards holding off for now, because right now it's impossible to not include a quantity field on every log, which means certain log types can never be synced, unless we write some special logic to exclude those fields. That wouldn't be too hard, but I feel like it's extra work that won't be necessary if we just wait until we can implement this the right way.

[mstenta]

Good question. I agree that we ultimately need field info to be available via the API. It probably won't take much on my end to add that - just need to carve out some time for it.

I'm kinda OK with merging in your commit now, though - even if that leads to errors in some cases. It's becoming clear that the lack of log types in the app is causing confusion (like with Walt in chat today unable to sync a Soil Test log).

Do the error messages reveal the field(s) that are problematic so they can be fixed?

[jgaehring]

Do the error messages reveal the field(s) that are problematic so they can be fixed?

I think so. It's been a long journey getting the error handling for sync events to work the way we want, but I believe it's at the point now that we always get a good error message back from the server in these types of events, and that that error is displayed to the user. Might not be the most obvious message, but enough to indicate where the problem is, perhaps with a little extra assistance from the Riot channel or forum.

The only issue, which will require some attention, is that with certain issues there is no way to the user to fix it. For instance, with quantities: there will always be a quantity field included in the POST request, even if its empty, and that throws an error. There's no way for the user to eliminate that field from the request. Which comes back to my original comment:

[...] unless we write some special logic to exclude those fields. That wouldn't be too hard, but I feel like it's extra work that won't be necessary if we just wait until we can implement this the right way.

So yea, do we think that's worth it, even if it's just a stop-gap initiative?

[mstenta]

I believe it's at the point now that we always get a good error message back from the server in these types of events

That's good.

For instance, with quantities: there will always be a quantity field included in the POST request, even if its empty, and that throws an error.

Could the push code be optimized to only include fields that have data in them?

That might be good overall. Reduce request sizes in general a bit.

unless we write some special logic to exclude those fields

Maybe that's what you meant by this? I wouldn't want to add logic that hard-codes knowledge of what fields are available on each log type... because that would end up getting ripped out. But if there was some general logic to only include non-empty values, I think that would work.

do we think that's worth it

I think these would all be steps toward the greater goal, and would serve as a stop-gap in the meantime, without adding code that we're going to just rip out. Does that seem correct to you?

[jgaehring]

I think these would all be steps toward the greater goal, and would serve as a stop-gap in the meantime, without adding code that we're going to just rip out. Does that seem correct to you?

Yea, I think you make a good point about not including any fields that have no data in them (might need to be some exceptions to that... like, would the name field need to be an empty string at least?). That would definitely be an improvement worth keeping, and wouldn't be a hack, like I feel it would be to otherwise hardcode info about specific types of logs (we're already doing that a little, but hopefully that can be removed once we can get more data about log types from the server).

[mstenta]

Cool yea that seems like a good step to take then.

might need to be some exceptions to that... like, would the name field need to be an empty string at least?

Right... that would be an exception. We don't want to send a blank name. I think we decided that when we were talking about auto-naming logs (we don't want to let the server auto-name them, because we aren't able to easily retrieve that from the server automatically/immediately).

[jgaehring]

I'm going to add this to #141 so I can tackle the issue before 1.0.0 release. I think along with better error-handling techniques, in the event there are edge cases we're not considering, this should be a viable solution.

[jgaehring]

I think we decided that when we were talking about auto-naming logs (we don't want to let the server auto-name them, because we aren't able to easily retrieve that from the server automatically/immediately).

Oh right, forgot about that. Yea, we'll probably want to figure that out too.