md-systems / crm_core

Drupal 8 port of the CRM Core module
https://www.drupal.org/project/crm_core
11 stars 10 forks source link

Reimplement Matcher with plugin instances support #14

Closed Berdir closed 9 years ago

Berdir commented 9 years ago

PR for #12

Can't comment on the diff, so opening the PR for that :)

Berdir commented 9 years ago

Nice work!

mbovan commented 9 years ago

Pushed the latest changes. Most of the @Berdir's suggestions are applied. Some part of them are skipped and set as todo's for followups. I added replies on the feedback comments. They are hidden now as diff is outdated. :D

Patches to apply for Collect and Inmail

Regarding tests, FieldMatcherTest is pasing, fixed some parts and commented out most of the code in DefaultEngineTest and MatcherTest classes. We have to see what we can use/need from those tests...

Followups: #17, #18

mirodietiker commented 9 years ago

Did a manual test...

After saving a matcher, the system states "The configuration has been saved." without any reference to the matcher saved. But i'm still on the matcher add form... So either a redirect is missing to the edit form, or to the overview form.

The threshold value (required) is not saved. I don't know why, but i can only enable the field name (string) to match. All others are disabled.

Other than creating a matcher, i don't know how i can test a matcher. Did you already provide the collect integration? I like to test it.

mbovan commented 9 years ago

@mirodietiker:

  1. Added redirection to the matcher overview page.
  2. Fixed threshold.
  3. I think because String is the only field handler that is not broken. That is the same in 8.0.x branch, so I didn't make any fixes there in this issue.
  4. There are patches for Collect/Inmail in my previous comment that should be applied. Then, you can enable collect_demo.
mbovan commented 9 years ago

Updated Collect patch (loading a matcher from configuration instead of hard-coding to inmail_individual) with these changes and changes made in Remove event-based processing issue: