Skip to content

Fix "last submission time" parsing issue#25818

Closed
regisb wants to merge 1 commit intoopenedx:masterfrom
regisb:regisb/fix-datetime-parsing
Closed

Fix "last submission time" parsing issue#25818
regisb wants to merge 1 commit intoopenedx:masterfrom
regisb:regisb/fix-datetime-parsing

Conversation

@regisb
Copy link
Contributor

@regisb regisb commented Dec 9, 2020

We are affected by the following python-dateutil issue:
dateutil/dateutil#163

The symptoms are described here:
https://openedx.atlassian.net/jira/software/projects/BTR/boards/641?selectedIssue=BTR-25

In a nutshell: when creating an exercise with a 10 seconds answer delay
on a server that is configured with a non-UTC tzdata (but UTC TIME_ZONE
django setting), students are faced with error messages stating that
they have to wait 4h 59min 50s.

The deeper issue is that dateutil incorrectly parses the datetime
object. This issue is resolved by simply upgrading python-dateutil.

I realise that dateutil was constrained to an older version before. We
will have to see whether this change causes production errors.

This is ready for review.

@openedx-webhooks openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Dec 9, 2020
@openedx-webhooks
Copy link

Thanks for the pull request, @regisb! I've created OSPR-5294 to keep track of it in JIRA, where we prioritize reviews. Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

@regisb
Copy link
Contributor Author

regisb commented Dec 9, 2020

cc @jmbowman because you were the one who downgraded python-dateutil in https://github.com/edx/edx-platform/pull/22703

Copy link
Contributor

@jmbowman jmbowman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The followup from that revert was captured as ARCHBOM-802 . As noted there, this should be safe now that we're no longer running Python 3.5, but we should keep an eye out for errors after the deployment just in case.

@natabene
Copy link
Contributor

@regisb Thank you for your contribution. This is ready for our review.

@jmbowman
Copy link
Contributor

@regisb I think this is ready to merge once the merge conflicts are resolved. We may want to defer until after the holidays, since we have an edx-platform code freeze starting tomorrow.

@natabene
Copy link
Contributor

@jmbowman Too late to merge? We still have 40 minutes till code freeze :)

@jmbowman
Copy link
Contributor

Still needs to be rebased and get through tests, so yeah, probably.

@natabene
Copy link
Contributor

@jmbowman Got it, thanks for clarifying.

@regisb
Copy link
Contributor Author

regisb commented Dec 22, 2020

Rebased!

@natabene
Copy link
Contributor

Too late to merge today, but setting as ready to merge for after holidays.

@natabene
Copy link
Contributor

@regisb Could you look into conflicts? Once resolved we can probably try to merge. CC @jmbowman

@regisb regisb force-pushed the regisb/fix-datetime-parsing branch from 6c72b12 to 7696439 Compare January 13, 2021 17:20
@regisb
Copy link
Contributor Author

regisb commented Jan 13, 2021

@natabene 👍

@natabene
Copy link
Contributor

@jmbowman Might be ready to merge when you have a chance.

@jmbowman
Copy link
Contributor

Argh, I'm actually 2 weeks behind on reading GitHub notification emails now; trying to work through that backlog today, please rebase again and I'll merge. You may want to ping me on Slack just to be sure I don't miss it again.

@regisb regisb force-pushed the regisb/fix-datetime-parsing branch from 7696439 to 387222c Compare January 29, 2021 10:16
@regisb
Copy link
Contributor Author

regisb commented Jan 29, 2021

@jmbowman rebased!

@arbrandes
Copy link
Contributor

@bradenmacdonald, is this something you have time to approve as CC? Asking on behalf of the build-test-release working group.

@jmbowman
Copy link
Contributor

jmbowman commented Feb 1, 2021

A separate PR tried to upgrade python-dateutil on Friday, and we had to roll back because it caused problems on prod with loading pickled datetime instances. We thought that would have been resolved by the Python 3.8 upgrade, but haven't had time yet to investigate whether we hit the same issue again or a similar new one. We'll be doing that in the next couple of days, but afraid we can't merge this until we get to the bottom of that.

@arbrandes
Copy link
Contributor

Ah, thanks for the update @jmbowman! Didn't mean to sidestep you - figured you might've been too busy to look at this one.

@openedx-webhooks openedx-webhooks added blocked by other work PR cannot be finished until other work is complete and removed engineering review labels Feb 3, 2021
@regisb regisb force-pushed the regisb/fix-datetime-parsing branch from 387222c to 20d096f Compare February 19, 2021 12:28
@natabene
Copy link
Contributor

Update: this is still blocked by edX, waiting on the work Jeremy mentioned.

@natabene
Copy link
Contributor

@jmbowman Could you please update us whether the work you mentioned has been complete? Can this PR be merged now?

@openedx-webhooks openedx-webhooks added engineering review and removed blocked by other work PR cannot be finished until other work is complete labels Mar 10, 2021
@natabene
Copy link
Contributor

I talked to @jmbowman, this is still by https://openedx.atlassian.net/browse/BOM-2245 being investigated.

@openedx-webhooks openedx-webhooks added blocked by other work PR cannot be finished until other work is complete and removed engineering review labels Mar 10, 2021
@natabene
Copy link
Contributor

Looks like still blocked.

@arch-bom-gocd-alerts
Copy link

📣 💥 Heads-up: You must either rebase onto master or merge master into your branch to avoid breaking the build.

We recently removed diff-quality and introduced lint-amnesty. This means that the automated quality check that has run on your branch doesn't work the same way it will on master. If you have introduced any quality failures, they might pass on the PR but then break the build on master.

This branch has been detected to not have commit 2e33565 as an ancestor. Here's how to see for yourself:

git merge-base --is-ancestor 2e335653 regisb/fix-datetime-parsing && echo "You're all set" || echo "Please rebase onto master or merge master to your branch"

If you have any questions, please reach out to the Architecture team (either #edx-shared-architecture on Open edX Slack or #architecture on edX internal).

@regisb regisb force-pushed the regisb/fix-datetime-parsing branch from 20d096f to f564521 Compare June 14, 2021 08:46
We are affected by the following python-dateutil issue:
dateutil/dateutil#163

The symptoms are described here:
https://openedx.atlassian.net/jira/software/projects/BTR/boards/641?selectedIssue=BTR-25

In a nutshell: when creating an exercise with a 10 seconds answer delay
on a server that is configured with a non-UTC tzdata (but UTC TIME_ZONE
django setting), students are faced with error messages stating that
they have to wait 4h 59min 50s.

The deeper issue is that dateutil incorrectly parses the datetime
object. This issue is resolved by simply upgrading python-dateutil.

I realise that dateutil was constrained to an older version before. We
will have to see whether this change causes production errors.

This is for: openedx/wg-build-test-release#9
@regisb regisb force-pushed the regisb/fix-datetime-parsing branch from f564521 to 745cc72 Compare June 14, 2021 09:41
@edx-status-bot
Copy link

Your PR has finished running tests. There were no failures.

@natabene
Copy link
Contributor

natabene commented Apr 5, 2022

@regisb We are about to resume work that will unblock this PR. Sorry for the long wait.

@regisb
Copy link
Contributor Author

regisb commented Jun 6, 2022

This issue is currently being handled elsewhere:

#30255
#30530

@regisb regisb closed this Jun 6, 2022
@openedx-webhooks openedx-webhooks added rejected and removed blocked by other work PR cannot be finished until other work is complete labels Jun 6, 2022
@openedx-webhooks
Copy link

@regisb Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

7 participants

Comments