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

Minimal app for Paicines: issues #16

Closed alexadamsmith closed 6 years ago

alexadamsmith commented 6 years ago

Hey @jgaehring and @mstenta, I wanted to start a thread for discussing our progress towards the most basic version of a farmOS app that Paicines could use. First, I'd like to recap what we discussed in our meeting to make sure I understand what's needed.

Mike has offered in the past to help with the quantity attribute Jamie has offered to write a script that will accept a file location, and package an image into a request body that can easily be sent to the server.

Does that sound like a good summary of where we're at?

alexadamsmith commented 6 years ago

Hey again @jgaehring - I wanted to update you on the issue we discussed on Friday with saving new records to the database. Currently, new records are not saving to the database. I took one more stab at untangling the issue, without success. Here's what I learned...

jgaehring commented 6 years ago

Hey @alexadamsmith, first in response to:

Jamie has offered to write a script that will accept a file location, and package an image into a request body that can easily be sent to the server.

I'm still all for doing this. I just want to make sure I know what the output should look like, and what parameters you'll want to pass in. I'm assuming it should look pretty much the way they specify for the restws_file module, correct? So, like their example, but with farm_field_image as the property, instead of field_image? I'm wondering about the other properties. Will I need to append this property to the rest of the log? If so, in addition getting a uri string as a parameter, I'll also need that log object. Using a spread operator, it would look something like this: 

function formatImageObservation (fileUri, theRestOfTheLog) {
  const base64String = (_fileUri) => {
    // Do some encoding here and return a string...
  }
  return {
    ...theRestOfTheLog,
    farm_field_image: base64String(fileUri)
  }
}

Or it could just overwrite the uri, if it was already a property on that log, such as:

function formatImageObservation (fullLog) {
  const base64String = (uri) => {
    // Do some encoding here and return a string...
  }
  return {
    ...fullLog,
    farm_field_image: base64String(fullLog.farm_field_image)
  }
}

Another option is I could just write something that returns the base64String within the switch statement you're currently using in your formatState function, so you can just call it as:

switch (j) {
    // cases dealing with other properties as 'j'...
    case 'imageUri':
        newLog.imageUri = base64String(currentLog[j]);
        break;
}

There are a lot of options, so I wonder if it makes sense If I hold off until I can look at the specific context where you want to call this function? In the meantime, I think I've got a pretty good grip on the encoding, it should be a quick matter to supply that function whenever you're ready for it.

jgaehring commented 6 years ago

@alexadamsmith , regarding new records not saving to the db... It worked for me when I updated the spa/logFactory.js to have the wasPushedToServer property. Only the data/logFactory.js had that property previously. Let me know if it doesn't work for you.

I think this highlights how we could leverage the log factories a little more, now that the data plugin and SPA have their own copies, so that each is less dependent on the other. Ideally, I think the data plugin's logFactory should be responsible for formatting logs the way it wants every time it saves a record; what was happening here is it was just taking the newly created record exactly as it received it from the SPA, and dumping it into WebSQL, which is how the wasPushedToServer property got stripped off. In the long run, we definitely want both the data plugin and the SPA to be storing that property, so it's a quick fix worth keeping, but an added precaution would be to make sure the createRecord action passed it through the logFactory before passing it to saveRecord. That's something we can work on down the line, perhaps after I get the SPA separated (more on that later).

(Also, on that note, I was wondering if you'd considered putting the functionality of formatState into the logFactory? That's the kind of transformation I was originally thinking we could extend logFactory to handle—ie, taking a log as it was formatted for the store, and returning it in the format required for the REST API. Again, perhaps a consideration for a later time).

jgaehring commented 6 years ago

Just submitted a PR to fix the above issue with saving new records to the db.

While I was working on it, I also noticed that we're also getting duplicated logs in the Vuex store again. I think this is an outcome of the shoddy way I initially set up the login mixin to reroute to the Login.vue. The bug goes away the login plugin is not injected to the spa-loader. I need to do an overhaul of the login plugin, so I'm just going to open a new issue for now so I can get back to it then (after I'm done separating the SPA repo).

mstenta commented 6 years ago

Hi @alexadamsmith - @jgaehring and I spent some time talking through the goals last Friday and started a Google Doc that outlines them. I meant to share with you, but forgot. I wrote up a user story for the paddock observations, to give us something to work from. Here it is:

I am a ranch worker, tasked with moving animals and recording observations of paddock forage quality when I do that. I want to be able to open the app on my phone when I am at each paddock, snap a photo, enter notes and a quality rating, and save that so that it ends up in my farmOS database. I need to record multiple observations at a time, and may not have internet access during that time. At the end of the day, I will be back within cell/wifi access, and I would like to either be able to sync manually, or trust that they will sync automatically.

I also proposed some changes to the overall UI workflow to overcome what I saw as some potential issues with our current approach. One in particular is that it's currently not possible to get to the "New Observation" screen if you are offline and have not already logged in. So I suggest that we change how/when the authentication stuff happens so that it is not required to create an observation, and is only required when you are actually trying to send data to the remote server. I sketched up a mockup of what it might look like. Jamie and I talked through a lot of it, so it might make sense to chat with you directly if you have time to bring you up to speed on the thinking.

mstenta commented 6 years ago

Here is the mockup I put together: https://jsfiddle.net/4nL9khx3/21/

mstenta commented 6 years ago

I would also like to take some time and clean up the Github issue queue. We have gotten in the habit of using the issue queue for long discussions about many different things. I would like to be more focused with them and use issues for individual specific tasks or discussions. I may go through and close some of the old issues today, and break some things out into new individual issues.

alexadamsmith commented 6 years ago

Hi Jamie - sadly the issue with saving new records to the DB does not appear to be resolved after the Fix logfactory commit. I had tried adding wasPushedToServer to the spa logfactory after our conversation last week. I'm curious what happens when you run the app, create a record, close the app, and re-open the app. Do you see content of the previously saved record? What I see is {"log_owner":"","notes":"","quantity":"","name":"","type":"","timestamp":"","done":false,"isCachedLocally":true,"wasPushedToServer":false}

jgaehring commented 6 years ago

Huh. Ok, that is odd.

It's saving for me, whatever notes I save are there, and everything else. I am not compiling with Cordova, just running the dev server in Chrome, so that's the behavior I observe on refresh. Are you experiencing the same problem in both the browser and on iOS?

alexadamsmith commented 6 years ago

I will try testing it out in the browser..

On Fri, Apr 13, 2018 at 8:59 AM, James Gaehring notifications@github.com wrote:

Huh. Ok, that is odd.

It's saving for me, whatever notes I save are there, and everything else. I am not compiling with Cordova, just running the dev server in Chrome, so that's the behavior I observe on refresh. Are you experiencing the same problem in both the browser and on iOS?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/farmOS/farmOS-native/issues/16#issuecomment-381127475, or mute the thread https://github.com/notifications/unsubscribe-auth/ARAC8NsuJdfncqjV8Kus6ujO1JFC7Ipyks5toKE8gaJpZM4TN4XA .

alexadamsmith commented 6 years ago

Hi @jgaehring and @mstenta - I just pushed a new branch called camera, in which I've obtained access to image URIs via Cordova. Right now we can...

One thing I had trouble with was triggering an action in NewObservation after an asynchronous task (getting the image uri from the camera) completes. My comments on that are at lines 136-138 of NewObservation.vue

mstenta commented 6 years ago

Awesome @alexadamsmith ! Jamie also just completed #19 - which splits the repo into farmOS-native and farmOS-client. Do you think your branch belongs in farmOS-native? Or would it also need to be split?

What do you think about closing this issue and creating new specific issues for each specific task/feature? I find it very hard to keep things straight when we have long mixed-use issues like this.

mstenta commented 6 years ago

@alexadamsmith FYI I created an issue for "Photo uploads" in general over in the new farmOS-client repo (sent you an invite to that also, by the way): https://github.com/farmOS/farmOS-client/issues/2

If the photo upload process requires some things in both repos, we should create two separate issues... one for the specific things needed in farmOS-client and one for farmOS-native.

jgaehring commented 6 years ago

@alexadamsmith , that's great!

I see there are some changes in NewObservations.vue which would belong in the client repo. Those changes should be super easy to separate out and add to the client repo though.

@mstenta , would it make more sense to add this repo as an upstream (or downstream?) remote to the farmos-client repo and just grab that diff, or would be be just as well to do it by hand, which I think would probably be easier? Thinking in terms of version history. There's also at least one little chunk of the client repo that should get moved back into the native repo, concerning the retrieval of tokens, and I'm wondering if that should be migrated a similar way.

mstenta commented 6 years ago

Just doing it by hand would be easier in this case. You can still attribute the commit to Alex with the --author parameter. Up to you, though...

alexadamsmith commented 6 years ago

Hi @mstenta and @jgaehring - I would like to do some more app work this weekend, but I'm unclear on how to work with the new split-repo structure. The things I'm planning to do are

Nearly all the changes would be made to the data component and login module.

Is it all right if I just work on this stuff within my current branch (camera) rather than fully integrating it into the new split scheme? I know it's a pain, but i just have a really limited amount of time and want to make a few more contributions to the Cordova side of things.

alexadamsmith commented 6 years ago

Hey @mstenta and @jgaehring! I was able to do a bit more work on the native app today, and I think I've dealt with all the Cordova-specific hurdles that we had along the way to our minimal viable app.

Inconveniently for you guys, I haven't been able to bring my work into alignment with the new split repo structure. I wasn't quite sure how to make the switch, and wanted to address some Cordova issues while I had a bit of time available. So I just committed everything to the camera branch of the farmOS-native repo.

Things the app can now do:

Things the app CAN'T yet do, but will need to:

I've added some comments to the code, but if any of it still looks perplexing I'm happy to help figure it out! I won't be doing more coding this summer, but I can definitely help interpret the stuff I wrote.

There's still some work to be done, and I wish I could keep chipping away at this, because I feel like we're very close! But sadly the farm waits for no app... Mike, if you feel like setting up Cordova and bringing this thing together, I'll be glad to talk about my Cordova experiences!

mstenta commented 6 years ago

Thanks @alexadamsmith! Appreciate your help on this! And totally understand the responsibilities of the season starting up again. We'll look forward to working with you again in the fall/winter perhaps! :-)

We'll let you know if we have any questions as we merge this stuff into the new repo structure. Thanks again for all the work you've done!

jgaehring commented 6 years ago

That's great @alexadamsmith ! It shouldn't be a problem to get those changes merged into their respective repos.

jgaehring commented 6 years ago

I'm wondering if we want to close this issue and reopen Alex's list of "CAN'T yet do"s as separate issues here in the native repo.

In the client repo, we've got a game plan for the next major leg of development, focusing on the Observations page.

Meanwhile, I think it's a real good time to attend to some housekeeping:

I may split some of those into their own separate issues, too, if there aren't issues for them already, so we can start chipping away at them one by one.

mstenta commented 6 years ago

I'm wondering if we want to close this issue and reopen Alex's list of "CAN'T yet do"s as separate issues here in the native repo.

Yea that sounds good to me - easier to track them individually, IMO.

Meanwhile, I think it's a real good time to attend to some housekeeping:

Agree on all those points!

jgaehring commented 6 years ago

I've created separate issues for all the items I listed in my last comment, as well as features @alexadamsmith mentioned in https://github.com/farmOS/farmOS-native/issues/16#issuecomment-383436070 .

Now that we can track all those issues separately, I'm closing this issue.