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

Trigger entity update on submission edit #1044

Merged
merged 3 commits into from
Nov 20, 2023
Merged

Conversation

ktuite
Copy link
Member

@ktuite ktuite commented Nov 10, 2023

Closes getodk/central#517 by letting submission.update.version events also be used to update entities.

All the scenarios listed in the issue are tested.

processing pending submissions

I also tested what happens when some submissions that are meant to update entities get picked up in the "pending submissions" when a dataset that requires approval for entity creation gets toggled to no longer require approval. Since we don't know ahead of time which submissions create vs. update entities, we can't really treat them differently here.

However, entity updates should get processed right away (regardless of if approval is required) so this probably wont come up often. The main situation this comes up is if a entity update submission comes in, the worker hasn't come by to process it yet, and the user toggles the dataset flag in the time between submission and worker processing. Or if a submission comes in after they toggle the dataset, but the worker still hasn't run yet.

Within this flow, it could also happen that dataset update event gets queued after the submission events, then the worker processes the individual submissions, and by the time it gets to the bulk processing event, there is nothing left to process.

There's a lot going on but ultimately none of it seems like a problem.

Also because of how entity updates get processed, there is no link from the update event back to the parent bulk processing dataset.update event.

frontend changes

I don't actually think anything needs to change here on the front end.

This shows two updates, one which came from a submission.update.version event and one from a submission.create event. We just show the submission and don't mention the specific kind of submission event for an entity update.
Screenshot 2023-11-15 at 3 50 47 PM

Here's the submission feed
Screenshot 2023-11-15 at 3 55 35 PM

I made a conditional update form on staging that works locally and will work once this PR is merged.

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

This PR is mostly tests.

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

@matthew-white matthew-white linked an issue Nov 10, 2023 that may be closed by this pull request
3 tasks
@matthew-white

This comment was marked as resolved.

@matthew-white

This comment was marked as resolved.

Base automatically changed from ktuite/create_or_update to master November 14, 2023 19:59
@matthew-white
Copy link
Member

I've filed getodk/central#547 related to processing pending submissions that (failed to) update entities. I think any change for that should come in a follow-up PR: we should merge this one first. I'll finish taking a look at this PR tomorrow.

@matthew-white
Copy link
Member

matthew-white commented Nov 17, 2023

I also tested what happens when some submissions that are meant to update entities get picked up in the "pending submissions" when a dataset that requires approval for entity creation gets toggled to no longer require approval. … There's a lot going on but ultimately none of it seems like a problem.

I agree with this. If there are unprocessed entity updates at the time of the dataset.update, then they will get processed either before or during when the dataset.update is processed — and either possibility seems fine.

As I mentioned in getodk/central#547, part of me would like a dataset.update to only trigger entity creation, not entity updates. However, I don't think that works given that update="1" create="1" is a possibility, when update is attempted before create. There's not necessarily a neat division between submissions that create entities and submissions that update them.

I'm wondering wonder whether we should update the text in DatasetPendingSubmissions in Frontend to make it clear that pending submissions will be processed for any entity action, not just creation. Right now, the text says:

Convert all pending Submissions to Entities now.
Change the setting and create Entities out of {count} Submissions not yet Approved or Rejected.

I would lean toward no change. Like you said, entity updates should be processed quickly, and it doesn't really matter when they are processed relative to when the dataset.update is processed. Also, I don't know how common it will be for users to opt into requiring submission approval for entity creation, yet at the same time use entity updates, which never require approval. What do you think about DatasetPendingSubmissions? Any change needed?

Comment on lines 3355 to 3357
await asAlice.patch('/v1/projects/1/datasets/people')
.send({ approvalRequired: true })
.expect(200);
Copy link
Member

@matthew-white matthew-white Nov 17, 2023

Choose a reason for hiding this comment

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

Duplicate of above I think. (The entity list has already been configured to require approval.)

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 it'd be a good idea to test that when there are multiple versions of a pending submission, only the last is used when the submission is processed as part of a dataset.update. I'm pretty sure it already works like that. Here are a couple of ideas:

Example 1

  • The entity list is configured to create entities on submission approval.
  • A submission is created that would create an entity on approval.
  • The submission is edited to create a different entity.
  • The entity list is reconfigured to create entities on submission receipt. Pending submissions are processed (?convert=true is specified).
  • The submission should only create the second entity, not the first.

Example 2

  • The entity list is configured to create entities on submission approval.
  • A submission is created that would create an entity on approval.
  • The submission is edited to not create any entity at all.
  • The entity list is reconfigured to create entities on submission receipt. Pending submissions are processed (?convert=true is specified).
  • The submission should not create an entity.

@ktuite
Copy link
Member Author

ktuite commented Nov 20, 2023

I would lean toward no change. Like you said, entity updates should be processed quickly, and it doesn't really matter when they are processed relative to when the dataset.update is processed. Also, I don't know how common it will be for users to opt into requiring submission approval for entity creation, yet at the same time use entity updates, which never require approval. What do you think about DatasetPendingSubmissions? Any change needed?

I don't think we need to change any wording, I hope it's a scenario that doesn't come up often! But if we are eventually able to identify submissions that do not create entities (because we track form actions), we should probably skip them here.

@ktuite ktuite merged commit 7305434 into master Nov 20, 2023
5 checks passed
@ktuite ktuite deleted the ktuite/sub_single_action branch November 20, 2023 22:44
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.

Submission edits can create or update an entity, but only once
2 participants