getodk / briefcase

ODK Briefcase is a Java application for fetching and pushing forms and their contents. It helps make billions of data points from ODK portable. Contribute and make the world a better place! ✨💼✨
https://docs.getodk.org/briefcase-intro
Other
60 stars 154 forks source link

Issue 841 create form metadata when pulling from all source types #842

Closed ggalmazor closed 4 years ago

ggalmazor commented 4 years ago

Closes #841

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

Tried to reproduce the issue by pulling forms from Collect and Form Definition using the UI.

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

This is the cleanest way of solving the issue I came up with. An alternative approach would have been to change the underlying (legacy) code to deal with the form metadata port, but that felt too complicated and risky.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

TLDR:

This PR is forced to improve the threading and error management code of the (legacy) pull from collect code to ensure that the process is atomic (metadata is created only when a form is successfully pulled).

This made it possible to improve user feedback on the CLI operation, since now we can free ourselves from being coupled to the EventBus in the CLI, and use the standard out directly without the complicated threading model and async task coordination from before.

We were also to bridge the legacy code's TerminationFuture object with the JobRunner's status object, so that the operation can be cancelled again.

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.

Nope.

ggalmazor commented 4 years ago

Thanks for your review, @dcbriccetti!

kkrawczyk123 commented 4 years ago

Tested with success! Verified on Ubuntu, MacOS, Windows.

verified cases:

checked for regression:

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