Accountless subscriptions #5

Merged
merged 19 commits into from May 15, 2013

Conversation

Projects
None yet
2 participants
@johanb
Contributor

johanb commented May 9, 2013

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
app/models/spree/hominid/subscriber.rb
+ end
+
+ def subscribed_changed?
+ true

This comment has been minimized.

@johanb

johanb May 9, 2013

Contributor

Since I always return true here, all other "attribute_changed?" methods have no effect, we can safely assume that if a user enters his e-mail he is not subscribed yet right ?

@johanb

johanb May 9, 2013

Contributor

Since I always return true here, all other "attribute_changed?" methods have no effect, we can safely assume that if a user enters his e-mail he is not subscribed yet right ?

Refactor Subscriber class to use more conventional methods from Activ…
…eModel.

This has certain benefits:

*  Easier to create forms
*  Allot of validation for free
*  Allows validating of email to not be blank
*  Adds mass-assignment protection.
@johanb

This comment has been minimized.

Show comment
Hide comment
@johanb

johanb May 9, 2013

Contributor

@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.

Contributor

johanb commented May 9, 2013

@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

This comment has been minimized.

Show comment
Hide comment
@joshnuss

joshnuss May 9, 2013

Contributor

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
Contributor

joshnuss commented May 9, 2013

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

This comment has been minimized.

Show comment
Hide comment
@joshnuss

joshnuss May 9, 2013

Contributor

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.

Contributor

joshnuss commented May 9, 2013

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

This comment has been minimized.

Show comment
Hide comment
@joshnuss

joshnuss May 9, 2013

Contributor

@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?

Contributor

joshnuss commented May 9, 2013

@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

This comment has been minimized.

Show comment
Hide comment
@johanb

johanb May 9, 2013

Contributor

@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 ?

Contributor

johanb commented May 9, 2013

@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

This comment has been minimized.

Show comment
Hide comment
@joshnuss

joshnuss May 9, 2013

Contributor

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
.

Contributor

joshnuss commented May 9, 2013

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 added some commits May 9, 2013

Quite a lot of changes included:
*  Add segmentation to the interface
*  Automatically segment all users, but don't segmentize subscribers
*  Add a preference for adding the ID of a segment
@johanb

This comment has been minimized.

Show comment
Hide comment
@johanb

johanb May 9, 2013

Contributor

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) this 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 ?

Contributor

johanb commented May 9, 2013

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) this 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

This comment has been minimized.

Show comment
Hide comment
@johanb

johanb May 11, 2013

Contributor

@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.

Contributor

johanb commented May 11, 2013

@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

This comment has been minimized.

Show comment
Hide comment
@joshnuss

joshnuss May 11, 2013

Contributor

@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

Contributor

joshnuss commented May 11, 2013

@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

This comment has been minimized.

Show comment
Hide comment
@johanb

johanb May 12, 2013

Contributor

@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.

Contributor

johanb commented May 12, 2013

@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

This comment has been minimized.

Show comment
Hide comment
@johanb

johanb May 14, 2013

Contributor

@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.

Contributor

johanb commented May 14, 2013

@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

This comment has been minimized.

Show comment
Hide comment
@johanb

johanb May 14, 2013

Contributor

TODO:

  • Remove the deface override
  • Check if static segment exist otherwise create it
  • Static segment name defaults to "customers" but should be configurable
  • 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
Contributor

johanb commented May 14, 2013

TODO:

  • Remove the deface override
  • Check if static segment exist otherwise create it
  • Static segment name defaults to "customers" but should be configurable
  • 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

johanb and others added some commits May 14, 2013

Add note in read me about adding a deface override to add in the subs…
…criber form

Add convenience methods for creating and or finding a static segment by name + added a preference for the preferred static segment name

Pass in segment name instead of id

Use the retrieved static_segment_id over the old passed in segment_id

@joshnuss joshnuss merged commit 0d13d6e into DynamoMTL:master May 15, 2013

@joshnuss

This comment has been minimized.

Show comment
Hide comment
@joshnuss

joshnuss May 15, 2013

Contributor

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

Contributor

joshnuss commented May 15, 2013

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

mrogers-5s referenced this pull request in mrogers-5s/spree_chimpy Apr 4, 2018

# This is a combination of 6 commits.
# This is the 1st commit message:

fix(lib/spree/chimpy/interface/products.rb): Fix bug POST call for new products when variants have n

Modify the self.variant_hash method of the Products interface
-- Only include the :image_url if a
vairant image is present. This is an optional key, but causes an error in the MailChimp API if set
to nil. It should be excluded if it's not defined on the object.

# The commit message #2 will be skipped:

# feat(Carts): WIP: Add support for Abandoned Carts in MailChimp
#
# Initial setup of Abandoned Cart support for MailChimp
# -- Add new Cart related classes to mirror
# Order classes
#
# TODO: Find the right place to leverage Cart events for the MC API calls

# The commit message #3 will be skipped:

# feat(Carts): Modify Order state_machine
#
# Add call to `send_cart_to_mail_chimp` on `before_transition` to ANY state EXCEPT complete (On the
# transition to "complete" an ORDER will be sent to MailChimp, and any associated CART will be removed

# The commit message #4 will be skipped:

# feat(Order / Cart Tracking): Fix error in hash builder methods for Orders/Carts
#
# Fix errors in hash builders for Orders/Carts

# The commit message #5 will be skipped:

# feat(Carts): Fix return value from hash generation methods for carts/orders

# The commit message #6 will be skipped:

# feat(carts): Add support for tracking Carts
#
# Add support for tracking Carts in MailChimp
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment