When you set at Publish Options in a content type in default options to default status = not published, save_draft module is enabled and configured to allow all authenticated users to save draft, the button publish appears to all users who have the permission to create a new content type, even when you want this new content needs to be approved before publication.

Comments

beeradb’s picture

Status: Active » Needs review
StatusFileSize
new836 bytes

The issue here is that the access check for the save button and the draft button were too tightly coupled. The reasoning behind this seems to be to save users from improper configuration of user permissions, potentially allowing a user to save a draft which they do not have access to view.

I would argue that we can just strip that out, as we can't really save a site builder from improperly configuring permissions. Here's a patch which strips that check out, and as a result respects default node options.

beeradb’s picture

Issue summary: View changes

Just to complete the issue

rooby’s picture

Title: Node are always published when save_draft module is enabled » Publish button always available to users with node edit access
Version: 7.x-1.4 » 7.x-1.x-dev
Issue summary: View changes
Status: Needs review » Postponed (maintainer needs more info)

We can't just pull out all this functionality.

We need to make this configurable and the default settings for existing users of the module should be the same as current functionality.

I'm still not 100% clear on what is being asked for.

Currently if a user has the save draft permission and the permission to edit the given node, they get all the save draft related buttons, including publish.

Is what you want, to force users to be able to save a draft and then that user cannot publish the content until it has been approved by someone else first?

If so that is not really what this module is for, you should use the workflow module or the workbench module, which allow you to have content approval workflows before content is published.

This module is effectively just a more user friendly way of using the published checkbox.

Marking as postponed until we have clarified what the request is.

markdorison’s picture

If a node's content type is defaulted to be not published, and a user does not have the administer content permission, they should not be able to save a published node. In the current state of the module, a user is able to bypass this and publish the node.

jcisio’s picture

Category: Bug report » Feature request
Status: Postponed (maintainer needs more info) » Closed (duplicate)
Related issues: +#1120164: is a separate permission needed?

I was going to file a new issue "More granular access check for save_draft" and I found this one.

Users without "administer nodes" normally could not publish a node unless it is configured to be published by default. Another case is modules like https://www.drupal.org/project/override_node_options allow users without that "God" permission to publish nodes. So save_draft would have to manage these cases.

Because otherwise user_access('save draft') is not flexible and we don't have any xxx_alter mechanism for that.

PS: However I checked the latest dev release and it seems that it was fixed (but not in the latest stable in April 2011). Mark as duplicate of #1120164: is a separate permission needed?. So I think there is nothing left to do (other than making a new stable release). If you use hook_form_alter to modify the node options, then it is your responsibility to do that *before* save_draft with something like the following:

/**
 * Implements hook_module_implements_alter().
 */
function CUSTOM_module_implements_alter(&$implementations, $hook) {
  if (($hook == 'form_alter') && isset($implementations['save_draft'])) {
    $group = $implementations['save_draft'];
    unset($implementations['save_draft']);
    $implementations['save_draft'] = $group;
  }
}
jcisio’s picture

Category: Feature request » Bug report
Status: Closed (duplicate) » Needs work

Sorry I read #1 again and it is a separate bug. #4 is for integration with override_node_options.

In this case I think the patch #1 can be considered to be in the right direction. It is not that an user can edit unpublished node that he can publish it. Now it depends on what we want when a user edit a node that he can't publish:

- Don't change anything, as if he doesn't have "save draft" permission.
- Change the text from "Save" to "Save as draft" to keep the UI consistent.

In no way there is a button to publish that node.

jcisio’s picture

Status: Needs work » Needs review
StatusFileSize
new1.33 KB

Here is a patch. I don't include a permission for more granular access check because I think the "save draft" permission should not override the current more granular access control (using a contrib module, or hook_form_alter to add/remove the node status checkbox etc.). It should respect the presence of that checkbox. It does one thing, and does it correctly. Otherwise it can't be considered as a drop-in replacement for the old "Publish" button.

Because it is a behavioral change, we can add to the release note that in some rare cases, to restore the original behavior, a module like override_node_options is required. In that case, all roles with "save draft" permission should be attributed with the "override NODE_TYPE published option" permission.

For reference, in D8 the same thing happens. There is no "save draft" permission, only the text that changes based on current node status and node type options: (in NodeForm.php)

    if ($element['submit']['#access'] && \Drupal::currentUser()->hasPermission('administer nodes')) {
      // isNew | prev status » default   & publish label             & unpublish label
      // 1     | 1           » publish   & Save and publish          & Save as unpublished
      // 1     | 0           » unpublish & Save and publish          & Save as unpublished
      // 0     | 1           » publish   & Save and keep published   & Save and unpublish
      // 0     | 0           » unpublish & Save and keep unpublished & Save and publish
      // ...
    }
rooby’s picture

Otherwise it can't be considered as a drop-in replacement for the old "Publish" button.

That is not all this module does. Saving a draft also allows you to save incomplete data, even for required fields.

I understand the problem properly now however I don't think that either of the patches sufficiently solve the issue since they stop users from being able to save a new node as a draft if they don't have access to the status checkbox.

The problem is complex because you have 3 factors:

  • Can the user access the status checkbox
  • Can the user save drafts
  • Does the node default to published or unpublished

These patches are good in that they stop a user without access to the status field from saving a new draft on published content, which means they would be changing the status, however it breaks the draft saving functionality in some cases, for example where nodes are unpublished by default and a user wants to be able to save a draft.

I think we also need some changes to the form for which buttons the user can see, instead of the all or nothing approach of the current patches.

Maybe in the case where the user doesn't have access to status?:

  • For new nodes defaulting to published: Show both buttons, so the user can iterate on drafts then save as published as intended.
  • For new nodes defaulting to unpublished: Only show the draft button, so the user can iterate on drafts but not save a published version.
  • For existing published nodes: Only show the submit button, so the user can not save a draft and unpublish the node.
  • For existing unpublished nodes: Only show the draft button, so the user can iterate on drafts but not save a published version.

Because it is a behavioral change, we can add to the release note that in some rare cases, to restore the original behavior, a module like override_node_options is required. In that case, all roles with "save draft" permission should be attributed with the "override NODE_TYPE published option" permission.

I don't think this will be such a rare case. It definitely needs to be in the release notes and probably also a note on the project page and/or a message in an update function.

dsnopek’s picture

StatusFileSize
new1.34 KB

Here's a new patch that (effectively) implements these cases from comment #7:

Maybe in the case where the user doesn't have access to status?:

  1. For new nodes defaulting to published: Show both buttons, so the user can iterate on drafts then save as published as intended.
  2. For new nodes defaulting to unpublished: Only show the draft button, so the user can iterate on drafts but not save a published version.
  3. For existing published nodes: Only show the submit button, so the user can not save a draft and unpublish the node.
  4. For existing unpublished nodes: Only show the draft button, so the user can iterate on drafts but not save a published version.

... except it does it in a simplified way to prevent the patch from getting too complex. Here's how that works:

When save_draft_access() returns FALSE, there is just a "Save" button on the form which does Drupal's default thing. That means for a content type that defaults to unpublished, on new content, if the user can't change the status, it will be unpublished (case nr 2 above). And for existing content, if the user can't change the status, then saving will keep it with the same status (cases nr 3 and 4 above).

So, really, all we need to do is have save_draft_access() return TRUE for new nodes defaulting to published (case nr 1 above), and FALSE for all the other cases. This doesn't change the button labels as described above, but I'm not sure it really needs to.

I think we also need some changes to the form for which buttons the user can see, instead of the all or nothing approach of the current patches.

I think this all or nothing approach actually works fine, because "nothing" (Drupal's default behavior) actually matches the effective result that we want!

Of course, this still needs tests, to validate all of the above.

dsnopek’s picture

StatusFileSize
new6.45 KB
new5.11 KB

Here's changes to the existing tests to make them work with this patch, and some new tests that validate the behavior of the patch.

One thing to note: with the set of cases laid out in comment #7, we have to give the $this->save_draft_user the "administer nodes" permission, because otherwise they don't have access to the status element on the form, and so trigger the new cases.

I think this points to something that might have been overlooked in those set of cases. We have case nr 1:

1. For new nodes defaulting to published: Show both buttons, so the user can iterate on drafts then save as published as intended.

... but, they can't actually "iterate on drafts then save as published as intended" because of case nr 3:

3. For existing unpublished nodes: Only show the draft button, so the user can iterate on drafts but not save a published version.

Once they've saved their new node as a draft (per case nr 1), when they edit it again, they won't be able to publish it (per case nr 3).

So, maybe this logic needs to be amended slightly? What if we say that if a node type is publish by default, and the current user is the owner (so, likely the one who created it), and it's unpublished, that they'll be able to publish it?

dsnopek’s picture

StatusFileSize
new6.97 KB
new3.48 KB

Here's a new patch that implements my idea at the end of comment #9, so that a user who doesn't have access to status can save a node as draft, edit it a bunch and then publish it later.

Please let me know what you think!

cboyden’s picture

Status: Needs review » Reviewed & tested by the community

We've been using this patch for a while and it's working as expected.