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

I1544 backend dependencies #1554

Merged

Conversation

pushyamig
Copy link
Contributor

@pushyamig pushyamig commented Nov 17, 2023

Fixes# 1544

Changes Involved

  1. zoneinfo default timezone. migration from using pytz to using zoneinfo.
  2. Trusted Origin change: Needs an HTTPS in the configuration
  3. Cron changes are from update to SQLAlchemy V2 and psycopg V3
    1. As part of this change to postgres new driver update, you cannot make 2 queries in single format structure under the coding pattern cron.py adopted. So need to split the submission query into two.
    2. So I need to implement a different connection for this and need to run in a session. Otherwise it won't remember the temporary table all_assign_sub.
  4. django-graphene had a major upgrade as well and they removed the Promise Based Loader.
    1. I have used a new Library to resolve issues. (before it crashed Assignment planning view)
    2. At some point we need to convert all the GraphQL implementation code to adopt to this new pattern
    3. But there is an issue Open to support Promise based Loader
  5. The changes are in MyLA beta. You can access it using Canvas Beta.
    1. Ran Cron for these changes by nulling the date_last_updated .So It took about 9min to run about 56 courses.

docs/loading_data.md Outdated Show resolved Hide resolved
df = pd.DataFrame(result.fetchall(), columns=result.keys())
df = df.drop_duplicates(keep='first')
df.to_sql(con=engine, name='submission', if_exists='append', index=False)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Split up the Submission into 2 since new postgres driver did not supported 2 queries in one. I have details on this in the PR description

def execute_db_query(query: str, params: List = None) -> ResultProxy:
with engine.connect() as connection:
def execute_db_query(query: str, params: Dict = None) -> ResultProxy:
with engine.begin() as connection:
Copy link
Contributor Author

@pushyamig pushyamig Nov 17, 2023

Choose a reason for hiding this comment

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

I have to make this change otherwise the changes were not Auto-committed(As part of the SQLALchemy v2 changes)

@@ -160,8 +164,6 @@ def has_change_permission(request, obj=None):
def has_delete_permission(request, obj=None):
return False

admin.site.register(AcademicTerms, TermAdmin)
admin.site.register(Course, CourseAdmin)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All these change are part of prior Django 3.x version, and Django-upgrade lib made these changes.

Dockerfile Show resolved Hide resolved
@pushyamig pushyamig marked this pull request as ready for review November 17, 2023 19:55
@jaydonkrooss
Copy link
Contributor

build was successful (after changing the is_superuser setting which was blocking me initially), and the cron job ran successfully for both courses I was testing in beta

@pushyamig
Copy link
Contributor Author

@zqian and @jonespm , i would like you both review this code since it has changes to area where I might not contributed to the code like cron and GraphQL.

@pushyamig
Copy link
Contributor Author

@jonespm and @zqian If this PR is not reviewed that I have to unnecessarily blocking other people working on an issue or I might endup fixing all their issue if merged before this due to library upgrade .

docs/github_actions.md Show resolved Hide resolved
dashboard/settings.py Show resolved Hide resolved
dashboard/management/commands/term.py Show resolved Hide resolved
dashboard/graphql/view.py Show resolved Hide resolved
@pushyamig
Copy link
Contributor Author

@zqian I have resolved the issues you commented on in the PR

Copy link
Member

@zqian zqian left a comment

Choose a reason for hiding this comment

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

Tested cron and tool. All work well.

@pushyamig pushyamig merged commit b5f8752 into tl-its-umich-edu:master Dec 18, 2023
1 check failed
@zqian zqian linked an issue Dec 18, 2023 that may be closed by this pull request
@jennlove-um
Copy link
Contributor

Cron runs fine and no issues detected in beta testing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support/help with promise-based resolvers Upgrade Django backend
4 participants