Can the 'entities' sheet in an XLSForm contain comments?

spwoodcock · April 29, 2024, 12:00pm

1. What is the issue? Please be detailed.

I often include a notes column in my survey forms, particularly if distributing to others.
It makes it easier to understand what particular fields do.

This comment is from a while ago: How can I track comments in an XLSForm? - #2 by LN

It suggests that parsing forms with unrecognised fields shouldn't be an issue for the 'survey' sheet.

However, if additional unrecognised fields are included on the new 'entities' sheet, then an error is thrown by pyxform:

The entities sheet included the following unexpected column(s): 'notes'. These columns are not supported by this version of pyxform. Please either: check the spelling of the column names, remove the columns, or update pyxform.

From the following function in the code:

github.com

XLSForm/pyxform/blob/e25612efe5b47241bddf0bb95304c61a76079bd5/pyxform/entities/entities_parsing.py#L127


      
          
              if not is_valid_xml_tag(save_to):
                  if isinstance(save_to, bytes):
                      save_to = save_to.encode("utf-8")
          
                  raise PyXFormError(
                      f"{error_start} '{save_to}'. Entity property names {const.XML_IDENTIFIER_ERROR_MESSAGE}"
                  )
          
          
          def validate_entities_columns(row: Dict):
              extra = {k: None for k in row.keys() if k not in EC.value_list()}
              if 0 < len(extra):
                  fmt_extra = ", ".join(f"'{k}'" for k in extra.keys())
                  msg = (
                      f"The entities sheet included the following unexpected column(s): {fmt_extra}. "
                      f"These columns are not supported by this version of pyxform. Please either: "
                      f"check the spelling of the column names, remove the columns, or update "
                      f"pyxform."
                  )
                  raise PyXFormError(msg)

My question is, should the 'entities' sheet be able to ignore unrecognised fields, allowing for comments? Or is this only possible in the 'survey' sheet?

2. What steps can we take to reproduce this issue?

Create a new form with an 'entities' tab, but include an invalid column, such as notes.

3. What have you tried to fix the issue?

Checked the code!
I can make a PR if it helps, if a consensus is made on the approach to be taken.

4. Upload any forms or screenshots you can share publicly below.

My 'entities' sheet looks like this, in my survey form:

Survey form attached:

buildings(1).xls (107.5 KB)

Also my entity registration form:

entities_registration.xls (109 KB)

LN · April 29, 2024, 7:03pm

Good question, it's true that it's not consistent.

We added the column check because we've been layering on entity functionality and it's relatively likely to try to use a form with new Entity functionality with an older version of the Entity implementation. We tried to make that scenario as easy to troubleshoot as possible. For example, if you convert a form with an entity_id column with an older version of pyxform, because of the check you point to, you'll get an error and know that something went wrong. Without the check, the column would be ignored and you'd have to figure out that either pyxform, your form server, or both are out of date.

The other sheets also can suffer from this issue but it seemed less serious to us because the accepted columns have been mostly stable for some time and usually additions are somewhat advanced. Additionally, typically the failure can be identified immediately whereas with entities you may have to create an entity, attempt to update it, etc, and generally take several steps before you even notice there's a problem.

What do you think about this reasoning?

For your situation, could you consider locking the sheet? Adding a comment or note? If those don't seem reasonable, we could consider explicitly recognizing a notes column.

spwoodcock · April 29, 2024, 7:19pm

Thanks for the fast response!

I think that's a perfectly reasonable justification

Locking the sheet would be fine for my needs, I just wanted to make sure this is intended behaviour before moving forward.