-
-
Notifications
You must be signed in to change notification settings - Fork 559
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
XWIKI-22787: Pinned Pages are lost on space move for non admin user #3886
Conversation
* Start work to have specific handling of WebPreferences in case of move/rename/copy of a full space * WIP: needs to add tests
* Fix unused import
* Provide integration test
DocumentReference newHomeReference = getSpaceHomeReference(newReference); | ||
DocumentReference oldHomeReference = getSpaceHomeReference(oldReference); | ||
if (!this.concernedEntities.containsKey(oldHomeReference) || !hasAccess(Right.EDIT, newHomeReference)) { | ||
this.logger.error("You don't have sufficient permissions over the home document of WebPreferences " |
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.
This message seems misleading for the first case where the entities to be moved don't contain the main page of the space. Also, thinking of https://forum.xwiki.org/t/batch-move-pages-design/16559, can't we imagine a move operation that moves all child pages but not the main page and where the pinning state of these child pages should be moved?
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.
This message seems misleading for the first case where the entities to be moved don't contain the main page of the space
Right
Also, thinking of https://forum.xwiki.org/t/batch-move-pages-design/16559, can't we imagine a move operation that moves all child pages but not the main page and where the pinning state of these child pages should be moved?
Apparently this wasn't discussed in https://forum.xwiki.org/t/copy-move-delete-the-webpreferences-page-along-with-its-parent-page/16332 so I'm not really sure how we would deal with such scenario. I'm wondering if it wouldn't be an actual option to add in the batch move new design. In any case I won't address that use case here: we'd create an improvment for it.
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.
This message seems misleading for the first case where the entities to be moved don't contain the main page of the space
Actually the condition was wrong, I fixed it.
@@ -63,13 +64,28 @@ protected void runInternal() throws Exception | |||
// Process | |||
progressManager.startStep(this); | |||
setContextUser(); | |||
|
|||
// FIXME: this should probably be the selected entities only... |
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.
Do you still plan to address this?
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.
That's what I started to check as part of #3887. I'm not planning to handle this as part of the current PR at least.
...toring-api/src/main/java/org/xwiki/refactoring/internal/job/AbstractEntityJobWithChecks.java
Outdated
Show resolved
Hide resolved
if (isWebPreferences) { | ||
DocumentReference oldHomeReference = getSpaceHomeReference(oldReference); | ||
if (!this.concernedEntities.containsKey(oldHomeReference) || !hasAccess(Right.DELETE, oldHomeReference)) { | ||
this.logger.error("You don't have sufficient permissions over the home document of WebPreferences " |
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.
Same as the other comment, this message seems misleading.
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.
Same as above, I fixed the condition.
* Fix wrong conditions * Add unit tests
Jira URL
Changes
Description
Clarifications
Screenshots & Video
Executed Tests
TBD
Expected merging strategy