IBX-10854: Added ability to hide content item drafts#667
Conversation
| // If content is in draft state, mainLocationId is yet not set | ||
| if ($contentInfo->mainLocationId !== null) { |
There was a problem hiding this comment.
If this is true, then a dedicated ContentInfo method should probably be introduced.
There was a problem hiding this comment.
@Steveb-p
Okay, so I first thought about adding function like this:
+ public function isInitialDraft(): bool
+ {
+ return $this->status === self::STATUS_DRAFT && $this->currentVersionNo === 1;
+ }
However, then I realized that this is not needed for ContentInfo as existing ContentInfo::isDraft() will do just fine. This method will only return true if Content in question has never been published. Once Content is published, ContentInfo::isDraft() will still return false when you create new drafts for that content.
Fixed in acbb2d8
alongosz
left a comment
alongosz
left a comment
There was a problem hiding this comment.
AFAIR this is by-design. There's no purpose to hiding content drafts because content drafts can be read by people with versionread policy, the same as hidden content can be accessed. Are you trying to fix some fatal error or enable hiding drafts? TL;DR;.
yes, it is. If you don't want content to be visible at time of publishing.
@alongosz edited.... |
|
There was a problem hiding this comment.
Doesn't this mean, with this change, that for drafts every user has ability to hide & reveal?
Because we won't be checking permission resolver for drafts.
There was a problem hiding this comment.
Explanation of what happens when $targets=[] is addressed in #667 (comment)
|
@vidarl please make sure #667 (comment) is addressed, PR is rebased and the CI lits green. |
vidarl
force-pushed
the
IBX-10854_Not_possible_to_hide_content_draft
branch
from
acbb2d8 to
ca2b306
Compare
@konradoboza |
vidarl
force-pushed
the
IBX-10854_Not_possible_to_hide_content_draft
branch
from
e6a3f58 to
6671254
Compare
vidarl
force-pushed
the
IBX-10854_Not_possible_to_hide_content_draft
branch
2 times, most recently
from
91fd72c to
af81e00
Compare
|
|
@alongosz, @konradoboza, @Steveb-p : Can you have a new look on this one. I think all review comments has been addressed |
alongosz
changed the title
vidarl
force-pushed
the
IBX-10854_Not_possible_to_hide_content_draft
branch
from
3e36e6e to
ca88c8c
Compare
| if ($reveal) { | ||
| $this->contentService->revealContent($draftContentInfo); | ||
| } |
There was a problem hiding this comment.
... ok, I see I missed it. This is confusing and unexpected. You have a method that is called createContentForHideRevealDraftTests yet you're kinda performing here already a test for that first code path. These should be 2 separate test cases, ideally using some builder method like that to avoid repetition. How about this one just returns a draft and there are separate tests:
- publishing draft, creating another draft and checking hiding / revealing
- checking hiding / revealing on a never published draft.
There was a problem hiding this comment.
Note
These are legacy integration tests, we shouldn't expand it but rather use RepositoryTestCase. But given you've put some work into it already, I'll allow it. Just saying for the future :)
There was a problem hiding this comment.
sorry, this confuses me. So new integration tests should extend Ibexa\Tests\Integration\Core\RepositoryTestCase, not Ibexa\Tests\Integration\Core\Repository\BaseTest, but still be placed in the the Ibexa\Tests\Integration\Core\Repository\ namespace. correct ?
|
…to hide content draft" This reverts commit 0de3f32.
Co-authored-by: Konrad Oboza <konrad.oboza@ibexa.co>
Co-authored-by: Konrad Oboza <konrad.oboza@ibexa.co>
Co-authored-by: Paweł Niedzielski <pawel.tadeusz.niedzielski@gmail.com> Co-authored-by: Konrad Oboza <konrad.oboza@ibexa.co>
vidarl
force-pushed
the
IBX-10854_Not_possible_to_hide_content_draft
branch
from
a8709d3 to
c2ea69b
Compare
|
6 participants
It is not possible to publish content in hidden state
For QA:
Note
Edited by @alongosz
Requires security testing, e.g., if user who doesn't have hide/reveal permissions is now able to do it.
Documentation: