diff --git a/mypy.ini b/mypy.ini index eee85b592ff7..49d6f61b29d6 100644 --- a/mypy.ini +++ b/mypy.ini @@ -3,4 +3,16 @@ follow_imports = silent ignore_missing_imports = True allow_untyped_globals = True exclude = tests -files = openedx/core/djangoapps/content/learning_sequences/,openedx/core/types +plugins = + mypy_django_plugin.main, + mypy_drf_plugin.main +files = + openedx/core/djangoapps/content/learning_sequences/, + openedx/core/djangoapps/content_staging, + openedx/core/djangoapps/content_libraries, + openedx/core/djangoapps/xblock, + openedx/core/types + +[mypy.plugins.django-stubs] +# content_staging only works with CMS; others work with either, so we run mypy with CMS settings. +django_settings_module = "cms.envs.test" diff --git a/openedx/core/djangoapps/content/learning_sequences/data.py b/openedx/core/djangoapps/content/learning_sequences/data.py index d7efe0e0a166..d7b706bb431a 100644 --- a/openedx/core/djangoapps/content/learning_sequences/data.py +++ b/openedx/core/djangoapps/content/learning_sequences/data.py @@ -213,7 +213,7 @@ def not_deprecated(self, _attribute, value): course_visibility: CourseVisibility = attr.ib(validator=attr.validators.in_(CourseVisibility)) # Entrance Exam ID - entrance_exam_id = attr.ib(type=str) + entrance_exam_id = attr.ib(type=Optional[str]) def __attrs_post_init__(self): """Post-init hook that validates and inits the `sequences` field.""" diff --git a/openedx/core/djangoapps/content/learning_sequences/migrations/0014_remove_user_partition_group_duplicates.py b/openedx/core/djangoapps/content/learning_sequences/migrations/0014_remove_user_partition_group_duplicates.py index 0469daa6ad23..480559e37da4 100644 --- a/openedx/core/djangoapps/content/learning_sequences/migrations/0014_remove_user_partition_group_duplicates.py +++ b/openedx/core/djangoapps/content/learning_sequences/migrations/0014_remove_user_partition_group_duplicates.py @@ -1,5 +1,5 @@ # Generated by Django 2.2.24 on 2021-07-07 18:34 - +# type: ignore from django.db import migrations from django.db.models import Count, Min diff --git a/openedx/core/djangoapps/content/learning_sequences/views.py b/openedx/core/djangoapps/content/learning_sequences/views.py index 671242fd47cb..25f078d9f7a8 100644 --- a/openedx/core/djangoapps/content/learning_sequences/views.py +++ b/openedx/core/djangoapps/content/learning_sequences/views.py @@ -201,7 +201,7 @@ def _determine_user(self, request, course_key: CourseKey) -> types.User: # Just like in masquerading, set real_user so that the # SafeSessions middleware can see that the user didn't # change unexpectedly. - target_user.real_user = request.user + target_user.real_user = request.user # type: ignore return target_user _course_masquerade, user = setup_masquerade(request, course_key, has_staff_access) diff --git a/openedx/core/djangoapps/content_libraries/api.py b/openedx/core/djangoapps/content_libraries/api.py index 7b44ec7d51a4..a01ef1891363 100644 --- a/openedx/core/djangoapps/content_libraries/api.py +++ b/openedx/core/djangoapps/content_libraries/api.py @@ -268,14 +268,6 @@ class LibraryBundleLink: opaque_key = attr.ib(type=LearningContextKey, default=None) -class AccessLevel: # lint-amnesty, pylint: disable=function-redefined - """ Enum defining library access levels/permissions """ - ADMIN_LEVEL = ContentLibraryPermission.ADMIN_LEVEL - AUTHOR_LEVEL = ContentLibraryPermission.AUTHOR_LEVEL - READ_LEVEL = ContentLibraryPermission.READ_LEVEL - NO_ACCESS = None - - # General APIs # ============ diff --git a/openedx/core/djangoapps/content_staging/admin.py b/openedx/core/djangoapps/content_staging/admin.py index a40a13913be4..4c4feb290be6 100644 --- a/openedx/core/djangoapps/content_staging/admin.py +++ b/openedx/core/djangoapps/content_staging/admin.py @@ -30,12 +30,12 @@ class UserClipboardAdmin(admin.ModelAdmin): search_fields = ('user__username', 'source_usage_key', 'content__display_name') readonly_fields = ('source_context_key', 'get_source_context_title') + @admin.display(description='Content') def content_link(self, obj): """ Display the StagedContent object as a link """ url = reverse('admin:content_staging_stagedcontent_change', args=[obj.content.pk]) return format_html('{}', url, obj.content) - content_link.short_description = 'Content' + @admin.display(description='Source Context Title') def get_source_context_title(self, obj): return obj.get_source_context_title() - get_source_context_title.short_description = 'Source Context Title' diff --git a/openedx/core/djangoapps/content_staging/api.py b/openedx/core/djangoapps/content_staging/api.py index 87616b706dd6..ddb1681c289e 100644 --- a/openedx/core/djangoapps/content_staging/api.py +++ b/openedx/core/djangoapps/content_staging/api.py @@ -44,7 +44,7 @@ def get_user_clipboard(user_id: int, only_ready: bool = True) -> UserClipboardDa ) -def get_user_clipboard_json(user_id: int, request: HttpRequest = None): +def get_user_clipboard_json(user_id: int, request: HttpRequest | None = None): """ Get the detailed status of the user's clipboard, in exactly the same format as returned from the @@ -88,7 +88,7 @@ def get_staged_content_static_files(staged_content_id: int) -> list[StagedConten sc = _StagedContent.objects.get(pk=staged_content_id) def str_to_key(source_key_str: str): - if not str: + if not source_key_str: return None try: return AssetKey.from_string(source_key_str) diff --git a/openedx/core/djangoapps/content_staging/models.py b/openedx/core/djangoapps/content_staging/models.py index d1a790e3b2f7..c3db6148c68c 100644 --- a/openedx/core/djangoapps/content_staging/models.py +++ b/openedx/core/djangoapps/content_staging/models.py @@ -1,6 +1,7 @@ """ Models for content staging (and clipboard) """ +from __future__ import annotations import logging from django.contrib.auth import get_user_model diff --git a/openedx/core/djangoapps/content_staging/views.py b/openedx/core/djangoapps/content_staging/views.py index 25219536b592..51501968dcc8 100644 --- a/openedx/core/djangoapps/content_staging/views.py +++ b/openedx/core/djangoapps/content_staging/views.py @@ -189,7 +189,7 @@ def _save_static_assets_to_clipboard( content: bytes | None = f.data md5_hash = "" # Unknown if content: - md5_hash = hashlib.md5(f.data).hexdigest() + md5_hash = hashlib.md5(content).hexdigest() # This asset came from the XBlock's filesystem, e.g. a video block's transcript file source_key = usage_key # Check if the asset file exists. It can be absent if an XBlock contains an invalid link. diff --git a/openedx/core/djangoapps/xblock/apps.py b/openedx/core/djangoapps/xblock/apps.py index 27add726ef1d..afebd2ec71c1 100644 --- a/openedx/core/djangoapps/xblock/apps.py +++ b/openedx/core/djangoapps/xblock/apps.py @@ -1,12 +1,11 @@ """ Django app configuration for the XBlock Runtime django app """ - - from django.apps import AppConfig, apps from django.conf import settings from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers +from .data import StudentDataMode class XBlockAppConfig(AppConfig): @@ -53,7 +52,7 @@ def get_runtime_system_params(self): editing XBlock content in the LMS """ return dict( - student_data_mode='persisted', + student_data_mode=StudentDataMode.Persisted, ) def get_site_root_url(self): @@ -77,7 +76,7 @@ def get_runtime_system_params(self): editing XBlock content in Studio """ return dict( - student_data_mode='ephemeral', + student_data_mode=StudentDataMode.Ephemeral, ) def get_site_root_url(self): diff --git a/openedx/core/djangoapps/xblock/data.py b/openedx/core/djangoapps/xblock/data.py new file mode 100644 index 000000000000..1da850caed19 --- /dev/null +++ b/openedx/core/djangoapps/xblock/data.py @@ -0,0 +1,13 @@ +""" +Data structures for the XBlock Django app's python APIs +""" +from enum import Enum + + +class StudentDataMode(Enum): + """ + Is student data (like which answer was selected) persisted in the DB or just stored temporarily in the session? + Generally, the LMS uses persistence and Studio uses ephemeral data. + """ + Ephemeral = 'ephemeral' + Persisted = 'persisted' diff --git a/openedx/core/djangoapps/xblock/runtime/runtime.py b/openedx/core/djangoapps/xblock/runtime/runtime.py index c1599edaff66..f0a81c6e5fa8 100644 --- a/openedx/core/djangoapps/xblock/runtime/runtime.py +++ b/openedx/core/djangoapps/xblock/runtime/runtime.py @@ -1,8 +1,9 @@ """ Common base classes for all new XBlock runtimes. """ - +from __future__ import annotations import logging +from typing import Callable from urllib.parse import urljoin # pylint: disable=import-error import crum @@ -13,12 +14,13 @@ from django.contrib.auth import get_user_model from django.core.cache import cache from django.core.exceptions import PermissionDenied -from functools import lru_cache # lint-amnesty, pylint: disable=wrong-import-order from eventtracking import tracker +from opaque_keys.edx.keys import UsageKey, LearningContextKey from web_fragments.fragment import Fragment +from xblock.core import XBlock from xblock.exceptions import NoSuchServiceError -from xblock.field_data import SplitFieldData -from xblock.fields import Scope +from xblock.field_data import FieldData, SplitFieldData +from xblock.fields import Scope, ScopeIds from xblock.runtime import KvsFieldData, MemoryIdManager, Runtime from xmodule.errortracker import make_error_tracker @@ -33,7 +35,9 @@ from common.djangoapps.xblock_django.user_service import DjangoXBlockUserService from lms.djangoapps.courseware.model_data import DjangoKeyValueStore, FieldDataCache from lms.djangoapps.grades.api import signals as grades_signals +from openedx.core.types import User as UserType from openedx.core.djangoapps.xblock.apps import get_xblock_app_config +from openedx.core.djangoapps.xblock.data import StudentDataMode from openedx.core.djangoapps.xblock.runtime.blockstore_field_data import BlockstoreChildrenData, BlockstoreFieldData from openedx.core.djangoapps.xblock.runtime.ephemeral_field_data import EphemeralKeyValueStore from openedx.core.djangoapps.xblock.runtime.mixin import LmsBlockMixin @@ -79,7 +83,16 @@ class XBlockRuntime(RuntimeShim, Runtime): # This runtime can save state for users who aren't logged in: suppports_state_for_anonymous_users = True - def __init__(self, system, user): + # Instance variables: + + user: UserType | None + user_id: int | str | None + # dict of FieldData stores for our loaded XBlocks. Key is the block's scope_ids. + block_field_datas: dict[ScopeIds, FieldData | None] + # dict of FieldDataCache objects for XBlock with database-based user state + django_field_data_caches: dict[LearningContextKey, FieldDataCache] + + def __init__(self, system: XBlockRuntimeSystem, user: UserType | None): super().__init__( id_reader=system.id_reader, mixins=( @@ -99,17 +112,17 @@ def __init__(self, system, user): self.user_id = get_xblock_id_for_anonymous_user(user) else: self.user_id = self.user.id - self.block_field_datas = {} # dict of FieldData stores for our loaded XBlocks. Key is the block's scope_ids. - self.django_field_data_caches = {} # dict of FieldDataCache objects for XBlock with database-based user state + self.block_field_datas = {} + self.django_field_data_caches = {} - def handler_url(self, block, handler_name, suffix='', query='', thirdparty=False): + def handler_url(self, block, handler_name: str, suffix='', query='', thirdparty=False): """ Get the URL to a specific handler. """ if thirdparty: log.warning("thirdparty handlers are not supported by this runtime for XBlock %s.", type(block)) - url = self.system.handler_url(usage_key=block.scope_ids.usage_id, handler_name=handler_name, user=self.user) + url = self.system.handler_url(block.scope_ids.usage_id, handler_name, self.user) if suffix: if not url.endswith('/'): url += '/' @@ -122,7 +135,7 @@ def handler_url(self, block, handler_name, suffix='', query='', thirdparty=False def resource_url(self, resource): raise NotImplementedError("resource_url is not supported by Open edX.") - def local_resource_url(self, block, uri): + def local_resource_url(self, block: XBlock, uri: str) -> str: """ Get the absolute URL to a resource file (like a CSS/JS file or an image) that is part of an XBlock's python module. @@ -132,7 +145,7 @@ def local_resource_url(self, block, uri): absolute_url = urljoin(site_root_url, relative_url) return absolute_url - def publish(self, block, event_type, event_data): + def publish(self, block: XBlock, event_type: str, event_data: dict): """ Handle XBlock events like grades and completion """ special_handler = self.get_event_handler(event_type) if special_handler: @@ -140,7 +153,7 @@ def publish(self, block, event_type, event_data): else: self.log_event_to_tracking_log(block, event_type, event_data) - def get_event_handler(self, event_type): + def get_event_handler(self, event_type: str) -> Callable[[XBlock, dict], None] | None: """ Return an appropriate function to handle the event. @@ -156,7 +169,7 @@ def get_event_handler(self, event_type): return self.handle_completion_event return None - def log_event_to_tracking_log(self, block, event_type, event_data): + def log_event_to_tracking_log(self, block: XBlock, event_type: str, event_data: dict) -> None: """ Log this XBlock event to the tracking log """ @@ -168,11 +181,11 @@ def log_event_to_tracking_log(self, block, event_type, event_data): with tracker.get_tracker().context(event_type, log_context): track_function(event_type, event_data) - def handle_grade_event(self, block, event): + def handle_grade_event(self, block: XBlock, event: dict): """ Submit a grade for the block. """ - if not self.user.is_anonymous: + if self.user and not self.user.is_anonymous: grades_signals.SCORE_PUBLISHED.send( sender=None, block=block, @@ -184,7 +197,7 @@ def handle_grade_event(self, block, event): grader_response=event.get('grader_response') ) - def handle_completion_event(self, block, event): + def handle_completion_event(self, block: XBlock, event: dict): """ Submit a completion object for the block. """ @@ -196,7 +209,7 @@ def handle_completion_event(self, block, event): completion=event['completion'], ) - def applicable_aside_types(self, block): + def applicable_aside_types(self, block: XBlock): """ Disable XBlock asides in this runtime """ return [] @@ -211,7 +224,7 @@ def add_node_as_child(self, block, node, id_generator=None): # Deny access to the inherited method raise NotImplementedError("XML Serialization is only supported with BlockstoreXBlockRuntime") - def service(self, block, service_name): + def service(self, block: XBlock, service_name: str): """ Return a service, or None. Services are objects implementing arbitrary other interfaces. @@ -235,6 +248,8 @@ def service(self, block, service_name): elif service_name == "completion": return CompletionService(user=self.user, context_key=context_key) elif service_name == "user": + if self.user is None: + raise RuntimeError("Cannot access user service when there is no user bound to the XBlock.") if self.user.is_anonymous: deprecated_anonymous_student_id = self.user_id else: @@ -244,13 +259,13 @@ def service(self, block, service_name): self.user, # The value should be updated to whether the user is staff in the context when Blockstore runtime adds # support for courses. - user_is_staff=self.user.is_staff, + user_is_staff=self.user.is_staff, # type: ignore anonymous_user_id=self.anonymous_student_id, # See the docstring of `DjangoXBlockUserService`. deprecated_anonymous_user_id=deprecated_anonymous_student_id ) elif service_name == "mako": - if self.system.student_data_mode == XBlockRuntimeSystem.STUDENT_DATA_EPHEMERAL: + if self.system.student_data_mode == StudentDataMode.Ephemeral: return MakoService(namespace_prefix='lms.') return MakoService() elif service_name == "i18n": @@ -283,7 +298,7 @@ def service(self, block, service_name): service = super().service(block, service_name) return service - def _init_field_data_for_block(self, block): + def _init_field_data_for_block(self, block: XBlock) -> FieldData: """ Initialize the FieldData implementation for the specified XBlock """ @@ -292,10 +307,10 @@ def _init_field_data_for_block(self, block): student_data_store = None elif self.user.is_anonymous: # This is an anonymous (non-registered) user: - assert self.user_id.startswith("anon") + assert isinstance(self.user_id, str) and self.user_id.startswith("anon") kvs = EphemeralKeyValueStore() student_data_store = KvsFieldData(kvs) - elif self.system.student_data_mode == XBlockRuntimeSystem.STUDENT_DATA_EPHEMERAL: + elif self.system.student_data_mode == StudentDataMode.Ephemeral: # We're in an environment like Studio where we want to let the # author test blocks out but not permanently save their state. kvs = EphemeralKeyValueStore() @@ -324,7 +339,7 @@ def _init_field_data_for_block(self, block): Scope.preferences: student_data_store, }) - def render(self, block, view_name, context=None): + def render(self, block: XBlock, view_name: str, context: dict | None = None): """ Render a specific view of an XBlock. """ @@ -364,7 +379,7 @@ def render(self, block, view_name, context=None): return fragment - def _lookup_asset_url(self, block, asset_path): # pylint: disable=unused-argument + def _lookup_asset_url(self, block: XBlock, asset_path: str): # pylint: disable=unused-argument """ Return an absolute URL for the specified static asset file that may belong to this XBlock. @@ -387,14 +402,11 @@ class XBlockRuntimeSystem: class can be used with many different XBlocks, whereas each XBlock gets its own instance of XBlockRuntime. """ - STUDENT_DATA_EPHEMERAL = 'ephemeral' - STUDENT_DATA_PERSISTED = 'persisted' - def __init__( self, - handler_url, # type: Callable[[UsageKey, str, Union[int, ANONYMOUS_USER]], str] - student_data_mode, # type: Union[STUDENT_DATA_EPHEMERAL, STUDENT_DATA_PERSISTED] - runtime_class, # type: XBlockRuntime + handler_url: Callable[[UsageKey, str, UserType | None], str], + student_data_mode: StudentDataMode, + runtime_class: type[XBlockRuntime], ): """ args: @@ -403,8 +415,8 @@ def __init__( handler_url( usage_key: UsageKey, handler_name: str, - user_id: Union[int, str], - ) + user: User | AnonymousUser | None + ) -> str student_data_mode: Specifies whether student data should be kept in a temporary in-memory store (e.g. Studio) or persisted forever in the database. @@ -416,18 +428,17 @@ def __init__( self.runtime_class = runtime_class self.authored_data_store = BlockstoreFieldData() self.children_data_store = BlockstoreChildrenData(self.authored_data_store) - assert student_data_mode in (self.STUDENT_DATA_EPHEMERAL, self.STUDENT_DATA_PERSISTED) + assert student_data_mode in (StudentDataMode.Ephemeral, StudentDataMode.Persisted) self.student_data_mode = student_data_mode - self._error_trackers = {} - def get_runtime(self, user): + def get_runtime(self, user: UserType | None) -> XBlockRuntime: """ Get the XBlock runtime for the specified Django user. The user can be a regular user, an AnonymousUser, or None. """ return self.runtime_class(self, user) - def get_service(self, block, service_name): + def get_service(self, block, service_name: str): """ Get a runtime service @@ -436,14 +447,5 @@ def get_service(self, block, service_name): XBlockRuntime. """ if service_name == 'error_tracker': - return self.get_error_tracker_for_context(block.scope_ids.usage_id.context_key) + return make_error_tracker() return None # None means see if XBlockRuntime offers this service - - @lru_cache(maxsize=32) - def get_error_tracker_for_context(self, context_key): # pylint: disable=unused-argument - """ - Get an error tracker for the specified context. - lru_cache makes this error tracker long-lived, for - up to 32 contexts that have most recently been used. - """ - return make_error_tracker() diff --git a/openedx/core/types/user.py b/openedx/core/types/user.py index 84f26a0aee3f..95b1fec607fc 100644 --- a/openedx/core/types/user.py +++ b/openedx/core/types/user.py @@ -1,7 +1,8 @@ """ Typing utilities for the User models. """ +from typing import Union import django.contrib.auth.models -User = django.contrib.auth.models.User +User = Union[django.contrib.auth.models.User, django.contrib.auth.models.AnonymousUser] diff --git a/requirements/constraints.txt b/requirements/constraints.txt index c96ddb4ba00a..5f37f95298ae 100644 --- a/requirements/constraints.txt +++ b/requirements/constraints.txt @@ -113,3 +113,8 @@ djangorestframework<3.15.0 # tests failing with greater version. Fix this in separate ticket. pillow<10.0.0 + +# The version of django-stubs we can use depends on which Django release we're using +# 1.16.0 works with Django 3.2 through 4.1 +django-stubs==1.16.0 +djangorestframework-stubs==3.14.0 # Pinned to match django-stubs. Remove this when we can remove the above pin. diff --git a/requirements/edx/development.in b/requirements/edx/development.in index ffdc299eddd9..5909f60268ff 100644 --- a/requirements/edx/development.in +++ b/requirements/edx/development.in @@ -16,6 +16,8 @@ click # Used for perf_tests utilities in modulestore django-debug-toolbar # A set of panels that display debug information about the current request/response +django-stubs # Typing stubs for Django, so it works with mypy +djangorestframework-stubs # Typing stubs for DRF mypy # static type checking pywatchman # More efficient checking for runserver reload trigger events vulture # Detects possible dead/unused code, used in scripts/find-dead-code.sh diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 6b6183120e96..f2f956401e44 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -358,6 +358,8 @@ django==3.2.20 # django-splash # django-statici18n # django-storages + # django-stubs + # django-stubs-ext # django-user-tasks # djangorestframework # drf-jwt @@ -576,6 +578,13 @@ django-storages==1.9.1 # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt # edxval +django-stubs==1.16.0 + # via + # -c requirements/edx/../constraints.txt + # -r requirements/edx/development.in + # djangorestframework-stubs +django-stubs-ext==4.2.2 + # via django-stubs django-user-tasks==3.0.0 # via # -r requirements/edx/doc.txt @@ -618,6 +627,10 @@ djangorestframework==3.14.0 # openedx-blockstore # ora2 # super-csv +djangorestframework-stubs==3.14.0 + # via + # -c requirements/edx/../constraints.txt + # -r requirements/edx/development.in djangorestframework-xml==2.0.0 # via # -r requirements/edx/doc.txt @@ -1226,7 +1239,10 @@ multidict==6.0.4 # aiohttp # yarl mypy==1.4.1 - # via -r requirements/edx/development.in + # via + # -r requirements/edx/development.in + # django-stubs + # djangorestframework-stubs mypy-extensions==1.0.0 # via mypy mysqlclient==2.2.0 @@ -1721,6 +1737,7 @@ requests==2.31.0 # analytics-python # coreapi # django-oauth-toolkit + # djangorestframework-stubs # edx-bulk-grades # edx-drf-extensions # edx-enterprise @@ -2012,6 +2029,7 @@ tomli==2.0.1 # -r requirements/edx/testing.txt # build # coverage + # django-stubs # import-linter # mypy # pip-tools @@ -2043,6 +2061,9 @@ typing-extensions==4.7.1 # asgiref # astroid # django-countries + # django-stubs + # django-stubs-ext + # djangorestframework-stubs # faker # fastapi # grimp