Issue 459 avoid page load blocks

ggalmazor commented 5 years ago

Closes #459

What has been done to verify that this works as intended?

I've used Chrome's inspector for all these tests with the "Disable cache" from the network tab enabled.

Version note blocking issues

Start Aggregate in localhost and verify that I get the "you're up to date" note.
Restart Aggregate, disable the network connection and verify that I get the "version check failed" note.

Google maps JS blocking issues

Start Aggregate and verify with the inspector that the script gets eventually loaded.
Restart Aggregate, disable the network connection and verify that I get an error but the page still loads.

Why is this the best possible solution? Were any other approaches considered?

First I tried to modify the client's JSNI implementation in LayoutUtils.java to handle errors related to the requests to GitHub's API, but I got to very complicated half-solutions due to conflicts between the blocking nature of GWT Java UI building and the asynchronous XMLHttpRequest.

Then I decided to switch to an all-Java server-side solution. At the expense of having a new GWT service, now we completely avoid client-side JSNI code and the asynchronous interaction is entirely made through GWT RPC calls, which is results in much simpler client code.

Thanks to moving this code to the server, we're also able to throttle-control calls to GitHub's API (and avoid nasty temporary bans due to spamming it with requests). Now, instead the clients (potentially many) doing calls to GH, it's the server who does 1 request per 24 hours max.

Are there any risks to merging this code? If so, what are they?

None.

Do we need any specific form for testing your changes? If so, please attach one

No.

Does this change require updates to documentation? If so, please file an issue at https://github.com/opendatakit/docs/issues/new and include the link below.

No.

lognaturel commented 5 years ago

Moving the version check to the server seems great all around. 👍 Would be good to get a QA sanity check and ideally it can go in 2.0.3.

kkrawczyk123 commented 5 years ago

Tested with success!

@opendatakit-bot unlabel "needs testing" @opendatakit-bot label "behavior verified"

getodk / aggregate