An unintended consequence of #1205736 is that forms can no longer be submitted that were previously allowed. The most compelling example is that of someone with sufficient access to edit submissions being able to edit the form, but not actually save the changes.

The check for just $node->webform['status'] isn't sufficient to prevent validation of the form. The test should be the same as that which allows the form itself to be displayed.

Patch forthcoming as soon as I find my hip boots.

Since this was committed to 8.x but not any other branch, only 8.x will need a backport. (Or is that a foreport?)

Comments

danchadwick’s picture

Status: Active » Needs review
StatusFileSize
new1.39 KB

@quicksketch -- please chime in if you have insights.

In testing with the 7.x-4.x-dev (after rc3, but before rc4), I found some surprising and I think incorrect behavior.

First, I loosened the test in validation for closed. If the submission already exists (i.e. is a draft or finished submission), then then webform_submission_access(..., 'edit') is used to determine whether the submission should be allowed. If the form is presented, then it should be allowed to submit. This part is clear. If the submission doesn't exist, then is should be allowed only if the webform is still open.

Second, in the case where a new submission is being created (not saved yet) and the webform is closed by another user, the webform can still be saved as a draft because drafts bypass validation. Once the form is saved as a draft, it can then be submitted as allowed above. This loophole is clearly not correct.

Third, I was somewhat surprised that users with permission to edit their own submissions can do so even when the webform is closed. This means that they can both finish drafts and edit existing drafts (including submitting them). Since hook_webform_submission_access only works to allow additional access, if you want to deny users permission to edit their submissions after the webform is closed, you would need to configure the permissions to not allow editing and then grant it yourself in hook_webform_submission_access. This is a bit awkward, but I'm not sure it merits exposing UI to cover this case (and that of edit any submission when the webform is closed).

danchadwick’s picture

While the #1 patch was functionally correct, it did introduce a small API change. Form validation was run for the Save Draft button. Webform's validation checked for which button was clicked and stopped after validating that it was still allowed to submit the form itself, but before validating any of the elements. This means that if a developer added a form validation function to the form, it would be called when Save Draft was click, which is probably not what the developer wanted. In addition, each validation routine would have to check which button was clicked.

This patch splits the validation function into a "prevalidate" phase that simply checks to see if it is okay to submit the form. Then the normal element validation is run in a second phase. The Save Draft button only runs the prevalidate function. The form itself runs both.

I have tested this with creating new forms, saving drafts of existing drafts, submitting drafts, and multi-page (previous page, next page). It works in all the cases I could think to test. I am not using conditionals, but the conditional code is unchanged.

Since I think this is a pretty important regression fix, I'm going to commit this to 7.x-dev.

  • Commit 1a09891 on 7.x-4.x by DanChadwick:
    Issue #2277593 by DanChadwick: Fixed inability to edit submission when...
  • Commit 1552358 on 8.x-4.x by DanChadwick:
    Issue #2277593 by DanChadwick: Fixed inability to edit submission when...
danchadwick’s picture

Version: 7.x-4.x-dev » 8.x-4.x-dev
Assigned: Unassigned » danchadwick
Status: Needs review » Fixed

Committed to 7.x-4.x and 8.x-4.x. Since the issue which created the regression wasn't applied to other branches, I don't think this needs to go into 6.x and 7.x-3.x.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

danchadwick’s picture

Version: 8.x-4.x-dev » 7.x-4.x-dev