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 156 forks source link

Push Central attachments to draft endpoint and publish draft #870

Closed lognaturel closed 4 years ago

lognaturel commented 4 years ago

Closes #860

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

Updated the relevant test and did manual testing against https://sandbox.central.opendatakit.org/.

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

Initially I thought I'd try to support all versions of Central but I decided that it's not worth it. It's relatively easy to upgrade Central and users really shouldn't be running on old versions. As far as we know there's no behavior that was deprecated that would require users to stay on pre-0.8. I did try to give a useful error message in case of trying to push to an older Central so users can recover.

In terms of code structure, I tried to follow existing patterns as much as possible.

The first commit isn't entirely related but it came up because I was going to lower the level of some errors that are showing up in Sentry but then I realized that they weren't intended to show up in Sentry and are only doing so because the last release used the old release logback file. I made sure @yanokwa's release project removes this and updated the readme.

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?

The only intentional change is to fix the bug. The only code touched is push to Central and that operation is at risk of regressions.

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

No.

codecov-commenter commented 4 years ago

Codecov Report

Merging #870 into master will increase coverage by 0.07%. The diff coverage is 60.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #870      +/-   ##
============================================
+ Coverage     48.23%   48.31%   +0.07%     
- Complexity     1639     1646       +7     
============================================
  Files           195      195              
  Lines         10394    10422      +28     
  Branches        751      753       +2     
============================================
+ Hits           5014     5035      +21     
- Misses         5022     5027       +5     
- Partials        358      360       +2
Impacted Files Coverage Δ Complexity Δ
...t/briefcase/push/central/PushToCentralTracker.java 42.67% <41.66%> (+0.20%) 15.00 <1.00> (+1.00)
...ndatakit/briefcase/push/central/PushToCentral.java 57.43% <44.44%> (-0.85%) 29.00 <1.00> (+1.00) :arrow_down:
...takit/briefcase/reused/transfer/CentralServer.java 82.31% <100.00%> (+0.90%) 37.00 <1.00> (+1.00)
...g/opendatakit/briefcase/reused/UncheckedFiles.java 44.53% <0.00%> (-1.69%) 31.00% <0.00%> (ø%)
...ndatakit/briefcase/reused/http/RequestBuilder.java 83.52% <0.00%> (+2.35%) 39.00% <0.00%> (+1.00%)
...g/opendatakit/briefcase/ui/export/ExportPanel.java 50.00% <0.00%> (+3.84%) 15.00% <0.00%> (+2.00%)

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 c4eb41b...a06717c. Read the comment docs.

lognaturel commented 4 years ago

That's totally right, @ggalmazor, thanks. I'm going to merge since I'm starting to get conflicts with a spike for pushing multiple form versions to Central and I'll file an issue to add the test.