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

enhace #793: add Odata sanitized property name #873

Conversation

sadiqkhoja
Copy link
Contributor

@sadiqkhoja sadiqkhoja commented May 17, 2023

Closes getodk/central-frontend#793

What has been done to verify that this works as intended?

Integration Test

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

NA

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

No

Does this change require updates to the API documentation? If so, please update docs/api.md as part of this PR.

Yes

Before submitting this PR, please make sure you have:

  • run make test-full and confirmed all checks still pass OR confirm CircleCI build passes
  • verified that any code from external sources are properly credited in comments or that everything is internally sourced

@sadiqkhoja sadiqkhoja requested a review from ktuite May 17, 2023 16:45
@@ -268,6 +269,7 @@ const getMetadata = (dataset) => async ({ all, Datasets }) => {
if (!propertiesMap.has(property.name)) {
propertiesMap.set(property.name, {
...property,
name: sanitizeFieldsForOdata ? sanitizeOdataIdentifier(property.name) : property.name,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because of the update modal, I think Frontend needs to know both the actual property name and the OData identifier. It needs to know the actual property name so that it can make the correct PATCH request from the update modal in the entities table. Rather than overwriting name, what do you think about adding a second property, maybe odataName?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about I return name and odataName in all situations and remove odata query parameter?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds good to me. 👍

@sadiqkhoja sadiqkhoja requested review from matthew-white and removed request for ktuite May 17, 2023 18:25
docs/api.md Outdated
@@ -4608,6 +4609,7 @@ These are in alphabetic order, with the exception that the `Extended` versions o
+ isNew: `true` (boolean, required) - Whether or not this Property is new (will be created by publishing the Draft Form).

## Property Detailed (object)
+ name: `first_name` (string, required) - The name of the Property.
+ name: `the.age` (string, required) - The name of the Property.
+ odataName: `the_age` (string, required) - OData santized name of the property.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking that a little more explanation might be helpful:

The name of the property as it will appear in OData. OData property names can only contain alphanumeric characters and underscores.

@sadiqkhoja sadiqkhoja merged commit 8b7d8d0 into getodk:master May 17, 2023
@sadiqkhoja sadiqkhoja changed the title enhace #793: query param to sanitize property names enhace #793: add Odata sanitized property name Jun 1, 2023
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.

Dataset properties that are not valid OData identifiers are always empty in entities table
2 participants