search

mautic / mautic

Mautic: Open Source Marketing Automation Software.
https://www.mautic.org
Other
6.82k stars 2.5k forks source link

FEAT: Adds WHERE filter to ForeignFuncFilterQueryBuilder #13854

Open putzwasser opened 1 week ago

putzwasser commented 1 week ago
Q A
Bug fix? (use the a.b branch) πŸ”΄
New feature/enhancement? (use the a.x branch) 🟒
Deprecations? πŸ”΄
BC breaks? (use the c.x branch) πŸ”΄
Automated tests included? 🟒
Related user documentation PR URL mautic/user-documentation#...
Related developer documentation PR URL mautic/developer-documentation-new#...
Issue(s) addressed Fixes #...

Description

This PR adds the possibility to use ForeignFuncFilterQueryBuilder with a custom WHERE clause.

This makes it possible to create more sophisticated segment filters more easily. The SegmentFilterSubscriber in the PointBundle is an example for this (but it uses ForeignFuncFilterQueryBuilder).

This is already possible in ForeignValueFilterQueryBuilder (L44), but not in ForeignFuncFilterQueryBuilder. This PR makes both filter "behave" similarly.

Under the hood: CustomMappedDecorator::getForeignContactColumn() looks for the ContactSegmentFilterCrate field foreign_table_field.

NOTE: This PR builds on #13851 (this PR's branch is based on that PR).

πŸ“‹ Steps to test this PR:

  1. Open this PR on Gitpod or pull down for testing locally (see docs on testing PRs here)

  2. Add a custom table to your database for a custom segment filter. Use this SQL (it creates the table test_table and adds 4 rows of data to it):

    /*!40101 SET NAMES utf8mb4 */;

--
-- Database: `mautic`
--

-- --------------------------------------------------------

--
-- Table structure for table `test_table`
--

CREATE TABLE `test_table` (
  `id` int(11) NOT NULL,
  `contact_id` int(11) NOT NULL,
  `some_value` varchar(255) NOT NULL,
  `some_bool` tinyint(1) NOT NULL
);

--
-- Dumping data for table `test_table`
--

INSERT INTO `test_table` (`id`, `contact_id`, `some_value`, `some_bool`) VALUES
(1, 1, 'foo', 1),
(2, 1, 'bar', 1),
(3, 2, 'lorem', 1),
(4, 3, 'ipsum', 0);

--
-- Indexes for table `test_table`
--
ALTER TABLE `test_table`
  ADD PRIMARY KEY (`id`);

--
-- AUTO_INCREMENT for table `test_table`
--
ALTER TABLE `test_table`
  MODIFY `id` int(11) NOT NULL AUTO_INCREMENT, AUTO_INCREMENT=5;

  3. Create a test segment filter subscriber. Create the file app/bundles/CoreBundle/EventListener/TestSegmentFilterSubscriber.php in your CoreBundle's event listener directory.

    <?php

declare(strict_types=1);

namespace Mautic\CoreBundle\EventListener;

use Mautic\LeadBundle\Event\LeadListFiltersChoicesEvent;
use Mautic\LeadBundle\Event\SegmentDictionaryGenerationEvent;
use Mautic\LeadBundle\LeadEvents;
use Mautic\LeadBundle\Provider\TypeOperatorProviderInterface;
use Mautic\LeadBundle\Segment\OperatorOptions;
use Mautic\LeadBundle\Segment\Query\Filter\ForeignFuncFilterQueryBuilder;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;
use Symfony\Contracts\Translation\TranslatorInterface;

class TestSegmentFilterSubscriber implements EventSubscriberInterface
{
    public function __construct(
        private TypeOperatorProviderInterface $typeOperatorProvider,
        private TranslatorInterface $translator,
    ) {
    }

    public static function getSubscribedEvents(): array
    {
        return [
            LeadEvents::LIST_FILTERS_CHOICES_ON_GENERATE => [
                ['onGenerateSegmentFiltersAddPointGroups', -10],
            ],
            LeadEvents::SEGMENT_DICTIONARY_ON_GENERATE   => [
                ['onSegmentDictionaryGenerate', 0],
            ],
        ];
    }

    public function onGenerateSegmentFiltersAddPointGroups(LeadListFiltersChoicesEvent $event): void
    {
        // Only show for segments and not dynamic content addressed by https://github.com/mautic/mautic/pull/9260
        if (!$event->isForSegmentation()) {
            return;
        }

        $event->addChoice('lead', 'test_segment_filter', [
            'label'      => 'Test Filter',
            'properties' => ['type' => 'number'],
            'operators'  =>  $this->typeOperatorProvider->getOperatorsIncluding([
                OperatorOptions::EQUAL_TO,
                OperatorOptions::GREATER_THAN,
                OperatorOptions::LESS_THAN,
                OperatorOptions::GREATER_THAN_OR_EQUAL,
                OperatorOptions::LESS_THAN_OR_EQUAL,
            ]),
            'object'     => 'lead',
        ]);
    }

    public function onSegmentDictionaryGenerate(SegmentDictionaryGenerationEvent $event): void
    {
        $event->addTranslation('test_segment_filter', [
            'type'                => ForeignFuncFilterQueryBuilder::getServiceId(),
            'foreign_table'       => 'test_table',
            'foreign_table_field' => 'contact_id',
            'table'               => 'leads',
            'table_field'         => 'id',
            'func'                => 'count',
            'field'               => 'id',
            'where'               => 'some_bool = 1'
        ]);
    }
}

    This creates a new custom filter. This filter counts how many times a lead is referenced in the new test_table IF the column some_bool is true (Note: WHERE can be any filter condition, though).

  4. Clear the cache.

  5. Create a new segment and use the new filter: image

  6. Run php bin/console mautic:segments:rebuild

  7. Contact with ID 1 gets added to your new segment.

codecov[bot] commented 1 week ago

Codecov Report

Attention: Patch coverage is 0% with 10 lines in your changes missing coverage. Please review.

Project coverage is 62.17%. Comparing base (fadcf26) to head (c5343b4). Report is 5 commits behind head on 5.x.

:exclamation: Current head c5343b4 differs from pull request most recent head 2df00f6

Please upload reports for the commit 2df00f6 to get more accurate results.

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/mautic/mautic/pull/13854/graphs/tree.svg?width=650&height=150&src=pr&token=JJAV4AZFpm&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mautic)](https://app.codecov.io/gh/mautic/mautic/pull/13854?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mautic) ```diff @@ Coverage Diff @@ ## 5.x #13854 +/- ## ============================================ - Coverage 62.18% 62.17% -0.01% - Complexity 34220 34221 +1 ============================================ Files 2252 2252 Lines 102328 102336 +8 ============================================ - Hits 63628 63627 -1 - Misses 38700 38709 +9 ``` | [Files](https://app.codecov.io/gh/mautic/mautic/pull/13854?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mautic) | Coverage Ξ” | | |---|---|---| | [...nt/Query/Filter/ForeignValueFilterQueryBuilder.php](https://app.codecov.io/gh/mautic/mautic/pull/13854?src=pr&el=tree&filepath=app%2Fbundles%2FLeadBundle%2FSegment%2FQuery%2FFilter%2FForeignValueFilterQueryBuilder.php&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mautic#diff-YXBwL2J1bmRsZXMvTGVhZEJ1bmRsZS9TZWdtZW50L1F1ZXJ5L0ZpbHRlci9Gb3JlaWduVmFsdWVGaWx0ZXJRdWVyeUJ1aWxkZXIucGhw) | `100.00% <ΓΈ> (ΓΈ)` | | | [...ent/Query/Filter/ForeignFuncFilterQueryBuilder.php](https://app.codecov.io/gh/mautic/mautic/pull/13854?src=pr&el=tree&filepath=app%2Fbundles%2FLeadBundle%2FSegment%2FQuery%2FFilter%2FForeignFuncFilterQueryBuilder.php&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mautic#diff-YXBwL2J1bmRsZXMvTGVhZEJ1bmRsZS9TZWdtZW50L1F1ZXJ5L0ZpbHRlci9Gb3JlaWduRnVuY0ZpbHRlclF1ZXJ5QnVpbGRlci5waHA=) | `2.85% <0.00%> (-0.37%)` | :arrow_down: | ... and [1 file with indirect coverage changes](https://app.codecov.io/gh/mautic/mautic/pull/13854/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mautic)
putzwasser commented 1 week ago

I would love to and tried.

I could figure out what to change for #13851 and this PR, but the remaining code is (still) too complex for me to figure out how to write a proper test.

I'd appreciate assistance here :)

escopecz commented 1 week ago

The best way would be to make a functional test where you'd create a contact and a segment with the filters that causes this bug and run the command to build the segment and then assert that the contact was added to that contact correctly. Can this be tested anyhow on Mautic itself? For example by updating the lead_email_read_count segment filter?

putzwasser commented 1 week ago

ForeignFuncFilterQueryBuilder and ForeignValueFilterQueryBuilder are pretty similar. The latter has a test already.

I guess, a proper test should be similar to the one for ForeignValueFilterQueryBuilder. I just fail to replicate and adapt it.

Can this be tested anyhow on Mautic itself? For example by updating the lead_email_read_count segment filter?

It all depends on the filter conditions. You don't need the additional DB table as "it just creates the SQL". This can be teted. (If you know how :sweat_smile: )

escopecz commented 1 week ago

I'm wondering whether if you change this line: https://github.com/mautic/mautic/blob/58c1903e8e2d76b8938948e826bdea015e2e3a28/app/bundles/LeadBundle/Services/ContactSegmentFilterDictionary.php#L83 to use contact_id, would that segment work? This way you can simplify the testing steps and write functional test for it as well. Does it make sense?

putzwasser commented 1 week ago

That's not the issue. My problem is that the ContactSegmentFilterDictionary isn't populated correctly.

The class uses

    private function fetchFiltersFromSubscribers(): void
    {
        if ($this->dispatcher->hasListeners(LeadEvents::SEGMENT_DICTIONARY_ON_GENERATE)) {
            $event = new SegmentDictionaryGenerationEvent($this->filters);
            $this->dispatcher->dispatch($event, LeadEvents::SEGMENT_DICTIONARY_ON_GENERATE);
            $this->filters = $event->getTranslations();
        }
    }

to get/create the filter.

I pushed my WIP test. I tried to mock/recreate subscribing to LeadEvents::SEGMENT_DICTIONARY_ON_GENERATE but I'm doing something wrong. The dictionary is still empty:

.^ Mautic\LeadBundle\Segment\ContactSegmentFilter^ {#874
  +contactSegmentFilterCrate: Mautic\LeadBundle\Segment\ContactSegmentFilterCrate^ {#291
    -glue: "and"
    -field: "contact_id"
    -object: "lead"
    -type: "number"
    -filter: "2"
    -operator: "gt"
    -sourceArray: array:8 [
      "glue" => "and"
      "field" => "contact_id"
      "object" => "lead"
      "type" => "number"
      "operator" => "gt"
      "properties" => array:1 [
        "filter" => "2"
      ]
      "filter" => "0"
      "display" => null
    ]
    -nullValue: null
    -mergedProperty: []
  }
  -filterDecorator: Mautic\LeadBundle\Segment\Decorator\CustomMappedDecorator^ {#834
    #contactSegmentFilterOperator: Mautic\LeadBundle\Segment\ContactSegmentFilterOperator^ {#796
      -filterOperatorProvider: Mautic\LeadBundle\Provider\FilterOperatorProvider^ {#828
        -cachedOperators: []
        -dispatcher: Mock_EventDispatcherInterface_bb01641d^ {#655 …3}
        -translator: Mock_TranslatorInterface_69a4af1a^ {#803 …3}
      }
    }
=====↓↓↓↓↓=====
    #dictionary: Mautic\LeadBundle\Services\ContactSegmentFilterDictionary^ {#830
      -filters: []
      -dispatcher: Mock_EventDispatcherInterface_bb01641d^ {#655 …3}
    }
  }
=====↑↑↑↑↑=====
  -schemaCache: Mock_TableSchemaColumnsCache_84d8166a^ {#872 …5}
  -filterQueryBuilder: Mautic\LeadBundle\Segment\Query\Filter\ForeignFuncFilterQueryBuilder^ {#606
    -parameterNameGenerator: Mautic\LeadBundle\Segment\RandomParameterName^ {#402
      #lastUsedParameterId: 0
    }
    -dispatcher: Mock_EventDispatcherInterface_bb01641d^ {#655 …3}
  }
  -batchLimiters: []
}

This dump comes from:

class ForeignFuncFilterQueryBuilder extends BaseFilterQueryBuilder
{
    public function applyQuery(QueryBuilder $queryBuilder, ContactSegmentFilter $filter): QueryBuilder
    {
        dump($filter);

It should be

    #dictionary: Mautic\LeadBundle\Services\ContactSegmentFilterDictionary {#1800
      -filters: array:44 [
        ...
          "type" => "mautic.lead.query.builder.foreign.func"
          "foreign_table" => "test_table"
          "foreign_table_field" => "contact_id"
          "table" => "leads"
          "table_field" => "id"
          "func" => "count"
          "field" => "id"
          "where" => "some_bool = 1"
        ]
      ]

43 filter items are Mautic's default filters and the last one the custom filter.

escopecz commented 1 week ago

I pushed my WIP test

I don't see it

putzwasser commented 1 week ago

I noticed, I just committed (to the wrong branch) and didn't push 5 min ago :facepalm:

Came here to check it is published now. But you already caught me :sweat_smile:

escopecz commented 1 week ago

I don't see anything obvious. Try to go line by line with Xdebug to understand what's going on.