Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

M3: Point email action improvements + Refactor PointEventHelper #8161

Closed
wants to merge 26 commits into from

Conversation

kuzmany
Copy link
Member

@kuzmany kuzmany commented Nov 27, 2019

Please be sure you are submitting this against the staging branch.

Q A
Bug fix?
New feature?
Automated tests included?
Related user documentation PR URL
Related developer documentation PR URL
Issues addressed (#s or URLs) #8055
BC breaks?
Deprecations?

Description:

This PR originally published #7330 resolved Mautic 3 Refactor PointEventHelper issue #8055

This PR try figure out trigger points for each entity in points action reported in this issue #7320

  • added support for events system (eventName instead of callback, callback should be depraced and refactored in future)
  • added new column internal_id to point_lead_action_log table - ID or string - in thu future we can store URL for page hit and then we can run points action on each visits
  • added new option to email open point action - categories limit and execute one time/each email

image

Steps to test this PR:

  1. Load up this PR
  2. Create point actions - opens email type
  3. Test step by step with different point actions
  • Allow repeat the action - should change points everytime
  • Execute only: Once - should change points just one time
  • Execute only: For each email - should change points with every email ID - that means If I will create point action just for 3 emails, and all 3 emails my contact open , contact point score adjust point score each time
  1. Test also categories filter

List deprecations along with the new alternative:

  1. callback depraced on triggerAction
    'eventName' => EmailEvents::ON_POINT_CHANGE_ACTION_EXECUTED,

    Use eventName like in other parts of Mautic.

@kuzmany kuzmany changed the title Points action update Point email action improvements Nov 27, 2019
@kuzmany kuzmany added this to In progress in Mautic 3 Nov 27, 2019
@kuzmany kuzmany added the WIP PR's that are not ready for review and are currently in progress label Nov 27, 2019
@kuzmany kuzmany changed the title Point email action improvements M3: Point email action improvements + Refactor PointEventHelper Nov 28, 2019
@kuzmany kuzmany added ready-to-test PR's that are ready to test and removed WIP PR's that are not ready for review and are currently in progress labels Nov 28, 2019
@kuzmany kuzmany moved this from In progress to Needs code review and/or test in Mautic 3 Nov 28, 2019
@kuzmany kuzmany added WIP PR's that are not ready for review and are currently in progress and removed ready-to-test PR's that are ready to test labels Nov 28, 2019
@kuzmany kuzmany moved this from Needs code review and/or test to In progress in Mautic 3 Nov 28, 2019
@escopecz escopecz removed this from In progress in Mautic 3 Dec 4, 2019
@escopecz
Copy link
Sponsor Member

escopecz commented Dec 4, 2019

@kuzmany I removed this PR from the M3 project because we want to track only issues there. And please check Travis.

{
$action = $changeActionExecutedEvent->getPointAction();

if ($action->getType() != 'email.open') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we can use the checkContext that I suggested be added to the PointChangeActionExecutedEvent.

@@ -77,6 +77,12 @@ public function addAction($key, array $action)
$action
);

//Support for old way with callback and new event based system
//Could be removed after all events will be refactored to events. The key 'eventName' will be mandatory and 'callback' will be removed.
if (!array_key_exists('callback', $action) && !array_key_exists('eventName', $action)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it's m2-to-m3 I think we should drop support for callbacks now and refactor to events

$this->eventDetails = $eventDetails;
$this->completedActions = $completedActions;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a checkContext method here?

public function checkContext(string $context): bool
{
    return $this->pointAction->getType() === $context;
}

*
* @return bool
*/
private function invokeCallback(Point $action, Lead $lead, $eventDetails, array $settings)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should drop callback support altogether

@kuzmany
Copy link
Member Author

kuzmany commented Dec 11, 2019

@escopecz so this PR is removed from Mautic3 or not?

@escopecz escopecz added this to the 3.0.0 milestone Dec 11, 2019
@escopecz
Copy link
Sponsor Member

@kuzmany it will to go into M3. We want to get rid of MauticFactory. We are not certain we will be able to do so for all occurrences but we definitely want to merge in WIP PRs like this one. Did I answer your question or is there something else behind it?

@dongilbert
Copy link
Member

Please rebase this to the 3.x branch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WIP PR's that are not ready for review and are currently in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants