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

Add background-geopoint question type which exposes xforms-value-changed event with odk:setgeopoint action #726

Merged

Conversation

RuthShryock
Copy link
Contributor

@RuthShryock RuthShryock commented Oct 2, 2024

Closes #716

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

This solution introduces a new background-geopoint question type that triggers the odk:setgeopoint action when the xforms-value-changed event occurs. The design and implementation were based on discussions from this forum post. The solution followed the proposal and was straightforward to implement based on the details in the issue.

What are the regression risks?

Because this introduces a new question type and modifies how setvalue triggers are handled, there is a risk of breaking existing workflows that rely on the current setvalue behavior. I refactored some functions to handle both setvalue and the new setgeopoint triggers, however, this needs to be tested thoroughly to ensure that the changes don’t break any of the existing setvalue functionality.

Does this change require updates to documentation?

Yes: XLSForm/xlsform.github.io#277

Before submitting this PR, please make sure you have:

  • included test cases for core behavior and edge cases in tests
  • run python -m unittest and verified all tests pass
  • run ruff format pyxform tests and ruff check pyxform tests to lint code
  • verified that any code or assets from external sources are properly credited in comments

pyxform/question.py Outdated Show resolved Hide resolved
pyxform/question.py Outdated Show resolved Hide resolved
pyxform/question.py Outdated Show resolved Hide resolved
pyxform/survey.py Outdated Show resolved Hide resolved
@lindsay-stevens
Copy link
Contributor

Thanks for this PR - looks like it will do what was agreed earlier. My comments are mainly to polish it: increase test coverage, optimise error messages, try to avoid performance regressions / tech debt.

@RuthShryock RuthShryock force-pushed the add_background-geopoint_question_type branch from 053647e to 8770059 Compare October 9, 2024 16:52
@lognaturel
Copy link
Contributor

Thanks and welcome, @RuthShryock!

@tiritea
Copy link

tiritea commented Oct 14, 2024

Are there any further outstanding issues that need to be addressed to merge this PR?

@lognaturel
Copy link
Contributor

@lindsay-stevens I've only taken a quick look and it's looking good. Could you please do a final review, especially looking at the perf concerns you had brought up?

Maybe if there are smaller things pending we can file an follow-on issue as we sometimes do. Thanks!

RuthShryock and others added 8 commits October 19, 2024 11:46
…an odk:setgeopoint action in response to an xforms-value-changed event
… setvalue question types and also when used in groups and repeats
- builder.py:
  - de-duplicate elif call to create_question_from_dict
  - use relevant class dictionaries directly instead of passing around
  - replace regex with strip() to avoid replacing whitespace everywhere
    in the question name (whitespace invalid in question names).
- question.py:
  - avoid calling self.get_root() repeatedly as it iterates the form
    (up the tree from the current question).
  - remove `if` block in nest_set_nodes since it is only called when
    there are items to be processed.
- question_types.py:
  - add validation functions for workbook_to_json. used external strings
    for re-use, easier testing, and (maybe someday) i18n.
- xls2json.py:
  - move background-geopoint questions to workbook_to_json from
    survey.py and question.py to allow row references in errors
- test_background_geopoint.py:
  - change the test class name to match unittest idiom / test discovery
  - remove `note` question from all tests since this is not relevant to
    the feature being tested or any specific edge cases of interest.
  - remove xpath expressions testing only the presence of groups, since
    these are tested already as part of the xpath to find the inputs
    within the groups.
  - readability improvements:
    - structured test method names
    - state the overall test case expectation in docstring
    - indicate the topic of the xpaths
    - wrap long xpaths and use single-quotes for predicates
    - shorten test fixture names and tidy up markdown formatting
@lindsay-stevens lindsay-stevens force-pushed the add_background-geopoint_question_type branch from 153aa61 to f673525 Compare October 19, 2024 00:49
@lindsay-stevens
Copy link
Contributor

@lognaturel I added a commit to address the remaining review feedback. Could you please take a look and merge/rebase if it looks good to you? Thanks

@lognaturel lognaturel merged commit a724215 into XLSForm:master Oct 21, 2024
10 checks passed
@lognaturel
Copy link
Contributor

Thanks, everyone!

@RuthShryock RuthShryock deleted the add_background-geopoint_question_type branch October 22, 2024 14:11
jnm added a commit to kobotoolbox/kpi that referenced this pull request Nov 7, 2024
question type by upgrading pyxform to v2.2.0, which includes
XLSForm/pyxform#726
jnm added a commit to kobotoolbox/kpi that referenced this pull request Nov 7, 2024
### 📣 Summary
Support the new `background-geopoint` question type [[read
more]](XLSForm/pyxform#716) by upgrading
pyxform to v2.2.0, which includes XLSForm/pyxform#726


### 👀 Preview steps

1. Create a project by uploading an XLSForm with a `background-geopoint`
question, for example
([Simple_background_geo.xlsx](https://github.com/user-attachments/files/17510897/Simple_background_geo.xlsx))
1. Deploy the project
1. Download the project's XML XForm and verify it against the example
here:
XLSForm/pyxform#716 (comment).
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.

New Feature: expose xforms-value-changed event with odk:setgeopoint action
4 participants