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

Store dates from cron "as-is", remove time conversion from cron #1490

Closed
jonespm opened this issue Mar 6, 2023 · 6 comments · Fixed by #1556
Closed

Store dates from cron "as-is", remove time conversion from cron #1490

jonespm opened this issue Mar 6, 2023 · 6 comments · Fixed by #1556
Assignees

Comments

@jonespm
Copy link
Member

jonespm commented Mar 6, 2023

Thank you for contributing to this project!

Describe your problem or feature you'd like added

It was noted on some recent fixes like #1484 that MyLA is storing values in local time. We had some discussion that this probably isn't needed and the application could do the conversion in the API (either in GraphQL or in the view) to the value in the settings. This might require some minor migration or we can just require dropping the tables and re-running the cron.

This would probably be made easier if the old assignment view API's were removed #1492 and the cron for the UDW #1493 was also removed.

@jennlove-um
Copy link
Contributor

Use the LTI launch timezone value to detect it. Another option would be to use the browser timezone, but LTI is the best approach since it will match the timezone in Canvas.

@jennlove-um
Copy link
Contributor

@jaydonkrooss Assigning you as an observer on this one.

@pushyamig
Copy link
Contributor

Here is the time zone related change with newest Django 4 upgrade.
https://docs.djangoproject.com/en/4.2/releases/4.0/#zoneinfo-default-timezone-implementation

@jaydonkrooss jaydonkrooss moved this from To do to In progress in MyLA-2024.01.01 Dec 7, 2023
@lsloan
Copy link
Member

lsloan commented Dec 7, 2023

I recommended this issue to Jaydon. He can discuss it with me as he makes progress with it.

@jaydonkrooss
Copy link
Contributor

jaydonkrooss commented Dec 8, 2023

From what I've been gathering on the previous issue #1484, it seems that we're not trying to have any more local time conversions when populating the MyLA database, instead putting the conversion client-side. Based on this commit for that initial timezone fix, I'm thinking the only fields we care about in this case are local_date from assignment and grade_posted_local_date from submission. It seems that other datetimes are being imported from UDP without conversion; please share if there are other fields I'm not aware of that I should be looking at.

Specific changes I'm considering:

  1. drop the local_date field and use already provided due_date on the frontend
  2. edit the grade_posted_local_date to simply be grade_posted without a local time conversion
    (I'm not sure if this field is even used on the frontend so I don't anticipate any UI regression we'd have to test)

lsloan added a commit that referenced this issue Dec 21, 2023
Also cleaned up some bad grammar and grouped some related lines together.
lsloan added a commit that referenced this issue Dec 21, 2023
lsloan added a commit that referenced this issue Dec 21, 2023
Some comments were vague.  More explicit comments make configuring the app easier to understand.
MyLA-2024.01.01 automation moved this from In progress to Review/QA Jan 16, 2024
lsloan added a commit that referenced this issue Jan 16, 2024
…conversion

remove local-time fields, add time zone (iss. #1490, #1491)
@jennlove-um
Copy link
Contributor

Cron runs fine and no issues detected in beta testing.

@jennlove-um jennlove-um moved this from Review/QA to Done in MyLA-2024.01.01 Jan 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

5 participants