-
Notifications
You must be signed in to change notification settings - Fork 345
Add api endpoint to duplicate document #570
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
Conversation
src/backend/core/tests/documents/test_api_documents_media_auth.py
Outdated
Show resolved
Hide resolved
a138321
to
95241cd
Compare
@lunika done implementing your feedbacks 🙏 |
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 you have existing data the last migration does not succeed.
I got a bit confused about the with_accesses
as a query string instead of from the payload.
Except that works well! I used it frontend side, I challenged it a bit, works like excepted.
src/backend/core/migrations/0013_remove_is_public_add_field_attachments_and_duplicated_from.py
Outdated
Show resolved
Hide resolved
from core import enums | ||
|
||
|
||
def base64_yjs_to_xml(base64_string): |
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.
Nice to see it used ^^
95241cd
to
bddfd20
Compare
src/backend/core/migrations/0018_remove_is_public_add_field_attachments_and_duplicated_from.py
Outdated
Show resolved
Hide resolved
bddfd20
to
88c1cbb
Compare
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 didn't retry all the process, but the migration works fine now.
"""Extract text from base64 yjs document.""" | ||
|
||
blocknote_structure = base64_yjs_to_xml(base64_string) | ||
soup = BeautifulSoup(blocknote_structure, "html.parser") |
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.
Look if the lxml parser can be used. It seems faster.
88c1cbb
to
5fa6784
Compare
@lunika ready for final call. |
5fa6784
to
d4b5552
Compare
d4b5552
to
8fd1ec4
Compare
Documents content is stored in the Ydoc format. We need a util to extract it as xml/text.
These 2 actions had factorized code but a few iterations lead to spaghetti code where factorized code includes "if" clauses. Refactor abstractions so that code factorization really works.
Tests were forgotten. While writing the tests, I fixed a few edge cases like the possibility to connect to the collaboration server for an anonymous user.
Migration tests should not import and use factories or models directly from the code because they would not be in sync with the database in the state that each state needs to test it. Instead the migrator object passed as argument allows us to retrieve a minimal version of the models in sync with the state of the database that we are testing. What we get is a minimal model and we need to simulate all the methods that we could have on the real model and that are needed for testing.
We took this opportunity to refactor the way access is controlled on media attachments. We now add the media key to a list on the document instance each time a media is uploaded to a document. This list is passed along when a document is duplicated, allowing us to grant access to readers on the new document, even if they don't have or lost access to the original document. We also propose an option to reproduce the same access rights on the duplicate document as what was in place on the original document. This can be requested by passing the "with_accesses=true" option in the query string. The tricky point is that we need to extract attachment keys from the existing documents and set them on the new "attachments" field that is now used to track access rights on media files.
We can't prevent document editors from copy/pasting content to from one document to another. The problem is that copying content, will copy the urls pointing to attachments but if we don't do anything, the reader of the document to which the content is being pasted, may not be allowed to access the attachment files from the original document. Using the work from the previous commit, we can grant access to the readers of the target document by extracting the attachment keys from the content and adding themto the target document's "attachments" field. Before doing this, we check that the current user can indeed access the attachment files extracted from the content and that they are allowed to edit the current document.
The idea behind wrapping choices in `lazy` function was to allow overriding the list of languages in tests with `override_settings`. This was causin makemigrations to keep on including the field in migrations when it is not needed. Since we finally don't override the LANGUAGES setting in tests, we can remove it to fix the problem.
8fd1ec4
to
ff8b7e4
Compare
Purpose
We want users to be able to duplicate a document.
resolves #335 and #567
Proposal
While doing this work I was brought to refactor the way the
media-auth
andcollaboration-auth
actions were factorizing code and add missing tests for these actions.