chore: add audit log tests #4764
Conversation
| // technically, it's a map, so we adjust the type here. | ||
| type ExtendedAuditLog = Omit<AuditLog, "additional_fields"> & { | ||
| additional_fields: Record<string, string> | ||
| } |
There was a problem hiding this comment.
Not proud of this lol
I am not sure how to amend the BE type - the DB needs it stored as JSON which seems to correspond to string when make gen is run. Open to suggestions.
There was a problem hiding this comment.
Not totally sure but gonna check it out! This seems like a reasonable workaround to me for the time being though.
There was a problem hiding this comment.
Edit: ignore, see next comment
So we could get closer by adding:
case "encoding/json.RawMessage":
return TypescriptType{ValueType: "object"}, nil
to scripts/apitypings/main.go:691. But that gets us object which we would still need to typecast (better than string though).
To fully resolve I think we might have to type the thing fully with every possible additional field of using json.RawMessage and make every property optional.
Or we have to add separate audit log types for every type of audit log that has additional fields and on the frontend we check which it is with some kind of function (a: AuditLog) => a is WorkspaceBuildAuditLog type of deal.
I feel like making it an object and casting is pretty decent for now though.
@Emyrk just in case you have some insight
There was a problem hiding this comment.
Er wait looks like Record<string, string> is working without explicitly needing to add workspaceName to the types so we can just add this to scripts/apitypings/main.go:691:
case "encoding/json.RawMessage":
return TypescriptType{ValueType: "Record<string, string>"}, nil
coderd/audit.go
Outdated
| workspaceBytes := []byte(alog.AdditionalFields) | ||
| var workspaceResourceInfo WorkspaceResourceInfo | ||
| _ = json.Unmarshal(workspaceBytes, &workspaceResourceInfo) | ||
| str += " for workspace " + workspaceResourceInfo.WorkspaceName | ||
| err := json.Unmarshal(workspaceBytes, &workspaceResourceInfo) | ||
| if err != nil { | ||
| api.Logger.Error(ctx, "could not unmarshal workspace name for friendly string", slog.Error(err)) | ||
| } | ||
| str += " for " + workspaceResourceInfo.WorkspaceName |
There was a problem hiding this comment.
What do you think about doing:
| workspaceBytes := []byte(alog.AdditionalFields) | |
| var workspaceResourceInfo WorkspaceResourceInfo | |
| _ = json.Unmarshal(workspaceBytes, &workspaceResourceInfo) | |
| str += " for workspace " + workspaceResourceInfo.WorkspaceName | |
| err := json.Unmarshal(workspaceBytes, &workspaceResourceInfo) | |
| if err != nil { | |
| api.Logger.Error(ctx, "could not unmarshal workspace name for friendly string", slog.Error(err)) | |
| } | |
| str += " for " + workspaceResourceInfo.WorkspaceName | |
| str += " for {target}" |
Then the frontend can replace("{target}", auditLog.additional_fields.workspaceName) which lets the backend put the target wherever it wants if we want to change up this string in the future (rather than requiring it be the last item).
There was a problem hiding this comment.
much simpler; lets me remove the logger, too.
There was a problem hiding this comment.
lets me remove the logger
good point!!
| // remove the "{target}" identifier in the string description as we don't use it | ||
| const amendedDescription = auditLog.description.substring( | ||
| 0, | ||
| auditLog.description.lastIndexOf(" "), | ||
| ) | ||
| return amendedDescription | ||
| .replace("{user}", `<strong>${auditLog.user?.username}</strong>`) | ||
| .replace( | ||
| auditLog.additional_fields.workspaceName, | ||
| `<strong>${auditLog.additional_fields.workspaceName}</strong>`, | ||
| ) |
There was a problem hiding this comment.
I think the replace on the name here will not match the name since it was stripped out in amendedDescription
If workspace name is hello:
"admin stopped workspace build for hello" > "admin stopped workspace build for".replace("hello", "<strong>hello</strong>") == admin stopped workspace build for
If workspace name is workspace:
"admin stopped workspace build for workspace" > "admin stopped workspace build for".replace("workspace", "<strong>workspace</strong>") == admin stopped workspace build for
If we make the {target} change in my other comment we could do the replacement with something like:
let target = auditLog.resource_target.trim()
if (auditLog.resource_type === "workspace_build") {
target = auditLog.additional_fields.workspaceName
}
return auditLog.description
.replace("{user}", `<strong>${auditLog.user?.username.trim()}</strong>`)
.replace("{target}", `<strong>${target}</strong>`)There was a problem hiding this comment.
Ooof, just realized this had the exact same issue you already called out previously - sorry to make you type it out again! I guess I didn't have my coffee this morning.
There was a problem hiding this comment.
wrote tests in penance
😂 😂 😂
| // where target is a workspace instead of the workspace build, | ||
| // passed in on the FE via AuditLog.AdditionalFields rather than derived in request.go:35 |
This branch cleans up a couple of items: