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

Use XLSForm injection during project creation #1792

Merged
merged 7 commits into from
Sep 20, 2024

Conversation

spwoodcock
Copy link
Member

@spwoodcock spwoodcock commented Sep 17, 2024

What type of PR is this? (check all applicable)

  • πŸ• Feature
  • πŸ› Bug Fix
  • πŸ“ Documentation
  • πŸ§‘β€πŸ’» Refactor
  • βœ… Test
  • πŸ€– Build or CI
  • ❓ Other (please specify)

Related Issue

Fixes #1722

Describe this PR

  • PR Update form by injecting mandatory fields and validate itΒ #1763 added the osm-fieldwork update_xlsform work to the validate-form endpoint.
  • This PR also adds the functionality during the project creation.
  • Users can upload a custom form full of question and have the required FMTM fields injected automatically.
  • Removes the custom XML manipulation that was difficult to understand / maintain, replaced with much easier Pandas-based XLSForm concatenation.

Review Guide

  • Rebuild the docker image
  • Run the API
  • Create a new project using a custom form
  • Also test using the default / bundled forms

Checklist before requesting a review

[optional] What gif best describes this PR or how it makes you feel?

@spwoodcock spwoodcock added the backend Related to backend code label Sep 17, 2024
@spwoodcock spwoodcock requested a review from Sujanadh September 17, 2024 23:59
@spwoodcock spwoodcock self-assigned this Sep 17, 2024
@github-actions github-actions bot added enhancement New feature or request frontend Related to frontend code labels Sep 17, 2024
@spwoodcock
Copy link
Member Author

@Sujanadh sorry to distract from the fmtm-splitter work, but could you please take a look at this PR tomorrow?

I was coding into the night, so no doubt have missed something. Please check everything works as intended / looks good to you πŸ˜„

This will finalise the work on injecting XLSForm fields and allow #1653 to be easily solved.

Thank you!

@Sujanadh
Copy link
Contributor

sorry i didn't see the PR , i will test it

@Sujanadh
Copy link
Contributor

Encountered few issues while testing will post it here briefly.

@spwoodcock
Copy link
Member Author

Thanks! There are definitely some bugs to find πŸ˜†

I didn't actually get to test updating an existing form either, so that probably needs some work!

@Sujanadh
Copy link
Contributor

Sujanadh commented Sep 18, 2024

HTTPStatus.UNPROCESSABLE_ENTITY: XLSForm is invalid: There are two sections with the name survey_questions.
yes while appending there is a problem since we need to get common fields and then append likewise

I used default form here.

@Sujanadh
Copy link
Contributor

Sujanadh commented Sep 18, 2024

when trying to check custom form also there is issue

{
  "detail": "XLSForm is invalid: There has been a problem trying to replace ${existing} with the XPath to the survey element named 'existing'. There is no survey element with this name."
}

this was the particular reason that i had to first append mandatory fields before testing for validation.

oh actually my bad , i should debug on for custom form , it works fine on custom form

@Sujanadh
Copy link
Contributor

i don't think we need to validate it again in generate-project-date since we append and validate using validate-form endpoint then return valid appended form which is used while generating project data. For default form we don't need to do anything , instead we can change xls form to allow to create new entity with new geopoint.

@spwoodcock
Copy link
Member Author

spwoodcock commented Sep 18, 2024

Thanks for the tests!

The issue about existing is from the old XLSForm that has calculated fields based on this. As you say, using a custom form without this logic works fine.

Agree we could validate once only!
The reason I left it in is because the API is independent from the frontend.

While it's quite likely our frontend is the only user of the API, calling validate-form prior to generating project files, we can't guarantee it. So validating again is just an addition check, without much performance penalty - what do you think?

@spwoodcock
Copy link
Member Author

i don't think we need to validate it again in generate-project-date since we append and validate using validate-form endpoint then return valid appended form which is used while generating project data. For default form we don't need to do anything , instead we can change xls form to allow to create new entity with new geopoint.

Regarding the default forms, I already updated the forms to remove the verification fields (so they are injected).

I will update them again now to remove the FMTM mandatory fields entirely, and have them injected the same way as a custom form.

This should improve maintainability, as we don't need to update the entire XLSForm library when we change something in the mandatory fields πŸ˜„

Related issue I just made: hotosm/osm-fieldwork#300

@spwoodcock
Copy link
Member Author

HTTPStatus.UNPROCESSABLE_ENTITY: XLSForm is invalid: There are two sections with the name survey_questions. yes while appending there is a problem since we need to get common fields and then append likewise

I used default form here.

This is a valid error though! We are having issues with the dataframe merging logic

@Sujanadh
Copy link
Contributor

Thanks for the tests!

The issue about existing is from the old XLSForm that has calculated fields based on this. As you say, using a custom form without this logic works fine.

Agree we could validate once only! The reason I left it in is because the API is independent from the frontend.

While it's quite likely our frontend is the only user of the API, calling validate-form prior to generating project files, we can't guarantee it. So validating again is just an addition check, without much performance penalty - what do you think?

Ah got it, you are right that makes sense.

@spwoodcock
Copy link
Member Author

Made some updates!

Hopefully fixed project creation and XLSForm updating πŸ˜„

@spwoodcock
Copy link
Member Author

spwoodcock commented Sep 20, 2024

I made some updates to osm-fieldwork for this, so the main things that need to be done:

  • Do some tests all the way through to the ODK Collect usage, to check the logic all works as intended.
  • Deploy this to staging for Monday.

I am merging this now so we can more easily test - we can make another PR if needed.

@spwoodcock spwoodcock merged commit 74e3d2a into development Sep 20, 2024
10 of 11 checks passed
@spwoodcock spwoodcock deleted the refactor/xlsform-injection branch September 20, 2024 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend Related to backend code enhancement New feature or request frontend Related to frontend code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Injecting mandatory FMTM fields into custom XLSForms
2 participants