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

[14.0][ADD] dms_version: DMS Version Control #265

Open
wants to merge 7 commits into
base: 14.0
Choose a base branch
from

Conversation

yweng8111
Copy link

@yweng8111 yweng8111 commented Oct 30, 2023

Superseed: #177
DMS Version Control

I have made some changes from an old PR, that are designed by @marylla

@pedrobaeza
Copy link
Member

There's something weird in the commit history

@yweng8111 yweng8111 force-pushed the 14.0-add-dms_version branch 2 times, most recently from da42c73 to 80f1a25 Compare October 30, 2023 17:50
@marylla
Copy link
Contributor

marylla commented Nov 1, 2023

Here is my description of the design to explain what we wanted to achieve:

Configuration

  • allow versioning on storage level (only for type "database" and "filestore" -> NOT "attachment" currently)
  • allow versioning on directory level
  • allow versioning on file level
  • allow changing the versioning setting later only for people with special group + track change in chatter of dms file

DMS Files

  • link the "origin" file when creating a new version
    • If a file is newly created, origin is self
    • If a version is created for a file, origin is copied (so that the entire history of a file knows which file was the origin)
  • use active-boolean to mark newest version
    • There is only one active file for all files with the same origin (unique contraint).
    • Files with active = False are readonly to avoid creating new versions from archived files.
  • link the "parent" file when creating a new version
    • If a file is newly created, parent is False.
    • If a version is created for a file, parent is the "old" file.
  • count the number of versions for files with the same origin_id (unique contraint for version number and origin)
  • only show files with active=True in tree and kanban views
  • track changes of almost all fields of a file
  • add a smartbutton in file form view to show all versions with the same orgin (attention: show archived files)
  • add icon in kanban view if a file is not the origin (to mark a file as modified)
  • color file in yellow in tree view that is not the origin (to mark a file as modified)
  • add a filter to show all modified files (parent != False)
  • add a filter to show all not-modified files (parent = False)
  • add a filter to show all archived files (active=False)
  • do not allow to use the usual action "Unarchive" - instead use the restore button in archived files

Versioning

  • change the content field of a file to create a new version
    • do not allow to change the content if the file is active=False
    • The old file is the parent of the new file.
    • The old file is now active=False.
    • The new file is active=True.
    • A chatter message is created in the new file to know from which file it was created.
    • A chatter message is created in the parent file to know which file was generated from it.
    • A chatter message is created in the origin file (if origin != parent) to see the entire history of all versions in the origin file.

Restoring

  • click a button in form or tree view to restore an archived file
    • The old archived file is now active=True.
    • If an active=True file existed with the same origin it now is active=False.
    • A chatter message is created in the previously archived and now restored file.
    • A chatter message is created in the previously active and now archived file (if one existed).
    • A chatter message is created in the origin file (if origin != restored file and origin != previous active file).

image

@marylla
Copy link
Contributor

marylla commented Nov 1, 2023

I will test as soon as @yweng8111 fixed the commit history and the other PR errors.

@marylla
Copy link
Contributor

marylla commented Nov 1, 2023

@agent-z28 FYI

@yweng8111 yweng8111 force-pushed the 14.0-add-dms_version branch 2 times, most recently from 2170603 to 8eeb0ca Compare November 7, 2023 15:04
@yweng8111
Copy link
Author

yweng8111 commented Nov 8, 2023

@pedrobaeza I've already solved that problem in commit history. But there are still some errors in pre-commit, that I don't get locally. Did I do something wrong?

@pedrobaeza
Copy link
Member

You need to execute the copier update to get a newest template fixing the pre-commit problem. Please do it in a separate PR, and once merged, you can rebase here.

@yweng8111
Copy link
Author

yweng8111 commented Nov 20, 2023

You need to execute the copier update to get a newest template fixing the pre-commit problem. Please do it in a separate PR, and once merged, you can rebase here.

Thanks for the Tip.
I've just used the default setting for "copier update" is this correct?

$ copier update --trust
Updating to template version 1.19.2
🎤 Which Odoo version are we deploying in this branch?                                                                                                                                                                                        
   14.0                                                                                                                                                                                                                                       
🎤 What's the organization slug? If you are creating https://github.com/OCA/server-tools, then write "OCA" here.                                                                                                                              
   OCA                                                                                                                                                                                                                                        
🎤 Tell me the Organization name. It's supposed to be human-readable. It will be used in the author key of the __manifest__ files.                                                                                                            
   Odoo Community Association (OCA)                                                                                                                                                                                                           
🎤 What's the repo slug? If you are creating https://github.com/OCA/server-tools, then write "server-tools" here.                                                                                                                             
   dms                                                                                                                                                                                                                                        
🎤 Tell me the project name. It's supposed to be human-readable. So, server-tools project could be named like "Tools for server environment(s)"                                                                                               
   Document Management System modules for Odoo                                                                                                                                                                                                
🎤 Tell me the project website. It will be used in the website key of the __manifest__ files.                                                                                                                                                 
   https://github.com/OCA/dms                                                                                                                                                                                                                 
🎤 Please write a short description about what this repo is about.                                                                                                                                                                            
   OCA modules for DMS                                                                                                                                                                                                                        
🎤 Which CI system to use ?                                                                                                                                                                                                                   
   GitHub                                                                                                                                                                                                                                     
🎤 Which Odoo flavor should be used for CI tests ?                                                                                                                                                                                            
   Both                                                                                                                                                                                                                                       
🎤 Use pyproject.toml instead of setup.py.                                                                                                                                                                                                    
   No                                                                                                                                                                                                                                         
🎤 Generate requirements.txt from addons manifests and optional overrides in setup.py files.                                                                                                                                                  
   Yes                                                                                                                                                                                                                                        
🎤 Use ruff and ruff-format instead of flake8, autoflake, pyupgrade, isort, black.                                                                                                                                                            
   No                                                                                                                                                                                                                                         
🎤 Are there in this repo modules that don't get along with their friends? If so, list them here (YAML format) and they will be tested in separate jobs.
Beware, if rebel modules should stay separated in groups, you should join them with commas, which could be misinterpreted by YAML.
Example: ["rebel_module_1,rebel_module_2", even_more_rebel_module]                                                                                                                                                                            
   []                                                                                                                                                                                                                                         
🎤 Do you need to install wkhtmltopdf? Usually only needed if you're going to test PDF report generation.                                                                                                                                     
   No                                                                                                                                                                                                                                         
🎤 GitHub action checks the minimum development status?                                                                                                                                                                                       
   Yes                                                                                                                                                                                                                                        
🎤 GitHub action checks the manifest license?                                                                                                                                                                                                 
   Yes                                                                                                                                                                                                                                        
🎤 GitHub action runs codecov/codecov-action?                                                                                                                                                                                                 
   Yes                                                                                                                                                                                                                                        
🎤 GitHub action updates .pot files?                                                                                                                                                                                                          
   Yes                                                                                                                                                                                                                                        
🎤 Create GitHub 'stale' action?                                                                                                                                                                                                              
   Yes                                                                                                                                                                                                                                        
🎤 Any extra environment variables to inject into the CI tests

Example: {"KEY": "VALUE"}                                                                                                                                                                                                                     
   {}                                                                                                                                                                                                                                         
🎤 convert_readme_fragments_to_markdown (bool)                                                                                                                                                                                                
   No

@pedrobaeza
Copy link
Member

The copier template already exists. You just need to update it (copier -f --trust).

@yweng8111 yweng8111 force-pushed the 14.0-add-dms_version branch 3 times, most recently from af1f457 to 3e587aa Compare November 21, 2023 19:29
@yweng8111
Copy link
Author

@marylla @agent-z28 Please Review

@marylla
Copy link
Contributor

marylla commented Nov 30, 2023

@yweng8111 I don't remember if we talked about this before, but... the "has versioning" is not automatically set in directories if the storage type is "attachment". And even if I change it manually, the newly created subdirectories and their files won't get the "has versioning" setting.

Did we forget this or was there a reason why we didn't implement it?

@yweng8111
Copy link
Author

yweng8111 commented Dec 8, 2023

Did we forget this or was there a reason why we didn't implement it?

No we don't forget this. I have commented it in the estimation. I will send you the Link later.

@marylla
Copy link
Contributor

marylla commented Dec 8, 2023

Okay, for all people interested in dms_version module... it is only implemented for storage type "database" and "filestore"...
because the attachment in attachment-storages is not saved in dms.file but in ir.attachment. We would have to make complex changes and wanted to start with the simpler cases...

but we're happy if someone has an idea how we can develop this... @victoralmau maybe you want to intervene here?

@victoralmau
Copy link
Member

Okay, for all people interested in dms_version module... it is only implemented for storage type "database" and "filestore"... because the attachment in attachment-storages is not saved in dms.file but in ir.attachment. We would have to make complex changes and wanted to start with the simpler cases...

but we're happy if someone has an idea how we can develop this... @victoralmau maybe you want to intervene here?

In the initial approach I thought in #177 it was only thought for storage type database or filestore and I think it is the most reasonable. Considering the use case of attachments would be out of the initial scope (in my opinion) since you would have to archive attachments (something that probably has a lot of side effects).

PS: It is very difficult for me to know the changes that have been done since #177 because they have not kept the initial commits and added an extra one with the corresponding changes.

@len-foss
Copy link

len-foss commented Dec 16, 2023

If you really wanted to implement versioning on attachments, it would be possible to have a record "dms.file.history" and move the attachments to it, but that would require some work and also require more configuration with the attachment storages.

If think it's fine to skip versioning on attachment. It should just be written upfront in the module documentation, README, etc.
There's just a slight problem with the current implementation: you can set "versioning to true" on a DMS Storage of type attachment, and similarly on a directory on such a storage, which means risks of misconfiguration and confusing issues. I think there should be a python constraint to avoid such risks (see 9528309).

I've tested the module in v16 in conjunction with #275
There needs to be an additional 1 line glue module to make both work correctly by the way (the storage_path needs to be reset during the revision copy).

The main issue I had debugging this is that the stack of write is insanely long, due to the various hacks that both base_revision and dms perform. To manage that I think there is one patch in dms that really improves readability and performance (77c4efa and c899326).

Assuming create_revision works correctly, 8b6999b skip the check so that archiving the current revision is taken care of in base_revision, reducing further the number of calls to write. I think this commit is more debatable, but what it changes is already kind of a hack, and it's much better in terms of performance.

There was a small bug in that you could not unarchive a file that had no revision, fixed in dfdfbbc.

Last I changed the create to work in batch in 0eb2226.
I'm wondering: can't we remove altogether the writing of origin_id in the create case? I don't see its purpose. However I'm not familiar with base_revision so there may be a good reason.

Sorry for the wall of text, check https://github.com/lambdao-dev/dms/commits/14.0-add-dms_version-len/ for the code :-)

@marylla
Copy link
Contributor

marylla commented Feb 1, 2024

@yweng8111 Can you please have a look at what @len-foss and @victoralmau wrote.

  • We should write in the Readme that it is not working for attachment storages.
  • We should add the constraint that you cannot set / see the has_versioning boolean in attachment storages, its directories and files - so that the user won't be confused.
  • Can you check if you can keep the initial commits from the original pull request as @victoralmau suggested?
  • Do we need the origin_id at the first origin dms file for some filters or domains? Can we skip setting it at the creation process or do we need it?
  • Can you also check the archiving / restoring process in base_revision and if we can use it for the current version and dms files without versions?

I am not very familiar with the technical background of what @len-foss described regarding the "stack of write is insanely long". Can you please check this and get it somehow integrated in your code changes? He mentioned a patch that maybe does the trick.

@yweng8111
Copy link
Author

yweng8111 commented Feb 21, 2024

  • [ v ] We should write in the Readme that it is not working for attachment storages.
  • [ v ] We should add the constraint that you cannot set / see the has_versioning boolean in attachment storages, its directories and files - so that the user won't be confused.
  • [ v ] Can you check if you can keep the initial commits from the original pull request as @victoralmau suggested?
  • [ v ] Do we need the origin_id at the first origin dms file for some filters or domains? Can we skip setting it at the creation process or do we need it?

I just follow your design and we need it also for the one2many field "all_revision_ids"

  • [ ? ] Can you also check the archiving / restoring process in base_revision and if we can use it for the current version and dms files without versions?

I don't understand what you mean. Can you show me later? @marylla

I am not very familiar with the technical background of what @len-foss described regarding the "stack of write is insanely long". Can you please check this and get it somehow integrated in your code changes? He mentioned a patch that maybe does the trick.

I have integrated some code from @len-foss, Thanks for the tips.
But I don't want to do some changes like the write() function in core module "dms" in this PR.

Copy link

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

@github-actions github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Jun 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale PR/Issue without recent activity, it'll be soon closed automatically.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants