search

farmOS / field-kit

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

Sync behavior not as expected #313

Closed jgaehring closed 4 years ago

jgaehring commented 4 years ago

From https://github.com/farmOS/farmOS-client/issues/275#issuecomment-590214535:

Sync logic (i.e. which source masters which at sync time): I'm still not sure how this works -i think i had many sync anomalies due to the previous issue, so i'm just starting to test again with my newly cleaned-out log- but based on early experience, i must question your assertion: "If the server has more recent data than what's in Field Kit, then the data in Field Kit will be overwritten." I think that's the right logic to apply, but it's not what i see happening so far. One exception i see is: create and sync an Observation in FK, edit it on desktop (renaming the default date/timestamp to something meaningful), sync from FK again, and the record stays in FK with the default name (now out of sync w/ renamed item in desktop). Another exception i see is: create an Activity todo (i.e. not done) for myself on desktop, sync from FK, and it shows up there... But then if it remains undone and is further edited in Desktop, the edited item then shows up on subsequent sync in FK as a redundant log entry, above the previous unedited log entry. A 3rd exception i encountered is: create an todo (i.e. not done) activity in desktop, sync from FK, and it works... But then if it remains undone and i sync again, it showed up as an error w/ following msg:

Error while syncing "Irrigate nursery seedlings" / The website encoutered an unexpected error. Please try again later.

mstenta commented 4 years ago

The The website encoutered an unexpected error. Please try again later. jumps out at me as a server error message. I would be curious to see the server logs when this happens - that might provide clues.

mstenta commented 4 years ago

Checked the logs and found this error:

TypeError: Argument 2 passed to RestWSBaseFormat::getResourceReferenceValue() must be of the type array, string given, called in /var/www/html/profiles/farm/modules/contrib/restws/restws.formats.inc on line 278 in RestWSBaseFormat->getResourceReferenceValue() (line 311 of /var/www/html/profiles/farm/modules/contrib/restws/restws.formats.inc).

@jgaehring Perhaps related to the recent changes to farmLog and default values?

jgaehring commented 4 years ago

Huh, yea, it could be due to farmLog and default values? I haven't seen an error like that before though. Seems strange to me there would be a string instead of an array though; I'd expect perhaps a null instead of an array if it was related to default values. It doesn't provide any more detail, does it?

mstenta commented 4 years ago

No more details, unfortunately.

mstenta commented 4 years ago

RestWSBaseFormat::getResourceReferenceValue() must be of the type array, string given

This suggests to me that it's a reference field that is being given an invalid value. For example, area reference, asset reference, term reference, etc. Those should always be arrays, but it seems that in this case a string is being sent.

jgaehring commented 4 years ago

Right, that was my interpretation. But to the best of my knowledge, all the default_values we're getting from the server are either [] or null, so I don't know how a string would have gotten in there.

mstenta commented 4 years ago

Can you link to the section(s) of code where this might be taking place? Maybe it can be replicated locally.

jgaehring commented 4 years ago

Can you link to the section(s) of code where this might be taking place? Maybe it can be replicated locally.

You mean where the default value is being assigned?

Here are the relevant bits of code for that...

For createLog: https://github.com/farmOS/farmOS-client/blob/8a40d392299d0df6f12686397979ecfc5a12c043/src/utils/farmLog.js#L16-L19

For updateLog: https://github.com/farmOS/farmOS-client/blob/8a40d392299d0df6f12686397979ecfc5a12c043/src/utils/farmLog.js#L62-L65

For formatLogForServer: https://github.com/farmOS/farmOS-client/blob/8a40d392299d0df6f12686397979ecfc5a12c043/src/utils/farmLog.js#L103-L111

For mergeLogFromServer: https://github.com/farmOS/farmOS-client/blob/8a40d392299d0df6f12686397979ecfc5a12c043/src/utils/farmLog.js#L186-L189

mstenta commented 4 years ago

Two of those set default_value: def = null by default in the first lines, but createLog and mergeLogFromServer do not. Could that difference be causing any issues? Or is that intentional perhaps? Just taking stabs in the dark here as I learn how this works. :-)

jgaehring commented 4 years ago

Just taking stabs in the dark here as I learn how this works. :-)

Yea, sorry, it's a still little messy and complicated (though much less than its predecessor makeLog).

Two of those set default_value: def = null by default in the first lines, but createLog and mergeLogFromServer do not. Could that difference be causing any issues?

Hm, maybe. The reason for assigning a default for def is that, in the case of updateLog and formatLogFromServer, you'll notice the following comment:

https://github.com/farmOS/farmOS-client/blob/8a40d392299d0df6f12686397979ecfc5a12c043/src/utils/farmLog.js#L59-L61

So if the entries array is based on the previous log object instead of an actual schema, then it will not have a corresponding default_value for each key. That's the case in which I wanted to explicitly assign def to null, as opposed to undefined, so I'd have a known value I could check against (rather than having to check for both undefined and null).

This is a moot point for createLog, because there is no previous log object being passed to it. mergeLogFromServer is the one place where I neglected to add that default value of null; I think I was going on the assumption that if we had a viable connection to the server, then we should also have a viable schema for whatever types of logs we were getting from the server, but perhaps to be thorough we could add that default there too. Still, I'm not sure how that could be responsible, but I may just need to think it through more.

jgaehring commented 4 years ago

One exception i see is: create and sync an Observation in FK, edit it on desktop (renaming the default date/timestamp to something meaningful), sync from FK again, and the record stays in FK with the default name (now out of sync w/ renamed item in desktop).

I believe I have this fixed with commit d57fda7.

Another exception i see is: create an Activity todo (i.e. not done) for myself on desktop, sync from FK, and it shows up there... But then if it remains undone and is further edited in Desktop, the edited item then shows up on subsequent sync in FK as a redundant log entry, above the previous unedited log entry.

Also fixed by d57fda7.

A 3rd exception i encountered is: create an todo (i.e. not done) activity in desktop, sync from FK, and it works... But then if it remains undone and i sync again, it showed up as an error w/ following msg:

Error while syncing "Irrigate nursery seedlings" / The website encoutered an unexpected error. Please try again later.

This one, however, I still cannot reproduce. It's possible this might be fixed by e3a5fa1. I'll get another release out early next week with these changes, and if you're able, @ludwa6, it'd be great for you to test this out again.

I'm closing for now but we can reopen if we can confirm that that 3rd case can be reproduced.