Skip to content

chore: make upgrade #226

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.

Sign up for GitHub

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

Merged
taimoor-ahmed-1 merged 2 commits into from
Sep 5, 2025
Merged

chore: make upgrade #226

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2aca238
chore: make upgrade
samuelallan72 Aug 29, 2025
f83e44f
fix: fix minor bugs caught by updated type checker
samuelallan72 Sep 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions forum/api/threads.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,11 @@ def update_thread(
)
backend.update_thread(thread_id, **update_thread_data)
thread = backend.get_thread(thread_id)
if thread is None:
log.error(
"Forumv2RequestError for update thread request - retrieving updated thread to send in response."
Copy link

@bradenmacdonald bradenmacdonald Sep 4, 2025

Choose a reason for hiding this comment

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

Suggested change
"Forumv2RequestError for update thread request - retrieving updated thread to send in response."
"ForumV2RequestError for update thread request - retrieving updated thread to send in response."

This is the only part I wasn't sure about. It says "retrieving updated thread" but I don't see it doing that here.

Copy link
Contributor Author

@samuelallan72 samuelallan72 Sep 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The general flow in this method (pseudocode) is:

  • thread: Thread = validate_object("CommentThread", id) # raises error if doesn't exist
  • update_thread()
  • thread: Thread|None = get_thread(id) # returns None if doesn't exist

So on line 265, thread should be the thread after the updates on line 263. However, nothing in the type system guarantees the thread wasn't deleted between line 263 and line 264. So the check is to keep the types happy, although there's a very very small chance of that happening.

Alternatively, we can change the get_thread call on line 265 to validate_object("CommentThread", id), which will raise an error instead or returning None. I'm not sure if there's any particular reason to use one over the other though, which is why I left it as-is and just focused on fixing the type error.

)
raise ForumV2RequestError(f"Thread no longer exists with Id: {thread_id}")

try:
return prepare_thread_api_response(
Expand Down
6 changes: 3 additions & 3 deletions forum/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ def get_handler_by_name(name: str) -> Signal:


def prepare_comment_data_for_get_children(
children: list[dict[str, Any]]
children: list[dict[str, Any]],
) -> list[dict[str, Any]]:
"""Prepare children data to be used in serializer."""
children_data = []
Expand Down Expand Up @@ -174,15 +174,15 @@ def get_group_ids_from_params(params: dict[str, Any]) -> list[int]:
"""
if "group_id" in params and "group_ids" in params:
raise ValueError("Cannot specify both group_id and group_ids")
group_ids: str | list[str] = []
if group_id := params.get("group_id"):
return [int(group_id)]
elif group_ids := params.get("group_ids", []):
if isinstance(group_ids, str):
return [int(x) for x in group_ids.split(",")]
elif isinstance(group_ids, list):
return [int(x) for x in group_ids]
return group_ids
# if it's not a str or list, it's invalid, so drop it.
return []


def get_commentable_ids_from_params(params: dict[str, Any]) -> list[str]:
Expand Down
99 changes: 51 additions & 48 deletions requirements/base.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,31 +4,31 @@
#
# make upgrade
#
amqp==5.2.0
amqp==5.3.1
# via kombu
annotated-types==0.7.0
# via pydantic
asgiref==3.8.1
asgiref==3.9.1
# via django
attrs==24.2.0
attrs==25.3.0
# via openedx-events
beautifulsoup4==4.12.3
beautifulsoup4==4.13.5
# via -r requirements/base.in
billiard==4.2.1
# via celery
camel-converter[pydantic]==4.0.1
# via meilisearch
celery==5.4.0
celery==5.5.3
# via event-tracking
certifi==2024.8.30
certifi==2025.8.3
# via
# elasticsearch
# requests
cffi==1.17.1
# via pynacl
charset-normalizer==3.4.0
charset-normalizer==3.4.3
# via requests
click==8.1.7
click==8.2.1
# via
# celery
# click-didyoumean
Expand All @@ -38,15 +38,15 @@ click==8.1.7
# edx-django-utils
click-didyoumean==0.3.1
# via celery
click-plugins==1.1.1
click-plugins==1.1.1.2
# via celery
click-repl==0.3.0
# via celery
code-annotations==1.8.0
code-annotations==2.3.0
# via edx-toggles
django==4.2.16
django==4.2.23
# via
# -c https://raw.githubusercontent.com/edx/edx-lint/master/edx_lint/files/common_constraints.txt
# -c https:/raw.githubusercontent.com/edx/edx-lint/master/edx_lint/files/common_constraints.txt
# -r requirements/base.in
# django-crum
# django-waffle
Expand All @@ -60,71 +60,69 @@ django-crum==0.7.9
# via
# edx-django-utils
# edx-toggles
django-waffle==4.1.0
django-waffle==5.0.0
# via
# edx-django-utils
# edx-toggles
djangorestframework==3.15.2
djangorestframework==3.16.1
# via -r requirements/base.in
dnspython==2.7.0
# via pymongo
edx-ccx-keys==1.3.0
edx-ccx-keys==2.0.2
# via openedx-events
edx-django-utils==7.0.0
edx-django-utils==8.0.0
# via
# edx-toggles
# event-tracking
# openedx-events
edx-opaque-keys[django]==2.11.0
edx-opaque-keys[django]==3.0.0
# via
# edx-ccx-keys
# openedx-events
edx-search==4.1.1
edx-search==4.1.3
# via -r requirements/base.in
edx-toggles==5.2.0
edx-toggles==5.4.1
# via
# edx-search
# event-tracking
elasticsearch==7.13.4
# via
# -c https://raw.githubusercontent.com/edx/edx-lint/master/edx_lint/files/common_constraints.txt
# -c https:/raw.githubusercontent.com/edx/edx-lint/master/edx_lint/files/common_constraints.txt
# -r requirements/base.in
# edx-search
event-tracking==3.0.0
event-tracking==3.3.0
# via edx-search
fastavro==1.9.7
fastavro==1.12.0
# via openedx-events
idna==3.10
# via requests
jinja2==3.1.4
jinja2==3.1.6
# via code-annotations
kombu==5.4.2
kombu==5.5.4
# via celery
markupsafe==3.0.2
# via jinja2
meilisearch==0.31.6
meilisearch==0.37.0
# via edx-search
mysqlclient==2.2.5
mysqlclient==2.2.7
# via -r requirements/base.in
newrelic==10.2.0
# via edx-django-utils
openedx-atlas==0.6.2
openedx-atlas==0.7.0
# via -r requirements/base.in
openedx-events==9.15.0
openedx-events==10.5.0
# via event-tracking
pbr==6.1.0
# via stevedore
prompt-toolkit==3.0.48
packaging==25.0
# via kombu
prompt-toolkit==3.0.52
# via click-repl
psutil==6.1.0
psutil==7.0.0
# via edx-django-utils
pycparser==2.22
# via cffi
pydantic==2.9.2
pydantic==2.11.7
# via camel-converter
pydantic-core==2.23.4
pydantic-core==2.33.2
# via pydantic
pymongo==4.4.0
pymongo==4.14.1
# via
# -r requirements/base.in
# edx-opaque-keys
Expand All @@ -135,39 +133,41 @@ python-dateutil==2.9.0.post0
# via celery
python-slugify==8.0.4
# via code-annotations
pytz==2024.2
pytz==2025.2
# via event-tracking
pyyaml==6.0.2
# via code-annotations
requests==2.32.3
requests==2.32.5
# via
# -r requirements/base.in
# meilisearch
six==1.16.0
six==1.17.0
# via
# edx-ccx-keys
# event-tracking
# python-dateutil
soupsieve==2.6
soupsieve==2.8
# via beautifulsoup4
sqlparse==0.5.1
sqlparse==0.5.3
# via django
stevedore==5.3.0
stevedore==5.5.0
# via
# code-annotations
# edx-django-utils
# edx-opaque-keys
text-unidecode==1.3
# via python-slugify
typing-extensions==4.12.2
typing-extensions==4.15.0
# via
# beautifulsoup4
# edx-opaque-keys
# pydantic
# pydantic-core
tzdata==2024.2
# via
# celery
# kombu
# typing-inspection
typing-inspection==0.4.1
# via pydantic
tzdata==2025.2
# via kombu
urllib3==1.26.20
# via
# elasticsearch
Expand All @@ -179,3 +179,6 @@ vine==5.1.0
# kombu
wcwidth==0.2.13
# via prompt-toolkit

# The following packages are considered to be unsafe in a requirements file:
# setuptools
Loading