-
-
Notifications
You must be signed in to change notification settings - Fork 835
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
Mail Extender #2012
Mail Extender #2012
Conversation
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.
Good suggestion!
Tests I'd like to see:
- Calling
add
with a new driver makes the new driver appear in the/api/mail-settings
endpoint - Calling
add
allows replacing existing drivers (e.g. a different set of fields appearing in/api/mail-settings
)
Thank you! ❤️
fa15390
to
d87cd40
Compare
Added! The tests are currently failing, but that's due to an underlying issue that I've fixed in #2036. |
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.
Good stuff, thank you!
Done, this now depends on #2052 to be merged first. We're going to need this authenticated request functionality anyway, so might as well get it done now! |
a06a6c1
to
24c76d7
Compare
24c76d7
to
16669ff
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.
Looks good, thanks!
(I'll make some minor tweaks directly in master after merging.)
- Use private over protected - Use "public" API for building requests in tests - Add more assertions - Formatting - Use correct parameter order for assertions Refs #2012.
Improves result of #1986
Fixes part of #1891
Changes proposed in this pull request:
Reviewers should focus on:
Confirmed
composer test
).