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

Extract email templates into separate files #335

Merged
merged 9 commits into from
Jul 13, 2023

Conversation

krestenlaust
Copy link
Member

@krestenlaust krestenlaust commented Jun 20, 2023

Resolves #331

  • Welcome mail (mail.py)
  • Deposited stregdollars (utils.py)
  • Deposited stregdollars (shame) (utils.py)

I'm using djangos templating engine.

Not sure about how to organize the new files, I've just named the folder message_templates (in case we want to implement something like push notifications in the future😉)

@jonasKjellerup
Copy link
Contributor

Consider putting the templates in stregsystem/templates/mail instead.

@krestenlaust
Copy link
Member Author

Consider putting the templates in stregsystem/templates/mail instead.

My initial thought was that templates was an www-folder and the email templates would get served as well then. I'll go ahead and move them :)

@krestenlaust
Copy link
Member Author

The goal of the PR has been reached, but I haven't tested the code yet, as I'm using the web VS Codes in Github. I'll test it when I get home.

We should consider moving the send_payment_mail function into mail.py. I believe the reason it's in utils.py, is to avoid a circular dependency, but I'm not entirely sure. There's some code repetition in sending the emails — which could more easily be resolved by having both send_payment_mail and send_welcome_mail in the same file.

@krestenlaust
Copy link
Member Author

Moving the email templates into templates results in the following error, when applying the data fixture,

python.exe manage.py testserver stregsystem/fixtures/testdata.json:
django.template.exceptions.TemplateDoesNotExist: Problem installing fixture 'C:\Users\kress\source\repos\stregsystemet\stregsystem/fixtures\testdata.json': templates/mail/welcome.html

Stacktrace:

WARNING: Not in production mode, If you are running on the server, stop right now
Creating test database for alias 'default'...
Traceback (most recent call last):
  File "manage.py", line 22, in <module>
    execute_from_command_line(sys.argv)
  File "C:\Users\kress\source\repos\stregsystemet\venv\lib\site-packages\django\core\management\__init__.py", line 381, in execute_from_command_line
    utility.execute()
  File "C:\Users\kress\source\repos\stregsystemet\venv\lib\site-packages\django\core\management\__init__.py", line 375, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "C:\Users\kress\source\repos\stregsystemet\venv\lib\site-packages\django\core\management\base.py", line 323, in run_from_argv
    self.execute(*args, **cmd_options)
  File "C:\Users\kress\source\repos\stregsystemet\venv\lib\site-packages\django\core\management\base.py", line 364, in execute
    output = self.handle(*args, **options)
  File "C:\Users\kress\source\repos\stregsystemet\venv\lib\site-packages\django\core\management\commands\testserver.py", line 37, in handle
    call_command('loaddata', *fixture_labels, **{'verbosity': verbosity})
  File "C:\Users\kress\source\repos\stregsystemet\venv\lib\site-packages\django\core\management\__init__.py", line 148, in call_command
    return command.execute(*args, **defaults)
  File "C:\Users\kress\source\repos\stregsystemet\venv\lib\site-packages\django\core\management\base.py", line 364, in execute
    output = self.handle(*args, **options)
  File "C:\Users\kress\source\repos\stregsystemet\venv\lib\site-packages\django\core\management\commands\loaddata.py", line 72, in handle
    self.loaddata(fixture_labels)
  File "C:\Users\kress\source\repos\stregsystemet\venv\lib\site-packages\django\core\management\commands\loaddata.py", line 114, in loaddata
    self.load_label(fixture_label)
  File "C:\Users\kress\source\repos\stregsystemet\venv\lib\site-packages\django\core\management\commands\loaddata.py", line 181, in load_label
    obj.save(using=self.using)
  File "C:\Users\kress\source\repos\stregsystemet\venv\lib\site-packages\django\core\serializers\base.py", line 223, in save
    models.Model.save_base(self.object, using=using, raw=True, **kwargs)
  File "C:\Users\kress\source\repos\stregsystemet\venv\lib\site-packages\django\db\models\base.py", line 793, in save_base
    update_fields=update_fields, raw=raw, using=using,
  File "C:\Users\kress\source\repos\stregsystemet\venv\lib\site-packages\django\dispatch\dispatcher.py", line 175, in send
    for receiver in self._live_receivers(sender)
  File "C:\Users\kress\source\repos\stregsystemet\venv\lib\site-packages\django\dispatch\dispatcher.py", line 175, in <listcomp>
    for receiver in self._live_receivers(sender)
  File "C:\Users\kress\source\repos\stregsystemet\stregsystem\signals.py", line 14, in after_member_save
    send_welcome_mail(instance)
  File "C:\Users\kress\source\repos\stregsystemet\stregsystem\mail.py", line 32, in send_welcome_mail
    html = render_to_string("templates/mail/welcome.html", context)
  File "C:\Users\kress\source\repos\stregsystemet\venv\lib\site-packages\django\template\loader.py", line 61, in render_to_string
    template = get_template(template_name, using=using)
  File "C:\Users\kress\source\repos\stregsystemet\venv\lib\site-packages\django\template\loader.py", line 19, in get_template
    raise TemplateDoesNotExist(template_name, chain=chain)
django.template.exceptions.TemplateDoesNotExist: Problem installing fixture 'C:\Users\kress\source\repos\stregsystemet\stregsystem/fixtures\testdata.json': templates/mail/welcome.html

Process finished with exit code 1

@krestenlaust
Copy link
Member Author

I resolved the issue. I had used the wrong path for the templates.

I've now also moved the send_payment_mail into mail.py.

@krestenlaust krestenlaust marked this pull request as ready for review June 24, 2023 20:43
@codecov-commenter
Copy link

codecov-commenter commented Jun 26, 2023

Codecov Report

Merging #335 (186e8cd) into next (31d8c2a) will increase coverage by 0.51%.
The diff coverage is 100.00%.

❗ Current head 186e8cd differs from pull request most recent head 84a4072. Consider uploading reports for the commit 84a4072 to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##             next     #335      +/-   ##
==========================================
+ Coverage   80.14%   80.65%   +0.51%     
==========================================
  Files          32       32              
  Lines        2820     2817       -3     
  Branches      218      217       -1     
==========================================
+ Hits         2260     2272      +12     
+ Misses        524      510      -14     
+ Partials       36       35       -1     
Impacted Files Coverage Δ
stregsystem/utils.py 89.04% <ø> (+13.50%) ⬆️
stregsystem/mail.py 84.21% <100.00%> (+14.21%) ⬆️
stregsystem/models.py 88.15% <100.00%> (ø)
stregsystem/views.py 45.91% <100.00%> (ø)

Copy link
Contributor

@jonasKjellerup jonasKjellerup left a comment

Choose a reason for hiding this comment

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

Looks good. Just needs a run through black (make sure to run it with the newest version using a recent python version, otherwise it will continue to fail in CI).

@krestenlaust
Copy link
Member Author

I'm not sure what to do, I can't figure out where to read the output of the "Black formatter check", I have zero experience with Githubs CI/CD

@jonasKjellerup
Copy link
Contributor

You don't really need to do anything with the output from the "Black formatter check" CI. You just need to run black formatter locally (https://github.com/psf/black) and commit the changes.

@krestenlaust
Copy link
Member Author

Interesting... Are we using a different config than default for Black?

@jonasKjellerup
Copy link
Contributor

jonasKjellerup commented Jun 26, 2023

The config can be found in the CI definition:

args: --check --target-version py36 --line-length 120 --skip-string-normalization --exclude '(migrations|urls\.py)' stregsystem stregreport kiosk

@krestenlaust
Copy link
Member Author

It appears the formatting issues black had were unrelated to this PR, but actually originating from a commit back in November 0e1b65a. I included a commit to resolve the formatting issue. It should be ready now @jonasKjellerup, I'd probably squash, as 9 commits for this refactor is a little much 😅

@jonasKjellerup
Copy link
Contributor

Oh yea; forgot about that.

@falkecarlsen will you take a look at this and merge at your convenience?

Copy link
Member

@falkecarlsen falkecarlsen left a comment

Choose a reason for hiding this comment

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

Thanks, great to have this decluttered a bit :shipit:

@falkecarlsen falkecarlsen merged commit 17ef290 into f-klubben:next Jul 13, 2023
3 checks passed
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.

Extract email templates into individual template files
4 participants