Audit log #1324
Audit log #1324
Conversation
ramonski
added
Addition ➕
PR: Not Ready ⛔️
P3: Important
Version 1.3
|
I think after migrating to the new audit log, we can completely remove the old "Log" tab. Actually we could even provide a mechanism to link to a specific version like we do with the calculations at the moment. Something like this "fakes" an object with a given version: from senaite.core.supermodel import SuperModel
model = SuperModel(self.context)
model.data = { ... } # set the internal data dictionary with the snapshot data |
There was a problem hiding this comment.
Awesome and beautiful work!. I've just left some minor comments.
Agree to remove "Log" tab and display this new view instead.
Also, would be nice to remove the "Log" section from the Analysis "info" popup and display the audit log instead (having to go to the analysis audit log through the url is not easy).
Remember to edit CHANGES.rst. Besides the addition of this audit-log functionality, remember to mention that the auto-save in Sample view has been removed!.
| ("time", { | ||
| "title": _("Time"), "sortable": False}), | ||
| ("modified", { | ||
| "title": _("Date Modified"), "sortable": False}), |
There was a problem hiding this comment.
As far as I understand, time displays the exact time the snapshot took place, while modified displays the value for the attribute with same name from the object/brain. I think both fields will always have same value (maybe with 1 sec. of difference). I would suggest to display one of the two in the listing (I prefer "modified", but with _("Time") as the column title).
There was a problem hiding this comment.
time comes actually from the review history and is there to simply pass in the whole review history when creating the snapshot. Since no modification takes place in this case, the time shows the timestamp of the workflow transition
| ("modified", { | ||
| "title": _("Date Modified"), "sortable": False}), | ||
| ("actor", { | ||
| "title": _("Actor"), "sortable": False}), |
There was a problem hiding this comment.
We also need to display the full name of the actor when the modification took place.
bika/lims/browser/auditlog.py
Outdated
|
|
||
| :returns: List of snapshots | ||
| """ | ||
| from bika.lims.subscribers.auditlog import get_storage |
There was a problem hiding this comment.
Why to not move this import at the top of the class?
There was a problem hiding this comment.
Yes, I named the method before the same as the import. However, I left it there because I don't like to import from subscribers and this reminds me to refactor that block
|
|
||
| # Modification Date | ||
| m_date = metadata.get("modified") | ||
| item["modified"] = self.to_localized_time(m_date) |
There was a problem hiding this comment.
See my comment re "time" vs "modified" above. I would only display one of the two
There was a problem hiding this comment.
Yes, but unfortunately no modification takes place on wf transitions...
bika/lims/subscribers/auditlog.py
Outdated
| return | ||
|
|
||
| # take a new snapshot | ||
| take_snapshot(obj) |
There was a problem hiding this comment.
I would do the following here:
metadata = dict()
if IObjectEditedEvent.providedBy(event):
metadata["action"] = "Edit"
# Take a new snapshot
take_snapshot(obj, **metadata)
bika/lims/subscribers/auditlog.py
Outdated
| return | ||
|
|
||
| # take a new snapshot | ||
| take_snapshot(obj) |
There was a problem hiding this comment.
I would do:
# take a new snapshot
take_snapshot(obj, action="Created")There was a problem hiding this comment.
Good change, will do that 👍
Exciting idea! Better in a new PR though! |
bika/lims/upgrade/v01_03_001.py
Outdated
| from bika.lims.api.security import get_user | ||
| from bika.lims.api.security import get_roles | ||
| from DateTime import DateTime | ||
| from bika.lims.interfaces import IAnalysis |
There was a problem hiding this comment.
Can you move this imports at the top or is there any reason why you have to add them here?
There was a problem hiding this comment.
Same as above. I think the has_snapshots, take_snapshot, is_auditable should live somewhere else...
These views appear when accessing the audit log
Description of the issue/feature this PR addresses
This PR adds a full Audit Log to SENAITE.
Every time a new object is created or an existing object is modified, a snapshot is taken with the help of
senaite.core.supermodel. The object is automatically marked with anIAuditableinterface and a new tab "Audit Log" is displayed, where the differences of the current version to the previous version can be analysed.An upgrade step migrates all existing contents to provide a default audit trail which contains one snapshot per review history entry. However, Analyses are skipped in this process to make the upgrade step run faster.
Current behavior before PR
Log tab only shows workflow changes
Desired behavior after PR is merged
Audit log tab shows full audit trail of the object
--
I confirm I have tested this PR thoroughly and coded it according to PEP8
and Plone's Python styleguide standards.