Follow-up to: #1825044: Turn contact form submissions into full-blown Contact Message entities, without storage

Problem

  • The user contact form is not a contact message bundle currently, because it has a "dynamic recipient."
  • For the same reason, it is also not fieldable.

Goal

  • Turn the user contact form into a (locked) category/bundle.

Details

  • Technically, the recipient of the user contact form is delivered through an external data source/context; an argument in the route. In modern blocks/layout/services speak, it could be expressed with an $account context, and the contact category configuration would use a recipient value that is a token, [recipient:mail].
  • However, at this point it's unclear whether and how this could be implemented, so it is probably better to simply introduce a new, contact.category.user bundle that is "locked" (non-deletable) and special-case it.

Proposed solution

  1. Add a contact.category.user bundle.
  2. Add a locked: 1 property to the user bundle; adjust the category admin UI to disallow deletion of locked categories.
  3. Add a condition to CategoryFormController to not expose the recipients field for the user bundle.
  4. Automatically populate the message $recipients from the passed-in $account->mail context.

Blocked by

Files: 
CommentFileSizeAuthor
#36 1856556-contact-user-36.patch14.56 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 57,404 pass(es).
[ View ]
#36 1856556-contact-user-36-interdiff.txt699 bytesBerdir
#35 1856556-contact-user-35-interdiff.txt1.42 KBBerdir
#35 1856556-contact-user-35.patch14.57 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 56,136 pass(es).
[ View ]
#33 Contact form categories.png21.36 KBdas-peter
#33 Anonymous_User_Contact_Form_Mail.txt796 bytesdas-peter
#33 Anonymous_User_Contact_Form_Mail_ChangedMail.txt860 bytesdas-peter
#33 Anonymous_User_Contact_Form_Mail_Adjusted_Category.txt843 bytesdas-peter
#30 1856556-contact-user-26-test-only.patch11.68 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 57,049 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#30 1856556-contact-user-26.patch13.75 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 55,693 pass(es).
[ View ]
#30 1856556-contact-user-26-interdiff.txt3.46 KBBerdir
#27 1856556-contact-user-25.patch11.42 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#26 1856556-contact-user-25-interdiff.txt2.07 KBBerdir
#25 1856556-contact-user-25.patch11.42 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 55,938 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#25 1856556-contact-user-25-interdiff.txt2.07 KBBerdir
#20 1856556-contact-user-20.patch9.35 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 55,887 pass(es).
[ View ]
#20 1856556-contact-user-20-interdiff.txt4.64 KBBerdir
#17 interdiff.txt2.68 KBandypost
#17 1856556-contact-user-17.patch8.09 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 55,429 pass(es).
[ View ]
#14 interdiff.txt0 bytesandypost
#14 1856556-contact-user-13.patch7.66 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 56,024 pass(es), 6 fail(s), and 1 exception(s).
[ View ]
#13 interdiff.txt2.22 KBandypost
#13 1856556-contact-user-12.patch7.65 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#8 interdiff.txt6.04 KBandypost
#8 1856556-contact-user-8.patch7.55 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 55,965 pass(es), 8 fail(s), and 0 exception(s).
[ View ]
#7 1856556-contact-user_7.patch7.39 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 54,165 pass(es).
[ View ]

Comments

Status:Postponed» Active

Category:task» bug
Priority:Normal» Major

This problem is biting us in #1875992: Add EntityFormDisplay objects for entity forms, so it's actually a bug.

@amateescu PLease describe a problem with pointed patch? Is there possibility to make a stab or this one a hard dependency?

Priority:Major» Normal

That's just a normal task, which makes this just a normal bug.

Re #3: You can see the problem here: http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/modul...

There is no hard dependency, this one just needs to be fixed :)

Also we could use "enabled" property of the configurable to allow-disallow usage of personal form

Status:Active» Needs review
StatusFileSize
new7.39 KB
PASSED: [[SimpleTest]]: [MySQL] 54,165 pass(es).
[ View ]

Initial patch with tests.
I also disable machine name change for 'user' category.

StatusFileSize
new7.55 KB
FAILED: [[SimpleTest]]: [MySQL] 55,965 pass(es), 8 fail(s), and 0 exception(s).
[ View ]
new6.04 KB

Re-roll
- new access check
- added accessController

+++ b/core/modules/contact/config/contact.category.user.ymlundefined
@@ -0,0 +1,5 @@
+id: user

I named mine personal, initially considered user as well but like personal better as it matches what it's called in e.g. the page callback.

+++ b/core/modules/contact/lib/Drupal/contact/CategoryAccessController.phpundefined
@@ -0,0 +1,29 @@
+  public function deleteAccess(EntityInterface $entity, $langcode = LANGUAGE_DEFAULT, User $account = NULL) {
+    // Do not allow delete 'user' category used for personal contact form.
+    return user_access('administer contact forms') && $entity->id() !== 'user';
+  }

Needs a re-roll due to the refactored access controller, this is now checkAccess() with $operation.

That said, I like using entity access for this. See below, we should probably block edit es well.

+++ b/core/modules/contact/lib/Drupal/contact/CategoryFormController.phpundefined
@@ -37,7 +37,8 @@ public function form(array $form, array &$form_state, EntityInterface $category)
       '#machine_name' => array(
         'exists' => 'contact_category_load',
       ),
-      '#disabled' => !$category->isNew(),
+      // Do not allow change 'user' category used for personal contact form.
+      '#disabled' => !$category->isNew() || $category->id() == 'user',

I've completely hidden the edit form as well, not sure what's better. Being able to edit this seems rather useless as none of the options make any sense (no label/description is ever displayed, it can't be the default category).

+++ b/core/modules/contact/lib/Drupal/contact/CategoryListController.phpundefined
@@ -34,6 +34,10 @@ public function getOperations(EntityInterface $entity) {
+    if (!$entity->access('delete')) {
+      unset($operations['delete']);

Not here, but we should do this and edit access checks by default in the parent implementation. That will require an access controller for all config entities but we want that anyway I think.

#8: 1856556-contact-user-8.patch queued for re-testing.

This is blocking #1983548: Convert contact message entities to the new Entity Field API, which is part of a critical meta, so this has to happen anyway, whether it's major or not ;)

The entityInfo() hack can be removed now that personal/user is a real category/bundle, see the interdiff in my patch over there. And some checks that should probably be a == personal check now instead of checking the Same for two todo's although I didn't dynamically preset the category recipients as that seems hackish to me.

Maybe add an isPersonal() method to the class as well, for the few checks that we need?

StatusFileSize
new7.65 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
new2.22 KB

So here's re-roll

StatusFileSize
new7.66 KB
FAILED: [[SimpleTest]]: [MySQL] 56,024 pass(es), 6 fail(s), and 1 exception(s).
[ View ]
new0 bytes

And another place

diff --git a/core/modules/contact/contact.pages.inc b/core/modules/contact/contact.pages.inc
index 92eccc2..423cd72 100644
--- a/core/modules/contact/contact.pages.inc
+++ b/core/modules/contact/contact.pages.inc
@@ -76,7 +76,7 @@ function contact_personal_page($recipient) {
   $message = entity_create('contact_message', array(
     'recipient' => $recipient,
-    'category' => 'user',
+    'category' => 'personal',
   ));
   return entity_get_form($message);
}

Being able to edit this seems rather useless

Nice idea, so approach to hide Edit isPersonal() seems cleaner until we get #1831928: Support a 'locked' property on ConfigEntities

Status:Needs review» Needs work

The last submitted patch, 1856556-contact-user-13.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new8.09 KB
PASSED: [[SimpleTest]]: [MySQL] 55,429 pass(es).
[ View ]
new2.68 KB

Finally, should be green

+++ b/core/modules/contact/lib/Drupal/contact/CategoryAccessController.phpundefined
@@ -0,0 +1,34 @@
+    if ($operation == 'delete') {
+      // Do not allow delete 'personal' category used for personal contact form.
+      return user_access('administer contact forms', $account) && $entity->id() !== 'personal';

As mentioned above, I'd suggest the same check for the update operation and hide edit complete.

+++ b/core/modules/contact/lib/Drupal/contact/CategoryFormController.phpundefined
@@ -37,7 +37,8 @@ public function form(array $form, array &$form_state) {
-      '#disabled' => !$category->isNew(),
+      // Do not allow change 'user' category used for personal contact form.
+      '#disabled' => !$category->isNew() || $category->id() == 'personal',
@@ -73,6 +74,18 @@ public function form(array $form, array &$form_state) {
+  protected function actions(array $form, array &$form_state) {
+    $actions = parent::actions($form, $form_state);
+
+    $actions['delete']['#access'] = $this->entity->access('delete');
+
+    return $actions;
+

Then these changes aren't necessary anymore.

+++ b/core/modules/contact/lib/Drupal/contact/CategoryListController.phpundefined
@@ -34,6 +34,10 @@ public function getOperations(EntityInterface $entity) {
+    if (!$entity->access('delete')) {
+      unset($operations['delete']);

Then add the same check here with update/edit and add a @todo for moving that check to the parent implementation.

And as mentioned above, let's add isPersonal(), port over the removal of entityInfo() and remove the @todo's/update checks in the form controller.

Status:Needs review» Needs work

The access check stuff is happening in #1995048: EntityListController::getOperations() should respect access checks. Don't think it's a blocker though, will just need a re-roll once one issue got in.

Status:Needs work» Needs review
StatusFileSize
new4.64 KB
new9.35 KB
PASSED: [[SimpleTest]]: [MySQL] 55,887 pass(es).
[ View ]

Something like this.

Priority:Normal» Major
Issue tags:+sprint, +Entity Field API

This is blocking the contact message NG conversion, tagging accordingly.

I reviewed the patch, but didn't manually test the functionality.
As I don't really know all this stuff, the only thing I can say is that the patch looks clean and I don't see anything that would speak against setting it to RTBC.

Status:Needs review» Reviewed & tested by the community

Nice

Status:Reviewed & tested by the community» Needs work

The isPersonal() method is added to the interface and implemented, but it's never called?

Status:Needs work» Needs review
StatusFileSize
new2.07 KB
new11.42 KB
FAILED: [[SimpleTest]]: [MySQL] 55,938 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Hm, yes, a few checks were on $message->recipient but some where wrong now that there is a category for personal messages.

StatusFileSize
new2.07 KB

Hm, yes, a few checks were on $message->recipient but some where wrong now that there is a category for personal messages.

StatusFileSize
new11.42 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Oh, would help to upload the actual patch as well.

Oh, I did above and double-posted the interdiff? WTH?

Status:Needs review» Needs work
Issue tags:-sprint, -Entity Field API

The last submitted patch, 1856556-contact-user-25.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+sprint, +Entity Field API

#27: 1856556-contact-user-25.patch queued for re-testing.

StatusFileSize
new3.46 KB
new13.75 KB
PASSED: [[SimpleTest]]: [MySQL] 55,693 pass(es).
[ View ]
new11.68 KB
FAILED: [[SimpleTest]]: [MySQL] 57,049 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Ok, fixed the testfail and added test coverage for actually sending personal contact messages, we don't have that now. The added test coverage fails without the previous fix.

To manually test this, you need a system that can send out mails. Not sure if simplytest.me supports that?

Apply the patch, enable the contact module if it's not yet already. Then make sure you can use the send contact messages on the personal contact tab on a user, make sure it's using the configured e-mail address of the user to send the e-mail to and there aren't any errors.

Have a look at the contact categories (below structure), make sure that you can not edit or delete the personal contact category but you can add, edit and delete others.

Issue tags:+Needs manual testing

Tagging.

I set the permission so that the anonymous user was able to use the personal contact forms:
Sending mails seems to work, and a changed e-mail address is reflected too.
I can't access the personal contact category in the UI but manually admin/structure/contact/manage/personal/edit works and I can make and save changes (as administrator).
However, the changes don't seem to have an effect when a personal contact form is used - e.g. the recipient of the mail is still the one of the user and not the new value from the changed category settings.
So I think that's ok that way, right?

Status:Needs review» Reviewed & tested by the community
Issue tags:-Needs manual testing

@das-peter Thanx a lot
Now it's ready

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new14.57 KB
PASSED: [[SimpleTest]]: [MySQL] 56,136 pass(es).
[ View ]
new1.42 KB

Almost ;)

Didn't apply anymore and that page should actually not be accessible. Which can be fixed easily by specifying the correct route access definition as we already do for delete.

Also added tests for that and noticed strange fails where it accidently used personal as the category to test stuff on and that obviously failed. Just removed the random $id there and rely on the id that we create earlier in the same test, not visible in the interdiff.

StatusFileSize
new699 bytes
new14.56 KB
PASSED: [[SimpleTest]]: [MySQL] 57,404 pass(es).
[ View ]

@andypost mentioned that we don't need the ' there.

Status:Needs review» Reviewed & tested by the community

Nice clean-up

+++ b/core/modules/contact/lib/Drupal/contact/Tests/ContactSitewideTest.phpundefined
@@ -139,9 +152,6 @@ function testSiteWideContact() {
-    $categories = entity_load_multiple('contact_category');
-    $id = key($categories);

Not sure I get the reason of this. But it was introduced in #1588422-60: Convert contact categories to configuration system

PS: Probably it was introduced to make sure that category exists

Title:Try to convert user contact form into a contact category/bundleConvert user contact form into a contact category/bundle
Status:Reviewed & tested by the community» Fixed

Committed 99861e8 and pushed to 8.x. Thanks!

We need to address #1849158: Expose contact category-specific forms and/or their URLs somewhere / #1997692: Regression: category selector missing on site wide contact form before too much more work on contact categories occurs.

Issue tags:-sprint

Yay, removing from sprint.

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

Issue summary:View changes

Updated issue summary.