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

Add default mailing list interests #6

Merged
merged 12 commits into from
Apr 23, 2014

Conversation

charleswaddell
Copy link
Contributor

No description provided.

$this->app->db,
sprintf(
'select * from MailingListInterest
where instance %s %s order by displayorder',
Copy link
Member

Choose a reason for hiding this comment

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

We typically align select and where statements

@isagrant isagrant changed the title Add default interests Add default mailing list interests Apr 16, 2014
}
$class_name = SwatDBClassMap::get(
'DeliveranceMailingListInterestWrapper'
);
Copy link
Member

Choose a reason for hiding this comment

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

DeliveranceMailingListInterestWrapper.php needs to be required on the page. Currently this generates a fatal error.

Copy link
Member

Choose a reason for hiding this comment

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

As I needed it for testing, fixed in charleswaddell#1

@isagrant
Copy link
Member

DeliveranceSignupPage.php and DeliveranceMailChimpSignupPage.php both need their copyright years bumped as well.

parent::init();

$this->row_wrapper_class = SwatDBClassMap::get(
'DeliveranceMailingListInerest'
Copy link
Member

Choose a reason for hiding this comment

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

Typo, should be DeliveranceMailingListInterest

Copy link
Member

Choose a reason for hiding this comment

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

As I needed it for testing, fixed in charleswaddell#1

@isagrant
Copy link
Member

Remember to run the following sql on all sites:

alter table MailingListInterest add is_default boolean not null default false;

@charleswaddell
Copy link
Contributor Author

The SQL has been run on all stage and live databases.

@charleswaddell charleswaddell mentioned this pull request Apr 16, 2014
$this->app->notifier->send(
'newsletter_signup',
array(
'site' => $this->app->config->notifier->site,
'list' => $list->getShortname(),
'interests' => $this->getInterests(),
'interests' => $interests,
Copy link
Member

Choose a reason for hiding this comment

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

This should still use $this->getInterests() in case subclasses customize the list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not completely sure what to do about this one. The $this->getInterests() has been removed since it now lives on the list.

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps grab interests from the getSubscriberInfo() method, or add back a getInterests() method to do just that?

It's not the end of the world that it doesn't completely match the subscriber information, but it would be better if we set it up to.

charleswaddell and others added 4 commits April 16, 2014 16:01
This method was barely used, and gets an array of the full interest groupings from MailChimp.
…rests

Renaming old getInterests() to getInterestGroupings().

public function getDefaultSubscriberInfo()
{
$info = parent::getDefaultSubscriberInfo();
Copy link
Member

Choose a reason for hiding this comment

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

parent::getDefaultSubscriberInfo() is abstract. Either make it return an empty array, or assign info to an empty array here.

isagrant added a commit that referenced this pull request Apr 23, 2014
@isagrant isagrant merged commit fd9e680 into silverorange:master Apr 23, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants