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

added helper for new recipient form #502

Merged
merged 6 commits into from
Mar 28, 2018

Conversation

kraigh
Copy link
Contributor

@kraigh kraigh commented Oct 20, 2017

Solves #454

Added a new "Recipients" helper that can do two things:

  1. Generate an HTML form with a customizable URL to allow users to submit their name and email.
  2. Create a new recipient object when the form is submitted.

I included code in the examples directory, as well as unit tests.

A helper for "lists" should also probably be created, showing how to add a created recipient to a list.

First time contributor- so please let me know if I named anything inappropriately, or if there is a way to improve this implementation! I tried to copy patterns from the "Mail" helper as much as possible.

@thinkingserious thinkingserious added the status: code review request requesting a community code review or review from Twilio label Oct 20, 2017
@SendGridDX
Copy link

SendGridDX commented Oct 20, 2017

CLA assistant check
All committers have signed the CLA.

$this->setHtml($html);
}

public function setHtml($html)

Choose a reason for hiding this comment

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

What's the point of having a setter if we're using the constructor already?.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great point- I was trying to follow the pattern in lib/helpers/mail/Mail.php but you're right, it's not necessary. Will remove.

{
public $html;

public function __construct($url)

Choose a reason for hiding this comment

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

can you add a doc block at least to know what is the type of this parameter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea- will do.

$this->setEmail($email);
}

public function setFirstName($firstName)

Choose a reason for hiding this comment

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

Same question as above, if we're using the constructor to set the properties, what's the point of having setters?.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above.


class RecipientForm
{
public $html;

Choose a reason for hiding this comment

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

Why are the properties public?, we're breaking OOP encapsulation doing this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will change.

{
public function testRecipientsForm()
{
$form = new RecipientForm('http://www.examle.com/recipientFormSubmit');
Copy link

@albert-agelviz albert-agelviz Oct 20, 2017

Choose a reason for hiding this comment

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

Maybe it would make much more sense if we could cast this object to string to have a representation of RecipientForm in string, e.g:

(string) new RecipientForm('http://www.examle.com/recipientFormSubmit');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense- will do.

*/
class Recipient implements \JsonSerializable
{
public $firstName;

Choose a reason for hiding this comment

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

Same question as above for the public properties, doing this breaks OOP ecapsulation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will fix.

@kraigh
Copy link
Contributor Author

kraigh commented Oct 21, 2017

Thanks for the feedback @shonjord - I have addressed them in dfce02a. Let me know if the fixes are adequate.

@kraigh
Copy link
Contributor Author

kraigh commented Oct 24, 2017

@thinkingserious could I get some feedback on this PR? Is this what you were looking for in #454 ?

@thinkingserious
Copy link
Contributor

@kraigh

We have not been able to merge your Pull Request, but because you are awesome - we wanted to make sure you could still get a SendGrid Hacktoberfest shirt.

Please go fill out our swag form before Nov 5th and we will send the shirt! (We know that you might have tried this before and it didn’t work, sorry about that!)

You have till Nov 5th to fill out this form in order to get the Hacktoberfest shirt!

Thank you for contributing during Hacktoberfest! We hope to see you in the repos soon! Just so you know, we always give away a SendGrid shirt for your first ever non-Hacktoberfest PR that gets merged.

@thinkingserious thinkingserious added the type: twilio enhancement feature request on Twilio's roadmap label Feb 28, 2018
@thinkingserious thinkingserious merged commit 3161223 into sendgrid:master Mar 28, 2018
@thinkingserious
Copy link
Contributor

Hello @kraigh,

Thanks again for the PR!

We appreciate your contribution and look forward to continued collaboration. Thanks!

Team SendGrid DX

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty: hard fix is hard in difficulty status: code review request requesting a community code review or review from Twilio type: twilio enhancement feature request on Twilio's roadmap
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants