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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Status: Postponed » Active
amateescu’s picture

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.

andypost’s picture

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

webchick’s picture

Priority: Major » Normal

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

amateescu’s picture

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 :)

andypost’s picture

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

andypost’s picture

Status: Active » Needs review
FileSize
7.39 KB

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

andypost’s picture

Re-roll
- new access check
- added accessController

andypost’s picture

Berdir’s picture

+++ 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.

Berdir’s picture

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

Berdir’s picture

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?

andypost’s picture

So here's re-roll

andypost’s picture

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);
 }
andypost’s picture

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.

andypost’s picture

Status: Needs work » Needs review
FileSize
8.09 KB
2.68 KB

Finally, should be green

Berdir’s picture

+++ 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.

Berdir’s picture

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.

Berdir’s picture

Status: Needs work » Needs review
FileSize
4.64 KB
9.35 KB

Something like this.

Berdir’s picture

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

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

das-peter’s picture

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.

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Nice

catch’s picture

Status: Reviewed & tested by the community » Needs work

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

Berdir’s picture

Status: Needs work » Needs review
FileSize
2.07 KB
11.42 KB

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

Berdir’s picture

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

Berdir’s picture

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.

andypost’s picture

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

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

Berdir’s picture

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.

Berdir’s picture

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.

Berdir’s picture

Issue tags: +Needs manual testing

Tagging.

das-peter’s picture

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?

andypost’s picture

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

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

Berdir’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
14.57 KB
1.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.

Berdir’s picture

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

andypost’s picture

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

alexpott’s picture

Title: Try to convert user contact form into a contact category/bundle » Convert 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: Create contact form block before too much more work on contact categories occurs.

Berdir’s picture

Issue tags: -sprint

Yay, removing from sprint.

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.