Fix facility sync elapsed time #12158
Conversation
Build Artifacts
|
|
Hi @AlexVelezLl, the fix is working correctly at 2024-05-13_12-25-07.mp4Also, we have the same sync feature at data.mp4 |
github-actions
bot
added
APP: Device
| for (const index in newTasks) { | ||
| const task = newTasks[index]; | ||
| if (this.taskIdsToWatch.includes(task.id)) { | ||
| if (task.status === TaskStatuses.COMPLETED) { | ||
| if ( | ||
| task.status === TaskStatuses.COMPLETED || |
There was a problem hiding this comment.
We werent taking into account that repeating tasks doesnt have a "completed" status when they are synced and thats why we hadnt the last_successful_sync value updated.
|
Thank you @pcenov! I have addressed the issue in both places. |
| return false; | ||
| } | ||
| const prevTask = prevTasks.find(({ id }) => id === task.id); | ||
| if (!prevTask) { |
There was a problem hiding this comment.
Hrm, wondering why there is this "previous task" at all, the API should only be returning a single job status from the backend for an id.
If it is happening, liable to be a frontend state persisting which would go away if we refreshed the page.
There was a problem hiding this comment.
Probably, the name should be improved. Here, by previousTasks, I mean the previous snapshot of the tasks list. The thing is that for repeating tasks, we never had the task with a "COMPLETED" status, and thereby this line is not executed, and we do not have the updated value of facility.last_successful_sync .
The problem is that repeating tasks keep the same reference for multiple executions, and they never end in a "COMPLETED" status because, after the "RUNNING" state, they pass to "QUEUED" again when the execution ends. So the way I found to determine if a single task run has been completed was comparing the current state of the task with the previous snapshot.
There was a problem hiding this comment.
Yeah - my point was that if this is working it's because we had data from a previous fetch where the task was in progress, so it's quite possible that on a refresh of the page we would still get an issue here.
I don't think this needs to be resolved now - but there's definitely some cleanup we can do overall for how we handle tasks on the frontend (probably through a core useTasks composable that consolidates a lot of this logic).
| TaskResource.clear(this.syncTaskId); | ||
| this.syncTaskId = ''; |
There was a problem hiding this comment.
Why did this need to be moved? The clear method returns a promise, so if we want to wait until after the clear we would need to await it.
There was a problem hiding this comment.
Oh no no, I am not sure the reason for setting an empty string to syncTaskId, but the thing was that if we first set this.syncTaskId = ''; and after we execute TaskResource.clear(this.syncTaskId);, we would be excecuting TaskResource.clear with an empty string, and because of this, it was throwing an exception, and the code below wasn't executing, and therefore the last sync value wasn't being updated.
There was a problem hiding this comment.
Ah, that makes a lot of sense, yes!
|
Hi @AlexVelezLl I confirm that the reported issues are fixed now and there are no new issues observed while manually testing. LGTM! |
Summary
Refresh elapsed time since last sync in the Facility sync status
References
Closes #10759
Reviewer guidance
Reproduce #10759
Testing checklist
PR process
Reviewer checklist
yarnandpip)