Skip to content
This repository has been archived by the owner on Apr 16, 2022. It is now read-only.

Issue 695 Support empty manifest URLs when pulling forms #704

Merged
merged 1 commit into from
Feb 13, 2019
Merged

Issue 695 Support empty manifest URLs when pulling forms #704

merged 1 commit into from
Feb 13, 2019

Conversation

ggalmazor
Copy link
Contributor

Closes #695

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

On Onadata

On Briefcase

  • Configure the aggregate server with https://odk.ona.io/{username} (and your Ona credentials)
  • Pull form

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

This is a small change to check that whatever value coming on the manifestUrl is an url. This will prevent trying to get manifests when the server reports a <manifest/> tag when downloading a blank form (which is the default behavior in Ona).

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?

N/A

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

@ggalmazor ggalmazor added this to the v1.14.0 milestone Feb 7, 2019
Copy link
Member

@yanokwa yanokwa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't test, but this LGTM. I would have done fd.getManifestUrl().isEmpty(). @dcbriccetti whatcha think?

@kkrawczyk123
Copy link
Contributor

kkrawczyk123 commented Feb 8, 2019

I have verified pr and master with steps described in #695 on Ubuntu, Mac and Windows and on all I have the same results, successfully pulled form in all tries. I wasn't able to reproduce the issue.
The only Failed try was when I copied ona url directly (extra / at the and of the url) but I had Authentication error then. Interesting was also then even if status was failed, the form was pulled to sd.
So pr is verified but I am not sure if additional testing won't be necessary.
cc @yanokwa

@dcbriccetti
Copy link
Contributor

Didn't test, but this LGTM. I would have done fd.getManifestUrl().isEmpty(). @dcbriccetti whatcha think?

LGTM? I searched for it but doubt you’re saying “looks like garbage to me” because that’s not your style. :-)

isEmpty instead of isUrl? Yes, assuming no self-respecting RemoteFormDefinition would accept and save an invalid URL. It would be nice to assume that if one is set, it is valid.

@ggalmazor
Copy link
Contributor Author

isEmpty instead of isUrl? Yes, assuming no self-respecting RemoteFormDefinition would accept and save an invalid URL. It would be nice to assume that if one is set, it is valid.

We don't have fancy value object features in place yet, but yes: RemoteFormDefinition should do that work for us and reject any offending value. Don't know when, but it will happen :)

@kkrawczyk123
Copy link
Contributor

@opendatakit-bot unlabel "needs testing"

@ggalmazor ggalmazor merged commit a6457ce into getodk:master Feb 13, 2019
@ggalmazor ggalmazor deleted the issue_695_avoid_error_when_there_is_no_manifest_url_or_it_is_empty branch February 13, 2019 09:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants