Skip to content
This repository has been archived by the owner on Jan 13, 2020. It is now read-only.

Add script to send out email reminder to update club pages #32

Merged
merged 23 commits into from
Jan 12, 2020
Merged

Conversation

ezwang
Copy link
Member

@ezwang ezwang commented Jan 8, 2020

  • Adds a ./manage.py remind command to send out an email to all active clubs reminding them to update their club pages.

@ezwang ezwang requested a review from cphalen January 8, 2020 23:49
@ezwang ezwang added the enhancement New feature or request label Jan 8, 2020
@coveralls
Copy link

coveralls commented Jan 9, 2020

Coverage Status

Coverage increased (+1.0%) to 81.237% when pulling e3e96d0 on remind into d24b473 on master.

if not resp.ok:
self.stdout.write(self.style.ERROR('{} has broken image {}'.format(club.id, club.image.url)))
image_ok = False
if club.image.url.startswith('http'):
Copy link

Choose a reason for hiding this comment

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

Nit: if a club has an image served over http and not https it is a security vulnerability and the browser will raise some complaints with the https cert for the website. Any way we could make them swap over to https?

Also, depending on how many images are originally defined but then down the line the URL no longer points to the asset, do we think it could be worth moving to storing images in like S3? Obv not something we'd tackle in this PR

Copy link
Member Author

Choose a reason for hiding this comment

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

We actually should be fine with the http/https thing, since we already self-host all of their images on S3 already. The only case where the image URL is not hosted/controlled by us was during the Groups Online @ Penn import script.

The check in the code is more for local development environment vs production environment, in the local environment you can't properly do a requests.head because the server isn't running in the management script.

@@ -0,0 +1,20 @@
{% extends 'emails/base.txt' %}
Copy link

Choose a reason for hiding this comment

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

Would be great down the line to have a markdown to txt and to html engine. Not sure how hard that would be to set up with the email template but would be good to be more DRY

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, a simpler alternative solution would just be to strip the HTML tags from the HTML response and use that for the text response.

However, there are some slight differences, for example links in HTML can actually be links, but in the text version we tell the user to go to some URL.

I agree, somewhere down the line we could avoid a lot of duplication by combining the HTML and non-HTML templates.

@ezwang ezwang merged commit d03dcb7 into master Jan 12, 2020
@ezwang ezwang deleted the remind branch January 12, 2020 21:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

4 participants