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

Features/entities/auto convert sub to entities #870

Conversation

sadiqkhoja
Copy link
Contributor

@sadiqkhoja sadiqkhoja commented May 12, 2023

Moves #864 forward

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.md as part of this PR.

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 force-pushed the features/entities/auto-convert-sub-to-entities branch from a4ae37d to 062d4f9 Compare May 16, 2023 14:38
const processSubmissionEvent = (event) =>
_processionSubmissionEvent(event, event.details.submissionId, event.details.submissionDefId, event.acteeId);

const createEntitiesFromPendingSubmissions = (pendingSubmissions, event) => (container) =>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

eventId for logging should be submission create event or latest update event

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and add information about this grand event in details column of entity_source

Copy link
Member

Choose a reason for hiding this comment

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

Just my two cents, I vote for making the dataset.update event the primary source event. That's because the dataset.update event is the event in the causal chain that immediately preceded the creation of the entity. Sort of like how with submission approval, the approval is just the last step that caused the creation of the entity. The entity …/audits endpoint should still return the submissionCreate property, but it seems potentially helpful for the sourceEvent returned to be the dataset.update event. No strong feelings, feel free to ignore!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will you show this event on the frontend?

@sadiqkhoja sadiqkhoja force-pushed the features/entities/auto-convert-sub-to-entities branch from 505a2d8 to 32d7df6 Compare May 18, 2023 18:29
@sadiqkhoja sadiqkhoja marked this pull request as ready for review May 18, 2023 19:57
@sadiqkhoja sadiqkhoja requested a review from ktuite May 18, 2023 19:57
update.audit = (datasets, data) => (log) => log('dataset.update', datasets, { data });
update.audit = (datasets, data, autoConvert) => (log) => log('dataset.update', datasets, { data, autoConvert });

const _unprocessedSubmissions = (datasetId, fields) => sql`
Copy link
Member

Choose a reason for hiding this comment

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

I think draft submissions need to be excluded: see #880. I think the flag submissions.draft can be used.

update.audit = (datasets, data) => (log) => log('dataset.update', datasets, { data });
update.audit = (datasets, data, autoConvert) => (log) => log('dataset.update', datasets, { data, autoConvert });

const _unprocessedSubmissions = (datasetId, fields) => sql`
Copy link
Member

Choose a reason for hiding this comment

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

I think deleted submissions (for forms in the trash) should also be excluded. I don't think something in the trash should cause side effects elsewhere in the project.

@sadiqkhoja sadiqkhoja merged commit e9dba19 into getodk:master May 19, 2023
@matthew-white matthew-white mentioned this pull request May 19, 2023
5 tasks
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.

3 participants