-
Notifications
You must be signed in to change notification settings - Fork 131
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add email-templates endpoints #117
Conversation
} | ||
|
||
/** | ||
* Create a Rule. A token with scope create:rules is needed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wha
import com.fasterxml.jackson.annotation.JsonProperty; | ||
|
||
/** | ||
* Class that represents an Auth0 Guardian Factor object. Related to the {@link com.auth0.client.mgmt.GuardianEntity} entity. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's Guardian got to do with this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more copy paste issues 🙉
@JsonProperty("subject") | ||
private String subject; | ||
@JsonProperty("syntax") | ||
private String syntax; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be an enum for additional validation? Although only supported value is liquid
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HMMM. I discussed this with the server guys. The only allowed value is liquid
but they say the option is only there for backwards compatibility, so we shouldn't be hardcoding it to liquid
and lowering the param count for example. I don't think offering an enum/constant helps but I don't mind adding one. Would you prefer me to add it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you set the param default to liquid
? Then they don't need to think about it but in the future if they need to, they can change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small things
f3e5fcd
to
2f3555f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Waiting to merge? |
Add the email-templates endpoints
https://auth0.com/docs/api/management/v2#!/Email_Templates/get_email_templates_by_templateName
Things open to discuss:
Method visibility (scope) of the values that can't be changed once set. The template name ("template" property in the object) can't be changed once set. Since the same
Template
class is used on the create and update methods, does it make sense to have 2 constructors, one that accepts all the values required for creation (4 or 5 values, can be seen on the corresponding test) and one without any values so people uses the first one for creation and the other for existing templates update?The methods were added following a REST pattern of parameters
doAction(id, data)
. In this particular case, the "Template Id" is the template's name, used on the path. And it must match the one defined indata.template
, or that property should be ignored at all when updating templates. WDYT?