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

fix: Remove upcoming events from after event email text #6624

Merged
merged 4 commits into from Nov 26, 2019

Conversation

codedsun
Copy link
Contributor

Fixes #6623

Short description of what this resolves:

Changes text of email

Checklist

  • I have read the Contribution & Best practices Guide and my PR follows them.
  • My branch is up-to-date with the Upstream development branch.
  • The unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • All the functions created/modified in this PR contain relevant docstrings.

@auto-label auto-label bot added the fix label Nov 26, 2019
@codecov
Copy link

codecov bot commented Nov 26, 2019

Codecov Report

Merging #6624 into development will increase coverage by 0.02%.
The diff coverage is 20%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #6624      +/-   ##
===============================================
+ Coverage        65.06%   65.08%   +0.02%     
===============================================
  Files              297      297              
  Lines            15258    15253       -5     
===============================================
  Hits              9927     9927              
+ Misses            5331     5326       -5
Impacted Files Coverage Δ
app/api/helpers/query.py 44% <ø> (ø) ⬆️
app/api/helpers/system_mails.py 100% <ø> (ø) ⬆️
app/api/helpers/scheduled_jobs.py 24.39% <0%> (+0.95%) ⬆️
app/api/helpers/mail.py 29.29% <100%> (ø) ⬆️

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 dcb8b4e...e4dc5a1. Read the comment docs.

@codedsun
Copy link
Contributor Author

@iamareebjamal Check this

@iamareebjamal
Copy link
Member

Read the complete issue again

@codedsun
Copy link
Contributor Author

codedsun commented Nov 26, 2019

Take out the list of upcoming events and instead link to eventyay.com

@iamareebjamal
Currently they are linked to? as from the code the frontend_url is grabbed from settings.py. But can you explain me ...what they meant either the frontend url is wrong ? or the event id's are wrong?

@@ -63,7 +63,7 @@
'message': (
u"Hi {email},<br/>" +
u"Thank You for participating in our event. We hope you enjoyed it. "
u"Please check the list of more upcoming events. <br />" +
u"Please check out other upcoming events around you on eventyay.com <br />" +
Copy link
Member

Choose a reason for hiding this comment

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

Take out the list of upcoming events and instead link to eventyay.com
@codedsun

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please read my comment here
#6624 (comment)

@kushthedude

Copy link
Member

Choose a reason for hiding this comment

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

Remove the list of upcoming events which are being shown .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Omg, I thought take out means extraction -_-

@codedsun
Copy link
Contributor Author

@kushthedude Review now!

@@ -63,8 +63,7 @@
'message': (
u"Hi {email},<br/>" +
u"Thank You for participating in our event. We hope you enjoyed it. "
u"Please check the list of more upcoming events. <br />" +
u"Here are the upcoming events: {upcoming_events}. Get ready!! "
u"Please check out other upcoming events around you on eventyay.com <br />"
Copy link
Member

Choose a reason for hiding this comment

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

This repository is for open-event-server, not eventyay.com

Copy link
Contributor Author

Choose a reason for hiding this comment

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

have to use this via frontend_url? @iamareebjamal

@codedsun
Copy link
Contributor Author

@iamareebjamal check now

@@ -29,13 +29,6 @@ def send_after_event_mail():
from app import current_app as app
with app.app_context():
events = Event.query.filter_by(state='published', deleted_at=None).all()
upcoming_events = get_upcoming_events()
Copy link
Member

Choose a reason for hiding this comment

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

If get_upcoming_events is not used anywhere else, comment on it saying TODO: Unused function. Remove

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@codedsun
Copy link
Contributor Author

@iamareebjamal Check now

@codedsun
Copy link
Contributor Author

codedsun commented Nov 26, 2019 via email

Copy link
Member

@kushthedude kushthedude left a comment

Choose a reason for hiding this comment

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

Lgtm

@iamareebjamal iamareebjamal changed the title fix: After event email text fix: Remove upcoming events from after event email text Nov 26, 2019
@iamareebjamal iamareebjamal merged commit e9b879a into fossasia:development Nov 26, 2019
codedsun added a commit to codedsun/open-event-server that referenced this pull request Dec 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

After-Event Email with unrelated events
4 participants