Skip to content
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 ttl_sec argument to post_email_verification request. #145

Merged

Conversation

paul-millar
Copy link
Contributor

Changes

Add ttl_sec argument to post_email_verification request.

References

The post_email_verification endpoint supports a ttl_sec parameter which this gem does not currently support. See: https://auth0.com/docs/api/management/v2#!/Tickets/post_email_verification

Testing

The tickets spec was adjusted to include the relevant change here. Admittedly this is blind as I couldn't for the life of me figure our how to run the specs. If anyone can point me to some documentation or guide me through this then that would be great - otherwise I assume this is a relatively non-impactful change and shouldn't break anything.

Checklist

@joshcanhelp
Copy link
Contributor

Thank you @digitaldawn, I'll take a look as soon as I can!

Copy link
Contributor

@joshcanhelp joshcanhelp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@digitaldawn - Alright, I read through this and everything looks fine. Tests can be run with this command:

$ bundle exec rake test

But the CI handled it here so you're good to go.

Added a few pieces of feedback here. Otherwise, looks great!

lib/auth0/api/v2/tickets.rb Outdated Show resolved Hide resolved
spec/lib/auth0/api/v2/tickets_spec.rb Show resolved Hide resolved
@paul-millar
Copy link
Contributor Author

Hi @joshcanhelp, apologies I should have been clearer with the test scenario. I was able to run the tests but they were all failing as it was looking for credentials (and I wasn't sure to get these).
I managed to pull them out of the VCR cassettes so can now run the tests. Will get this touched up.

…nteger.

Added specs to ensure this is the case.
@paul-millar
Copy link
Contributor Author

I think I've now added the changes you requested :)

@joshcanhelp
Copy link
Contributor

Thanks @digitaldawn! Looks good at first glance here, I'll review as soon as I can and get this merged.

Copy link
Contributor

@joshcanhelp joshcanhelp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you @digitaldawn

@joshcanhelp joshcanhelp merged commit ee56670 into auth0:master Dec 5, 2018
@joshcanhelp joshcanhelp added this to the v4.6.0 milestone Dec 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants