search

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

Fix #818 #840

Closed cletusajibade closed 4 years ago

cletusajibade commented 4 years ago

Closes #818

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

Pulling from Aggregate server through CLI

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

This solution checks to make sure the metadata directory can be resolved. No other approach considered.

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?

Change does not affect users.

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

cletusajibade commented 4 years ago

Hi, @cletusajibade!

I don't think this method should throw any exception. See other similar methods like the one in FormStatus. Their goal is just to build Path objects for us without checking whether they exist or not.

Also, I'm not convinced that the proper way to respond to this would be to throw an error, since the involved paths can be missing the first time we want to create them.

I think that a better approach would be to ensure that the parent directory exists by creating it before trying to write the metadata file. Could you consider this approach, please?

Ok, I will try this approach.

cletusajibade commented 4 years ago

Hi, @ggalmazor , I sent a PR again yesterday.

ggalmazor commented 4 years ago

~I'm not sure if you want to continue with this PR or #846. Once you tell me what you want to do, I'll review~

I've guessed that this PR is the one you'd want to consider for the change.

codecov-io commented 4 years ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (master@fd229a4). Click here to learn what that means. The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #840   +/-   ##
=========================================
  Coverage          ?   48.66%           
  Complexity        ?     1643           
=========================================
  Files             ?      192           
  Lines             ?    10328           
  Branches          ?      745           
=========================================
  Hits              ?     5026           
  Misses            ?     4944           
  Partials          ?      358
Impacted Files Coverage Δ Complexity Δ
...case/model/form/FileSystemFormMetadataAdapter.java 71.18% <66.66%> (ø) 19 <1> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update fd229a4...38d1def. Read the comment docs.

ggalmazor commented 4 years ago

(Future reminder to squash merge this PR)

kkrawczyk123 commented 4 years ago

I wasn't able to reproduce an issue in multiple tries so I guess we have success here! Verified on Ubuntu, MacOS and Windows.

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