Skip to content
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

document history/versioning in Core #2124

Closed
wants to merge 3 commits into from
Closed

document history/versioning in Core #2124

wants to merge 3 commits into from

Conversation

aruizramon
Copy link
Contributor

A few open items for discussion:

  • What interval do we want the comparison/sorting to be on?
  • Do we want to keep the version trail if a document is deleted, or delete the history as well? The latter is already built.
  • Opt-in for versioning...currently it's module-by-module in a doctype "Document Versioning Settings". We could just as easily add a check in DocType.
  • The test is one long test covering both insert and update. It's hard to break it up since insertion can be tested in an update test anyways.

@@ -0,0 +1,152 @@
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why call this temp?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is, by definition, temporary.

Since actually figuring out which fields have changed shouldn't happen right when the user saves (can take too long), the old and new versions of the doc are dumped (as JSON) into the doc_history_temp entry. A worker queue processes these entries on whichever interval we decide on, makes the appropriate history entries, and then deletes the doc_history_temp record.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But does the user expect the changes to be shown instantly? Also is there a UI compontent to this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To your first point...I don't think so. Obviously, when testing this out, the user wants to see immediate feedback. But how many actual use cases are there when a user changes something, then immediately needs to see feedback for, as an example, pricing history? I think the short queue is plenty fast for real use as a tradeoff to avoid longer save times. Of course, if you feel otherwise, it's trivial to switch it to make the entry before save() is complete.

There is no UI component to this. There definitely could be reports built into the product in the future, but that's a whole new topic.

@@ -0,0 +1,33 @@
<!-- jinja -->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better make a "Single" type doctype instead of a page?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a single doctype - the html is just a display layer.

Due to the nature of users having modules in custom apps, and possibly needing to keep a history trail, getting all modules that exist and displaying them makes more sense. It's definitely crude, but it's an implementation with very little dependencies.

@rmehta
Copy link
Member

rmehta commented Oct 5, 2016

@aruizramon can you add some screeshots, use cases on how the user will view, update history?

There is also a Version DocType I created a long time back that saves all versions (as JSON). Can we extend that?

@aruizramon
Copy link
Contributor Author

@rmehta definitely.

I don't think users should be able to update the history at all - seems like each entry should only have delete permissions for the admin rather than edit.

The Version DocType looks like it's only creating entries for the Website module. I'll dig a bit deeper there and see if extending it makes sense.

Copy link
Member

@rmehta rmehta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aruizramon can you add some UI / screenshot on how the user can view the history / changes?

@rmehta rmehta closed this Dec 22, 2016
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants