Add api endpoint to duplicate document #570
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lunika
reviewed
Jan 22, 2025
src/backend/core/tests/documents/test_api_documents_media_auth.py
Outdated
Show resolved
Hide resolved
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.
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.
sampaccoud
force-pushed
the
add-api-endpoint-to-duplicate-document
branch
from
a138321 to
95241cd
Compare
|
@lunika done implementing your feedbacks 🙏 |
AntoLC
reviewed
Jan 28, 2025
Comment on lines
+12
to
+22
| def populate_attachments_on_all_documents(apps, schema_editor): | ||
| """Populate "attachments" field on all existing documents in the database.""" | ||
| Document = apps.get_model("core", "Document") | ||
|
|
||
| for document in Document.objects.all(): | ||
| document.attachments = extract_attachments(document.content) | ||
| document.save(update_fields=['attachments']) | ||
|
|
||
|
|
||
| class Migration(migrations.Migration): | ||
|
|
There was a problem hiding this comment.
When I try to apply the migration I get AttributeError: 'Document' object has no attribute 'content'.
It could be interesting to test the migration, there is a nice lib to do it django-test-migrations.
If you are interested, here some examples:
7e05800
5711bf5#diff-b94dbfcff3e13c1f70f5c42f5c071dde9fb1a4f884f80fe18a6ce522d5019476
There was a problem hiding this comment.
Exact, I have the same error ! The first time I applied the migration I didn't have documents saved !
| document to allow cross-access. | ||
|
|
||
| Optionally duplicates accesses if `with_accesses` is set to true | ||
| in the payload. |
There was a problem hiding this comment.
As it is now, with_accesses is not set from the payload but from the query string.
Why not using the payload btw ?
There was a problem hiding this comment.
No reason. Do you think it's better in the payload? I think you're right...
| @@ -395,9 +444,26 @@ class Document(BaseModel): | |||
| blank=True, | |||
| null=True, | |||
| ) | |||
| duplicated_from = models.ForeignKey( | |||
There was a problem hiding this comment.
Does this column have a purpose in the system or is it just to get some information ?
There was a problem hiding this comment.
For the moment just to keep the information as it seems legit. It is not being used for display or anything at the moment.
AntoLC
reviewed
Jan 28, 2025
| from core import enums | ||
|
|
||
|
|
||
| def base64_yjs_to_xml(base64_string): |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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-authandcollaboration-authactions were factorizing code and add missing tests for these actions.