[WIP] Fix #447 -- Sendmail plugin: Create new mail based on an old one

[asv-hungvt]

Hi @raphaelm ,

I am sticked at how to add the mail content to the new form. I used the initial but it said there was an error at the init() of the mail form. It supposed to accept the message argument and add the content when init right?

Another question, is thispull request created correctly? I used git push origin feature/new-send-mail but it said there was no branch as provided.

Thanks in advance :)

[raphaelm]

Hi! This looks like a good start. I left a few comments in there that should also help with your current problem.

Another question, is thispull request created correctly? I used git push origin feature/new-send-mail but it said there was no branch as provided.

Well, this one (#476) seems to work, while #455 did not. I'm not sure why, I'd probably need to see the exact error message of "git push" as well as a "git status" output to help. But we can just continue here.

[raphaelm]

This will cause an error whenever you don't have the from_log parameter. I would enclose all of this in a if 'from_log' in self.request.GET block.

[raphaelm]

Also, we need to

• Check that the user can actually use this LogEntry, e.g. LogEntry.objects.get(id=from_log_id, event=self.request.event, action_type='pretix.plugins.sendmail.sent').display() to narrow the choice down so people cannot access texts from other events
• Check for errors, i.e.

  try:
      message = …
  except LogEntry.DoesNotExist:
      raise Http404(_('You suppliad an invalid log entry ID'))
  

  using a from django.http import Http404 at the top of the file.

[asv-hungvt]

Everything works perfectly except the query it return a invalid id
These are the values:
from_log_id = 1
self.request.event = 2018
action_type = 'pretix.plugins.sendmail.sent'

the from_log_id is got from the history template as the URL query param. I can see the sql from the DoDT tab but couldn't look for the query for the from_log_id. Can you show me how can I check if the record is in the DB?

[raphaelm]

Hi @asv-hungvt, I'm unsure if I really understand your question correctly. Something like the following should work:

if 'from_log' in self.request.GET:
    try:
        message = LogEntry.objects.get(
            id=from_log_id, 
            event=self.request.event,
            action_type='pretix.plugins.sendmail.sent'
        ).display()
    except LogEntry.DoesNotExist:
        raise Http404(_('You supplied an invalid log entry ID'))

[raphaelm]

Drop this line, it doesn't make a lot of sense. What you want to do instead is kwargs['initial'] = {'message': message}. Django will later pass the kwargs to form_class in a different place.

[raphaelm]

This should be translatable, e.g. {% trans "Send a new email based on this" %}

[raphaelm]

Fine for debugging now, be sure to remove them later 😉

[raphaelm]

You can just access log.object_id in the template, so there's not really a need for this.

[asv-hungvt]

Hi @raphaelm ,

Can you check this #476 (comment)

[asv-hungvt]

hi @raphaelm ,

I think it is more reasonable when we use from_log={{ log.id }} instead of from_log={{ log.object_id }}

After using id, the message can be copied now. Please review the code :)

[raphaelm]

I think it is more reasonable when we use from_log={{ log.id }} instead of from_log={{ log.object_id }}

Sure, my bad 🙈

Please review the code :)

I'll do as soon as possible!

[raphaelm]

Hey! In general, the code is okay, but still doesn't work. 😉

First of all, when the from_log GET parameter is not set, you still execute the line kwargs['initial'] = {'message': message} which obviously fails because message has not been defined. You need to put that at the end of the try: block.

Secondly, if the query parameter is set, you don't insert the proper message into the form. You could do something like the following (with the appropriate imports):

                logentry = LogEntry.objects.get(
                    id=from_log_id,
                    event=self.request.event,
                    action_type='pretix.plugins.sendmail.sent'
                )
                message = LazyI18nString(logentry.parsed_data['message'])

[raphaelm]

You need to but this into the try part, otherwise this fails if you try to send an email without using an old email as the base.

[asv-hungvt]

The code is ok now, please review it. Thanks for pointing out the mistake ;)

[raphaelm]

Hi,

looks great! Travis complains about wrongly ordered imports, you can fix this with a simple isort -rc pretix automatically.

Cheers
Raphael

[codecov]

Codecov Report

Merging #476 into master will increase coverage by 1.32%.
The diff coverage is 30%.

@@            Coverage Diff             @@
##           master     #476      +/-   ##
==========================================
+ Coverage   80.57%   81.89%   +1.32%     
==========================================
  Files         166      165       -1     
  Lines       11429    11639     +210     
==========================================
+ Hits         9209     9532     +323     
+ Misses       2220     2107     -113

Impacted Files	Coverage Δ
src/pretix/plugins/sendmail/views.py	88.29% <30%> (-7%)	⬇️
src/pretix/plugins/pretixdroid/signals.py	37.5% <0%> (-9.87%)	⬇️
src/pretix/control/forms/organizer.py	68.33% <0%> (-6.67%)	⬇️
src/pretix/base/models/event.py	59.21% <0%> (-4.52%)	⬇️
src/pretix/base/signals.py	93.87% <0%> (-3.8%)	⬇️
src/pretix/base/ticketoutput.py	71.42% <0%> (-3.58%)	⬇️
src/pretix/plugins/ticketoutputpdf/ticketoutput.py	85.71% <0%> (-1.39%)	⬇️
src/pretix/control/context.py	92.15% <0%> (-1.33%)	⬇️
src/pretix/presale/views/checkout.py	85.71% <0%> (-0.78%)	⬇️
src/pretix/base/exporters/orderlist.py	36.66% <0%> (-0.63%)	⬇️
... and 30 more

[asv-hungvt]

Hi,

Yay it's done!

Cheers,
Hung

[raphaelm]

Yay! 🎈
Thank you very much for your contribution!

[asv-hungvt]

Thank you for your kind instruction ;)