-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
feat: Changed pdf datetime to event timezones #7452
Conversation
Codecov Report
@@ Coverage Diff @@
## development #7452 +/- ##
===============================================
+ Coverage 65.34% 65.38% +0.04%
===============================================
Files 265 265
Lines 13239 13235 -4
===============================================
+ Hits 8651 8654 +3
+ Misses 4588 4581 -7
Continue to review full report at Codecov.
|
@iamareebjamal - Review |
app/models/event_invoice.py
Outdated
return self.issued_at.astimezone(pytz.timezone(self.event.timezone)) | ||
|
||
@property | ||
def due_at_tz(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use datetime helper, it has timezone support already
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iamareebjamal return self.due_at.astimezone(datetime.astimezone(self.event.timezone))
like this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or you meant datetime(order.event.timezone)
like this in jinja template?
<span style="font-size: 15px;">From: {{ order.event.starts_at | datetime(order.event.timezone) }}</span><br> | ||
<span style="font-size: 15px;">To: {{ order.event.ends_at | datetime(order.event.timezone) }}</span> | ||
<span style="font-size: 15px;">From: {{ order.event.starts_at_tz | datetime(order.event.timezone) }}</span><br> | ||
<span style="font-size: 15px;">To: {{ order.event.ends_at_tz | datetime(order.event.timezone) }}</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was already according to event timezone
@iamareebjamal - Review |
app/models/order.py
Outdated
return self.created_at.astimezone(pytz.timezone(self.event.timezone)) | ||
|
||
@property | ||
def completed_at_tz(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I said in last review, not needed
app/templates/pdf/attendees_pdf.html
Outdated
@@ -97,10 +97,10 @@ <h3 style="text-align:center;">{{ holders[0].event.name }} - {{ ("Attendees List | |||
|
|||
{% if holder.order and holder.order.status == 'completed' %} | |||
<span | |||
class="datetime">{{ holder.order.completed_at | datetime }} - {{ holder.order.completed_at | humanize }}</span> | |||
class="datetime">{{ holder.order.completed_at_tz | datetime }} - {{ holder.order.completed_at_tz | humanize }}</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use the datetime helper as suggested in last review
font-size: 12px; | ||
background: #FFFFFF; | ||
font-family: Arial, sans-serif; | ||
font-size: 12px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
app/templates/pdf/orders.html
Outdated
@@ -103,9 +103,9 @@ <h3 style="text-align:center;">{{ event.name }} - {{ ("Order List") }}</h3> | |||
{{ ('NA') }} | |||
{% endif %}<br> | |||
{% if order.status == 'completed' %} | |||
<span class="datetime">{{ order.completed_at | datetime }}</span> | |||
<span class="datetime">{{ order.completed_at_tz | datetime }}</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved even further from expected since last review
edbe7b4
to
c63c60b
Compare
app/templates/pdf/event_invoice.html
Outdated
<div><span>DATE</span> {{ invoice.issued_at | date }}</div> | ||
<div><span>DUE DATE</span> {{ invoice.due_at | date }}</div> | ||
<div><span>DATE</span> {{ invoice.issued_at | datetime(event.timezone) }}</div> | ||
<div><span>DUE DATE</span> {{ invoice.due_at | datetime(event.timezone) }}</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is date and not datetime, make date helper timezone aware
2ecd492
to
417167b
Compare
<div><span>DATE</span> {{ invoice.issued_at | date }}</div> | ||
<div><span>DUE DATE</span> {{ invoice.due_at | date }}</div> | ||
<div><span>DATE</span> {{ invoice.issued_at | date(event.timezone) }}</div> | ||
<div><span>DUE DATE</span> {{ invoice.due_at | date(event.timezone) }}</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The date filter is not timezone aware. It'll fail. Please test the changes before asking for review. Reduce the request review cycle please. The changes on this PR either go opposite of what it should be or plain don't work.
Why don't you test it first?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't requested for review this tym. I will test and then ask for review and will make sure to reduce the request review cycle. Sorry for the inconvinience
if not date: | ||
return '' | ||
return date.strftime('%B %d, %Y') | ||
return date.astimezone(pytz.timezone(timezone)).strftime('%B %d, %Y') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return simple_datetime_display(date, timezone, '%B %d, %Y')
@iamareebjamal - Review |
if not date: | ||
return '' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if not date: | |
return '' |
Other emails contain time as well |
@iamareebjamal Correct? |
After event email and others as well |
@iamareebjamal As you mentioned after event email open-event-server/app/api/helpers/system_mails.py Lines 144 to 152 in 247e9bf
Are you referring to sent at field? as I can't see anyother field which uses time |
No, I must have forgot. If that's it, then yes, those are the only emails |
@iamareebjamal - Tested
|
Thanks! yeah 🎉 |
Fixes #7438
Short description of what this resolves:
feat: Changed pdf datetime to event timezones
Changes proposed in this pull request:
Checklist
development
branch.