Problem/Motivation

Users and admins are allowed to update a webform submission after it has been created. In an approval based workflow, submission data should be locked once it has been approved.

Proposed resolution

Provide a mechanism to lock a webform submission, which prevents it from being updated.

Remaining tasks

  • Add a lock property to WebformSubmission entity.
  • Add locking to the admin notes and flagging form. (WebformSubmissionNotesForm)
  • Update submission results to include a lock icon with an Ajax toggle. (WebformSubmissionListBuilder)
  • Update WebformSubmission access controls to check for locking. (WebformSubmissionAccessControlHandler)

Notes

  • Editing a submission's elements will be locked, which flagging and notes will always be updatable.
  • Submission admins can lock/unlock submissions

User interface changes

Locking will be added to submission results table and admin notes form.

API changes

  • \Drupal\webform\Entity\WebformSubmission::lock($lock = TRUE)
  • \Drupal\webform\Entity\WebformSubmission::unlock()
  • \Drupal\webform\Entity\WebformSubmission::isLocked()
  • \Drupal\webform\Entity\WebformSubmission::isUnlocked()

Data model changes

New lock property will be added to WebformSubmission entity.

References

Possible lock icons (SVG)

CommentFileSizeAuthor
#54 2888862-55.patch159.21 KBjrockowitz
#49 webform-submission-lock-2888862-48.patch158.63 KBjrockowitz
#46 webform-submission-lock-2888862-46.patch154.39 KBmferanda
#43 interdiff-2888862-31-42.txt96.36 KBjrockowitz
#42 webform-submission-lock-2888862-42.patch154.53 KBmferanda
#40 webform-submission-lock-2888862-40.patch152.84 KBmferanda
#36 webform-submission-lock-2888862-35.patch229.89 KBmferanda
#34 webform-submission-lock-2888862-34.patch224.82 KBmferanda
#31 webform-submission-lock-2888862-31.patch38.66 KBjrockowitz
#29 webform-submission-lock-2888862-28.patch38.25 KBjrockowitz
#24 webform-submission-lock-2888862-20.patch36.94 KBjrockowitz
#20 interdiff-2888862-14-19.txt35.35 KBjrockowitz
#20 webform-submission-lock-2888862-19.patch36.94 KBjrockowitz
#14 webform-submission-lock-2888862-14.patch25.76 KBmferanda
#13 webform-submission-lock-2888862-13.patch24.71 KBmferanda
#12 webform-submission-lock-2888862-12.patch25.52 KBmferanda
#9 webform-submission-lock-2888862-9.patch28.51 KBmferanda
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

jrockowitz created an issue. See original summary.

jrockowitz’s picture

Issue summary: View changes
jrockowitz’s picture

Status: Active » Closed (won't fix)

If someone wants this feature, please reopen this ticket.

mferanda’s picture

Definitely something I'd be interested in as a feature.

mferanda’s picture

Status: Closed (won't fix) » Active
jrockowitz’s picture

Status: Active » Postponed

I am marking this as postponed until someone is willing to do the work or pay someone to do the work

mferanda’s picture

Since this has lingered for a while, I could take a look.

mferanda’s picture

Assigned: Unassigned » mferanda
Status: Postponed » Active
mferanda’s picture

Ok, a lot of firsts here... git... drupal / symfony... phpstorm... dev through a mac... first patch... please don't shoot me if I'm doing things improperly.

mferanda’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 9: webform-submission-lock-2888862-9.patch, failed testing. View results

mferanda’s picture

mferanda’s picture

mferanda’s picture

mferanda’s picture

Realized last night that this is some type of unit testing added to the module to self-test. Running tests locally now and will look at adding tests for lock/unlock.

jrockowitz’s picture

@mferanda I probably can't test your patch til next week or after the New Year.

The only immediate recommendations/thoughts.

  • Property name should changed from 'submissionlock' to just 'lock'.
  • When a submission is locked users should be able to view the data but can't edit the data.
mferanda’s picture

lock is a reserved word with mysql, which is why I switched to submissionlock. I'm with you, submissionlock vs lock is definitely causing me some confusion in the code.

Users have the ability to view on lock. The ajax flip works for the lock, but something would have to be added to also refresh the dropbutton. Otherwise on lock, the first option is still "Edit" even though that would now be denied.

BTW, no worries on testing. This is nothing at all urgent, just something to obsess over while I'm off and learn something new.

jrockowitz’s picture

How about using ‘locked’?

Accessing a locked submission’s edit form is going to be tricky because we also have to account for paging between locked and unlocked submissions.

Maybe a locked submission’s form should be readonly.

jrockowitz’s picture

I found some time to review this. I will post a patch with feedback shortly.

jrockowitz’s picture

Status: Needs work » Needs review
FileSize
36.94 KB
35.35 KB

First off, @mferanda you did amazing job implementing this feature. Most of the changes are minor tweaks to make things consistent.

The only big change is not blocking access to a locked submission's edit but instead displaying a locked warning message.

Blocking full access to a submission is not needed and could have unexpected results. For example, certain users would be unable to 'unlock' a submission that they just locked.

Changes/Tweaks...

  • Change 'submissionlock' to 'locked'
  • Write update hook which add the 'locked' property to the WebformSubmission entity.
  • Update locked icon to match 'notes' and 'sticky' icon.
  • Add description to locked and notes checkboxes
  • Revert all changes to WebformSubmissionAccessControlHandler. (@see above note)
  • Add submission locked message to admin and webform settings.
  • Update WebformSubmissionForm to display the locked message.
  • Tweak labeling in 'webform-handler-action-summary.html.twig'.
  • Add locked support to 'WebformActionHandler'.
  • Add \Drupal\webform\WebformSubmissionInterface::setLocked
  • Universally reference 'locked' (the field name) instead of 'lock'.
  • Display 'Locked' with submission information.
  • Removed WebformSubmission::lock and WebformSubmission::unlock and use WebformSubmission::setLocked().
  • Fix broken tests

Tests that need to be written. We just need to find and duplicate all sticky related test.

  • \Drupal\webform\Tests\Views\WebformViewsBulkFormTest
  • \Drupal\webform\Tests\WebformSubmissionListBuilderTest
  • \Drupal\webform\Tests\Handler\WebformHandlerActionTest

  • jrockowitz committed 3642ee6 on 2888862-webform-submission-lock
    Issue #2888862 by mferanda: Provide a mechanism to lock a webform...
  • jrockowitz committed bf65ad4 on 2888862-webform-submission-lock
    Issue #2888862 by mferanda: Provide a mechanism to lock a webform...

  • jrockowitz committed d24dfa2 on 2888862-webform-submission-lock
    Issue #2888862 by mferanda: Provide a mechanism to lock a webform...

  • jrockowitz committed 4eb97b2 on 2888862-webform-submission-lock
    Issue #2888862 by mferanda: Provide a mechanism to lock a webform...
jrockowitz’s picture

Status: Needs review » Needs work

The last submitted patch, 24: webform-submission-lock-2888862-20.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

  • jrockowitz committed 4e17789 on 2888862-webform-submission-lock
    Issue #2888862 by mferanda: Provide a mechanism to lock a webform...

  • jrockowitz committed 356e61c on 2888862-webform-submission-lock
    Issue #2888862 by mferanda: Provide a mechanism to lock a webform...
jrockowitz’s picture

Status: Needs work » Needs review
jrockowitz’s picture

  • jrockowitz committed 6512dd2 on 2888862-webform-submission-lock
    Issue #2888862 by mferanda: Provide a mechanism to lock a webform...
jrockowitz’s picture

This is my last patch for the day. What remains is another round of QA and writing the automated test.

@mferanda Are you up for writing tests?

  • jrockowitz committed e04cad6 on 2888862-webform-submission-lock
    Issue #2888862 by mferanda: Provide a mechanism to lock a webform...
mferanda’s picture

@jrockowitz thanks for “amazing” as I feel like Im throwing darts in the dark. I’ll definitely take a look as soon as Im able. I started on a similar patch for the message but got stuck and ran out of time. Was able to branch off your patch. Much better with lock message.

mferanda’s picture

Status: Needs review » Needs work

The last submitted patch, 34: webform-submission-lock-2888862-34.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

mferanda’s picture

Ok, my last attempt for the night. I'm way too tired to be working on this today, anyway.

jrockowitz’s picture

We might want to consider using lock/unlock SVG icons that don't reposition the lock when toggled on/off.

https://material.io/icons/#ic_lock_open
https://getuikit.com/docs/icon

mferanda’s picture

@jrockowitz also... how about adding a setting on the form to automatically lock a submission after save?

jrockowitz’s picture

@mferanda You can use the action handler (\Drupal\webform\Plugin\WebformHandler\ActionWebformHandler) to lock a submission on save and/or any condition. I am pretty sure I set up this behavior but the tests still need to be created.

These tests need to be added to test form (webform.webform.test_handler_action.yml) and SimpleTest (\Drupal\webform\Tests\Handler\WebformHandlerActionTest)

BTW, your patch is including the .idea directory from PHPStorm. You should update your global .gitignore file.
@see https://gist.github.com/subfuzion/db7f57fff2fb6998a16c

mferanda’s picture

@jrockowitz yeah, looks like my .gitignore settings were written over. excluded that folder again.

The original lock link you sent had two different lock versions. I changed over to the one that doesn't change positions and also separated out the lock link SVG & CSS with on and off.

I'll take a look at the handler after this patch

jrockowitz’s picture

Please add...

# Ignore PHPStorm files
.idea

...to your global .gitignore settings and remove the changes to .gitignore from the patch.

mferanda’s picture

jrockowitz’s picture

FileSize
96.36 KB

I wanted to see what has changed since my patch from #31 so I created the attached interdiff.

jrockowitz’s picture

@mferanda I have a client that needs this feature. Are you okay with me writing tests and finishing this issue?

mferanda’s picture

@jrockowitz not a problem at all... go ahead.

mferanda’s picture

@jrockowitz here is where I stopped...

jrockowitz’s picture

Assigned: mferanda » jrockowitz

  • jrockowitz committed 49abb93 on 2888862-webform-submission-lock-v2
    Issue #2888862 by mferanda, jrockowitz: Provide a mechanism to lock a...
jrockowitz’s picture

Status: Needs work » Needs review
FileSize
158.63 KB

Status: Needs review » Needs work

The last submitted patch, 49: webform-submission-lock-2888862-48.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

  • jrockowitz committed 49c1e84 on 2888862-webform-submission-lock-v2
    Issue #2888862 by mferanda, jrockowitz: Provide a mechanism to lock a...
jrockowitz’s picture

Status: Needs work » Needs review

  • jrockowitz committed c2bf430 on 2888862-webform-submission-lock-v2
    Issue #2888862 by mferanda, jrockowitz: Provide a mechanism to lock a...
jrockowitz’s picture

jrockowitz’s picture

@mferanda I added the missing tests and made very few changes to the patch.

mferanda’s picture

@jrockowitz When I tried applying the patch to clean 8.x-5.x it said 3 base files were missing.

error: patch failed: includes/webform.install.update.inc:1839
error: includes/webform.install.update.inc: patch does not apply
error: tests/modules/webform_test/config/install/webform.webform.test_element_range.yml: No such file or directory
error: tests/modules/webform_test/config/install/webform.webform.test_form_states_server_custom.yml: No such file or directory
error: tests/modules/webform_test/config/install/webform.webform.test_handler_email_twig.yml: No such file or directory

Let me know if I'm doing something wrong...

jrockowitz’s picture

The patch only applies to the latest 8.x-5.x-dev release. The missing files were added after the release of 8.x-5.x-rc1.

Since I am using features branches, you can also checkout 2888862-webform-submission-lock-v2.

mferanda’s picture

@jrockowitz figured. Got the lock-v2 branch

1. Applied the patch
2. Webform install

Do you have to enable something now for lock to work / appear? I tried to click through things on the site, but not seeing anything. I'll have to look again tomorrow

jrockowitz’s picture

Did you run the database updates?

The lock should appear on the Contact form's results page.

/admin/structure/webform/manage/contact/results/submissions

mferanda’s picture

@jrockowitz I made a dumb mistake and reset everything, working fine now. Was definitely too tired to be messing with this last night.

1. checked out 2888862-webform-submission-lock-v2
2. installed webform
3. submitted a contact form test
4. locked/unlocked through submitted list ** Not really sure I like that... could be confusing without something saying it happened. probably just me
4a. When locked through list: Locked notification when trying to Edit (as it should). Can see submitted data through view screen and that it's locked (as it should). Notes can still be modified (as they should). However, could delete record that is locked and thinking maybe that should be blocked?
4b. When unlocked: can edit, view, or delete as expected
4c. Unlock and lock was reflected properly on notes page
4d. Browsed off and came back to list, cleared cache, etc... locked or unlocked was still saved and displaying properly for item
5. locked/unlocked through notes screen (all results same as 4)
6. Added a handler to the contact form to auto lock on complete
7. Submitted another test, was immediately locked due to auto handler. All 4 & 5 related checks are still fine.

Thumbs up!

mferanda’s picture

@jrockowitz my vote is now to leave the lock/unlock mouseover alone. I added it in the first place to work similar as the other quick click items. The only thing I sort of find annoying is that when it's locked, you still have the operations with default of EDIT. **I know that can be a little messy to also update, but just throwing it out there again. Perhaps one of those things someone else could customize for their own use.

On that note, what about adding some code to mod the list row with a lock/unlock related CSS tag? My thought on this was so that someone could add CSS to maybe highlight/bold or do whatever they wanted for row list on lock/unlock.

Last thing I have is the delete issue. All users + admin should be blocked from deleting a locked record along with edit. View only.

I don't mind taking a look at CSS Row and Delete block, just let me know.

jrockowitz’s picture

I think locking should only affect editing and not deleting.

Ultimately when it comes to deleting I think there should be a trash bin functionality which is something that is probably going to be provided by core's content moderation in the next year or so.

I think the current functionality is good enough and we can tweak things later.

mferanda’s picture

@jrockowitz works for me

  • jrockowitz committed 276dc1c on 8.x-5.x authored by mferanda
    Issue #2888862 by mferanda, jrockowitz: Provide a mechanism to lock a...
jrockowitz’s picture

Status: Needs review » Fixed

@mferanda Committed. Thanks!!!

Status: Fixed » Closed (fixed)

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