Adds support for MergeTriggerEmail API #18
Conversation
|
Just to note. We are also working to add AuthenticateServer and LoginWithCertificate login support as Responsys is recommending to us using that API for our integration. |
|
Hey ! Nice feature and very clean code! Thanks for respecting the code style of the project, that's 👍 About the authentication by certificate, this is awesome you're working on that. We would be very excited getting your pull request for it ! Also we're trying to get tests for our new features. Could you write some tests for this new feature ? We know everything's not tested in the gem, we're working on that. Basically what we would do is if the feature is from the api, we would try to make tests that are close to how the gem would be used by our users. To set up your test env, you would have to copy/paste the If any questions, somebody will help you here or in an issue you would open. Also if you have any feedback about the gem: integration time you dedicated for your app, how you're using it and for what features of the api, what infrastructure or what major improvements you would see in the future... This would help a lot and be very appreciated ! |
| @@ -21,6 +21,18 @@ def check_failures(outcome, recipients) | |||
| puts "failed:\n" + recipients[:recipient].to_s unless outcome[:success] | |||
| end | |||
| end | |||
|
|
|||
| def merge_trigger_email(campaign, record_data, trigger_data, merge_rule) | |||
There was a problem hiding this comment.
I would add ListMergeRule.new as the default value for merge_rule
|
You guys are doing an amazing job, kudos! And thank you! I had one somewhat related question - do you know of a dev sandbox instance of Responsys that we could build against, as a third-party software developer that doesn't have access to their own instance? Does such a thing even exist in the Responsys world? |
|
We're trying, thanks ! I don't think such thing exists as a public instance for any dev. A few months before our actual integration, the Responsys Team gave us credentials to a sandbox that helped a lot to start construct this gem. Not sure you can get one if you don't have a contract with them or at least in the process to. I guess you can let the pull request open til you get an access in the future. That would be necessary to test that code with a real access to the API. Even if you used the official documentation, it may be small differences. It happened to us! I think someone said one day "In theory, theory and practice are the same. In practice, they are not." :) |
|
Got it, thanks. Am trying to get access to a sandbox as well, will keep you updated. Appreciate the prompt response! |
|
Thanks for writeup. I will add some tests that would adhere to your style and adjust code as per your comments. Regarding certificate login you can already look at: https://github.com/granify/responsys-api/commit/1473dfacba5cdd4ae5ed0d79e5568442eed8eb16 (support-login-with-certificate branch based on this pull request branch). Apart from missing tests, it is already tested using our Responsys account and functional so you can expect pull request soon. |
|
Oh okay apparently you guys are not working together. Misunderstood. Glad I helped @arvindkrishnan. @moloh No problem. Hard to say something negative, it looks really good. Excited about your next PR(S?) ! |
|
Have any news about this ? We're wrapping things for version 1 and would love to see your work in there, especially the "AuthenticateServer and LoginWithCertificate login support" you talked about ! |
|
Thanks for remainder. Unfortunately our responsys integration project was progressing slower, now we have final integration done so i am sure that stuff actually works. I am not sure what is your timeline, but i will go over our changes, add tests and prepare working PR in few coming days. If that works for you then awesome. |
|
No problem! We'll release as soon as we have enough features to ship. Yours appear as essential for our v1 so we'll wait. No rush for the next version. Take your time to make it all beautiful and tested. 👍 |
|
Any updates on this? |
Adds MergeTriggerAPI call and necessary TriggerData object as per Responsys 6.21 API.