remove local-time fields, add time zone (iss. #1490, #1491)

[jaydonkrooss]

Changes added:

• Drop local time conversions from data fields (on submissions & assignments)
• Save LTI time_zone setting in SessionMiddleware to store timezone information
• Use saved time_zone to implement a time conversion on API level when needed for frontend (assignment due_date in graphql)
• Reimplement frontend consuming due_date times, extract date without doing any UTC conversion

[lsloan]

@jaydonkrooss, when I tried to run your migration (which I renamed to 0027_remove_or_change_local_date_fields.py), I got some errors…

[2023-12-21 09:30:59 -0500] [DEBUG] [utils.py:123] (0.001) SHOW FULL TABLES; args=None
  Applying dashboard.0027_remove_or_change_local_date_fields...
[2023-12-21 09:30:59 -0500] [DEBUG] [schema.py:136] ALTER TABLE `assignment` DROP COLUMN `local_date`; (params ())
[2023-12-21 09:30:59 -0500] [DEBUG] [utils.py:123] (0.020) None; args=()
…
django.db.utils.OperationalError: (1091, "Can't DROP 'local_date'; check that column/key exists")

I'm looking into it…

Update

Looks like it's just my problem. The migration had already been run under its original name, 0027_auto_20231211_1440. I'll just mark it as done and it should go on with the next migration, which I recently added.

[lsloan]

@jaydonkrooss, the changes I've committed saves the user's Canvas time zone to the User model. It looks like this in my local DB:

|id  |user_id          |sis_name|course_id        |current_grade|final_grade|enrollment_type  |time_zone       |
|----|-----------------|--------|-----------------|-------------|-----------|-----------------|----------------|
|1386|17700000000151503|lsloan  |17700000000374350|             |           |TeacherEnrollment|America/New_York|

Now we just need to use it when displaying timestamps to the user.

[lsloan]

@jaydonkrooss, my changes just save the time zone text as it was supplied by Canvas. In my case, that is America/New_York. When we use the TZ to display timestamps to the user, it might be inconvenient or inefficient to convert that string to a time zone offset value every time it's used. So, I can change the code to store that converted value in the User model instead. It can be converted to whichever format you think would be easiest to work with.

I have a couple of ideas. Which do you prefer, if any?

• string — '-04:00' (it may be without the colon)
• number — -4.0

[jonespm]

I just looked at this today, sorry for not keeping up with it. I'm not sure there's actually any need to persist this timezone information from the launch in the database. That would simplify this a little bit.

This app uses SessionMiddleware but we aren't making much use of it.

So I think in the lti_new.py we should just be able to just do something like

request.session["time_zone"] = time_zone:

Then the views could also retrieve that. This would eliminate the model changes and migrations here.

I haven't tried this and it doesn't look like we're using the sessions much here but I don't see any reason to persist when this will be available on every launch and we can always fall-back to the settings.TIME_ZONE. Maybe something to try after the break.

[lsloan]

@jonespm, thanks for the feedback. I agree that less model changes would be better, especially since the time zone is read from each launch request and updated in the model every time. The model is only used to provide the zone for subsequent requests following the LTI launch.

Session data could be more efficient for this purpose. I wasn't aware of the session middleware or that we used any session data in MyLA other than what Django itself may need.

So, the idea would be to get the time zone from the LTI launch parameters, but store it in the session rather than the model.

I'll give that a shot and with those changes, Jaydon could work on using the session data in the views.

[jaydonkrooss]

Zhen suggested the two of us review each other's code, for expediency. She wants to get this moved to testing ASAP.

I think your code looks good. I appreciate the changes you made and your attention to detail.

The review has a red "x" because it didn't pass a Codacy test. According to it the new complexity is… an additional blank line in dashboard/graphql/objects.py. If you correct that, we should be all set.

Unless you have any critique of the parts I contributed, you can then merge the PR.

https://app.codacy.com/gh/tl-its-umich-edu/my-learning-analytics/pullRequest?prid=13224791&bid=39353410#newIssuesView

Just pushed the whitespace fixes, thanks for pointing that out. Your changes look good as well, I like the descriptive logs and comments.