fix(enrichment): default to next week on weekends#1899
Conversation
|
coverage: 79.227% (-0.03%) from 79.252% — ZamanZahid:fix-enrichment-final into tjcsl:dev |
aarushtools
left a comment
aarushtools
left a comment
There was a problem hiding this comment.
Hi Zaman,
Thank you for the PR, can you look at my comments and implement them? Let me know if you have any questions
| if timezone.is_naive(weekend_start): | ||
| weekend_start = timezone.make_aware(weekend_start) |
There was a problem hiding this comment.
why is this necessary? aren't we using django utils timezone. I could be wrong
There was a problem hiding this comment.
this variable is also misleading in name - it's only the weekend start when the day is the first day of the weekend right?
There was a problem hiding this comment.
Your right, I removed the unnecessary timezone check, and also renamed the variable to make it more clear for what it is.
| weekend_enrichments = EnrichmentActivity.objects.visible_to_user(request.user).filter( | ||
| time__gte=weekend_start, | ||
| time__lt=weekend_end, | ||
| ) |
There was a problem hiding this comment.
A nitpick but I prefer time__range for consistency - it's what is already being used throughout the files (unless you don't want inclusive bounds)
There was a problem hiding this comment.
Alright I Changed this to use time__range for consistency.
| weekend_end = weekend_start + timedelta(days=(8 - local_time.isoweekday())) | ||
|
|
||
| weekend_enrichments = EnrichmentActivity.objects.visible_to_user(request.user).filter( | ||
| time__gte=weekend_start, |
There was a problem hiding this comment.
why not just use local time + timedelta for weekend_start, why is it starting from midnight?
There was a problem hiding this comment.
Changed it so weekend_start uses local_time + timedelta rather than starting from midnight.
| if weekend_enrichments.exists(): | ||
| return local_time | ||
|
|
||
| return local_time + timedelta(days=(8 - local_time.isoweekday())) |
There was a problem hiding this comment.
save the timedelta as a variable that is well named so it's clear what it's doing, esp. because you repeat this line twice
There was a problem hiding this comment.
Saved the timedelta in a variable so it is not repeated.
|
|
||
|
|
||
| def enrichment_context(request, date=None): | ||
| def get_default_enrichment_date(request): |
There was a problem hiding this comment.
docstrings - what is this function doing?
There was a problem hiding this comment.
I added a it but, this returns next week's date on weekends unless enrichments still are happening the same weekend.
ZamanZahid
force-pushed
the
fix-enrichment-final
branch
3 times, most recently
from
f0e0b04 to
f748d25
Compare
ZamanZahid
force-pushed
the
fix-enrichment-final
branch
from
42378e9 to
04290af
Compare
3 participants
Proposed changes
Brief description of rationale
Right now, the enrichment page shows the previous week on Saturdays and Sundays even though signups are closed after Friday. This changde makes it show the next week instead unless there is something happeningg that weekend.