Accountless subscriptions

[johanb]

This adds a "Subscriber" model as suggested in #2. Since I didn't own the issue I apparently can't attach the PR directly to that issue.

It's not ready to be merged yet. Intended to allow discussion about the implementation:

Some choices I've made which could be debated:

• Use a databaseless model for subscribers, since we wouldn't be able to keep track of the users anyway, it doesn't make sense to store them in both locations (Mailchimp & Spree) I think. Therefor I've otped for a simple model
• My main goal is to have an e-mail field with a submit button later on, therefor an e-mail adress is always required. However, I've allowed the use case of more extensive forms with other merge tags to be possible. If you were to create a form with say a "firstname", "lastname", "email", it will define methods for those and if they correspond to merge tags, they will be send in to mailchimp. The way this is currently implemented could probably use a cleanup.
• Currently it doesn't matter which attribute has changed, as long as 1 attribute has changed all merge tags get reposted to the server. Is this wise ?

TODO:

• Add a controller for accepting the form input, sanatizing it, and passing it to mailchimp
• Add an example form
• Subscribe full users to a different segment on the same list as subscribers

[johanb]

@joshnuss I'm trying to wrap my head around the intention of the "attributes_changed?" method in Spree::Hominid::Subscription. Why are you doing this ?

    def attributes_changed?
      Config.preferred_merge_vars.values.any? do |attr|
        @user.send("#{attr}_changed?")
      end
    end

can't you just use

 @changes.keys.any?

you've already duped the changes. I can see it's purpose if you intend to only update single merge vars, but you're currently not doing that.

I'm fine with either, can incorporate it in the PR if you want.

[joshnuss]

the purpose of the method is to check whether any a merge var (and only a merge var) has changed.

it could be that the user model has many attributes that are not merge vars, changing those should not incur an api call.

it also makes it possible to do dirty tracking for a computed merge var.

Spree::User.class_eval do
  def a_computed_merge_var
     # do a bunch of stuff
  end

  def a_computed_merge_var_changed?
     # let caller know if it changed. or return true to update hominid every time
  end
end

[joshnuss]

it is a little confusing though. so, im gonna change the "attributes_changed?" method name to "merge_vars_changed?". that explains better what the method does.

and the stuff about implementing a computed merge var will be added to the readme.

[joshnuss]

@johanb what do you think about using a regular active record model for the Subscriber class? there could be a few advantages:

• easy to know if a user is subscribed without needing an api call (yes we do have implement a handshake with mailchimp when user unsubscribes, but we already have this issue with the User model)
• when the subscription code runs synchronously (no delayed_job/sidekiq) should the api call error out, we at least have some recourse.
• slightly less code

what do u think?

[johanb]

@joshnuss that's also a possibility. I agree on all points although I never intend to run it synchronously do you ? I think that should be discouraged at all costs but I guess it has to be supported anyway.

ps. do you hang out in #spree (on irc) from time to time ?

[joshnuss]

im txrx on irc

On Thu, May 9, 2013 at 12:36 PM, Johan Bruning notifications@github.comwrote:

@joshnuss https://github.com/joshnuss that's also a possibility. I agree on all points although I never intend to run it synchronously do you ? I think that should be discouraged at all costs but I guess it has to be supported anyway.

ps. do you hang out in #spree (on irc) from time to time ?

— Reply to this email directly or view it on GitHubhttps://github.com/DynamoMTL/spree-hominid/pull/5#issuecomment-17674530 .

[johanb]

I've added a bit for the static segmenting but we have to make some decisions:

1. Should it be mandatory to segment your list ? or is that up to the user to choose ?
2. I've been clicking around mailchimp UI and I haven't found a convenient way to find the ID of the segment I had set up other then hovering over a link that says "Send to segment" (it's the id at the end) Not sure if it's an issue. Maybe I just haven't found the right place to look in Mailchimp's UI yet.

I should also still check if the passed segment id is valid on boot I think.

Note that this last commit broke 2 specs, I'm not sure why though. Maybe you have an idea @joshnuss ?

[johanb]

@joshnuss I'd love your feedback on this progress so far. The basic functionality is included now and works. The choices I've made for "defaults" are debatable however.

[joshnuss]

@johanb looks pretty good, may want to adjust the defaults a little but let's discuss it further on irc. i'm back on monday

[johanb]

@joshnuss sounds good to me. I've noticed there is one issue though. If the user is already signed-up, (and Spree chimpy doesn't know about this) Hominid will throw an exception (214). This is for both subscribers & users an issue if the data isn't synced. Since the user will be shown a 500. We should catch this exception I believe.

[johanb]

@joshnuss I'm hesitant to move forward on this without your input. Let me know when you have time to discuss. I'm on IRC most of the time.

[johanb]

TODO:

• [x] Remove the deface override
• [x] Check if static segment exist otherwise create it
• [x] Static segment name defaults to "customers" but should be configurable
• [x] Create method to sync static segment
• [ ] Add tests for controller
• [ ] Add documentation explaining the difference between a "subscriber" and a "subscribed user"
• [ ] Add documentation explaining how to set up the subscriber model for merge vars

[joshnuss]

All merged into master. @johanb Thanks for the awesome contribution!