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

Test develop.farmos.app against test.farmos.dev #468

Closed jgaehring closed 2 years ago

jgaehring commented 3 years ago

We'll need https://test.farmos.dev to have the changes from the 2.x-client_module branch.

After that we can put Field Kit 2.x through its paces on https://develop.farmos.app.

jgaehring commented 3 years ago

@mstenta, I just mentioned this issue over in #287, with regards to what kinds of server work may still be necessary to support field modules going forward. Perhaps when you merge the 2.x-client_module branch into the test deployment, we could use the opportunity to take a quick survey of what if anything is still required, and open issues for tracking where appropriate. cc @paul121

jgaehring commented 3 years ago

Just updating after our chat today...

It doesn't look like there are any major requirements for Field Modules at this point, so it makes sense to merge the client_module branch so it can be part of the farmOS alpha5 release. :tada: :rocket:

Just a note that the last commit on that branch is a test module, and should not be included. I really like the idea of putting that into a submodule of sorts for testing Field Kit Core going forward, but we can discuss that later down the line.

mstenta commented 2 years ago

I merged the 2.x-client_module into the farmOS 2.x branch, tagged 2.0.0-alpha5, deployed it to test.farmos.dev, and enabled the farm_client module. So it's ready for testing!

Just a note that the last commit on that branch is a test module, and should not be included. I really like the idea of putting that into a submodule of sorts for testing Field Kit Core going forward, but we can discuss that later down the line.

I actually decided to keep this, and I enabled it on test.farmos.dev as well - so you should see a console.log appear in Field Kit from it. :-)

jgaehring commented 2 years ago

Awesome, thanks @mstenta!

I'll reassign this to myself and pick this up when I resume work on FK 2.x-beta, after farmOS.js 2.x-beta is out the door.

jgaehring commented 2 years ago

Actually, just unassigned you. Anyone else is welcome to try it out in the meantime!

Just go to https://develop.farmos.app and sign into the test server.

jgaehring commented 2 years ago

Just at a glance, there are two issues, first client-side, second server-side (I think):

I've got a quick fix for the first one already in the works, though I may wait to try deploying it til I have time to circle back to other authentication issues like #471. In the meantime, there's a simple workaround which is to simply enter the entire host when logging in: "https://test.farmos.dev".

As for the second part, it looks like we haven't ported any of the functionality of the old v1 farm_access module over to v2. @mstenta set develop.farmos.app as an allowed origin on test.farmos.dev, just for the time being, but this will need to be added to farmOS itself for the long-term, presumably before its own beta release.

I will come back to these issues when I get to #471, but for now I'm going to turn my attention to #465, which I can still work on locally even if the deployed testing is still broken.

mstenta commented 2 years ago

I've fixed the server side stuff in my 2.x-fieldkit branch: https://github.com/farmOS/farmOS/compare/2.x...mstenta:2.x-fieldkit

Also renamed the module from farm_client to farm_fieldkit (per our renaming discussion in chat), and importantly: renamed the OAuth2 client ID from farm_client to simply fieldkit.

I'll merge this into 2.x soon before tagging the next release.

jgaehring commented 2 years ago

I'm guessing this is why my latest commit failed to pass tests when it ran on GH:

https://github.com/jgaehring/farmOS.js/runs/4595383113?check_suite_focus=true

mstenta commented 2 years ago

Yup! Just change that line to farm_fieldkit instead of farm_client.

You'll also need to change the client ID to fieldkit.

jgaehring commented 2 years ago

Well, I got the issue with the missing https:// scheme (1b10450), and I'm not seeing anything about CORS, but it seems the client_id of fieldkit is not being accepted by test.farmos.dev. Here's the full response:

{
    "error": "invalid_client",
    "error_description": "Client authentication failed",
    "message": "Client authentication failed"
}

Still not an urgent issue though.

mstenta commented 2 years ago

Ah this is probably because test.farmos.dev is still on 2.0.0-alpha5, which was still using farm_client as the name of the Field Kit OAuth2 client. So if you've updated this repo to use the new fieldkit client ID then it won't work. I'll update that soon to 2.0.0-beta6 and we can test this again...

paul121 commented 2 years ago

I think the fieldkit client will need to have the https://develop.farmos.app domain added as an allowed origin as well. This could be configured manually on test.farmos.dev, but I'm curious if we should just add this domain to farm_fieldkit in farmOS core. There shouldn't be any real risk in doing so?

I came across this while working on https://github.com/paul121/farm-tugboat-demo/issues/9 - I think this CORS origin issue is the only remaining issue before the tugboat demo could be used for testing as well :-)

mstenta commented 2 years ago

This could be configured manually on test.farmos.dev, but I'm curious if we should just add this domain to farm_fieldkit in farmOS core. There shouldn't be any real risk in doing so?

I did configure this manually on test.farmos.dev, and I think that it would be best practice to require manually adding it to instances that explicitly want to test against develop.farmos.app.

Thinking ahead, the need to test against develop.farmos.app specifically may become more and more rare, as the core FK code stabilizes and development shifts more to field modules, which are loaded from the farmOS server itself. Furthermore, once Field Kit v2 is deployed to farmOS.app, develop.farmos.app may end up being used primarily for "staging", while various Netlify branch previews will be used for development/testing. And each of those would need to be explicitly added to farm_fieldkit config anyway. So I think the need for develop.farmos.app will be rare, and so hard-coding it in farm_fieldkit isn't necessary. Better to require the conscious choice IMO.

I think this CORS origin issue is the only remaining issue before the tugboat demo could be used for testing as well :-)

This is very exciting! Including develop.farmos.app would be cool in this context in the short term, but I think in the medium/long term all of the same considerations described above will apply here as well. So it might make sense to take a more temporary approach if develop.farmos.app is needed in Tugboat right now. I think a Drush command can be used to modify the allowed origins config?

jgaehring commented 2 years ago

I came across this while working on paul121/farm-tugboat-demo#9 - I think this CORS origin issue is the only remaining issue before the tugboat demo could be used for testing as well :-)

Oh, awesome! I need to explore what you're doing with the Tugboat demos more. Seems like it could open up new possibilities in testing etc.

Thinking ahead, the need to test against develop.farmos.app specifically may become more and more rare, as the core FK code stabilizes and development shifts more to field modules, which are loaded from the farmOS server itself. Furthermore, once Field Kit v2 is deployed to farmOS.app, develop.farmos.app may end up being used primarily for "staging", while various Netlify branch previews will be used for development/testing.

:100:

I wonder, would it be possible to deploy a special Tugboat build to be tested against when a Netlify preview deployment is triggered by a PR on the develop branch?

mstenta commented 2 years ago

Update: test.farmos.dev is now running 2.0.0-beta1 and the fieldkit client is now activated.

jgaehring commented 2 years ago

Hmm, well there's still something wrong with the origin not being used the first time you try to log in; it now seems to be using a relative URL, /oauth/token. I suspect this is a bug with the remote.add() method I replaced remote.setHost with in farmOS/farmOS.js@7781ec7. So when a fresh install of FK first starts up, it has a host of '', which should be changed to 'https://test.farmos.dev' when you log in, but that doesn't appear to be happening.

There's actually a workaround for this right now, b/c if you refresh after trying to login once, the correct host is retrieved from localStorage at startup the next time. However, it appears we have a CORS error again:

Access to XMLHttpRequest at 'https://test.farmos.dev/oauth/token' from origin 'https://develop.farmos.app' has been blocked by CORS policy: No 'Access-Control-Allow-Origin' header is present on the requested resource.

@mstenta, I assume this is because you need to manually add that header again since updating test.farmos.dev to beta1?

mstenta commented 2 years ago

Oops! Yes that was my oversight. I removed the old client and installed the new one, but forgot to add https://develop.farmos.app to the new client's allowed origins. That's done now so worth another try!

Screenshot from 2022-01-03 09-56-08

jgaehring commented 2 years ago

CORS resolved!

Interesting though, the call to https://test.farmos.dev/api/client_module/client_module 404'ed. Looking at the links at /api, I see that that endpoint was changed to https://test.farmos.dev/api/field_module/field_module, I assume when the farm_client module was changed to farm_fieldkit. Should be easy enough to fix. Also, I should make this configuration syncing process more resilient so a failed request like that doesn't cause the whole process to fail.

Incidentally, now that farmOS is in beta, I should update my local server so it will be in parity with test.farmos.dev.

mstenta commented 2 years ago

Oops sorry! I thought I mentioned that change above, but apparently not. Glad you figured it out. :-)

paul121 commented 2 years ago

I wonder, would it be possible to deploy a special Tugboat build to be tested against when a Netlify preview deployment is triggered by a PR on the develop branch?

@jgaehring yes in theory that would be possible! Would this tugboat build be created for automated or manual testing? For manual testing, it might just be easier to allow demo users to modify the CORS settings to allow the special netlify URL in the fieldkit client themselves. In fact, that might just be an easier solution for https://github.com/paul121/farm-tugboat-demo/issues/9 altogether :-)

For automated testing I would suggest testing against a server created via Github actions like we do elsewhere. It should work the same, the only difference might be we need to configure some special hostnames and/or CORS settings so things work properly within the Github action runner.

jgaehring commented 2 years ago

Would this tugboat build be created for automated or manual testing?

I guess I was thinking for manual testing, maybe if we had some custom behavior or a field module we didn't intend to include with standard Field Kit and/or farmOS.

jgaehring commented 2 years ago

Huzzah!

Looks like the last batch of changes seems to have fixed many of the issues! I haven't tested beyond logging in (which now takes a very long time), but no major errors as far as I can tell.

I'll have to kick the tires on it a little more tomorrow, but feel free to try it out if either of you get a chance.

paul121 commented 2 years ago

Woohoo! I was able to login from a tugboat demo after modifying the fieldkit client to have the develop.farmos.app allowed origin.

jgaehring commented 2 years ago

Oh awesome! I'll definitely need to experiment with FK/Tugboat combos.

jgaehring commented 2 years ago

The latest development build pushed to https://develop.farmos.app seems to be working with the test server, so I think that's sufficient for the low level parity I was hoping to achieve with the local environment. Any additional bugs that crop up can be opened as their own issues.