-
Notifications
You must be signed in to change notification settings - Fork 908
add data-attributes and print form data in order for email #105
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
Conversation
I'm liking the look of this. I'll pull it down and give it some test runs shortly.
One quick thing that sprung to mind was although I really like the idea of not having touch the Google script at all, I was wondering if a data attribute for the to address is a potential issue? It feels like it might be a bit exposed to me, and some folks might want the option to keep it out of the public view.
I was thinking maybe keep the TO_ADDRESS var in the script, but have it as a ternary value, so if the data attribute is set, use it, but if not, use whatever is defined in the script as a fallback email address?
It's certainly very enticing to not have to republish the script each time I want to change the email address during testing though! 👍
That is a fair point @martynhoyer !
I think we can easily swing that, basically have two areas to set emails. We will want to clean up the tutorial so that people know about the choice.
Did you have any other suggestions / ideas? I am hoping to re-review this soon and get it pulled in so I can work on #94
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.
the only "danger" of allowing people to specify an email address to send email to in the form is that it can become a vehicle for spam/phishing.
(I agree with it as a feature in principal, just in practice I've seen it abused)
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.
Yes, you started a discussion of this over in #5 but then the concern of how to prevent spam is mentioned there and in #79
Is it worth the convenience? Not sure.
We could provide a flag instead, so that a data-attribute determines if an email is sent, but the email must be provided server-side. I could make this change easily. What should the default behavior be, without the data-attribute? To not send an email? Or to send one?
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.
@mckennapsean we just need to warn people that if they put a form on a website that allows people to send email to "arbitrary" email address it will eventually be abused.
The work-around I was considering was always "CC-ing" the owner of the script so they can keep an eye on 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.
Hmmm, fair point! I was thinking that the default support wouldn't be arbitrary / customizable by the end user directly, but anyone can technically alter the HTML and JS to do what they want! So I was thinking from an attacker perspective. Granted, attackers could spam email to you in many other ways without this script, so perhaps that is moot to avoid feature support for fear alone!
index.html
Outdated
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.
we should probably change this email address ... 😉
test.html
Outdated
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.
same here. this should be example@email.net or something generic. 👍
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 do, good suggestion! :)
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.
@mckennapsean feel free to assign the PR to me when you feel it's ready to ship. 👍
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 do! I have a few more things to work on, and I will assign you once I have cleaned up and tested the code.
Is anyone opposed to me adding more data-attributes (#94) to this PR? It'll make it bigger, but it will be along the similar mindset.
Made the necessary changes, will assign Nelson to take a look!
Just a reminder @nelsonic - to take a look, should be ready to merge, then I can update the online example too.
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.
this can be used to send spam email from a legitimate GMail account. it's not as unlikely as we would hope ... 🙄
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.
@mckennapsean looks good. 👀 👍
merging. 🚀
Fixes #76 and pulled in some ideas from #90 to store data in the HTML form element.
For example, the HTML element can contain a
data-sheet="responses"to send the data into responses sheet, or whichever sheet you want. By default, no data element uses the same default before (no breaking functionality). Similarly, you have to usedata-emailif you want to send an email in the Google Script (or set the variable in the Javascript).The motivation is that I am trying to make it easier for people to just use the Javascript and Google Script without changing them. They just change things in the HTML, which makes it easier to deploy changes, like changing emails or which spreadsheet to store data in.
I think there are more things we can move here, over time, but I wanted to see what people think of this idea.
Lastly, I have the form fields get stored as a stringified array and use that array later when printing out the email. This makes sure that the email gets printed out in order of the form data! :)