Add publish_needs_approve option to enforce approval before publishing
#27
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,125 @@ | ||
| <?php | ||
|
|
||
| use dokuwiki\plugin\structpublish\meta\Constants; | ||
| use dokuwiki\plugin\structpublish\meta\Revision; | ||
|
|
||
| /** | ||
| * Action component providing (localized) dw2pdf replacements | ||
| * | ||
| * @STATUS@ draft / approved / published | ||
| * @APPROVER@ user that approved the draft | ||
| * @APPROVALDATE@ date of approval | ||
| * @PUBLISHER@ user that published the page | ||
| * @PUBLISHDATE@ date of publishing | ||
| * @VERSION@ shown version | ||
| * @REVISION@ shown revision | ||
| * @LATESTPUBLISHEDVERSION@ latest published version | ||
| * @LATESTPUBLISHEDREVISION@ latest published revision | ||
| * | ||
| * @author Josquin Dehaene <jo@foobarjo.org> | ||
| * @license GPL 2 http://www.gnu.org/licenses/gpl-2.0.html | ||
| */ | ||
|
|
||
| class action_plugin_structpublish_dw2pdf extends DokuWiki_Action_Plugin | ||
| { | ||
| /** | ||
| * @var \helper_plugin_structpublish_db | ||
| */ | ||
| protected $dbHelper; | ||
|
|
||
| public function register(Doku_Event_Handler $controller) | ||
| { | ||
| $controller->register_hook('PLUGIN_DW2PDF_REPLACE', 'BEFORE', $this, 'provide_structpublish_replacements'); | ||
| $controller->register_hook('PLUGIN_DW2PDF_REPLACE', 'AFTER', $this, 'clean_structpublish_replacements'); | ||
| } | ||
|
|
||
| /** | ||
| * Provide StructPublish values for DW2PDF replacements | ||
| */ | ||
| public function provide_structpublish_replacements(Doku_Event $event) | ||
| { | ||
| global $ID; | ||
| global $INFO; | ||
| global $REV; | ||
|
|
||
| //force reload of the globals. usefull when coming from bookcreator | ||
| $keep = $ID; | ||
| $ID = $event->data['id']; | ||
| $INFO = pageinfo(); | ||
| $REV = $event->data['rev']; | ||
|
There was a problem hiding this comment. Why are you changing the globals here? An action handler for the PDF Plugin should not change globals. If that change is somehow necessary for rendering within a bookcreator process, that should probably be fixed in book creator or if that's not possible, at a proper place in dw2pdf, struct or structpublish (depending on who is at fault). But it should not be a side effect of handling PDF generation. There was a problem hiding this comment. When coming from BookCreator, $ID refers to the BookCreator page, so we cannot retrieve the metadata of the page currently being rendered. This is why we need to manually initialize the variables. |
||
|
|
||
| $this->dbHelper = plugin_load('helper', 'structpublish_db'); | ||
|
|
||
| if (!$this->dbHelper->isPublishable()) { | ||
| return; | ||
| } | ||
|
|
||
| // get revisions | ||
| $newestRevision = new Revision($ID, $INFO['currentrev']); | ||
| if ($REV) { | ||
| $shownRevision = new Revision($ID, $REV); | ||
| } else { | ||
| $shownRevision = $newestRevision; | ||
| } | ||
| $latestpubRevision = $newestRevision->getLatestPublishedRevision(); | ||
| $prevpubRevision = $shownRevision->getLatestPublishedRevision($REV ?: $INFO['currentrev']); | ||
| $prevapprovedRevision = $shownRevision->getLatestApprovedRevision($REV ?: $INFO['currentrev']); | ||
|
|
||
| // get redactor | ||
| $pageMeta = p_get_metadata($ID); | ||
| $event->data['replace']['@REDACTOR@'] = $pageMeta['last_change']['user']; | ||
|
There was a problem hiding this comment. redactor is the one who approved the the change? I am not opposed to the term but I don't think we use it anywhere else. Might make sense to streamline the naming. There was a problem hiding this comment. Perhaps the variable name could be improved—for example, $EDITOR for clarity—as it refers to the last editor of the page. approver and publisher are potentially different users There was a problem hiding this comment. yes, then "editor" is the better name. |
||
|
|
||
| // get lastest published version & revision | ||
| if ($latestpubRevision != null) { | ||
| $event->data['replace']['@LATESTPUBLISHEDVERSION@'] = $latestpubRevision->getVersion(); | ||
| $event->data['replace']['@LATESTPUBLISHEDREVISION@'] = $latestpubRevision->getRev(); | ||
|
There was a problem hiding this comment. I would add a couple of underscores for better readability, but that's nitpicking. |
||
| }else{ | ||
| $event->data['replace']['@LATESTPUBLISHEDVERSION@'] = $this->getLang("status_na"); | ||
| $event->data['replace']['@LATESTPUBLISHEDREVISION@'] = $this->getLang("status_na");; | ||
| } | ||
|
|
||
| // get status and revision | ||
| $event->data['replace']['@STATUS@'] = $this->getLang("status_" . $shownRevision->getStatus()); | ||
| $event->data['replace']['@REVISION@'] = $shownRevision->getRev(); | ||
|
|
||
| // status draft | ||
| if ($event->data['replace']['@STATUS@'] === $this->getLang("status_draft")) { | ||
| $event->data['replace']['@VERSION@'] = $this->getLang("status_draft"); | ||
| $event->data['replace']['@APPROVER@'] = $this->getLang("status_na"); | ||
| $event->data['replace']['@APPROVALDATE@'] = $this->getLang("status_na"); | ||
| $event->data['replace']['@PUBLISHER@'] = $this->getLang("status_na"); | ||
| $event->data['replace']['@PUBLISHDATE@'] = $this->getLang("status_na"); | ||
| } | ||
|
|
||
| // status approved | ||
| if ($event->data['replace']['@STATUS@'] === $this->getLang("status_approved")) { | ||
| $event->data['replace']['@VERSION@'] = $this->getLang("status_approved"); | ||
| $event->data['replace']['@APPROVER@'] = $shownRevision->getUser(); | ||
| $event->data['replace']['@APPROVALDATE@'] = $shownRevision->getDateTime(); | ||
| $event->data['replace']['@PUBLISHER@'] = $this->getLang("status_na"); | ||
| $event->data['replace']['@PUBLISHDATE@'] = $this->getLang("status_na"); | ||
| } | ||
|
|
||
| // status published | ||
| if ($event->data['replace']['@STATUS@'] === $this->getLang("status_published")) { | ||
| $event->data['replace']['@VERSION@'] = $shownRevision->getVersion(); | ||
| $event->data['replace']['@APPROVER@'] = $prevapprovedRevision->getUser(); | ||
| $event->data['replace']['@APPROVALDATE@'] = $prevapprovedRevision->getDateTime(); | ||
| $event->data['replace']['@PUBLISHER@'] = $shownRevision->getUser(); | ||
| $event->data['replace']['@PUBLISHDATE@'] = $shownRevision->getDateTime(); | ||
| } | ||
|
There was a problem hiding this comment. I would also prefix all variables with |
||
| $ID = $keep; | ||
| } | ||
|
|
||
| /** | ||
| * Clean up replacements in DW2PDF content | ||
| */ | ||
| public function clean_structpublish_replacements(Doku_Event $event) | ||
| { | ||
| $event->data['content'] = str_replace( | ||
| ['@APPROVER@', '@APPROVALDATE@', '@LATESTPUBLISHEDREVISION@', '@REVISION@', '@PUBLISHER@', '@PUBLISHDATE@', '@VERSION@', '@STATUS@', '@REDACTOR@' , '@LATESTPUBLISHEDVERSION@'], | ||
| ['', '', '', '', '', '', '', '', '', ''], | ||
| $event->data['content'] | ||
| ); | ||
|
There was a problem hiding this comment. I don't understand this handler. Why is is it necessary? Shouldn't the BEFORE handler already handle all of those? There was a problem hiding this comment. This clears the StructPublish variables to ensure they are not accidentally reused where they shouldn’t be. There was a problem hiding this comment. No, it runs a second replace operation on the page content. It does not clear any variables. What exactly is the issue you're trying to fix here? There was a problem hiding this comment. I might have miss something, but I was thinking this could clean up the replacements since it’s called on an AFTER hook: $controller->register_hook('PLUGIN_DW2PDF_REPLACE', 'AFTER', $this, 'clean_structpublish_replacements'); |
||
| } | ||
| } | ||
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 should use the AUTH_EDIT constant, not a number.
I assume this change is what you mentioned here:
@moka-dev Can you explain the scenario you're addressing here?
@annda can you have a look at this change from
$this->dbHelper->checkAccess()toauth_quickaclcheck()and chime in with your thoughts?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.
Previously, a user without a publish role but with write ACL could create a new page in a StructPublish-controlled section. However, once the page was saved, the user could no longer access it, which I assume is not the intended behavior. Any user with write ACL on a section should be able to create and edit pages within it—especially the ones they created—considering that approvers and publishers may be different users.
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 explanation does make sense to me and I vaguely remember discussion between me and @annda about this but not the outcome. @annda I think we should discuss this and then write down what we decide on the wiki page. I think this mechanism needs to be thought through and then be implemented, tested and documented.
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.
Right, we have been aware of this problem for some time, see #5 and #15
I will post more in the relevant issues, since this PR will be closed anyway and split it into smaller ones.