Improve doc move permissions#2219
Open
ZouicheOmar wants to merge 7 commits intosuitenumerique:mainfrom
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
WalkthroughAdds a new DocumentViewSet.detach POST action (transactional) to move a non-root document under the root as its last sibling, with explicit 400 responses for invalid targets and permission checks using a new Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/frontend/apps/e2e/__tests__/app-impress/doc-grid-move.spec.ts (1)
123-125:⚠️ Potential issue | 🟠 MajorUpdate expected overlay messages to match current permission copy.
Line 123 and Line 173 still assert old text. The grid now shows “administrator” for drag permission and “at least an editor” for drop permission, so these assertions will fail despite correct behavior.
Suggested test fix
- await expect(dragOverlay).toHaveText( - 'You must be at least the administrator of the target document', - ); + await expect(dragOverlay).toHaveText( + 'You must be at least an editor of the target document', + ); - await expect(dragOverlay).toHaveText( - 'You must be the owner to move the document', - ); + await expect(dragOverlay).toHaveText( + 'You must be an administrator to move the document', + );Also applies to: 173-175
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/apps/e2e/__tests__/app-impress/doc-grid-move.spec.ts` around lines 123 - 125, Update the two test assertions that still expect the old permission copy: replace the string in the expect(dragOverlay).toHaveText(...) call (referenced as dragOverlay) from "You must be at least the administrator of the target document" to the current copy "You must be an administrator of the target document", and update the other assertion (the one around lines 173-175 that asserts drop permission) to expect "You must be at least an editor of the target document" so both expectations match the current UI copy.src/frontend/apps/impress/src/features/service-worker/plugins/ApiPlugin.ts (1)
182-213:⚠️ Potential issue | 🔴 CriticalOffline-created
Docabilities are missing requireddetach.
newResponseis typed asDoc, but theabilitiesobject omitsdetach(a required boolean field). This creates a TypeScript type-contract violation that will prevent compilation.Suggested fix
abilities: { accesses_manage: true, accesses_update: true, accesses_view: true, ai_proxy: true, ai_transform: true, ai_translate: true, attachment_upload: true, children_create: true, children_list: true, collaboration_auth: true, comment: true, destroy: true, duplicate: true, + detach: true, favorite: true, invite_owner: true, link_configuration: true, media_auth: true, move: true,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/apps/impress/src/features/service-worker/plugins/ApiPlugin.ts` around lines 182 - 213, The abilities object on the offline-created Doc (newResponse) is missing the required boolean field detach, causing a TypeScript type error; update the abilities literal inside ApiPlugin.ts where newResponse is constructed (the variable named newResponse of type Doc) to include detach: false (or appropriate default true/false) alongside the other ability flags so the abilities property satisfies the Doc type contract.src/frontend/apps/e2e/__tests__/app-impress/doc-tree.spec.ts (1)
272-330:⚠️ Potential issue | 🟠 MajorThis test now asserts the opposite of the new detach rule.
After Line 298, the current user is downgraded to
Editor, but on Lines 303-307 they createdocChildthemselves. That makes this an editor-creator scenario, so"Move to my docs"should be enabled here. If you want the disabled path, the child needs to be created by someone else instead.Proposed fix
- test('At least administrator to detach a document', async ({ + test('An editor creator can detach a document', async ({ page, browserName, }) => { @@ await expect( page.getByRole('menuitem', { name: 'Move to my docs' }), - ).toHaveAttribute('aria-disabled', 'true'); + ).not.toHaveAttribute('aria-disabled', 'true'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/apps/e2e/__tests__/app-impress/doc-tree.spec.ts` around lines 272 - 330, The test currently downgrades the current user to Editor then creates docChild as that same user via createRootSubPage(page, browserName, ...), which makes the user the creator and enables "Move to my docs"; to test the disabled path, create the child as the other member instead: change the createRootSubPage call to use otherBrowserName (i.e., createRootSubPage(page, otherBrowserName, 'doc-tree-detach-child')) or otherwise ensure docChild is created by the other user so the final expectation that "Move to my docs" has aria-disabled="true" is valid.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/backend/core/api/viewsets.py`:
- Around line 1008-1013: When handling moves in the viewset (the branch using
target_document.is_root() and document.is_root()), retain the permission check
for the target root just like for non-root targets: ensure the requesting user
has the same required permission on target_document before allowing a
root-to-root reorder. Update the branch that currently skips permission
validation to call the existing permission check helper used elsewhere (e.g.,
self.check_object_permissions(request, target_document) or the same custom
permission method used for non-root moves) inside the target_document.is_root()
handling so root-to-root moves cannot be performed without rights on the target.
---
Outside diff comments:
In `@src/frontend/apps/e2e/__tests__/app-impress/doc-grid-move.spec.ts`:
- Around line 123-125: Update the two test assertions that still expect the old
permission copy: replace the string in the expect(dragOverlay).toHaveText(...)
call (referenced as dragOverlay) from "You must be at least the administrator of
the target document" to the current copy "You must be an administrator of the
target document", and update the other assertion (the one around lines 173-175
that asserts drop permission) to expect "You must be at least an editor of the
target document" so both expectations match the current UI copy.
In `@src/frontend/apps/e2e/__tests__/app-impress/doc-tree.spec.ts`:
- Around line 272-330: The test currently downgrades the current user to Editor
then creates docChild as that same user via createRootSubPage(page, browserName,
...), which makes the user the creator and enables "Move to my docs"; to test
the disabled path, create the child as the other member instead: change the
createRootSubPage call to use otherBrowserName (i.e., createRootSubPage(page,
otherBrowserName, 'doc-tree-detach-child')) or otherwise ensure docChild is
created by the other user so the final expectation that "Move to my docs" has
aria-disabled="true" is valid.
In `@src/frontend/apps/impress/src/features/service-worker/plugins/ApiPlugin.ts`:
- Around line 182-213: The abilities object on the offline-created Doc
(newResponse) is missing the required boolean field detach, causing a TypeScript
type error; update the abilities literal inside ApiPlugin.ts where newResponse
is constructed (the variable named newResponse of type Doc) to include detach:
false (or appropriate default true/false) alongside the other ability flags so
the abilities property satisfies the Doc type contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 317d1016-16f6-4757-8789-3674286c966d
📒 Files selected for processing (16)
src/backend/core/api/viewsets.pysrc/backend/core/models.pysrc/backend/core/tests/documents/test_api_documents_detach.pysrc/backend/core/tests/documents/test_api_documents_move.pysrc/backend/core/tests/documents/test_api_documents_retrieve.pysrc/backend/core/tests/documents/test_api_documents_trashbin.pysrc/backend/core/tests/external_api/test_external_api_documents.pysrc/backend/core/tests/test_models_documents.pysrc/frontend/apps/e2e/__tests__/app-impress/doc-grid-move.spec.tssrc/frontend/apps/e2e/__tests__/app-impress/doc-tree.spec.tssrc/frontend/apps/impress/src/features/docs/doc-management/types.tsxsrc/frontend/apps/impress/src/features/docs/doc-tree/api/useDetach.tsxsrc/frontend/apps/impress/src/features/docs/doc-tree/components/DocTreeItemActions.tsxsrc/frontend/apps/impress/src/features/docs/docs-grid/components/DocGridContentList.tsxsrc/frontend/apps/impress/src/features/docs/docs-grid/hooks/useDragAndDrop.tsxsrc/frontend/apps/impress/src/features/service-worker/plugins/ApiPlugin.ts
The move route is for moving a document into a parent document, yet it also handles moving a document outside of its parent, so we restrict it to it's original purpose.
Move route has been updated to handle doc move into parent doc. Updated the tests to reflect those updates.
Update the document grid list to enable drag and dropping documents to be more coherent : - At least admin to drag. - At least editor on the target document to drop.
Documents grid drag and drop have been updated with new permissions, update tests to reflect those changes.
Route to handle moving a sub document out of its parent making it a root document.
ZouicheOmar
force-pushed
the
improve-doc-permissions
branch
from
ffad78b to
c96b5c6
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/backend/core/tests/documents/test_api_documents_detach.py (1)
105-107: Assert the detached document’s root ordering, not only membership.These checks confirm the document became a root sibling, but they would still pass if the detach action inserted it in the wrong root position. If the API contract is to append the detached document as the last root sibling, add an ordering assertion against
target.get_siblings()after refresh.Also applies to: 134-136, 167-169, 199-201
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/tests/documents/test_api_documents_detach.py` around lines 105 - 107, The test currently only asserts membership of the detached document among root siblings (using target.get_siblings()) but not its position; after performing the detach and refreshing the involved models (e.g., call refresh on target/document to reload DB state), assert that the detached document is the last element in list(target.get_siblings()) to verify ordering (e.g., compare to list(...) [-1] or assert equality with expected ordering). Apply the same ordering assertion after refresh to the other similar checks at the blocks referenced (around 134-136, 167-169, and 199-201) so each detach case verifies the detached document was appended as the final root sibling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/backend/core/tests/documents/test_api_documents_detach.py`:
- Around line 60-62: The test asserts a DRF generic 403 payload with "detail"
but the detach action returns a custom 403 payload using "message" for
permission-denied responses; update the assertion in
src/backend/core/tests/documents/test_api_documents_detach.py to expect
{"message": "You do not have permission to perform this action."} (i.e. change
the "detail" key to "message") for the existing response variable so it matches
the detach action's payload.
In `@src/frontend/apps/e2e/__tests__/app-impress/doc-grid-move.spec.ts`:
- Around line 173-175: The test assertion for the drag-denial overlay is missing
the phrase "at least" and must match the component text rendered when canDrag is
false; update the expectation in the spec (the variable dragOverlay in
app-impress/doc-grid-move.spec.ts) to assert the full string "You must be at
least an administrator to move the document" so it matches the text emitted by
DocGridContentList.tsx when canDrag is false.
In `@src/frontend/apps/impress/src/i18n/translations.json`:
- Around line 550-551: The drag-overlay translation key used in
DocGridContentList.tsx is missing from translations.json; add the key "You must
be at least an administrator to move the document" (and similarly the
editor/move variant if needed) with the German translation (e.g., "Sie müssen
beim Dokument mindestens die Rolle \"Administrator\" haben") alongside the
existing "You must be at least an administrator of the document" entry so the
drag denial message renders correctly; update the same for the other occurrences
noted (around the other lines referenced) to ensure consistency.
---
Nitpick comments:
In `@src/backend/core/tests/documents/test_api_documents_detach.py`:
- Around line 105-107: The test currently only asserts membership of the
detached document among root siblings (using target.get_siblings()) but not its
position; after performing the detach and refreshing the involved models (e.g.,
call refresh on target/document to reload DB state), assert that the detached
document is the last element in list(target.get_siblings()) to verify ordering
(e.g., compare to list(...) [-1] or assert equality with expected ordering).
Apply the same ordering assertion after refresh to the other similar checks at
the blocks referenced (around 134-136, 167-169, and 199-201) so each detach case
verifies the detached document was appended as the final root sibling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f7242a67-c3c4-4326-be47-a815af47e9e6
📒 Files selected for processing (17)
src/backend/core/api/viewsets.pysrc/backend/core/models.pysrc/backend/core/tests/documents/test_api_documents_detach.pysrc/backend/core/tests/documents/test_api_documents_move.pysrc/backend/core/tests/documents/test_api_documents_retrieve.pysrc/backend/core/tests/documents/test_api_documents_trashbin.pysrc/backend/core/tests/external_api/test_external_api_documents.pysrc/backend/core/tests/test_models_documents.pysrc/frontend/apps/e2e/__tests__/app-impress/doc-grid-move.spec.tssrc/frontend/apps/e2e/__tests__/app-impress/doc-tree.spec.tssrc/frontend/apps/impress/src/features/docs/doc-management/types.tsxsrc/frontend/apps/impress/src/features/docs/doc-tree/api/useDetach.tsxsrc/frontend/apps/impress/src/features/docs/doc-tree/components/DocTreeItemActions.tsxsrc/frontend/apps/impress/src/features/docs/docs-grid/components/DocGridContentList.tsxsrc/frontend/apps/impress/src/features/docs/docs-grid/hooks/useDragAndDrop.tsxsrc/frontend/apps/impress/src/features/service-worker/plugins/ApiPlugin.tssrc/frontend/apps/impress/src/i18n/translations.json
✅ Files skipped from review due to trivial changes (1)
- src/backend/core/tests/documents/test_api_documents_trashbin.py
🚧 Files skipped from review as they are similar to previous changes (11)
- src/frontend/apps/impress/src/features/docs/doc-tree/components/DocTreeItemActions.tsx
- src/frontend/apps/impress/src/features/docs/docs-grid/hooks/useDragAndDrop.tsx
- src/frontend/apps/impress/src/features/service-worker/plugins/ApiPlugin.ts
- src/frontend/apps/impress/src/features/docs/doc-tree/api/useDetach.tsx
- src/backend/core/models.py
- src/frontend/apps/e2e/tests/app-impress/doc-tree.spec.ts
- src/frontend/apps/impress/src/features/docs/docs-grid/components/DocGridContentList.tsx
- src/backend/core/tests/test_models_documents.py
- src/backend/core/api/viewsets.py
- src/backend/core/tests/documents/test_api_documents_move.py
- src/backend/core/tests/documents/test_api_documents_retrieve.py
Comment on lines
+60
to
+62
| assert response.json() == { | ||
| "detail": "You do not have permission to perform this action." | ||
| } |
There was a problem hiding this comment.
Align the expected 403 payload with the detach action response.
The detach ability check returns a custom message payload, not DRF’s generic detail payload, for authenticated users who can access the document but lack detach.
🐛 Proposed test fix
assert response.status_code == 403
assert response.json() == {
- "detail": "You do not have permission to perform this action."
+ "message": (
+ "You do not have permission to move documents "
+ "as a sibling of this target document."
+ )
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| assert response.json() == { | |
| "detail": "You do not have permission to perform this action." | |
| } | |
| assert response.json() == { | |
| "message": ( | |
| "You do not have permission to move documents " | |
| "as a sibling of this target document." | |
| ) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/backend/core/tests/documents/test_api_documents_detach.py` around lines
60 - 62, The test asserts a DRF generic 403 payload with "detail" but the detach
action returns a custom 403 payload using "message" for permission-denied
responses; update the assertion in
src/backend/core/tests/documents/test_api_documents_detach.py to expect
{"message": "You do not have permission to perform this action."} (i.e. change
the "detail" key to "message") for the existing response variable so it matches
the detach action's payload.
Comment on lines
173
to
175
| await expect(dragOverlay).toHaveText( | ||
| 'You must be the owner to move the document', | ||
| 'You must be an administrator to move the document', | ||
| ); |
There was a problem hiding this comment.
Align this assertion with the actual drag-denial overlay text.
DocGridContentList.tsx renders You must be at least an administrator to move the document when !canDrag, so this expectation is missing at least and will fail against the current UI.
✅ Proposed fix
await expect(dragOverlay).toHaveText(
- 'You must be an administrator to move the document',
+ 'You must be at least an administrator to move the document',
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| await expect(dragOverlay).toHaveText( | |
| 'You must be the owner to move the document', | |
| 'You must be an administrator to move the document', | |
| ); | |
| await expect(dragOverlay).toHaveText( | |
| 'You must be at least an administrator to move the document', | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/frontend/apps/e2e/__tests__/app-impress/doc-grid-move.spec.ts` around
lines 173 - 175, The test assertion for the drag-denial overlay is missing the
phrase "at least" and must match the component text rendered when canDrag is
false; update the expectation in the spec (the variable dragOverlay in
app-impress/doc-grid-move.spec.ts) to assert the full string "You must be at
least an administrator to move the document" so it matches the text emitted by
DocGridContentList.tsx when canDrag is false.
Use the dedicated detach API Route for 'move to my docs'
Update translations according to changes on move document permissions.
ZouicheOmar
force-pushed
the
improve-doc-permissions
branch
from
c96b5c6 to
18cad6d
Compare
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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
This PR adresses issue #1409; to make doc move permissions more coherent, we want :
Updated the
moveendpoint to be restricted to moving document into parent documents,and added a dedicated
detachroute, to move a sub doc outside of its root parent document.Proposal
General requirements
CI requirements
git commit --signoff(DCO compliance)git commit -S)<gitmoji>(type) title description## [Unreleased]section (if noticeable change)