Correctly log instanceId in submission review update audit details

[ktuite]

Someone in the forum mentioned they expected (from reading the source code) to find instanceId in the audit table details when reviewing a submission.

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

Tests

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

I don't think we should spend too much time checking these audit details... but this one looked like it was intended to log something and then wasn't properly working. I checked that the submission def has nothing but the def id at that point in the code, and it makes sense to use the instance id on the submission instead.

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?

Bit more useful info exposed. Sounds like it might help some people trying to work with the external webhooks integration.

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

We don't document the schema of the audit log details.

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

[matthew-white]

I checked that the submission def has nothing but the def id at that point in the code

I wonder if this was a regression. It might be interesting to check on the QA server whether there are any old submission.update entries with an instanceId.

it makes sense to use the instance id on the submission instead.

That makes sense to me to use the instance ID of the logical submission rather than the instance ID from the latest submission edit. 👍 The submission.update event is about updating submission metadata and is different from submission.update.version, which is for edits.

We don't document the schema of the audit log details.

Maybe now is the time to do so. If we're going to commit to there being an instanceId on the forum, I think we should do so in the API docs as well. That way, we'll know as developers what's considered part of the official API vs. what we can feel free to change.

Proposal: document for each audit log action what details users can assume will exist. For now, it could just be instanceId on submission.update. We could document this in the same place where we list the audit log events.

Another thing I'm wondering is whether we should backfill instanceId. I think it could be confusing for both developers and users if this detail property only sometimes existed. If we're not going to backfill it, I think we should document that.

[ktuite]

I'll double-check the QA server, but when I looked through the code, it looked like when the instance ID was added to the audit log from the submission def (this PR in 2023) the def never had an actual instance ID attached to it. When reviewing submissions (the only way the submission update happens, i think?) this function Submissions.getByIdsWithDef is used and it does not populate the def instance ID (added in 2022).

[matthew-white]

When reviewing submissions (the only way the submission update happens, i think?)

I also think that's the only way. The only writable field of the Submission frame is reviewState.

it looked like when the instance ID was added to the audit log from the submission def (this PR in 2023) the def never had an actual instance ID attached to it.

You're probably right that we've just never logged it. 2023 is pretty recent.

I know that submission.update events can be shown in the entity activity feed in Frontend. I took a quick look at the EntityActivity and EntityFeedEntry components, and it doesn't look like they reference the instanceId of a submission.update event. Maybe we just never really needed instanceId to be in the details, so we didn't notice that it wasn't being logged.