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

Adding choice to exclude form submission data from email notifications #55

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

NathanQ
Copy link

@NathanQ NathanQ commented Dec 15, 2017

Hi @AccentDesign team,

We had a requirement to exclude the form data from the email notifications so I added a field exclude_form_data from form submission notifications. Setting default to true (because our needs, ha!) and, when true, the submission notification includes the form submission list url for the notified to go check it out.

For the link to the list to work, the settings require the 'django.contrib.sites' to be included in the installed apps and the SITE_ID to be set, i.e., SITE_ID = 1

I hope this doesn't add too much complexity and adds value.

Thanks for the great app,

Nathan

@codecov-io
Copy link

codecov-io commented Dec 15, 2017

Codecov Report

Merging #55 into master will decrease coverage by 1.14%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #55      +/-   ##
==========================================
- Coverage   99.83%   98.69%   -1.15%     
==========================================
  Files          14       14              
  Lines         610      613       +3     
  Branches       58       59       +1     
==========================================
- Hits          609      605       -4     
- Misses          0        6       +6     
- Partials        1        2       +1
Impacted Files Coverage Δ
wagtailstreamforms/models/form.py 94.69% <50%> (-5.31%) ⬇️
wagtailstreamforms/models/validators.py 100% <0%> (ø) ⬆️
wagtailstreamforms/models/submission.py 100% <0%> (ø) ⬆️
wagtailstreamforms/models/form_field.py 100% <0%> (ø) ⬆️
wagtailstreamforms/forms.py 100% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ce164f0...148557e. Read the comment docs.

@stuartaccent
Copy link
Contributor

Hi @NathanQ,

Sorry for the lack of response. Thanks for the above. There might be some stuff in this area we also may want to do. Will try to gather our requirements this week and report back.

Thanks
Stu

@stuartaccent
Copy link
Contributor

Hi @NathanQ

Hope you had a good xmas. We have finally been able to give this a bit more thought and have the following ideas.

  • We feel it may be better to set the default of the field to False. This will then ensure existing projects continue to run as expected. Created forms wont suddenly stop sending emails if people don't notice the additional option then we are in the realms of providing a resend function (thougth this may be a good addition later).
  • In terms of the url. It would be nice to be able to get it from wagtails site model. This will avoid the additional requirement of 'django.contrib.sites' being forced on everyone.
  • We are also thinking maybe provide an email template that can be changed in the settings rather than fixing the text in the send_email method.

What do you think for starters? We are more than happy to pick this up in the next few days and can use your pr for ref.

Hope this doesn't trample on anything, this is not our intention.

Cheers, Stu

@NathanQ
Copy link
Author

NathanQ commented Jan 10, 2018

Hi @stuartaccent,

Thank you for considering my request. I'd be happy to take on the first two bullets. The email template gets a little hairy with adding submitted context to a saved template. May consider taking a stab at it if it were something like what Stripe does where one can only can change a header and, maybe, the footer for a bit of personalization.

Thanks again!

@stuartaccent
Copy link
Contributor

Hi @NathanQ

Forget the template maybe we will pick this up later, was just an idea.

With regards to the other two, if you want to pick these up awesome, if not I will have a look when i get a chance. prob in the next few days.

Cheers, Stu

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants