Issue 210 validate publisher form

ggalmazor commented 5 years ago

Closes #208, #209, #210

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

Manually tried all field combinations in both publishers in the dev server.

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

No other approaches were considered. It's the narrowest change I could come up with that provides basic form validation.

An alternative would be to disable the button if the form is not valid. I don't have a strong opinion about this approach.

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

Common publisher form changes:

Alerts will be shown when no type of publisher or kind of upload (all, new, existing) is selected There is really no way to test this without tampering with the form, although validating forms is always a good idea. These validations will be handy in case we change the form and we open the option of not selecting an option on those selects.

Changes to the Google Spreasheet publisher form:

Users should expect a new publisher form that includes the owner's email:
Alerts will be shown if the user doesn't provide a workbook name or a valid owner's email address.

Changes to the JSON publisher form:

It won't ask for the owner's email, since it's not really required to create it.
In the publishers table, we use "N/A" in the "owner" column for JSON publishers.

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

Nope.

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.

kkrawczyk123 commented 5 years ago

@ggalmazor Validation on Google Spreadsheet works great! But I have noticed some issues:

when I input email address I did not receive the email with access to sheet and in the published data tab emails are cut off, screenshot attached:
Publish to JSON server is still allowed without input to any field on the dialog window. I think that URL field should be required one.

@opendatakit-bot unlabel "needs testing"

ggalmazor commented 5 years ago

Thanks, @kkrawczyk123!

I've added a couple of commits:

One that fixes the issue with the cropped email addresses
One that adds validation to the json publisher form (only checks the url field, ~and ensures that it's actually a URL~)

ggalmazor commented 5 years ago

I've left out url validation form the json publisher changes because it's more controversial than I thought it was going to be. There is no consensus on how a url should be validated in javascript (can't use Java for this one), and we will have to implement a custom regexp that meets all our needs: require http or https, support localhost, port numbers and simple paths.

In any case, if a user enters an invalid url, the publisher will fail and they will get feedback to make amends, therefore, I think it's safe to release this and improve it in a next release.

kkrawczyk123 commented 5 years ago

Tested with success! Confirmed that issues have been fixed.

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

getodk-bot commented 5 years ago

ERROR: Label "needs testing" does not exist and was thus not removed from this pull request.

getodk / aggregate