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.

Sign up for GitHub

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

Jump to bottom

[12.0] [IMP] dms: Attachment Integration #24

Merged
merged 2 commits into from
Oct 23, 2020
Merged

[12.0] [IMP] dms: Attachment Integration #24

merged 2 commits into from
Oct 23, 2020

Conversation

victoralmau
Copy link
Member

@victoralmau victoralmau commented Oct 5, 2020 •
edited
Loading

This feature aims to integrate dms with ir.attachment. That way, every time a new attachment is created, a dms.file is also created with a category according to the model the attachment is in.

Related to (V13): #9

Please @pedrobaeza review it

@Tecnativa TT25646

@victoralmau victoralmau mentioned this pull request Oct 5, 2020
Closed
@victoralmau victoralmau force-pushed the 12.0-imp-dms-attachment-integration branch from dcaa51f to eba83ce Compare October 5, 2020 15:20
@pedrobaeza pedrobaeza added this to the 12.0 milestone Oct 5, 2020
@pedrobaeza
Copy link
Member

Aren't you able to cherry-pick v13 commit and then put changes for v12 on top of? This way, you preserve the attribution.

ernestotejeda
ernestotejeda reviewed Oct 5, 2020
View reviewed changes
Copy link
Member

@ernestotejeda ernestotejeda left a comment

Choose a reason for hiding this comment

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

Code and functional review 👍 . Looks good.
But, I think you should include unit tests for the new functionality

@ernestotejeda
Copy link
Member

@victoralmau I got an error creating a file in an type-attachment storage:
Peek 2020-10-05 17-24

@victoralmau victoralmau force-pushed the 12.0-imp-dms-attachment-integration branch 4 times, most recently from ef18795 to b02fb0c Compare October 6, 2020 07:37
@victoralmau
Copy link
Member Author

@victoralmau I got an error creating a file in an type-attachment storage:
Peek 2020-10-05 17-24

Thanks for report this error, solved now.

@victoralmau
Copy link
Member Author

Code and functional review +1 . Looks good.
But, I think you should include unit tests for the new functionality

Thanks, althought in original PR not exist tests, added now.

@victoralmau victoralmau force-pushed the 12.0-imp-dms-attachment-integration branch 3 times, most recently from e292d64 to 024c1cd Compare October 6, 2020 10:15
@pedrobaeza pedrobaeza force-pushed the 12.0-imp-dms-attachment-integration branch from 024c1cd to 1b69efc Compare October 6, 2020 11:16
@victoralmau victoralmau force-pushed the 12.0-imp-dms-attachment-integration branch from 1b69efc to fad936e Compare October 6, 2020 11:36
@victoralmau victoralmau force-pushed the 12.0-imp-dms-attachment-integration branch 2 times, most recently from 5517baf to b1c650a Compare October 8, 2020 11:01
@pedrobaeza
Copy link
Member

Please don't solve conflicts through merge commits as stated other times.

@victoralmau victoralmau force-pushed the 12.0-imp-dms-attachment-integration branch 2 times, most recently from 1ebd5f3 to 98a4d68 Compare October 16, 2020 07:49
@victoralmau
Copy link
Member Author

Please don't solve conflicts through merge commits as stated other times.

Sorry, solved it (rebase)

@victoralmau victoralmau force-pushed the 12.0-imp-dms-attachment-integration branch from 98a4d68 to 17e4e52 Compare October 16, 2020 16:10
@victoralmau victoralmau mentioned this pull request Oct 20, 2020
Merged
@victoralmau victoralmau force-pushed the 12.0-imp-dms-attachment-integration branch from 17e4e52 to 6bc1fbe Compare October 21, 2020 08:34
@pedrobaeza pedrobaeza mentioned this pull request Oct 21, 2020
Closed
pedrobaeza
pedrobaeza requested changes Oct 21, 2020
View reviewed changes
dms/models/directory.py Outdated Show resolved Hide resolved
dms/models/directory.py Outdated Show resolved Hide resolved
dms/models/directory.py Outdated Show resolved Hide resolved
dms/models/directory.py Outdated Show resolved Hide resolved
dms/models/directory.py Outdated Show resolved Hide resolved
dms/models/dms_file.py Outdated Show resolved Hide resolved
dms/models/ir_attachment.py Outdated Show resolved Hide resolved
dms/models/ir_attachment.py Outdated Show resolved Hide resolved
dms/models/ir_attachment.py Outdated Show resolved Hide resolved
dms/models/storage.py Outdated Show resolved Hide resolved
@pedrobaeza
Copy link
Member

Put in the roadmap to add a migration procedure for converting an storage to attachment one for populating existing records with attachments as folders. Another roadmap point is to add a link from attachment view in chatter to linked documents.

@victoralmau victoralmau force-pushed the 12.0-imp-dms-attachment-integration branch 3 times, most recently from 861895d to 2c02657 Compare October 22, 2020 10:30
pedrobaeza
pedrobaeza requested changes Oct 22, 2020
View reviewed changes
dms/models/directory.py Show resolved Hide resolved
dms/models/dms_file.py Outdated Show resolved Hide resolved
dms/models/dms_file.py Outdated Show resolved Hide resolved
dms/models/ir_attachment.py Outdated Show resolved Hide resolved
dms/models/ir_attachment.py Outdated Show resolved Hide resolved
dms/models/ir_attachment.py Outdated Show resolved Hide resolved
dms/models/ir_attachment.py Outdated Show resolved Hide resolved
dms/models/ir_attachment.py Outdated Show resolved Hide resolved
@victoralmau victoralmau force-pushed the 12.0-imp-dms-attachment-integration branch 5 times, most recently from d52901b to 78b8d99 Compare October 22, 2020 14:09
pedrobaeza
pedrobaeza requested changes Oct 22, 2020
View reviewed changes
dms/models/directory.py Show resolved Hide resolved
dms/models/ir_attachment.py Show resolved Hide resolved
@victoralmau victoralmau force-pushed the 12.0-imp-dms-attachment-integration branch from 78b8d99 to 6fd248c Compare October 23, 2020 06:02
@pedrobaeza
Copy link
Member

OK, now it's good to continue. I merge as talked with @etobella by chat:

/ocabot merge major

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 12.0-ocabot-merge-pr-24-by-pedrobaeza-bump-major, awaiting test results.

@pedrobaeza
Copy link
Member

Now I think it's better to cherry-pick this into v13 than continue on the other PR. Please tell in the PR to see what the other contributors think.

@OCA-git-bot OCA-git-bot merged commit f64af72 into OCA:12.0 Oct 23, 2020
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at bd27a0f. Thanks a lot for contributing to OCA. ❤️

@victoralmau victoralmau mentioned this pull request Nov 6, 2020
Merged
@pedrobaeza pedrobaeza deleted the 12.0-imp-dms-attachment-integration branch August 8, 2023 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pedrobaeza pedrobaeza pedrobaeza approved these changes

@ernestotejeda ernestotejeda ernestotejeda approved these changes

Assignees
No one assigned
Labels
approved merged 🎉
Projects
None yet
Milestone
12.0
Development

Successfully merging this pull request may close these issues.
5 participants
@victoralmau @pedrobaeza @ernestotejeda @OCA-git-bot @luismalta