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

Reject system-reserved entity property names case-insensitively #482

Closed
lognaturel opened this issue Sep 12, 2023 · 9 comments · Fixed by getodk/central-backend#983
Closed
Assignees
Labels
backend Requires a change to the API server behavior verified Behavior has been manually verified entities Multiple Encounter workflows

Comments

@lognaturel
Copy link
Member

Problem description

We have two unusual system-reserved entity properties that are reserved to ensure seamless use with select questions in form design: name and label. Currently these are reserved on a case-sensitive basis. This is a problem for Collect: https://forum.getodk.org/t/entity-based-data-fetch-working-with-enketo-but-not-in-odk-collect/43040

URL of the page

Steps to reproduce the problem

Create an entity property with name Name.

Screenshot

Please don't attach images of QR codes, as those provide access to the server.

Expected behavior

System-reserved entity property names should be reserved case-insensitively to avoid confusion.

@matthew-white matthew-white added backend Requires a change to the API server entities Multiple Encounter workflows labels Sep 12, 2023
@ktuite ktuite self-assigned this Sep 12, 2023
@matthew-white matthew-white added the needs testing Needs manual testing label Sep 12, 2023
@srujner
Copy link

srujner commented Sep 14, 2023

Is it ready for testing?

@ktuite
Copy link
Member

ktuite commented Sep 14, 2023

yes, ready for testing!

@dbemke
Copy link

dbemke commented Sep 15, 2023

I’ve played around with property names and got to a point in which I’m not sure exactly how it works/should work. In a form creating entities in the name column I entered "Name” but in bind::entities:saveto collumn I entered "somethingrandom”. I can upload that form to server without error and the property somethingrandom exists. If I reverse columns (in bind::entities:saveto – Name; in name-somethingrandom) then I see "The entity definition within the form is invalid. Invalid Dataset property.”
Seems ok but just to double check if I understand well how it works : Is the name of the property verified by checking the bind::entities:saveto column only? And then that would mean that it’s allowed to enter "Name” in name column?

@dbemke
Copy link

dbemke commented Sep 15, 2023

There are two different warnings - not sure if it's ok?
If I put "Label" with capital letter in bind::entities:saveto there is

Image

If I put "label" in bind::entities:saveto there is

Image

@ktuite
Copy link
Member

ktuite commented Sep 15, 2023

This sounds okay to me!

The restrictions on name in the survey tab (the form field name) should be pretty loose. Name and somethingrandom should be fine - these are just the usual ODK form field names.

The entity property names, the ones in save_to column are much more restricted. name, label, and now Name can't be used... and there are other rules like no spaces.

There are two different places that these field name rules are being checked, and that's where the different errors are coming from, but I think that's okay.

  1. in pyxform when converting the xlsx file (the second error)
  2. within central when it checks the converted xml file (first error)

I think I'm going to change the error message in the first error from Central, though. It ought to mention Entity List instead of Dataset and I can also make it say the name of the property and the word "save_to".

@lognaturel
Copy link
Member Author

I think I'm going to change the error message in the first error from Central, though. It ought to mention Entity List instead of Dataset and I can also make it say the name of the property and the word "save_to".

Ooh, I really didn't think about Problems that might be surfaced raw. I was thinking backend would always use the word "Dataset." In this case, we could have pyxform do a case insensitive check so the message comes from there. In fact, I wish I'd thought of doing that first. Maybe that would be enough?

Then I see you added a note about problems with "Dataset" more generally in #466. Could we consider making sure all of those have a corresponding frontend string?

@lognaturel
Copy link
Member Author

@dbemke small note: I see you're using bind::entities:saveto which is fine but we now document save_to so you might want to start using that instead (it's an alias to bind::entities:saveto).

@dbemke
Copy link

dbemke commented Sep 18, 2023

Tested with success!

@srujner
Copy link

srujner commented Sep 18, 2023

Tested with success!

@srujner srujner added behavior verified Behavior has been manually verified and removed needs testing Needs manual testing labels Sep 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend Requires a change to the API server behavior verified Behavior has been manually verified entities Multiple Encounter workflows
Projects
Status: ✅ done
Development

Successfully merging a pull request may close this issue.

5 participants