artefactual-sdps / enduro

A tool to support ingest and automation in digital preservation workflows
https://enduro.readthedocs.io/
Apache License 2.0
4 stars 3 forks source link

Add timestamps to Preservation tasks #842

Closed Diogenesoftoronto closed 5 months ago

Diogenesoftoronto commented 5 months ago

This PR aims to add timestamps to Preservations Tasks and give additional detail to the user in the enduro dashboard about when archivematica jobs started and finished.

Closes #832

Diogenesoftoronto commented 5 months ago

I've added a few comments. Have you confirmed that the timestamps are being properly recorded? In the savePreservationTasks method, I see the following:

      pt := ConvertJobToPreservationTask(job)
      pt.PreservationActionID = jt.presActionID

      now := sql.NullTime{Time: jt.clock.Now(), Valid: true}
      pt.StartedAt = now
      pt.CompletedAt = now

With your changes, ConvertJobToPreservationTask is populating the timestamps but they're set to now right after. If you tested this PR and observed the timestamps recorded I suppose that'd become apparent.

Only thing I am worried about for this is that the tasks could very well be completed before this current time such that if any task that would have had a different completedAt time would have now as their time. I think this is a problem. Too fix this I could compare to check if the first value in the list is actually after the time completed. If it is then we know the value is both not null and the time that the task actually completed.

Diogenesoftoronto commented 5 months ago

I've added a few comments. Have you confirmed that the timestamps are being properly recorded? In the savePreservationTasks method, I see the following:

        pt := ConvertJobToPreservationTask(job)
        pt.PreservationActionID = jt.presActionID

        now := sql.NullTime{Time: jt.clock.Now(), Valid: true}
        pt.StartedAt = now
        pt.CompletedAt = now

With your changes, ConvertJobToPreservationTask is populating the timestamps but they're set to now right after. If you tested this PR and observed the timestamps recorded I suppose that'd become apparent.

Only thing I am worried about for this is that the tasks could very well be completed before this current time such that if any task that would have had a different completedAt time would have now as their time. I think this is a problem. Too fix this I could compare to check if the first value in the list is actually after the time completed. If it is then we know the value is both not null and the time that the task actually completed.

I fixed this concern. I also added some of the tests. There is better test coverage and I individually tested both of the functions to make sure they return the correct times.

Diogenesoftoronto commented 5 months ago

It looks great @sevein, I didn't realize you were suggesting removing the null check here, but I suppose that makes sense. Thanks for implementing the solutions to the tests as well. They look great. I have rebased and formatted the commit. It is ready to merge.