search

XLSForm / pyxform

A Python package to create XForms for ODK Collect.
BSD 2-Clause "Simplified" License
77 stars 134 forks source link

Add warning when `quality` not specified for `audio` question #574

Closed seadowg closed 2 years ago

seadowg commented 2 years ago

Work for getodk/collect#4828

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

Not a lot to discuss here. Just duplicated the way other warnings are output as part of xls2json.py.

What are the regression risks?

Again not a lot here. The only real changes are to audio parsing/rendering so that's where any risks would lie.

Before submitting this PR, please make sure you have:

lindsay-stevens commented 2 years ago

Thanks! I have two questions. Otherwise looks good.

seadowg commented 2 years ago

Could you please convert the xmlcontains to xmlxpath_match? Usage examples in test_trigger.py.

Done!

Is the new warning specific to Collect? e.g. if it is not relevant to Enketo then maybe it is better placed in ODK Validate.

It is specific to Collect. Enketo doesn't support the quality attribute yet as far as I'm aware. The goal is to get the warning into https://getodk.org/xlsform/ and Central when people upload their XLSForm, but I'm guessing Validate covers more cases as would also be used by people creating XForms directly? Are we able to output warnings from Validate that show up in XLSForm Online and Central? To be clear we don't want to block people from leaving out quality, we just want form designers to have a heads-up on the change in behaviour.

codecov-commenter commented 2 years ago

Codecov Report

Merging #574 (36598b8) into master (66c0d5b) will increase coverage by 0.00%. The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #574   +/-   ##
=======================================
  Coverage   84.20%   84.20%           
=======================================
  Files          25       25           
  Lines        3672     3673    +1     
  Branches      865      905   +40     
=======================================
+ Hits         3092     3093    +1     
  Misses        440      440           
  Partials      140      140
Impacted Files Coverage Δ
pyxform/xls2json.py 79.43% <100.00%> (+0.03%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 66c0d5b...36598b8. Read the comment docs.

lindsay-stevens commented 2 years ago

Great! Thanks for the xml__xpath_match change.

ODK Validate covers many general XForm validation scenarios. The default for pyxform is to run ODK Validate, and not run Enketo Validate unless selected. Both validators can be disabled. Any validator warnings are passed through when the XLSForm is processed by pyxform. It looks like in pyxform-http the ODK Validate option is True.

seadowg commented 2 years ago

OK so you suggest adding the warning to ODK Validate instead? If so, I can remove the warning here but keep the updated test matcher.

lindsay-stevens commented 2 years ago

Either ODK Validate, or Javarosa which seems to do most of the validation in ODK Validate.

Another approach could be to add an optional validator argument --collect_validate to the xls2xform.py CLI. Users such as XLSForm Online and Central could then opt-in to running XLSForm validations specific to Collect. When opted in, that argument would be passed down to workbook_to_json in xls2json.py so that the validation can be conditionally called at more or less the same place. Validation functions / implementation could live in a sub-package at pyxform/validators/collect.

@lognaturel do you have any preference for how to proceed with this?

lognaturel commented 2 years ago

In general, I think it’s fine to have Enketo or Collect warnings in pyxform. We know from info shared by folks on the ODK technical advisory board that many users use both clients for different contexts. Collect remains the client of choice.

There are other folks who use pyxform but they aren’t contributors and they can filter out warnings if they really don’t want them.

However, in this case, I’m afraid I don’t think a warning at form design time is helpful. The audio recording change is going to be most disruptive to folks who already have forms in production. Some information in Collect would help them but it’s probably not critical. At form design time, people usually try out their forms. Most folks probably won’t care about or will like the change. I expect that those who really need an external recording app will be rare and they can figure it out. Regardless, by the time folks get this pyxform change in their various form management tools they’ll likely already have the Collect update. Sorry to come in at the 11th hour with that opinion. Maybe something I’m overlooking will change my mind?

seadowg commented 2 years ago

Sorry to come in at the 11th hour with that opinion. Maybe something I’m overlooking will change my mind?

@lognaturel No worries! This was already discussed on the forum here however I'll elaborate on my thinking a little here.

We don't expect the change in Collect to go out until late Jan, and I'd assumed we could get this out to at least XLSForm online (if not Central) soon to give anyone designing forms with those tools a head-up before (in addition to the forum announcement). I also think it'd be helpful to keep the warning in for a while after the change for users designing new forms expecting an external recorder. Agreed we'll still miss people with forms already in production who don't pay attention to the forum or our other announcement channels, but this feels like a good effort to warn people. It also doesn't seem like a disruptive change to add this in so even if it's not a perfect solution I think it's worth it. If yourself and @yanokwa disagree that with me however then let's just can it.

yanokwa commented 2 years ago

After much back and forth with @lognaturel and @seadowg, I've decided that we shouldn't add any warning to pyxform because the warning really should go to enumerators and their project supervisors.

It doesn't hurt to have a warning here, but it is more noise for form designers who don't need to know that the default behavior is changing. And it sucks that we've already written the code, but those are sunk costs. Also, adding it now means removing it later.

I should note for the record (pun!) that we are also not adding a warning to Collect because we don't think this change will be that disruptive to users (and is likely to be better for them in some cases). Those that want different behavior can get it by changing the settings on device or in the form.

Finally, we keep telling folks that have campaigns to join the beta and read the release notes so they are informed of these issues. Maybe this will be one more nudge for them to do that.