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

Fixes to preferences center #7006

Merged
merged 22 commits into from Jan 30, 2019

Conversation

@kuzmany
Copy link
Contributor

kuzmany commented Dec 13, 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)
BC breaks?
Deprecations?

Description:

Preferences center did a lot of strange things. I don't remember which PR mess it, but it was during this code was changes https://github.com/kuzmany/mautic/blob/ef5c590baba41133371f9a53b76e3e7caeb3f257/app/bundles/LeadBundle/Form/Type/ContactFrequencyType.php#L53-L53

I noticed several issues:

Enabled preferences center - use default center (not landing page)
Generate a lot of fields even If you disabled it or enable some options. If you just enable preference center, still generate ugly frequency rules.

Enabled preferences center - use landing page as prefs center
You cannot for example use just segments in your prefs center

Steps to reproduce the bug:

  1. Test preferences center on default and landing page
  2. Notice a lot of issues

Steps to test this PR:

  1. Load up this PR
  2. Test again

@npracht npracht added this to the 2.15.1 milestone Dec 13, 2018

@kuzmany

This comment has been minimized.

Copy link
Contributor Author

kuzmany commented Dec 13, 2018

This PR is based on @johbuch test of 2.15. beta. Please consider as 2.15 beta bugfix. There were a few improvements to preferences center, but together doesn't work :(

@Woeler

@kuzmany kuzmany added WIP and removed Ready To Test labels Dec 14, 2018

@kuzmany kuzmany added Ready To Test and removed WIP labels Dec 14, 2018

@kuzmany

This comment has been minimized.

Copy link
Contributor Author

kuzmany commented Dec 14, 2018

@johbuch @npracht DNC fixed for both type of prefs center (default, custom landing page)

@Woeler Woeler modified the milestones: 2.15.1, 2.15.0 Dec 14, 2018

@johbuch
Copy link

johbuch left a comment

tested and it works ! thanks zdeno !

@npracht npracht added this to Ready to Test (confirmation) in Mautic 2 Jan 3, 2019

@npracht npracht moved this from Ready to Test (confirmation) to Code Review (2 required) in Mautic 2 Jan 3, 2019

@alanhartless alanhartless added this to Needs Second Test in 2.15.1 Jan 14, 2019

kuzmany added some commits Jan 15, 2019

@cStuartHardwick

This comment has been minimized.

Copy link

cStuartHardwick commented Jan 18, 2019

Maybe this does bring out the point that the code needs to perform a check for whether those options are enabled and catch the error and give a better error message instead of the Symfony error message that displays?

Don't know if that's possible..

Yes, that's not right. If you turn off PC elements on the email settings page, it breaks any existing custom PCs and causes them to display Symfony errors for those controls. That's definitely not right, because there may already be emails out in the world at the time.

When using a custom preference center, its configuration should trump that set under email settings. Email settings should only control the default generated PC.

Thx

You guys are making me want to learn Symfony & Twist. Cool tech.

@mbrinson

This comment has been minimized.

Copy link

mbrinson commented Jan 19, 2019

Darn, I missed the point about applying this against the "staging" branch.
I applied the fixes in my 2.15.0 environment and while it fixed the generated HTML output, I'm no longer able to edit the landing page. When I click "Edit" it spins for a bit, and then does nothing.

I'm hoping 2.15.1 gets released soon! Can't wait for this fix to become fully available. Thanks again kuzmany!

@mbrinson

This comment has been minimized.

Copy link

mbrinson commented Jan 19, 2019

Darn, I missed the point about applying this against the "staging" branch.
I applied the fixes in my 2.15.0 environment and while it fixed the generated HTML output, I'm no longer able to edit the landing page. When I click "Edit" it spins for a bit, and then does nothing.

I'm hoping 2.15.1 gets released soon! Can't wait for this fix to become fully available. Thanks again kuzmany!

Actually, it works against 2.15.0 as well. It turns out I had missed one ending bracket in the code I had copied from the diff. Awesome!

@kuzmany

This comment has been minimized.

Copy link
Contributor Author

kuzmany commented Jan 19, 2019

If works, please approve this PR

image

@mbrinson
Copy link

mbrinson left a comment

Works great now!

@cStuartHardwick

This comment has been minimized.

Copy link

cStuartHardwick commented Jan 19, 2019

Looks good. A few minor issues:

  1. If a custom preference center has any PC field on it that is turned off under email settings on the configuration screen, it generates a Symfony error at run time. This is a bug. It's a bug with a simple work-around (always turn on all options under email options when using preference center) but it's still a bug, and will certainly cause some users to have problem. This ought to be fixed or, at the very least, a check added to force all PC options to "Yes" when "Show contact preference settings" is "yes".
  2. The "Show contact preference settings " control under email settings on the configuration page is confusingly named. I would suggest "Used Custom Preference Centers" as better describing what it does, with revised help text. The help text implies that setting this to "Yes" turns off the default generated preference center, but that's wrong. Unless I'm mistaken, the default PC is still shown unless or until new emails are sent, for which a PC landing page has been associated.
  3. If a user who has "do not contact" set visits the custom preference center, there is no way for them to clear the "Do not contact flag". On the default generated unsubscribe screen, you have the option to "resubscribe" which clears this flag.
  4. The "Categorylist" control does not display when included on the custom preferences center landing page. I don't know what this is, as there is no "Categories" field on a contact (that I can find) and Categories do not allow you to add contacts to them like segments (as far as I can see) although the email settings panel does have a switch to enable showing the category list. Something is out of sync here.
@cStuartHardwick
Copy link

cStuartHardwick left a comment

Current bugs resolved. There are some other issues that ought to be addressed, but these are not breaking changes:

  1. If a custom preference center has any PC field on it that is turned off under email settings on the configuration screen, it generates a Symfony error at run time. This is a bug. It's a bug with a simple work-around (always turn on all options under email options when using preference center) but it's still a bug, and will certainly cause some users to have problem. This ought to be fixed or, at the very least, a check added to force all PC options to "Yes" when "Show contact preference settings" is "yes".
  2. The "Show contact preference settings " control under email settings on the configuration page is confusingly named. I would suggest "Used Custom Preference Centers" as better describing what it does, with revised help text. The help text implies that setting this to "Yes" turns off the default generated preference center, but that's wrong. Unless I'm mistaken, the default PC is still shown unless or until new emails are sent, for which a PC landing page has been associated.
    
@kuzmany

This comment has been minimized.

Copy link
Contributor Author

kuzmany commented Jan 20, 2019

  1. I didn't see any notice info. Last commit fixed it. Works for me properly
  2. Will see, maybe little re-arrange the configuration
  3. Require enable show_contact_frequency and same token on landing page
  4. You need enable Show category list / Any global category exist and also token on custom landing page
@cStuartHardwick

This comment has been minimized.

Copy link

cStuartHardwick commented Jan 20, 2019

Hi @kuzmany,
#1, the Syfony error message appearing on the PF, while minor, is definitely not fixed. It only appeared to be fixed because it only happens when some PC features are turned off under Configuration/Email settings but are used on an existing custom preference center landing page. When you do a commit, you reset all the PC settings to "Yes" under config. But if you turn some back to "No" as I have just done to confirm, the Symfony error messages return.

To be clear, I consider this a very minor thing with an easy work around and would really prefer it not hold up 2.15.1--but I don't want you to think it's fixed, either.

On #3 & #4, yes, I see that you are right. Sorry I missed that. Both of these are working correctly.

@kuzmany kuzmany dismissed stale reviews from cStuartHardwick and mbrinson via f21dcad Jan 21, 2019

@cStuartHardwick

This comment has been minimized.

Copy link

cStuartHardwick commented Jan 22, 2019

After the last change, PC is broken again and it's not longer possible to create or edit any emails.

@cStuartHardwick

This comment has been minimized.

Copy link

cStuartHardwick commented Jan 23, 2019

@kuzmany, I'm not real solid on the Github process, but I'm assuming your changes conflicts with someone else's and had to be reverted and reapplied? Should we be waiting to re-test here or will that be rolled in somewhere else?

Thx for all your work.

kuzmany added some commits Jan 24, 2019

@kuzmany

This comment has been minimized.

Copy link
Contributor Author

kuzmany commented Jan 29, 2019

This issue is related to this PR #7168
I merge it to this repo, should works now within few minutes

@alanhartless alanhartless moved this from Discussion to Needs Second Test in 2.15.1 Jan 29, 2019

Mautic 2 automation moved this from Changes Requested / Review to Ready to Test (first time) Jan 30, 2019

@alanhartless alanhartless merged commit 98403d8 into mautic:staging Jan 30, 2019

2 checks passed

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

Mautic 2 automation moved this from Ready to Test (first time) to Merged Jan 30, 2019

@alanhartless alanhartless moved this from Needs Second Test to Merged in 2.15.1 Jan 30, 2019

@leadsmania

This comment has been minimized.

Copy link

leadsmania commented Feb 15, 2019

Hi can I get this code for preference center? I sure would like to get it working again. Do I have to wait for 2.15.1? In my emails people are seeing a server error when they click on the unsubscribe link.

@leadsmania

This comment has been minimized.

Copy link

leadsmania commented Feb 15, 2019

I only have a small group of people that get emails but im worried that it looks unprofessional. That nice preference center me and my wife spent hours on would make us jolly if we can get to see it again.

@cStuartHardwick

This comment has been minimized.

Copy link

cStuartHardwick commented Mar 3, 2019

Hi can I get this code for preference center? I sure would like to get it working again. Do I have to wait for 2.15.1? In my emails people are seeing a server error when they click on the unsubscribe link.

In theory, you can download any branch from Git, and install on your site (and deal with potential merge issues later). I don't really see that as practical though, personally, even through I work in IT (in different technologies). There are databases changes as well as code changes, and the potential for things going wrong (or at least becoming a huge time sink) is large.

I too would dearly leave to give my subscribers a working preference center (which they have been complaining about) but see to practical way forward except to wait.

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.