Skip to content
2 changes: 2 additions & 0 deletions cms/djangoapps/contentstore/views/preview.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
from xblock.runtime import KvsFieldData

from openedx.core.djangoapps.video_config.services import VideoConfigService
from openedx.core.djangoapps.discussions.services import DiscussionConfigService
from xmodule.contentstore.django import contentstore
from xmodule.exceptions import NotFoundError as XModuleNotFoundError
from xmodule.modulestore.django import XBlockI18nService, modulestore
Expand Down Expand Up @@ -217,6 +218,7 @@ def _prepare_runtime_for_preview(request, block):
"cache": CacheService(cache),
'replace_urls': ReplaceURLService,
'video_config': VideoConfigService(),
'discussion_config': DiscussionConfigService(),
Copy link
Member

Choose a reason for hiding this comment

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

Will this service be needed to edit courses/discussions in studio? If so, we'll need to add it to load_services_for_studio too. If not, then this is fine.

}

block.runtime.get_block_for_descriptor = partial(_load_preview_block, request)
Expand Down
2 changes: 2 additions & 0 deletions lms/djangoapps/courseware/block_render.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@

from lms.djangoapps.teams.services import TeamsService
from openedx.core.djangoapps.video_config.services import VideoConfigService
from openedx.core.djangoapps.discussions.services import DiscussionConfigService
from openedx.core.lib.xblock_services.call_to_action import CallToActionService
from xmodule.contentstore.django import contentstore
from xmodule.exceptions import NotFoundError as XModuleNotFoundError
Expand Down Expand Up @@ -637,6 +638,7 @@ def inner_get_block(block: XBlock) -> XBlock | None:
'publish': EventPublishingService(user, course_id, track_function),
'enrollments': EnrollmentsService(),
'video_config': VideoConfigService(),
'discussion_config': DiscussionConfigService(),
}

runtime.get_block_for_descriptor = inner_get_block
Expand Down
49 changes: 49 additions & 0 deletions openedx/core/djangoapps/discussions/services.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
"""
Discussion Configuration Service for XBlock runtime.

This service provides discussion-related configuration and feature flags
that are specific to the edx-platform implementation
for the extracted discussion block in xblocks-contrib repository.
"""

from edx_django_utils.cache import DEFAULT_REQUEST_CACHE
from opaque_keys.edx.keys import CourseKey

from django.conf import settings
from openedx.core.djangoapps.django_comment_common.models import (
all_permissions_for_user_in_course
)
from openedx.core.djangoapps.discussions.models import DiscussionsConfiguration, Provider


class DiscussionConfigService:
Copy link
Member

Choose a reason for hiding this comment

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

The method bodies look good. Could you add type annotations to all the args and return values? Check out the new VideoConfigService for an example.

"""
Service for providing video-related configuration and feature flags.
"""

def has_permission(self, user, permission, course_id=None): # lint-amnesty, pylint: disable=missing-function-docstring
Copy link
Contributor

Choose a reason for hiding this comment

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

user_has_permission
seems better

Copy link
Member

Choose a reason for hiding this comment

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

@kdmccormick - as discussed in the meeting I will follow up with Dave on whether this should be in the UserService or stay here

Copy link
Member

Choose a reason for hiding this comment

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

@salman2013 After reading the code more myself, I think your approach is great. The has_permission check is not a general check--it is particular to discussions permissions. All the values of permission that you can pass in are like vote_for_comment, delete_comment`, and so on. So, I think it makes sense to keep this method in the new DiscussionConfigService.

Just two requests:

  • Please clarify in the docstring that this method only applies to discussion permissions, not permissions in general.
  • Please move the definition of has_permission from lms/djangoapps/discussion/django_comment_client/permissions.py over to openedx/core/djangoapps/django_comment_common/models.py. This way, the DiscussionConfigService can use the original definition without having to import from the lms code tree, which as you mentioned was causing an importlinter failure.

FYI @feanil

assert isinstance(course_id, (type(None), CourseKey))
request_cache_dict = DEFAULT_REQUEST_CACHE.data
cache_key = "django_comment_client.permissions.has_permission.all_permissions.{}.{}".format(
user.id, course_id
)
if cache_key in request_cache_dict:
all_permissions = request_cache_dict[cache_key]
else:
all_permissions = all_permissions_for_user_in_course(user, course_id)
request_cache_dict[cache_key] = all_permissions

return permission in all_permissions

def is_discussion_visible(self, course_key):
"""
Discussion Xblock does not support new OPEN_EDX provider
"""
provider = DiscussionsConfiguration.get(course_key)
return provider.provider_type == Provider.LEGACY

def is_discussion_enabled(self):
"""
Return True if discussions are enabled; else False
"""
return settings.FEATURES.get('ENABLE_DISCUSSION_SERVICE')
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return settings.FEATURES.get('ENABLE_DISCUSSION_SERVICE')
return settings.ENABLE_DISCUSSION_SERVICE

FEATURES items can now be accessed directly on the settings object. This is preferrable because you will get an exception or linter warning if the setting is misspelled or later removed

3 changes: 3 additions & 0 deletions openedx/core/djangoapps/xblock/runtime/runtime.py
Original file line number Diff line number Diff line change
Expand Up @@ -347,6 +347,9 @@ def service(self, block: XBlock, service_name: str):
# Import here to avoid circular dependency
from openedx.core.djangoapps.video_config.services import VideoConfigService
return VideoConfigService()
elif service_name == 'discussion_config':
from openedx.core.djangoapps.discussions.services import DiscussionConfigService
return DiscussionConfigService()

# Otherwise, fall back to the base implementation which loads services
# defined in the constructor:
Expand Down
Loading