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

Email contains "Yelp Love" alt text #13

Open
brenns10 opened this issue Feb 23, 2017 · 9 comments
Open

Email contains "Yelp Love" alt text #13

brenns10 opened this issue Feb 23, 2017 · 9 comments

Comments

@brenns10
Copy link
Collaborator

brenns10 commented Feb 23, 2017

The love notification email contains an image with alt text that is "Yelp Love". It should be changed so that it uses APP_NAME from the config module. You can find the template for the love notification email at themes/default/templates/email.html.

This was kinda "assigned" to @JKobyP but could also be a starter ticket for an open-source sprint.

@JKobyP
Copy link

JKobyP commented Mar 1, 2017

  1. Should I do the PR for this or should I let someone else take it?
  2. How do I test it before I deploy? I have the code written on a fork. Do I just need to create a new app with Google App Engine?

@brenns10
Copy link
Collaborator Author

brenns10 commented Mar 1, 2017

  1. That's up to you. I don't have an actual sprint planned yet, so fixing it yourself wouldn't really be taking a starter ticket from someone else.
  2. This is a kinda difficult one to test. You can definitely run the app on your machine without GAE, but emails don't send in dev (unless you use SendGrid, but I digress). The alternative is that you could set up a simple view that renders the email template, and then check to make sure the rendered email looks right. But to be honest, this almost doesn't need to even be verified.

brenns10 pushed a commit that referenced this issue Apr 5, 2017
Implement Gravatar support behind config variable.
@Nickatak
Copy link

Nickatak commented Jan 7, 2018

Hello! Is this still an issue that needs to be fixed? :D

@brenns10
Copy link
Collaborator Author

brenns10 commented Jan 7, 2018

Yes it is! I'd welcome a PR to fix it, and see about upstreaming the fix to Yelp/love if it hasn't been fixed there yet.

@Nickatak
Copy link

Nickatak commented Jan 7, 2018

Okay, do you mind if I take a crack at this in a bit (I've got to setup an environment so I can test it)? I'm very new to github in general, so if I mess up/am unclear about something, please let me know and I'll do my best to fix it. C:

@brenns10
Copy link
Collaborator Author

brenns10 commented Jan 7, 2018

Go for it! Testing emailing will be a bit difficult, but I think you can follow a page I set up in the wiki. Good luck :)

@brenns10
Copy link
Collaborator Author

brenns10 commented Jan 7, 2018

@Nickatak
Copy link

Nickatak commented Jan 7, 2018

Excellent, thank you very much!

@Nickatak
Copy link

Nickatak commented Jan 8, 2018

Hmm, this is going to take a while (probably a couple days at this rate). From a theoretical perspective (and basic knowledge of Flask), I understand the change that is supposed to happen; however, I am unable to get a proper testing environment to confirm and I feel that trying to PR/submit something that hasn't been tested is just asking for a disaster. I will continue working on this until I can get it up and running properly (I'm on Windows, so it's a little bit trickier than I'd like). Sorry it's taking so long. :C

Edit: :O I got it to work, now I just have to get this view to render (giving me a generic internal error for the hacky-function). Yay! It works entirely, okay, now I'm going to read up on how to submit a proper PR and I'll submit it when I'm done.

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

No branches or pull requests

3 participants