Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/extendend form schema #98

Merged
merged 34 commits into from Nov 2, 2020
Merged

Feature/extendend form schema #98

merged 34 commits into from Nov 2, 2020

Conversation

mtyszler
Copy link
Contributor

Extended Form Schema:

With this pull request I contribute a new function called form_schema_ext which has the same function signature as form_schema.

It extends the form schema by adding (multi-language) labels and (multi-language) choice lists from the ODK XML definition.

It closes #77 and addresses the API limitations discussed in getodk/central#172

@mtyszler
Copy link
Contributor Author

This is still work Work in Progress. After I do some stress testing, I'll add proper documentation and follow the test guidelines from contributing guidelines

@florianm florianm marked this pull request as ready for review October 30, 2020 01:26
@florianm
Copy link
Collaborator

florianm commented Oct 30, 2020

Thanks for the PR!
The failing GHA tests are a feature, not a bug:

Secrets are not passed to workflows that are triggered by a pull request from a fork. (because security)

If you want to run tests locally, your .Renviron should look (with your credentials for the ODK Central Sandbox):

ODKC_TEST_SVC="https://sandbox.central.getodk.org/v1/projects/14/forms/build_Flora-Quadrat-0-4_1564384341.svc"
ODKC_TEST_URL="https://sandbox.central.getodk.org"
ODKC_TEST_PID=14
ODKC_TEST_FID="build_Flora-Quadrat-0-4_1564384341"
ODKC_TEST_FID_ZIP="build_Spotlighting-0-6_1558333698"
ODKC_TEST_FID_ATT="build_Flora-Quadrat-0-1_1558330379"
ODKC_TEST_FID_GAP="build_Turtle-Track-or-Nest-1-0_1569907666"
ODKC_TEST_FID_WKT="build_Locations_1589344221"
ODKC_TEST_UN="..."
ODKC_TEST_PW="..."
ODKC_TEST_VERSION=1.0
RU_VERBOSE=TRUE
# Choose your favourite tz:
RU_TIMEZONE="Australia/Perth"
RU_RETRIES=3

Would you have a test form at hand with a few translations?

@mtyszler
Copy link
Contributor Author

mtyszler commented Oct 30, 2020

Hi @florianm , tks for the feedback.

I did have (most of) this locally set-up. I actually used that (as per contributions guidelines, with my un and pw):

# Required for testing
ODKC_TEST_URL="https://sandbox.central.getodk.org"
ODKC_TEST_PID=14
ODKC_TEST_FID="build_Flora-Quadrat-0-4_1564384341"
ODKC_TEST_FID_ZIP="build_Spotlighting-0-6_1558333698"
ODKC_TEST_FID_ATT="build_Flora-Quadrat-0-1_1558330379"
ODKC_TEST_FID_GAP="build_Turtle-Track-or-Nest-1-0_1569907666"
ODKC_TEST_FID_WKT="build_Locations_1589344221"
ODKC_TEST_UN="your@email.com"
ODKC_TEST_PW="..."
RU_TIMEZOME="Australia/Perth"

I did work locally (i.e, it passed the tests locally). I will re-run with the more detailed suggestion you did. Are you telling me that they will indeed always fail on PR?

There are still a few things I want to add:

  1. I want to test with non-default value for parameters (flatten, odata, parse). I might need to restrict this new function for ODK Central >=0.8 to avoid having to deal with the legacy version. Let me know if this would be ok
  2. I want to add a few additional forms for tests
  3. Adjust the example in the documentation
  4. I'll consider reading the hints as well

I do have a small question for you, @florianm. In the new function I use functions from xml2. You see I'm using the xml2:: format for calling those. Let me know if you'd prefer otherwise.

I also got some comments back from goodpractice I want to implement.

I'll switch back to ready once I'm done with it.

@mtyszler mtyszler marked this pull request as draft October 30, 2020 06:03
@mtyszler mtyszler marked this pull request as ready for review October 30, 2020 09:58
@mtyszler
Copy link
Contributor Author

Hi @florianm . I think now you can review. I added 3 additional test forms (you can see in the fid here

Looking forward to hearing from you

Copy link
Collaborator

@florianm florianm left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution, I've left some comments but this looks good.
I'll migrate the test forms to a new test server (ETA today if things go to plan) and will push updates to the main branch.
@mtyszler would you have a test form with two or more translations at hand?

R/form_schema_ext.R Show resolved Hide resolved
R/form_schema_ext.R Show resolved Hide resolved
R/form_schema_ext.R Outdated Show resolved Hide resolved
R/form_schema_ext.R Show resolved Hide resolved
@florianm
Copy link
Collaborator

florianm commented Nov 2, 2020

@mtyszler I've migrated the test server and sent you an invite to the email on your GH profile. Did the email arrive?

With the updated .Renviron settings from the contrib guide and your new credentials you should be able to run the tests locally. Do they work for you? I'm also experimenting with project permissions (you are a "project viewer") to prevent accidental submissions to the test forms (which slows down tests).

I've also added your three test forms for translations and submitted some data, although with shortened form titles and form IDs (apologies for slashing around in your forms). Feel free to add your test cases (or test stubs with test ideas) referencing these forms. You can either reference the form IDs directly or add settings / setters / getters / settings tests for the three I8n forms. The former is probably fine:

fid = Sys.getenv("ODKC_TEST_FID_I8N0", unset="I8n_no_lang")
fid = Sys.getenv("ODKC_TEST_FID_I8N1", unset="I8n_label_lng")
fid = Sys.getenv("ODKC_TEST_FID_I8N2", unset="I8n_label_choices")

@mtyszler
Copy link
Contributor Author

mtyszler commented Nov 2, 2020

Thanks @florianm .

@mtyszler I've migrated the test server and sent you an invite to the email on your GH profile. Did the email arrive?
With the updated .Renviron settings from the contrib guide and your new credentials you should be able to run the tests locally. Do they work for you? I'm also experimenting with project permissions (you are a "project viewer") to prevent accidental submissions to the test forms (which slows down tests).

Yes, I did receive the new access. I'll re-run tests locally and let you know .

I've also added your three test forms for translations and submitted some data, although with shortened form titles and form IDs (apologies for slashing around in your forms). Feel free to add your test cases (or test stubs with test ideas) referencing these forms. You can either reference the form IDs directly or add settings / setters / getters / settings tests for the three I8n forms. The former is probably fine:

No worries here... they are for testing purposes, so feel free to arrange as most logical/practical to you. By the way

fid = Sys.getenv("ODKC_TEST_FID_I8N0", unset="I8n_no_lang")
fid = Sys.getenv("ODKC_TEST_FID_I8N1", unset="I8n_label_lng")
fid = Sys.getenv("ODKC_TEST_FID_I8N2", unset="I8n_label_choices")

I'll use these in the tests. I had already included 3 specific tests. I'll see I can add one or two more.

@mtyszler
Copy link
Contributor Author

mtyszler commented Nov 2, 2020

Thanks for the contribution, I've left some comments but this looks good.
I'll migrate the test forms to a new test server (ETA today if things go to plan) and will push updates to the main branch.
@mtyszler would you have a test form with two or more translations at hand?

@florianm : I'll make sure I pull the main branch in. One of my current test forms already have English and French. Or do you mean something else?

@florianm
Copy link
Collaborator

florianm commented Nov 2, 2020

All good - I'm just reviewing the branch - OK if I send a few commits to it? Really nice code btw!
Are you happy for me to add you to the authors as contributor? Have you got an ORCID?

Edit: forgot I don't have write access to your branch, so seeing that the tests all pass I've merged the PR. So the current main branch should now contain your PR and my latest changes (both related to #96 and minor formatting on this PR).
Thanks again for the test forms as well - I discovered them on the Sandbox.

@florianm florianm merged commit c198cdd into ropensci:main Nov 2, 2020
@mtyszler
Copy link
Contributor Author

mtyszler commented Nov 2, 2020

Are you happy for me to add you to the authors as contributor?

Yes, happy to be listed as contributor 👍

Have you got an ORCID?

My ORCID is 000-0002-4573-0002
(btw, I just created. I got the question previously, but never looked into it. I'll explore it further now)

All good - I'm just reviewing the branch - OK if I send a few commits to it? Really nice code btw!
Edit: forgot I don't have write access to your branch, so seeing that the tests all pass I've merged the PR. So the current main branch should now contain your PR and my latest changes (both related to #96 and minor formatting on this PR).
Thanks again for the test forms as well - I discovered them on the Sandbox.

Hi @florianm . Tks a lot. I just saw you approved the pull requests and implemented your suggested edits. All great.

Tks for the quick turn-around. By the way, I thought I had given your write access, but if failed, apparently. Sorry about that.

If you're ok with it, I'll delete the branch and fork, as they are not needed anymore.

@florianm
Copy link
Collaborator

florianm commented Nov 2, 2020

Thanks again for the contribution, yep the fork is good to go! I'm waiting for a second PR on encryption and will then release a 1.0 and update the Zenodo release too.

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

Successfully merging this pull request may close these issues.

How to get labels and hints
2 participants