Add email template endpoints #251

joshcanhelp · 2018-04-22T15:39:31Z

Adding GET, PATCH, and POST for email templates API
Adding tests for these new methods
Added a more IDE-friendly method to create an ApiClient
Added static methods to the ApiTest class to allow for fixture setup

lbalmaceda

It feels a little simple because the methods do nothing more than setting the http method, the "email-templates" path and the body (if applies). But I haven't seen the rest of the library, so can't compare. Anyway:

I'd add in the "javadocs" the required scope to call that method. i.e. create:email-templates. You can get those from the api explorer docs.
Some methods require at least some parameters to be passed. i.e. for creating a new template you need to provide a name, for sure. Maybe it's worth to validate those cases locally before wasting time (and resources) in making a request we already know is going to fail.
Template names are limited to a few options. Maybe it's worth to provide some constants so users find easier to type them or add some local name validation?

lbalmaceda · 2018-04-24T20:00:53Z

src/API/Helpers/ApiClient.php

+        $builder->withHeaders($this->headers);
+
+        if ( in_array( $method, [ 'patch', 'post', 'put' ] ) ) {
+            $builder->withHeader( new ContentType('application/json') );


I know this works for when you need to send a body in the request, but probably would be good to check if all requests send the Accept header so the server knows what you're expecting back in the response.

I don't see an Accept header used in any of the examples in the docs. I'm wary of just dropping that in if it's not documented.

lbalmaceda · 2018-04-24T20:03:40Z

src/API/Management/EmailTemplates.php

+     *
+     * @return array
+     *
+     * @throws \Exception - if a 200 response was not returned from the API.


Be carefull. Some endpoints return success codes different to 200, i.e. create connection:

While this doesn't apply to email-templates, might be worth to check the rest of the entities if this was a copy-paste of that logic/javadocs.

I checked before submitting, all are 200 but good eyes 👍

lbalmaceda · 2018-04-24T20:15:01Z

tests/API/Management/EmailTemplatesTest.php

+    const EMAIL_TEMPLATE_NAME = 'enrollment_email';
+
+    /**
+     * Management API token with scopes read:email_templates, create:email_templates, update:email_templates


while this is true for calling all endpoints, some of them can be called with only 1 of this scopes. So this is not exactly true

This is in a test and for the tests here to run, it needs all three.

joshcanhelp · 2018-04-24T21:53:07Z

Thanks @lbalmaceda ... this is definitely more documentation than the other methods have but I'm trying to use it as a good template to go back through the others and fill it all out. Your suggestions are 👍

joshcanhelp · 2018-04-24T22:26:45Z

@lbalmaceda - Responses to your comment:

"required scope" - good call, added
"Some methods require at least some parameters ... validate those cases locally" - I thought about this and was on the fence. You'll always need a template name, sure, but everything else is subject to change, new fields to be added, etc. I can see the utility in checking for required fields now but that's more to update ... and frustrate developers down the road. I added a more clear comment on this but happy to keep talking about how this should be handled.
"Template names are limited" - Same general feelings as the above. I started to go down that route and thought that new templates would be a very likely scenario and if we're only whitelisting certain ones then we'd have to stay on top of additions there and add them quickly or someone is going to get a confusing kick-back from the SDK.

lbalmaceda · 2018-04-25T14:15:26Z

About local validation:

to create a new email-template you require all this properties at least: "template", "body", "from", "subject", "syntax", "enabled", the rest are optional.
to get/patch an existing email-template you require the "templateName".

About template names, the alternative I proposed (even on my PR) is to pre-define a set of constants that users can use to easily type a template's name. i.e. const TEMPLATE_ENROLL = "enrollment_email"

joshcanhelp · 2018-04-25T14:36:43Z

to create a new email-template you require all this properties at least: "template", "body", "from", "subject", "syntax", "enabled", the rest are optional.

For now but one could be added later and now the SDK is out of date. I'm just thinking about that as a pattern for the rest of the SDK. We're duplicating the validation that's happening on the server to save a handful of API calls at the cost of a lot of up-front and maintenance work.

$0.02

to get/patch an existing email-template you require the "templateName".

Correct, those are already there. I could require a template name for create but then the rest of the fields are required as well.

lbalmaceda · 2018-04-25T21:15:54Z

but one could be added later and now the SDK is out of date

False! If the server adds a new required parameter, the validation that now fails is the remote one and not the local. Yes, SDK is out of date and will need to be updated if you want the validation to detect that locally first. But that's not stopping people from calling this method. TBH I find this really useful! Otherwise every API/Entity would rely on a single generic class that receives a) the method, b) the path and c) the data/body. What's the point on having separate classes for them then? Would like to hear from someone else too (@cocojoe?).

Correct, those are already there.

But they are not being null-checked. Would it be fine if the request fires with a null template name?

joshcanhelp · 2018-05-02T21:01:54Z

composer.json

@@ -30,7 +30,7 @@
    }
  },
  "scripts": {
-    "test": "vendor/bin/phpunit",
+    "test": "SHELL_INTERACTIVE=1 vendor/bin/phpunit  --colors=always --verbose ",


Allows for additional data after the command (like path to a specific test) and makes the output a little more useful

lbalmaceda · 2018-05-03T17:39:47Z

src/API/Management/EmailTemplates.php

-     * @param array $data
-     *      An array of data to use for the new email, including a valid `template`.
-     *      See docs link below for required fields.
+     * @param string $template - the template name to create


I'd add "You can add any of the constants defined in this file" or something like that.. suggesting the users an easy way to access those values.. BTW this applies to all methods on this entity

lbalmaceda · 2018-05-03T17:43:25Z

src/API/Management/EmailTemplates.php

+        $data = [
+            'template' => (string) $template,
+            'from' => (string) $from,
+            'body' => (string) $body,


How does validation work here? And what happens since some of the params have a default value?

Anything with a default value is not required by the API, only template and from. The object you POST needs to have those keys but they can be empty.

$template and $from are both required for the method (IOW: need to have something passed in) but nothing beyond that. Looking through the rest of the SDK, this is how validation is handled for other endpoints. I could check for empty() but it's a bit of an edge case if someone sees that the param is required but passes in an empty string or null or something.

If it was empty, I'd have to throw and exception of some kind. Since this is a new method, I could do that even though none of the other ones do and make that a pattern to follow. If that template is empty, the HTTP client would throw an exception anyways so not a complete surprise.

I'm happy with this as-is but also open to more discussion.

I'm looking at the schema definition right now.. You shouldn't be setting values in the SDK if they are missing just because "need to have something passed in". Of course you'll need to pass something, that's why it's required. But let it fail gracefully! If the parameters are required, make the user manually set them, even if they want to set an empty string. The only exception I find for this is the syntax field which is there for backwards (read) compatibility and liquid should be used always as value when creating new templates.

Think of receiving an email in your personal inbox which doesn't have a subject set or which doesn't have a body at all. Would that make sense to you as a user of that Application? Don't you want to make sure that users of this SDK use the API well?

lbalmaceda

LGTM

…iClient method for ease of use

…ate, better tests

github-actions · 2022-08-24T00:20:40Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

joshcanhelp added the CH: Added label Apr 22, 2018

joshcanhelp added this to the v5-Next milestone Apr 22, 2018

lbalmaceda suggested changes Apr 24, 2018

View reviewed changes

lbalmaceda reviewed Apr 24, 2018

View reviewed changes

joshcanhelp force-pushed the add-email-template-endpoints branch from a314710 to a0bf285 Compare April 24, 2018 22:35

joshcanhelp force-pushed the add-email-template-endpoints branch from a0bf285 to 3e612ad Compare April 25, 2018 14:38

joshcanhelp commented May 2, 2018

View reviewed changes

lbalmaceda reviewed May 3, 2018

View reviewed changes

joshcanhelp force-pushed the add-email-template-endpoints branch 2 times, most recently from 9310228 to b4d17c4 Compare May 4, 2018 21:04

lbalmaceda approved these changes May 4, 2018

View reviewed changes

joshcanhelp force-pushed the add-email-template-endpoints branch from 5749cc1 to fac955b Compare May 8, 2018 14:41

joshcanhelp added 5 commits May 8, 2018 07:55

testing out the endpoint and adding a new resource

0515cf5

Add GET, PATCH, and POST methods for email templates; adding a new Ap…

cd72d85

…iClient method for ease of use

EmailTemplate endpoint tests; phpcs PSR-2 scan whitespace corrections

7cfdce2

adding verification for \Auth0\SDK\API\Management\EmailTemplates::cre…

d29bd5d

…ate, better tests

fix tests running on CI without .env file

fefdf6e

joshcanhelp force-pushed the add-email-template-endpoints branch from 84df7f1 to fefdf6e Compare May 8, 2018 14:55

joshcanhelp merged commit a068135 into master May 8, 2018

joshcanhelp deleted the add-email-template-endpoints branch May 8, 2018 16:00

github-actions bot locked as resolved and limited conversation to collaborators Aug 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add email template endpoints #251

Add email template endpoints #251

joshcanhelp commented Apr 22, 2018

lbalmaceda left a comment

lbalmaceda Apr 24, 2018

joshcanhelp Apr 24, 2018

lbalmaceda Apr 24, 2018

joshcanhelp Apr 24, 2018

lbalmaceda Apr 24, 2018

joshcanhelp Apr 24, 2018

joshcanhelp commented Apr 24, 2018

joshcanhelp commented Apr 24, 2018

lbalmaceda commented Apr 25, 2018

joshcanhelp commented Apr 25, 2018

lbalmaceda commented Apr 25, 2018

joshcanhelp May 2, 2018

lbalmaceda May 3, 2018 •

edited

Loading

lbalmaceda May 3, 2018

joshcanhelp May 3, 2018

lbalmaceda May 3, 2018

lbalmaceda left a comment

github-actions bot commented Aug 24, 2022

Add email template endpoints #251

Add email template endpoints #251

Conversation

joshcanhelp commented Apr 22, 2018

lbalmaceda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshcanhelp commented Apr 24, 2018

joshcanhelp commented Apr 24, 2018

lbalmaceda commented Apr 25, 2018

joshcanhelp commented Apr 25, 2018

lbalmaceda commented Apr 25, 2018

Choose a reason for hiding this comment

lbalmaceda May 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbalmaceda left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 24, 2022

lbalmaceda May 3, 2018 •

edited

Loading