Problem/Motivation

"Unsupported" plugin handles special case and is only used in match engine configuration form. Let's refactor config form in a way that will allow us to remove it. Also, when we finish #2705353: Implement common field matcher for simple field types we should get automatic support for most field types.

Is it OK to change UX of the configuration form if needed (listing unsupported fields outside of the table, ...).

See parent issue for more info.

Proposed resolution

- refactor engine config form to stop relying on specia-case field handler
- delete "Unsupported" field handler
- make sure we have test coverage for the configuration form

CommentFileSizeAuthor
#11 interdiff.txt2.09 KBslashrsm
#11 2705359_11.patch13.57 KBslashrsm
#9 interdiff.txt859 bytesslashrsm
#9 2705359_9.patch13.57 KBslashrsm
#7 interdiff.txt2.24 KBslashrsm
#7 2705359_7.patch13.52 KBslashrsm
#5 interdiff.txt9.52 KBslashrsm
#5 2705359_5.patch13.46 KBslashrsm
#3 2705359-3.patch8.44 KBsanja_m
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

slashrsm created an issue. See original summary.

sanja_m’s picture

sanja_m CreditAttribution: sanja_m at MD Systems GmbH commented
Assigned: Unassigned » sanja_m

Assigning to me.

sanja_m’s picture

sanja_m CreditAttribution: sanja_m at MD Systems GmbH commented
Status: Active » Needs review
FileSize
2705359-3.patch8.44 KB

Added patch.

slashrsm’s picture

slashrsm CreditAttribution: slashrsm at MD Systems GmbH commented
Status: Needs review » Needs work 
  1. +++ b/modules/crm_core_match/config/schema/crm_core_match.schema.yml
@@ -60,3 +60,6 @@ crm_core_match.configuration.default:
+    unsupported:
+          label: 'Unsupported'
+          type: string

    Why do we need this schema entry? 

  2. +++ b/modules/crm_core_match/src/Plugin/crm_core_match/engine/DefaultMatchingEngine.php
@@ -185,6 +180,20 @@ EOF
+    $unsupported_header = array(
+      'label' => $this->t('Name'),
+      'field_type' => $this->t('Field type'),
+    );
+
+    $form['unsupported'] = array(
+      '#title' => $this->t('Unsupported Fields'),
+      '#type' => 'table',
+      '#tree' => TRUE,
+      '#header' => $unsupported_header,
+      '#empty' => $this->t('There are no unsupported fields.'),
+      '#theme_wrappers' => array('form_element'),
+    );
+

    We can move $unsupported_headers inside the main table render array.

    This table could also use a description that would explain what "unsupported" means and how to change that.

  3. Consistently use array() vs []. There are both in the test class that this patch adds. Generally we prefer short form.
slashrsm’s picture

slashrsm CreditAttribution: slashrsm at MD Systems GmbH commented
Status: Needs work » Needs review
FileSize
2705359_5.patch13.46 KB
interdiff.txt9.52 KB

Status: Needs review » Needs work

The last submitted patch, 5: 2705359_5.patch, failed testing.

slashrsm’s picture

slashrsm CreditAttribution: slashrsm at MD Systems GmbH commented
Status: Needs work » Needs review
FileSize
2705359_7.patch13.52 KB
interdiff.txt2.24 KB

Status: Needs review » Needs work

The last submitted patch, 7: 2705359_7.patch, failed testing.

slashrsm’s picture

slashrsm CreditAttribution: slashrsm at MD Systems GmbH commented
Status: Needs work » Needs review
FileSize
2705359_9.patch13.57 KB
interdiff.txt859 bytes
thenchev’s picture

thenchev CreditAttribution: thenchev at MD Systems GmbH commented
Status: Needs review » Needs work 
  1. +++ b/modules/crm_core_match/src/Plugin/crm_core_match/engine/DefaultMatchingEngine.php
@@ -138,52 +133,75 @@ class DefaultMatchingEngine extends MatchEngineBase {
+        '#header' => [
+        'label' => $this->t('Name'),
+          'field_type' => $this->t('Field type'),
+        ],

    Indentation. label together with field_type should be one level deeper. 

  2. +++ b/modules/crm_core_match/src/Tests/DefaultMatchingEngineUITest.php
@@ -0,0 +1,79 @@
+ * Tests the UI for DeafultMatchingEngine.

    DefaultMatchingEngine 

  3. +++ b/modules/crm_core_match/src/Tests/DefaultMatchingEngineUITest.php
@@ -0,0 +1,79 @@
+    $this->assertNoFieldById('edit-configuration-rules-contact-idvalue-status', '', 'There is no checkbox for ID field');
+    $this->assertNoFieldById('edit-configuration-rules-contact-idvalue-operator', '', 'There is no ID operator');

    This should be contact-idvalue not individual? just checking

slashrsm’s picture

slashrsm CreditAttribution: slashrsm at MD Systems GmbH commented
Status: Needs work » Needs review
FileSize
2705359_11.patch13.57 KB
interdiff.txt2.09 KB

Rerolled and fixed.

thenchev’s picture

thenchev CreditAttribution: thenchev at MD Systems GmbH commented
Status: Needs review » Reviewed & tested by the community

Nothing to add. RTBC.

  • slashrsm committed 99c419c on 8.x-1.x 
    Issue #2705359 by slashrsm, sanja_m, Denchev: Remove "Unsupported" field...
slashrsm’s picture

slashrsm CreditAttribution: slashrsm at MD Systems GmbH commented
Status: Reviewed & tested by the community » Fixed

Committed.

Status: Fixed » Closed (fixed)

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