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

Allow property name from deleted form to be used in entity list #1182

Merged
merged 1 commit into from
Aug 30, 2024

Conversation

ktuite
Copy link
Member

@ktuite ktuite commented Aug 29, 2024

Closes getodk/central#695

When an entity list property was created through a draft form, it inserted a row of (datasetId, name, publishedAt) with null publishedAt. The query to insert a new property via the API (which would also immediately publish that property) flagged any new property insertion as a a conflict. The query has been updated to act as an upsert that will publish an unpublished property instead of failing.

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

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

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?

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

Before submitting this PR, please make sure you have:

  • run make test 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

@ktuite ktuite requested a review from lognaturel August 30, 2024 17:33
Copy link
Member

@lognaturel lognaturel left a comment

Choose a reason for hiding this comment

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

One question to check my understanding but unless I got that wrong this looks ready to me!

@@ -179,11 +179,18 @@ createPublishedDataset.audit.withResult = true;
const _createPublishedProperty = (property) => sql`
INSERT INTO ds_properties ("name", "datasetId", "publishedAt")
VALUES (${property.name}, ${property.datasetId}, clock_timestamp())
ON CONFLICT ("name", "datasetId")
DO UPDATE SET "publishedAt" = clock_timestamp()
WHERE ds_properties."publishedAt" IS NULL
Copy link
Member

@lognaturel lognaturel Aug 30, 2024

Choose a reason for hiding this comment

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

If two drafts forms would create the same property the property is still only in the database once, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the property is only listed once, but there's another table ds_property_fields that links those properties to the forms/form_defs/specific form fields that they came from (if they came from a form and not the API).

With two draft forms, it'll look like this in the database.

ds_properties [
  { id: 1, name: 'first_name', datasetId: 1, publishedAt: null },
  { id: 2, name: 'age', datasetId: 1, publishedAt: null }
]
ds_property_fields [
  { dsPropertyId: 1, formDefId: 3, path: '/name' },
  { dsPropertyId: 2, formDefId: 3, path: '/age' },
  { dsPropertyId: 1, formDefId: 4, path: '/name' },
  { dsPropertyId: 2, formDefId: 4, path: '/age' }
]

Copy link
Member

Choose a reason for hiding this comment

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

Great, makes sense. I was wondering whether there might be risk of the same property being published multiple times but it doesn’t seem so

@ktuite ktuite merged commit ef63c74 into master Aug 30, 2024
5 checks passed
@ktuite ktuite deleted the ktuite/entity_prop_bug branch August 30, 2024 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants