Closed jgaehring closed 2 years ago
There was some good discussion in #287 about maintaining some sort of state or timestamp on the server so FK could synchronize more smartly, specifically starting at https://github.com/farmOS/farmOS-client/issues/287#issuecomment-573253901.
Bumping this up to the beta milestone.
I want to reconsider how this could alleviate undue complexity from the Field Module API by removing control from the individual modules over how and when data is synced (at least for now). I discussed options yesterday with @mstenta and @paul121, and I think that intuition is essentially correct, but it opens up a can of worms about what the general rules for syncing should be.
Under the current paradigm, modules can call loadLogs
(or use the corresponding load*
method for any other entity) to access logs in IDB. They're requested from the database, then loaded into the Vuex store, where they are automatically passed to the modules root components as this.logs
. They are then modified via updateLog
and those modifications sent to the server with syncLogs
.
At least the very least I need to spend a little time to reevaluate that paradigm, and the overall conceptual framework for how farm data is accessed and modified by modules. Yesterday we tossed around the idea of a "draft" state, but this gets a little vague and conflates the primarily "DX-side" concept of an invalid or pending state of data, with the "UX-side" concept of an incomplete task, more or less equivalent to { status: 'in_progress' }
.
One idea is to shift away from this "loading/syncing" paradigm towards more of a "checkout" or "read/write" paradigm, where a module requests access to write to an entity and must manually relinquish that access before the changes are synced. There could be some kind of intermediary caching layer in there, too, so changes can't be lost, but are partitioned somehow from what's stored in the regular IDB stores.
Another idea is to use entity fields as a more "atomic" unit of change, so that we don't persist changes that happen while, say, the user is typing a message into the notes
field, but only once they "leave" that field (ie, onblur
). Again, this could be accompanied by some form of intermediary caching. Rules for this could be enforced if modules defined their own conventions with stricter validation than is normally applied to entities. So, for instance, if there is a module for moving animals between paddocks, it could create a convention on top of a particular type of movement log that requires an animal and location to be selected before it can be considered valid. Then this valid state could be a condition for whether or not the log is synced.
Perhaps some combination of these approaches could work, too, but in any event, these considerations would have significant bearing on the final API we present to Field Modules, so it seems prudent to evaluate them now, before beta release.
Yesterday we tossed around the idea of a "draft" state, but this gets a little vague and conflates the primarily "DX-side" concept of an invalid or pending state of data, with the "UX-side" concept of an incomplete task, more or less equivalent to
{ status: 'in_progress' }
.
This sums up the disadvantage of the "draft" idea very well! I see what you mean about creating confusion on the user side, by introducing a "psuedo" state alongside the existing "Log status" field - which currently allows the user to mark the log as "Pending" or "Done" (and who knows.. maybe farmOS core (or a module) will add a "Draft" status of it's own in the future!) Adding another visible thing that users need to understand in Field Kit is a step in the wrong direction.
That said, it's tricky, partially due to the way the server works right now. Any time an entity is updated via the API, it will create a new revision. So if we just start automatically syncing entities whenever a field is edited (eg: onblur
), then the server will end up creating a lot of unnecessary revisions.
This is largely a server issue, you're right @jgaehring! @paul121 pointed out in chat that the JSON:API module does not currently support the option to :ballot_box_with_check: "Create a new revision" in the same way that the user can do when they are editing an entity in the farmOS UI. So we "hacked" it with a bit of code in farmOS:
/**
* Implements hook_entity_presave().
*
* Forces revisions on all farm entities if the entity type supports them and
* the bundle has them enabled. This removes the option for users to disable a
* revision per-entity but as JSON:API doesn't support revisions yet, this is a
* trade-off that allows us to create revisions consistently on both the UI and
* the API.
*/
So fixing that JSON:API issue would be a big step!
But even so, it is also a client issue: because the client is also responsible for deciding when it is appropriate to :ballot_box_with_check: "create a new revision". But this could be a fun thing to explore, actually...
Maybe individual Field Modules could "checkout" a log, and in the process start a new revision. It then writes to that revision as many times as it wants, without creating a new revision on the server. The next time a Field Module "checks out" the log, a new revision would be created.
This does mean that two field modules would not be able to "check out" the same log at the same time, otherwise the revisions would get muddled (there is no "branching" in Drupal revisions - they are linear like commits). But maybe enforcing this would be a good thing!
All fun things to think about, but getting back to the core motivation for a moment...
I want to reconsider how this could alleviate undue complexity from the Field Module API by removing control from the individual modules over how and when data is synced (at least for now).
I wonder if some of the ideas from #243 could actually solve this? And side-step a lot of the considerations above...
- we could provide the syncing status of each log, similar to what @mstenta has suggested in Feature request: sync summary #209
- it would be the one standard module that every version of Field Kit would ship with, and would be available immediately on install, even before authenticating with the server
I'm imagining a Field Kit core "records" component, which provides a simple UI that lists all the records FK is currently storing in IDB. Perhaps there are sub-tabs for Assets, Logs, etc (similar to the "General" / "Movement" tabs in the logs module maybe?).
Alongside each record we could summarize its sync status, and allow the user to select one/many/all and "Sync" them. Deleting one/many/all in bulk would be another nice option, and would resolve another point from #243
- the user can clean up which logs are cached on the device, particularly those which are no longer accessible to other logs b/c of their filters
This would be keeping the sync process manual, as it is currently, and would limit when revisions are created based on when the user clicks "Sync". It would also serve as a place to more easily surface/organize sync errors, eg: when some records work and some don't.
It's probably a bit of a lift to build something like that before beta (or maybe Vue makes it easy? something similar to what you built in farmOS-aggregator frontend @paul121?), but if it side-steps a bunch of other issues (moving them post-beta) then maybe it's a net win? Would it help with any other issues @jgaehring?
But even so, it is also a client issue: because the client is also responsible for deciding when it is appropriate to ballot_box_with_check "create a new revision".
Field Kit does not use revisions at the moment, and if I implement a revision model in the future I intend to use something different from Drupal's revision model, because I'll want offline functionality built-in. Maybe we can shelve this consideration for now, since it is an implementation detail of the server, and not strictly part of FK.
I like where you're going by tying this in with #243 though.
Alongside each record we could summarize its sync status, and allow the user to select one/many/all and "Sync" them.
To clarify, are you suggesting this instead of exposing the this.syncLogs()
method in Field Modules, or in addition? I definitely like this idea if it supplants the need to manually sync logs within Field Modules. And I think it could be combined with automatic syncing, too, since it could facilitate less eager syncing rules by providing an escape hatch of sorts.
As for caching, I agree it would make sense to include caching status in the same place as syncing status, but I would prefer to stick with the concept of "cache settings" rather than designating individual entities for caching or not caching. Down the road, it would be nice to be able to set a flag on individual entities as "make available offline", but I'm less inclined to implement a "never cache" feature. Deletion is something we need to reconsider in general, with regard to #284 and #459 and whether or not we ever want to enable deleting entities on the server from within FK.
but if it side-steps a bunch of other issues (moving them post-beta) then maybe it's a net win? Would it help with any other issues @jgaehring?
Maybe there's a way we could build this out incrementally, if we start out by making the sync process something that is controlled by FK core, instead of the Tasks module, and move any buttons for the user to manually sync to the core UI. This could be done in the app bar, the drawer, the home page, its own "settings" or "records manager" page, or in several of those places at once. And we could start small, both in terms of manual syncing and automatic syncing. In fact, I already have implemented a degree of automatic syncing, by way of syncing everything in the cache every time the app starts up. Then we could implement more automatic syncing while simultaneously tweaking the manual sync UI, making it more robust and customizable, until we zero in on the sweet spot between the two.
In the meantime, this would achieve the goal of limiting the Field Module API surface for the beta release, so we're not including syncing functionality that we may decide later is too complex to maintain.
Closing in favor of #494.
From https://github.com/farmOS/farmOS-client/issues/273#issuecomment-562389307:
and https://github.com/farmOS/farmOS-client/issues/273#issuecomment-562691786: