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

Fix frequency rules #6896

Merged
merged 1 commit into from Nov 30, 2018

Conversation

Projects
5 participants
@kuzmany
Copy link
Contributor

kuzmany commented Nov 16, 2018

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) #6729
BC breaks?
Deprecations?

Description:

Based on this issue #6729 I notice bad condition If frequency rules should applied.

Steps to reproduce the bug:

  1. Go to configurations, Emails and set Default Frequency Rule to 1 each week
    image
  2. Create two campaign width send email action
  3. Run both campaign
  4. See contact profile page, both email send, even we set frequency rule

Steps to test this PR:

  1. Repeat all steps and see If second email is pending

@kuzmany kuzmany added Bug WIP labels Nov 16, 2018

@kuzmany kuzmany added this to the 2.15.0 milestone Nov 16, 2018

@kuzmany kuzmany added Ready To Test and removed WIP labels Nov 16, 2018

@Woeler Woeler requested a review from alanhartless Nov 16, 2018

@Woeler Woeler added this to To do in Testing 2.15.0 Nov 16, 2018

@Noa83

Noa83 approved these changes Nov 26, 2018

Copy link
Contributor

Noa83 left a comment

Tested with success.

@Woeler Woeler moved this from To do to Tested once in Testing 2.15.0 Nov 26, 2018

@npracht
Copy link
Member

npracht left a comment

It works 👍

@npracht npracht removed the request for review from alanhartless Nov 29, 2018

@npracht npracht moved this from Tested once to Ready to commit in Testing 2.15.0 Nov 29, 2018

@Woeler Woeler merged commit 7fffcef into mautic:staging Nov 30, 2018

2 checks passed

Scrutinizer Analysis: 2 new issues – Tests: passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Testing 2.15.0 automation moved this from Ready to commit to Merged Nov 30, 2018

@Woeler Woeler removed the Ready To Commit label Nov 30, 2018

@alanhartless

This comment has been minimized.

Copy link
Contributor

alanhartless commented Nov 30, 2018

This will apply frequency rules to all outgoing email including transactional which is not correct. Emails like form submissions emails "thank you for submitting our form!" should not be moderated by frequency rules.

This is not the bug and the PR should be converted. If you want the campaign to be moderated by frequency rules, you need to set the send email action to be a "marketing" email.

We have to be really careful with making "global" fixes for "localized" bugs.

@kuzmany

This comment has been minimized.

Copy link
Contributor Author

kuzmany commented Nov 30, 2018

@alanhartless
Can you explain how frequency rules should work? That means frequency rules are used just with marketing emails? Then feature is not documented and not clear for all clients

image

@npracht

This comment has been minimized.

Copy link
Member

npracht commented Nov 30, 2018

Moreover there is a misunderstood here. Transactional email are not email that you can unsubscribe from (autoresponder, purchase, account creation). Marketing email yes.
So transactional should be shoot to DNC (unsub) contacts.

Because it is not managed like that, i thought the the mention of Transactional here was a misuse and the brought feature was - for marketing emails, you can chose to do not send it a second time if the contact already had it from another campaign.


We'll have to take car of the feature definition and guideline in M3 around here and use the proper "words". I'll add it to the list of points to care about.

@alanhartless

This comment has been minimized.

Copy link
Contributor

alanhartless commented Nov 30, 2018

@npracht agreed but we should probably put that in it's own issue if it's not there already as that is a system wide behavior versus campaign behavior.

@kuzmany Sorry, I meant in the code I referenced. email_type should be hard coded to marketing rather than $type which will address the bug this PR was trying to resolve. The transactional vs marketing feature in the campaign action UI applies to emails across campaigns and is correctly handled by

.

@npracht

This comment has been minimized.

Copy link
Member

npracht commented Nov 30, 2018

@alanhartless i also agree. I even think we should keep that under the arm for next major. What i wanted to point out is that the fix itself is probably based on a feature misunderstood :)
Thanks for your feedback anyway !!

@kuzmany

This comment has been minimized.

Copy link
Contributor Author

kuzmany commented Nov 30, 2018

My fail guys. Settings from configuration doesn't mention transactional/marketing etc. This is reason why I assume this feature should work in another way. Now It's clear for me.

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.