{"action":"created","comment":{"url":"https://api.github.com/repos/solidusio/solidus/pulls/comments/71575235","id":71575235,"diff_hunk":"@@ -0,0 +1,55 @@\n+# Default sender class for Spree::NotificationDispatch, sends via\n+# ActionMailer.\n+#\n+# You can see the mapping from notification message types to\n+# ActionMailer classes and methods in `.mailer_dispatch_table`, which\n+# you can also customize to map to different mailers or methods.\n+#\n+# The particular argument list for each message type is\n+# specified in NotificationDispatch.signatures\n+class Spree::NotificationDispatch::ActionMailerDispatch\n+  class_attribute :mailer_dispatch_table\n+  # values are pairs of fully qualified mailer class names, and method names\n+  self.mailer_dispatch_table = {\n+    carton_shipped: ['Spree::CartonMailer', :shipped_email],\n+    order_confirm: ['Spree::OrderMailer', :confirm_email],\n+    order_cancel: ['Spree::OrderMailer', :cancel_email],\n+    order_inventory_cancel: ['Spree::OrderMailer', :inventory_cancellation_email],\n+    reimbursement_processed: ['Spree::ReimbursementMailer', :reimbursement_email]\n+  }","path":"core/app/models/spree/notification_dispatch/action_mailer_dispatch.rb","position":19,"original_position":19,"commit_id":"7d6b63b26e4269ca178a934a6f5573f0ccd9bfca","original_commit_id":"7d6b63b26e4269ca178a934a6f5573f0ccd9bfca","user":{"login":"jrochkind","id":149304,"avatar_url":"https://avatars.githubusercontent.com/u/149304?v=3","gravatar_id":"","url":"https://api.github.com/users/jrochkind","html_url":"https://github.com/jrochkind","followers_url":"https://api.github.com/users/jrochkind/followers","following_url":"https://api.github.com/users/jrochkind/following{/other_user}","gists_url":"https://api.github.com/users/jrochkind/gists{/gist_id}","starred_url":"https://api.github.com/users/jrochkind/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jrochkind/subscriptions","organizations_url":"https://api.github.com/users/jrochkind/orgs","repos_url":"https://api.github.com/users/jrochkind/repos","events_url":"https://api.github.com/users/jrochkind/events{/privacy}","received_events_url":"https://api.github.com/users/jrochkind/received_events","type":"User","site_admin":false},"body":"I think it important to be able to switch out an ActionMailer back-end for another back-end (likely using a third party HTTP API), _without_ having to copy-and-paste or reproduce standard logic around notifications -- such as suppressing a certain notification event. You should be able to turn off a certain notification event without worrying about whether it's an ActionMailer or another delivery mechanism, or worrying that someone later changing the back-end delivery method will break your attempt to disable certain messages. \r\n\r\nAdditionally, I think sub-classing is really no better than class_eval, just like class_eval it exposes the entirety of internals as dependencies of your customization. It's safest if you can write a new non-ActionMailer backend _without_ having to sub-class -- composition over inheritance, this keeps the contractual API as clear and as small as possible. \r\n\r\nSo splitting the logic between the NotificationDispatch and the particular back-end, by default ActionMailerDispatch, was an attempt to meet these design goals. \r\n\r\nThe lambda stuff definitely can be gotten rid of.  I don't think the lambda stuff knows too much about the actual mailers -- _all_ the lambda stuff knows about is the _method signature_ of the particular notification, it doens't actually know anything at all about the actual mailers.  Prior to this PR, the _only_ documentation or logic of \"what is the method signature of a particular notification event\" was to look at each individual ActionMailer class.  With the idea of being able to swap out an ActionMailer back-end for another delivery mechanism, I think this is unacceptable -- there needs to be a single central place that specifies exactly what signatures your custom back-end delivery needs to respond to. That's the only point of the \"lambda stuff\".   It could be dispensed with  if you don't think that's neccesary -- but I think there needs to be an easy  (and test-verifiable) answer to \"I want to implement a non-ActionMailer back-end, exactly what messages/signatures does it need to respond to in order to handle all notifications\", and the \"lambda stuff\" was the simplest possible solution to that I could think of. \r\n\r\nEvery element of this design is actually intentional to meet a particular design goal. It does look like several classes that each don't do much -- but I think this is what you get when you want simple objects with clean API's composed together (instead of inheritance) -- I think it's actually the sign of a good OO design ala Sandi Metz et al. ","created_at":"2016-07-20T17:59:34Z","updated_at":"2016-07-20T17:59:34Z","html_url":"https://github.com/solidusio/solidus/pull/1218#discussion_r71575235","pull_request_url":"https://api.github.com/repos/solidusio/solidus/pulls/1218","_links":{"self":{"href":"https://api.github.com/repos/solidusio/solidus/pulls/comments/71575235"},"html":{"href":"https://github.com/solidusio/solidus/pull/1218#discussion_r71575235"},"pull_request":{"href":"https://api.github.com/repos/solidusio/solidus/pulls/1218"}}},"pull_request":{"url":"https://api.github.com/repos/solidusio/solidus/pulls/1218","id":72532493,"html_url":"https://github.com/solidusio/solidus/pull/1218","diff_url":"https://github.com/solidusio/solidus/pull/1218.diff","patch_url":"https://github.com/solidusio/solidus/pull/1218.patch","issue_url":"https://api.github.com/repos/solidusio/solidus/issues/1218","number":1218,"state":"open","locked":false,"title":"NotificationDispatch central abstraction for transactional emails","user":{"login":"jrochkind","id":149304,"avatar_url":"https://avatars.githubusercontent.com/u/149304?v=3","gravatar_id":"","url":"https://api.github.com/users/jrochkind","html_url":"https://github.com/jrochkind","followers_url":"https://api.github.com/users/jrochkind/followers","following_url":"https://api.github.com/users/jrochkind/following{/other_user}","gists_url":"https://api.github.com/users/jrochkind/gists{/gist_id}","starred_url":"https://api.github.com/users/jrochkind/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jrochkind/subscriptions","organizations_url":"https://api.github.com/users/jrochkind/orgs","repos_url":"https://api.github.com/users/jrochkind/repos","events_url":"https://api.github.com/users/jrochkind/events{/privacy}","received_events_url":"https://api.github.com/users/jrochkind/received_events","type":"User","site_admin":false},"body":"Followed @jhawthorn's [ideas](https://github.com/solidusio/solidus/pull/1183#issuecomment-222553265), this is where I ended up. I think it looks pretty good, fairly low line count supporting lots of extension. I could use feedback before I add tests (yes, it's missing tests right now, yeah, I didn't do TDD here. although note it passes all current solidus tests without changing them, and there were existing tests ensuring email is sent at the right places!). \r\n\r\n* I was guided by customization/extension stories, trying to make sure there was a way to do common customizations. Including swapping out ActionMailer for another delivery mechanism (email or even non-email), and adding new notifications (solidus_auth_devise adds a password reset one)\r\n\r\n\r\n* I tried to provide mechanisms for these extension that do _not_ rely on sub-classing -- I don't think subclassing is really any safer/more desirable than `class_eval`, as sub-classing still has access to all private methods and iVars just like class_eval. \r\n\r\n* I like @jhawthorn's suggestion to separate public API from ActionMailer implementation entirely, but didn't think the 'message type' being sent in needed to be two params (`.new(:order, :confirm)`), one suffices (`.new(:order_confirm)`). Among other things, this makes the story for adding new notification types a lot simpler, as well as diverting just certain notification types from default solidus to your own implementation. \r\n\r\n* With the idea of swapping out the delivery backend, I thought it needed a clear specification in one place of what the possible messages were and what arguments each took. Having to hunt down the current implementation of all the various mailers wasn't good enough. So this does include some simple specification and run-time checking -- could be enhanced in the future to specify/check more things about args within this architecture. This is also where you add additional message types (like solidus_auth_devise and it's password_reset), `Spree::NotificationDispatch.signatures`, taking lambdas that raise on bad messages. \r\n\r\n* I think only/except actually is important frequently needed func -- this argument is supported by the existing  `Spree::OrderCancellations.send_cancellation_mailer`, which is really a subset of general only/except functionality (and could be deprecated for the general config, but hasn't yet been in this PR). So this is built-in, at a higher level than the swappable delivery mechanism. \r\n\r\n* You swap out delivery mechanisms with `Spree::NotificationDispatch.delivery_class_name = 'Whatever::Whatever'` (following general late pattern of string class name instead of class itself, to not have to worry about dev-mode reloading).  Your new sender class just needs a similar `SenderClass.new(:message_type).deliver(type-specific-args)`, and shouldn't need to subclass anything, occasionally might delegate/compose the default `ActionMailerDispatch`.\r\n\r\n* The default `ActionMailerDispatcher` lets you change what mailers/methods individual message types are dispatched to, with `Spree::NotificationDispatch::ActionMailerDispatch.mailer_dispatch_table`. \r\n Want to keep default solidus for order confirm, but use your own mailer for order cancel? No problem!   (Note this is a superset of existing `carton_shipped_email_class` pref, which only existed for that one set of message types. `Spree::Config.carton_shipped_email_class` _is_ deprecated in this PR). \r\n\r\n\r\n","created_at":"2016-06-03T16:15:13Z","updated_at":"2016-07-20T17:59:34Z","closed_at":null,"merged_at":null,"merge_commit_sha":"bf504cc9e97cec790f5b0ef316f90c46be71d4f7","assignee":null,"assignees":[],"milestone":null,"commits_url":"https://api.github.com/repos/solidusio/solidus/pulls/1218/commits","review_comments_url":"https://api.github.com/repos/solidusio/solidus/pulls/1218/comments","review_comment_url":"https://api.github.com/repos/solidusio/solidus/pulls/comments{/number}","comments_url":"https://api.github.com/repos/solidusio/solidus/issues/1218/comments","statuses_url":"https://api.github.com/repos/solidusio/solidus/statuses/7d6b63b26e4269ca178a934a6f5573f0ccd9bfca","head":{"label":"jrochkind:notification_dispatcher","ref":"notification_dispatcher","sha":"7d6b63b26e4269ca178a934a6f5573f0ccd9bfca","user":{"login":"jrochkind","id":149304,"avatar_url":"https://avatars.githubusercontent.com/u/149304?v=3","gravatar_id":"","url":"https://api.github.com/users/jrochkind","html_url":"https://github.com/jrochkind","followers_url":"https://api.github.com/users/jrochkind/followers","following_url":"https://api.github.com/users/jrochkind/following{/other_user}","gists_url":"https://api.github.com/users/jrochkind/gists{/gist_id}","starred_url":"https://api.github.com/users/jrochkind/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jrochkind/subscriptions","organizations_url":"https://api.github.com/users/jrochkind/orgs","repos_url":"https://api.github.com/users/jrochkind/repos","events_url":"https://api.github.com/users/jrochkind/events{/privacy}","received_events_url":"https://api.github.com/users/jrochkind/received_events","type":"User","site_admin":false},"repo":{"id":49728060,"name":"solidus","full_name":"jrochkind/solidus","owner":{"login":"jrochkind","id":149304,"avatar_url":"https://avatars.githubusercontent.com/u/149304?v=3","gravatar_id":"","url":"https://api.github.com/users/jrochkind","html_url":"https://github.com/jrochkind","followers_url":"https://api.github.com/users/jrochkind/followers","following_url":"https://api.github.com/users/jrochkind/following{/other_user}","gists_url":"https://api.github.com/users/jrochkind/gists{/gist_id}","starred_url":"https://api.github.com/users/jrochkind/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jrochkind/subscriptions","organizations_url":"https://api.github.com/users/jrochkind/orgs","repos_url":"https://api.github.com/users/jrochkind/repos","events_url":"https://api.github.com/users/jrochkind/events{/privacy}","received_events_url":"https://api.github.com/users/jrochkind/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/jrochkind/solidus","description":"Solidus, Rails eCommerce System","fork":true,"url":"https://api.github.com/repos/jrochkind/solidus","forks_url":"https://api.github.com/repos/jrochkind/solidus/forks","keys_url":"https://api.github.com/repos/jrochkind/solidus/keys{/key_id}","collaborators_url":"https://api.github.com/repos/jrochkind/solidus/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/jrochkind/solidus/teams","hooks_url":"https://api.github.com/repos/jrochkind/solidus/hooks","issue_events_url":"https://api.github.com/repos/jrochkind/solidus/issues/events{/number}","events_url":"https://api.github.com/repos/jrochkind/solidus/events","assignees_url":"https://api.github.com/repos/jrochkind/solidus/assignees{/user}","branches_url":"https://api.github.com/repos/jrochkind/solidus/branches{/branch}","tags_url":"https://api.github.com/repos/jrochkind/solidus/tags","blobs_url":"https://api.github.com/repos/jrochkind/solidus/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/jrochkind/solidus/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/jrochkind/solidus/git/refs{/sha}","trees_url":"https://api.github.com/repos/jrochkind/solidus/git/trees{/sha}","statuses_url":"https://api.github.com/repos/jrochkind/solidus/statuses/{sha}","languages_url":"https://api.github.com/repos/jrochkind/solidus/languages","stargazers_url":"https://api.github.com/repos/jrochkind/solidus/stargazers","contributors_url":"https://api.github.com/repos/jrochkind/solidus/contributors","subscribers_url":"https://api.github.com/repos/jrochkind/solidus/subscribers","subscription_url":"https://api.github.com/repos/jrochkind/solidus/subscription","commits_url":"https://api.github.com/repos/jrochkind/solidus/commits{/sha}","git_commits_url":"https://api.github.com/repos/jrochkind/solidus/git/commits{/sha}","comments_url":"https://api.github.com/repos/jrochkind/solidus/comments{/number}","issue_comment_url":"https://api.github.com/repos/jrochkind/solidus/issues/comments{/number}","contents_url":"https://api.github.com/repos/jrochkind/solidus/contents/{+path}","compare_url":"https://api.github.com/repos/jrochkind/solidus/compare/{base}...{head}","merges_url":"https://api.github.com/repos/jrochkind/solidus/merges","archive_url":"https://api.github.com/repos/jrochkind/solidus/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/jrochkind/solidus/downloads","issues_url":"https://api.github.com/repos/jrochkind/solidus/issues{/number}","pulls_url":"https://api.github.com/repos/jrochkind/solidus/pulls{/number}","milestones_url":"https://api.github.com/repos/jrochkind/solidus/milestones{/number}","notifications_url":"https://api.github.com/repos/jrochkind/solidus/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/jrochkind/solidus/labels{/name}","releases_url":"https://api.github.com/repos/jrochkind/solidus/releases{/id}","deployments_url":"https://api.github.com/repos/jrochkind/solidus/deployments","created_at":"2016-01-15T15:42:18Z","updated_at":"2016-01-15T15:42:21Z","pushed_at":"2016-06-24T15:20:45Z","git_url":"git://github.com/jrochkind/solidus.git","ssh_url":"git@github.com:jrochkind/solidus.git","clone_url":"https://github.com/jrochkind/solidus.git","svn_url":"https://github.com/jrochkind/solidus","homepage":"http://solidus.io/","size":63671,"stargazers_count":0,"watchers_count":0,"language":"Ruby","has_issues":false,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":0,"mirror_url":null,"open_issues_count":0,"forks":0,"open_issues":0,"watchers":0,"default_branch":"master"}},"base":{"label":"solidusio:master","ref":"master","sha":"542c9f6c7b8e41de64aed392181bd5a15dd79a36","user":{"login":"solidusio","id":11045486,"avatar_url":"https://avatars.githubusercontent.com/u/11045486?v=3","gravatar_id":"","url":"https://api.github.com/users/solidusio","html_url":"https://github.com/solidusio","followers_url":"https://api.github.com/users/solidusio/followers","following_url":"https://api.github.com/users/solidusio/following{/other_user}","gists_url":"https://api.github.com/users/solidusio/gists{/gist_id}","starred_url":"https://api.github.com/users/solidusio/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/solidusio/subscriptions","organizations_url":"https://api.github.com/users/solidusio/orgs","repos_url":"https://api.github.com/users/solidusio/repos","events_url":"https://api.github.com/users/solidusio/events{/privacy}","received_events_url":"https://api.github.com/users/solidusio/received_events","type":"Organization","site_admin":false},"repo":{"id":30985840,"name":"solidus","full_name":"solidusio/solidus","owner":{"login":"solidusio","id":11045486,"avatar_url":"https://avatars.githubusercontent.com/u/11045486?v=3","gravatar_id":"","url":"https://api.github.com/users/solidusio","html_url":"https://github.com/solidusio","followers_url":"https://api.github.com/users/solidusio/followers","following_url":"https://api.github.com/users/solidusio/following{/other_user}","gists_url":"https://api.github.com/users/solidusio/gists{/gist_id}","starred_url":"https://api.github.com/users/solidusio/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/solidusio/subscriptions","organizations_url":"https://api.github.com/users/solidusio/orgs","repos_url":"https://api.github.com/users/solidusio/repos","events_url":"https://api.github.com/users/solidusio/events{/privacy}","received_events_url":"https://api.github.com/users/solidusio/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/solidusio/solidus","description":"Solidus, Rails eCommerce System","fork":false,"url":"https://api.github.com/repos/solidusio/solidus","forks_url":"https://api.github.com/repos/solidusio/solidus/forks","keys_url":"https://api.github.com/repos/solidusio/solidus/keys{/key_id}","collaborators_url":"https://api.github.com/repos/solidusio/solidus/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/solidusio/solidus/teams","hooks_url":"https://api.github.com/repos/solidusio/solidus/hooks","issue_events_url":"https://api.github.com/repos/solidusio/solidus/issues/events{/number}","events_url":"https://api.github.com/repos/solidusio/solidus/events","assignees_url":"https://api.github.com/repos/solidusio/solidus/assignees{/user}","branches_url":"https://api.github.com/repos/solidusio/solidus/branches{/branch}","tags_url":"https://api.github.com/repos/solidusio/solidus/tags","blobs_url":"https://api.github.com/repos/solidusio/solidus/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/solidusio/solidus/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/solidusio/solidus/git/refs{/sha}","trees_url":"https://api.github.com/repos/solidusio/solidus/git/trees{/sha}","statuses_url":"https://api.github.com/repos/solidusio/solidus/statuses/{sha}","languages_url":"https://api.github.com/repos/solidusio/solidus/languages","stargazers_url":"https://api.github.com/repos/solidusio/solidus/stargazers","contributors_url":"https://api.github.com/repos/solidusio/solidus/contributors","subscribers_url":"https://api.github.com/repos/solidusio/solidus/subscribers","subscription_url":"https://api.github.com/repos/solidusio/solidus/subscription","commits_url":"https://api.github.com/repos/solidusio/solidus/commits{/sha}","git_commits_url":"https://api.github.com/repos/solidusio/solidus/git/commits{/sha}","comments_url":"https://api.github.com/repos/solidusio/solidus/comments{/number}","issue_comment_url":"https://api.github.com/repos/solidusio/solidus/issues/comments{/number}","contents_url":"https://api.github.com/repos/solidusio/solidus/contents/{+path}","compare_url":"https://api.github.com/repos/solidusio/solidus/compare/{base}...{head}","merges_url":"https://api.github.com/repos/solidusio/solidus/merges","archive_url":"https://api.github.com/repos/solidusio/solidus/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/solidusio/solidus/downloads","issues_url":"https://api.github.com/repos/solidusio/solidus/issues{/number}","pulls_url":"https://api.github.com/repos/solidusio/solidus/pulls{/number}","milestones_url":"https://api.github.com/repos/solidusio/solidus/milestones{/number}","notifications_url":"https://api.github.com/repos/solidusio/solidus/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/solidusio/solidus/labels{/name}","releases_url":"https://api.github.com/repos/solidusio/solidus/releases{/id}","deployments_url":"https://api.github.com/repos/solidusio/solidus/deployments","created_at":"2015-02-18T20:41:58Z","updated_at":"2016-07-20T16:01:43Z","pushed_at":"2016-07-20T16:55:06Z","git_url":"git://github.com/solidusio/solidus.git","ssh_url":"git@github.com:solidusio/solidus.git","clone_url":"https://github.com/solidusio/solidus.git","svn_url":"https://github.com/solidusio/solidus","homepage":"https://solidus.io/","size":70258,"stargazers_count":1013,"watchers_count":1013,"language":"Ruby","has_issues":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":304,"mirror_url":null,"open_issues_count":150,"forks":304,"open_issues":150,"watchers":1013,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/solidusio/solidus/pulls/1218"},"html":{"href":"https://github.com/solidusio/solidus/pull/1218"},"issue":{"href":"https://api.github.com/repos/solidusio/solidus/issues/1218"},"comments":{"href":"https://api.github.com/repos/solidusio/solidus/issues/1218/comments"},"review_comments":{"href":"https://api.github.com/repos/solidusio/solidus/pulls/1218/comments"},"review_comment":{"href":"https://api.github.com/repos/solidusio/solidus/pulls/comments{/number}"},"commits":{"href":"https://api.github.com/repos/solidusio/solidus/pulls/1218/commits"},"statuses":{"href":"https://api.github.com/repos/solidusio/solidus/statuses/7d6b63b26e4269ca178a934a6f5573f0ccd9bfca"}}}}
{"actor":{"display_login":"jrochkind"}}