Add `publish_needs_approve` option to enforce approval before publishing #27

moka-dev · 2025-03-13T14:40:56Z

This PR introduces a new configuration option, publish_needs_approve, that enforces approval before a page can be published. When this option is enabled, the "Publish" button will only be available if the page has been approved.

This PR introduces a new configuration option, `publish_needs_approve`, that enforces approval before a page can be published. When this option is enabled, the "Publish" button will only be available if the page has been approved.

…, F5, or back navigation This PR adds a safeguard to ensure that a revision is not approved or published multiple times due to form resubmission (e.g., F5 refresh or browser back navigation). Before committing to the database, the system now checks whether the page has already been approved or published.

This PR provide `dw2pdf` replacements for the `StructPublish` plugin. The following placeholders are now supported: - `@STATUS@` - Page status (`draft`, `approved`, `published`) - `@APPROVER@` - User who approved the draft - `@APPROVALDATE@` - Date of approval - `@PUBLISHER@` - User who published the page - `@PUBLISHDATE@` - Date of publishing - `@VERSION@` - Currently shown version - `@LATESTVERSION@` - Latest published version These replacements will be available when generating PDFs via DW2PDF, ensuring that the document reflects the current StructPublish metadata.

…f role Behavior Changes Editors (users with no role) can create and modify drafts in DokuWiki sections where they have ACL write permissions. Readers (users with no role) can only read published versions or see nothing in sections where they lack ACL write permissions. Changes Implemented Removed role-based checks before accessing struct data to achieve the expected banner behavior. Replaced role-based checks with ACL-based validation for access control. drafts in the database, storing user and timestamp

@revision

Add revision metadata to replacements Add the revision number of the currently shown page as @revision@ Add the revision number of the latest published version as @LATESTVERSIONREVISION@

…anup

Avoid overriding $REV value when a specific revision is requested

splitbrain

In general, I think all of this makes sense but needs a bit clean up. The unnecessary reformatting makes it hard to review and there are multiple things mixed into one PR.

I would like to see this PR replaced by multiple smaller PRs for:

bug fixes (one PR per fix)
PDF Replacements
Prevention of double submission
New Translations

All of them without reformatting existing code and with the issues addressed in my comments

splitbrain · 2025-07-31T12:12:20Z

action/banner.php

+        // link to newest draft, if exists, if not shown already, if user has ACL write
+        if ($newestRevision->getRev() != $shownRevision->getRev() 
+            && $newestRevision->getStatus() != Constants::STATUS_PUBLISHED 
+            && auth_quickaclcheck($ID) >= 2


This should use the AUTH_EDIT constant, not a number.

I assume this change is what you mentioned here:

I also modified the behavior for users with no role because I believed the original behavior was a bug. In my version, I corrected it so that users without a role can still access their pages after creation.

@moka-dev Can you explain the scenario you're addressing here?

@annda can you have a look at this change from $this->dbHelper->checkAccess() to auth_quickaclcheck() and chime in with your thoughts?

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.

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.

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.

splitbrain · 2025-07-31T12:15:50Z

action/banner.php

+                $form->addButton(
+                    'structpublish[' . Constants::ACTION_APPROVE . ']',
+                    $this->getLang('action_' . Constants::ACTION_APPROVE)
+                )->attr('type', 'submit');


note to self: this part seems to be the same, just reformatted

splitbrain · 2025-07-31T12:16:24Z

action/banner.php

+                    'structpublish[' . Constants::ACTION_PUBLISH . ']',
+                    $this->getLang('action_' . Constants::ACTION_PUBLISH)
+                )->attr('type', 'submit');
+            }


The button output is the same. Makes no sense to repeat it. The code should decide if the button is to be shown or not (based on config, access and status) and then just do it.

ok will change that

splitbrain · 2025-07-31T12:23:34Z

action/save.php

@@ -38,6 +42,8 @@ public function handleSave(Doku_Event $event)

        $revision = new Revision($id, $event->data['newRevision']);
        $revision->setStatus(Constants::STATUS_DRAFT);
+        $revision->setUser($_SERVER['REMOTE_USER']);
+        $revision->setTimestamp(time());


What's this about? It seems to be unrelated to this PR. Is it fixing a bug? Under which circumstances? I feel like this should be a separate PR?

It simply updates the metadata stored in the StructPublish database. Previously, the user and timestamp were available for approved and published revisions, but not for drafts, which remained anonymous.

splitbrain · 2025-07-31T12:24:26Z

action/show.php

-            !$dbHelper->isPublishable() ||
-            (auth_isadmin() && !$this->getConf('restrict_admin'))
+        if (!$dbHelper->isPublishable() 
+            || (auth_isadmin() && !$this->getConf('restrict_admin'))


note to self: there seem to be no changes here. just formatting.

Actually, I made a change on line 47. This is where we instruct StructPublish to show only the published revision of a page (or nothing) to the user, based on the ACL rather than their role. (I’ve already change the 2 for AUTH_EDIT)

Mentioned change:

https://github.com/cosmocode/dokuwiki-plugin-structpublish/pull/27/files/88db78f8a09a33166a7241bbba6213654c1d3b2c#diff-0d1526f2d7c15d1b3c96cc0afeba7a216fc07a425bbdf0e847d14714f33cd9daL47

splitbrain · 2025-07-31T12:39:48Z

action/dw2pdf.php

+        $keep = $ID;
+        $ID = $event->data['id']; 
+        $INFO = pageinfo();
+        $REV = $event->data['rev'];


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.

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.

splitbrain · 2025-07-31T12:41:22Z

action/dw2pdf.php

+
+        // get redactor
+        $pageMeta = p_get_metadata($ID);
+        $event->data['replace']['@REDACTOR@'] = $pageMeta['last_change']['user'];


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.

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

yes, then "editor" is the better name.

splitbrain · 2025-07-31T12:42:15Z

action/dw2pdf.php

+        // get lastest published version & revision
+        if ($latestpubRevision != null) {
+            $event->data['replace']['@LATESTPUBLISHEDVERSION@'] = $latestpubRevision->getVersion();
+            $event->data['replace']['@LATESTPUBLISHEDREVISION@'] = $latestpubRevision->getRev();


I would add a couple of underscores for better readability, but that's nitpicking.

splitbrain · 2025-07-31T12:43:06Z

action/dw2pdf.php

+            $event->data['replace']['@APPROVALDATE@'] = $prevapprovedRevision->getDateTime();
+            $event->data['replace']['@PUBLISHER@'] = $shownRevision->getUser();
+            $event->data['replace']['@PUBLISHDATE@'] = $shownRevision->getDateTime();
+        }


I would also prefix all variables with STRUCTAPPROVE_ to avoid any accidental clashes with other plugins.

splitbrain · 2025-07-31T12:44:42Z

action/dw2pdf.php

+            ['@APPROVER@', '@APPROVALDATE@', '@LATESTPUBLISHEDREVISION@', '@REVISION@', '@PUBLISHER@', '@PUBLISHDATE@', '@VERSION@', '@STATUS@', '@REDACTOR@' , '@LATESTPUBLISHEDVERSION@'],
+            ['', '', '', '', '', '', '', '', '', ''],
+            $event->data['content']
+        );


I don't understand this handler. Why is is it necessary? Shouldn't the BEFORE handler already handle all of those?

This clears the StructPublish variables to ensure they are not accidentally reused where they shouldn’t be.

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?

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');

foobarjo and others added 12 commits March 13, 2025 15:40

french translation

b06ec25

spanish translation

b0a2e57

add "email" to de/lang.php

c67fe74

reload globals for bookcreator

6e775b9

replacements for rev and lastpublished rev

32c60c0

Add revision metadata to replacements Add the revision number of the currently shown page as @revision@ Add the revision number of the latest published version as @LATESTVERSIONREVISION@

Make latest published replacements more explicit and perform code cle…

cebb655

…anup

Preserve provided REV when printing an older version

52bb9c0

Avoid overriding $REV value when a specific revision is requested

Take revision into account when exporting from BookCreator

88db78f

splitbrain requested changes Jul 31, 2025

View reviewed changes

This was referenced Aug 11, 2025

Enforce approval before publishing #28

Open

Insert version in the pdf export #21

Open

Add publish_needs_approve option to enforce approval before publishing #27

Are you sure you want to change the base?

Add publish_needs_approve option to enforce approval before publishing #27

Uh oh!

Conversation

moka-dev commented Mar 13, 2025

Uh oh!

splitbrain left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

splitbrain Jul 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Add `publish_needs_approve` option to enforce approval before publishing #27

Add `publish_needs_approve` option to enforce approval before publishing #27

splitbrain Jul 31, 2025 •

edited

Loading