Calendar Performance Update #241
Conversation
| except (ValueError, IndexError): | ||
| pass | ||
|
|
||
| if date and current_time.replace(tzinfo=None) < date.replace(tzinfo=None) < ( |
There was a problem hiding this comment.
@ashleyzhang01 could you explain this line here?
There was a problem hiding this comment.
I set the time zone for current_time to none because couldn't compare the datetime.datetime.now() with the date due to some time zone error. I guess if we moved back to the Django timezone, we might not have to do this since when we were using it before, that wasn't an issue?
There was a problem hiding this comment.
Sounds good, yea will revert back to timezone
| date = models.CharField(max_length=50, null=True, blank=True) | ||
| # NOTE: This is bad practice, though is necessary for the time being | ||
| # since frontends use the string date field | ||
| date_obj = models.DateTimeField(null=True, blank=True) |
There was a problem hiding this comment.
We probably need to change this in the future.
There was a problem hiding this comment.
Should have fields for a start date and a nullable end date bc some calendar events are ranges.
There was a problem hiding this comment.
Thanks for the reminder. I will leave this for you to do in a future events PR you create so we can group relevant code
| def get_queryset(self): | ||
| return CalendarEvent.objects.filter( | ||
| date_obj__gte=timezone.localtime(), | ||
| date_obj__lte=timezone.localtime() + timedelta(days=30), |
There was a problem hiding this comment.
For now we need this because we never clear the database. @ashleyzhang01 what do you think? Do you think the manage.py command should delete all previous CalendarEvents that way we don't have to re-do this filtering for events?
There was a problem hiding this comment.
We don't call the manage.py thing often enough that it deletes all previous. I had something in my Calendar method in the views.py for this-- can you move it in too?
Basically, it deletes any previous events when called each time, so we don't have to do the manage.py as often and don't have to filter through the time range as often.
judtinzhang
requested review from
vcai122,
ashleyzhang01,
tuneerroy,
dr-Jess and
jeofo
| def get_queryset(self): | ||
| return CalendarEvent.objects.filter( | ||
| date_obj__gte=timezone.localtime(), | ||
| date_obj__lte=timezone.localtime() + timedelta(days=30), |
There was a problem hiding this comment.
Wait also if it is only called once a month, then by the end of the month there won't be many future events left in the db, so we wouldn't really have any calendar events?
There was a problem hiding this comment.
Ah yes good point. I'll change the get_calendar to create CalendarEvents that exceed 30 days as well. Then we will leave this view as is.
| def handle(self, *args, **kwargs): | ||
| # scrapes almanac from upenn website | ||
| try: | ||
| resp = requests.get("https://almanac.upenn.edu/penn-academic-calendar") |
There was a problem hiding this comment.
hm could we make this a constant value, similar to how its handled here. and if we make the constant a name like UPENN_ALMANAC_WEBSITE then we can also get rid of the comment above since its self-explanatory
There was a problem hiding this comment.
sounds gooood tanner :D
|
|
||
|
|
||
| class Command(BaseCommand): | ||
| def handle(self, *args, **kwargs): |
There was a problem hiding this comment.
i know a lot of this code was just moved but if u understand it, could we break this function down into more modular components? the super nested try-except statements are a bit hard to read.
There was a problem hiding this comment.
agreed. just cleaned it up to make it more readable.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #241 +/- ##
==========================================
+ Coverage 91.32% 91.44% +0.11%
==========================================
Files 58 59 +1
Lines 2491 2502 +11
==========================================
+ Hits 2275 2288 +13
+ Misses 216 214 -2 ☔ View full report in Codecov by Sentry. |
No description provided.