-
Notifications
You must be signed in to change notification settings - Fork 75
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
Log when sub is force applied and apply create as update #1232
Conversation
Questions
|
One high-level question I have is what information needs to be logged. I see that Instead of adding to source details, I think it'd be nice to add defined columns to the table. The schema of the audit log details has kind of gotten hard to understand, and I'd be worried about a similar situation with the source details. How about adding these two columns to the table?
My opinion is that it'd be a good idea to avoid storing this information in two places in the database. I agree that the entity def source feels right. We don't currently show audit log events about entities in the Fronted audit log. I think we can figure out what to do if/when we decide to show them. We could probably join
I think there will probably need to be a new, separate query for querying the backlog that will join the backlog table with other tables. The query could fetch information it needs from the |
// in which case its ok to apply this create as an update | ||
if (err.problemCode === 409.3) { | ||
const rootDef = await Entities.getDef(dataset.id, entityData.system.id, new QueryOptions({ root: true })).then(o => o.orNull()); | ||
if (rootDef && rootDef.aux.source.details.updateAsCreate && rootDef.aux.source.details.forceProcessed) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we just check forceProcessed
and not check updateAsCreate
? Since only offline updates are put in the backlog, if the root def was forceProcessed
, I think that means that it was also an updateAsCreate
.
else if (entityData.system.create === '1' || entityData.system.create === 'true') | ||
maybeEntity = await Entities._createEntity(dataset, entityData, submissionId, submissionDef, submissionDefId, event, parentEvent, forceOutOfOrderProcessing); | ||
else if (entityData.system.create === '1' || entityData.system.create === 'true') { | ||
// note i dont think (???) forceOutOfOrderProcessing will ever be true here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense to me. Only entity updates are force-processed from the backlog, and entity updates are fully handled above. Explanatory comments might be helpful though.
const event = auditMap.get(def.id); | ||
if (event) | ||
v.source = event.source; | ||
v.source = mergeLeft(event.source, v.source); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could try v.source = { ...v.source, ...event.source }
with spread syntax
or Object.assign(v.source, event.source)
Note: at some point will need to update Entity.Def.Source forApi()
to merge details and forceProcess
This PR tried a couple different things: over-logging of interesting flags (was the entity processed? what update vs. create vs. upsert vs. other fallback path of entity processing were we on?) as well as enough logging to support Create-As-Update. I'm closing it in favor of #1251 which has minimal new capturing of the necessary information to be able to handle the create-as-update case. |
Closes getodk/central#702 and getodk/central#720
Should also help with getodk/central#699
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:
make test
and confirmed all checks still pass OR confirm CircleCI build passes