DEVPROD-4661: Correctly update has_annotations field in task annotation REST routes
#8545
Conversation
| @@ -36,7 +36,7 @@ func MoveIssueToSuspectedIssue(taskId string, taskExecution int, issue annotatio | |||
| if err != nil { | |||
| return errors.Wrapf(err, "finding and modifying task annotation for execution %d of task '%s'", taskExecution, taskId) | |||
| } | |||
| if annotation != nil && len(annotation.Issues) == 0 { | |||
There was a problem hiding this comment.
This was giving me a lint error that annotation cannot be nil by nature of how it's defined, so it's redundant
| @@ -152,8 +152,12 @@ func UpsertAnnotation(a *annotations.TaskAnnotation, userDisplayName string) err | |||
| ); err != nil { | |||
| return errors.Wrapf(err, "adding task annotation for task '%s'", a.TaskId) | |||
| } | |||
| if a.Issues != nil && len(a.Issues) > 0 { | |||
| return SetHasAnnotations(a.TaskId, a.TaskExecution) | |||
| if a.Issues != nil { | |||
There was a problem hiding this comment.
similarly, you don't need to check nil here -- length of the list will just return 0 if its nil
There was a problem hiding this comment.
ended up having to keep the nil check because it broke the existing tests (that someone previously added)
model/task/task_annotations.go
Outdated
| return SetHasAnnotations(a.TaskId, a.TaskExecution) | ||
| if a.Issues != nil { | ||
| if len(a.Issues) == 0 { | ||
| return UnsetHasAnnotations(a.TaskId, a.TaskExecution) |
There was a problem hiding this comment.
if this is too big of a refactor for this PR we can split a ticket, but i feel like we should just have one function, that sets annotations to true or false based on the length of issues (or at least that takes in a boolean, so we could just pass through len(a.Issues)==0 as the bool), rather than having this if statement
hasAnnotations := len(a.Issues) > 0
return UpdateHasAnnotations(a.TaskId, a.TaskExecution, hasAnnotations)
minnakt
changed the title
UnsetHasAnnotations in task annotation REST routeshas_annotations field in task annotation REST routes
DEVPROD-4661
Description
This task is returning Known Issue because the value of the cached field
has_annotationsistrue, even though thetask_annotationscollection has no issues recorded for this task and execution. This is a bug where thetask.has_annotationsfield is out of sync with thetask_annotationscollection.I could not replicate this bug through the GraphQL resolvers. When I added, moved, and removed issues from the UI, the
has_annotationsfield was always correctly in sync.The only time I could replicate this bug was by using the task annotations PUT rest route to clear the task annotations. The PUT route does not call
UnsetHasAnnotations, which can lead to thehas_annotationsfield being out of sync.Testing