Skip to content
This repository was archived by the owner on Feb 12, 2023. It is now read-only.

Issue 210 validate publisher form #416

Merged
merged 6 commits into from
Mar 5, 2019
Merged

Issue 210 validate publisher form #416

merged 6 commits into from
Mar 5, 2019

Conversation

ggalmazor
Copy link
Contributor

@ggalmazor ggalmazor commented Feb 12, 2019

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:
    image
  • 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.

- The JSON server publishers get a "mailto:N/A" value so that "N/A" gets displayed on the publishers table
- Show alerts when/if the user doesn't select the publisher type, the kind of upload (all, new, existing)
- Google Spreadsheet: show alerts when/if the user doesn't provide an owner's email or a workbook name
- It doesn't make sense to continue trying to get the owner's email from the logged in user's account, since email accounts are no longer supported.
- Instead, we add a field to the form and validate it.
@ggalmazor ggalmazor added this to the v2.0.1 milestone Feb 12, 2019
@kkrawczyk123
Copy link

@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:
    screenshot from 2019-03-05 10-15-25

  • 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
Copy link
Contributor Author

ggalmazor commented Mar 5, 2019

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
Copy link
Contributor Author

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
Copy link

Tested with success!
Confirmed that issues have been fixed.

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

@getodk-bot
Copy link
Member

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants