-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
implement revision control #136
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, and I think I understand it. I have two minor concerns, and I left comments about those.
or not self.instance | ||
or not request.user.has_perm("documents.change_document", self.instance) | ||
): | ||
expandable_fields = expandable_fields.copy().pop("revisions") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to use copy.deepcopy
here? When I was looking at DRF flex fields a while back, I remember them sometimes being more than a level deep.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good question, but in this case the only change I am ever making is removing the "revisions" key, which is only a change at the top level. Therefore, using a normal copy is sufficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If later on, a change alters something other than revisions, that could cause an unexpected change to the original. This might be a reasonable risk, but sometimes this creates a class of bug that is hard to figure out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the performance cost should be negligible in this case, I will add the deepcopy to future proof the code
self.document.pk, self.document.slug, self.version | ||
) | ||
if not storage.exists(destination): | ||
storage.copy(self.document.doc_path, destination) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hopefully, this never comes up, but since we're checking to see if something already exists for this version before copying, should we log/alert ourselves if something does already exist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a scenario in mind where this could happen in normal operation, but now cannot think of it. But I will add a log here so we can be aware if it happens.
Implement revision control for documents. Users with premium accounts may enable revision control. A revision is created upon turning it on, and then an additional revision is created for any destructive operation (re-processing, redacting, or page modification). The revisions can be viewed in the API for a document if you have edit access to the document by supplying a
expand=revisions
parameter. The revision documents are always private,regardless of if the document itself is public or private.