Skip to content

fix: Added xblock skill tag verification waffle switch #33399

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
irfanuddinahmad merged 1 commit into from
Oct 4, 2023
Merged

fix: Added xblock skill tag verification waffle switch #33399

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
14 changes: 10 additions & 4 deletions xmodule/tests/test_vertical.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,24 +5,26 @@
# pylint: disable=protected-access


import json
from collections import namedtuple
from datetime import datetime, timedelta
import json
from unittest.mock import Mock, patch

import pytz
import ddt
from fs.memoryfs import MemoryFS
import pytz
from django.contrib.auth.models import AnonymousUser
from django.test import override_settings
from edx_toggles.toggles.testutils import override_waffle_switch
from fs.memoryfs import MemoryFS
from openedx_filters import PipelineStep
from openedx_filters.learning.filters import VerticalBlockChildRenderStarted, VerticalBlockRenderCompleted

from ..vertical_block import XBLOCK_SKILL_TAG_VERIFICATION_SWITCH
from ..x_module import AUTHOR_VIEW, PUBLIC_VIEW, STUDENT_VIEW
from . import prepare_block_runtime
from .helpers import StubUserService
from .xml import XModuleXmlImportTest
from .xml import factories as xml
from ..x_module import STUDENT_VIEW, AUTHOR_VIEW, PUBLIC_VIEW

COMPLETION_DELAY = 9876

Expand Down Expand Up @@ -360,6 +362,7 @@ def test_render_studio_view(self):
},
},
)
@override_waffle_switch(XBLOCK_SKILL_TAG_VERIFICATION_SWITCH, True)
def test_vertical_block_child_render_started_filter_execution(self):
"""
Test the VerticalBlockChildRenderStarted filter's effects on student view.
Expand All @@ -382,6 +385,7 @@ def test_vertical_block_child_render_started_filter_execution(self):
},
},
)
@override_waffle_switch(XBLOCK_SKILL_TAG_VERIFICATION_SWITCH, True)
def test_vertical_block_child_render_is_skipped_on_filter_exception(self):
"""
Test VerticalBlockChildRenderStarted filter can be used to skip child blocks.
Expand All @@ -405,6 +409,7 @@ def test_vertical_block_child_render_is_skipped_on_filter_exception(self):
},
},
)
@override_waffle_switch(XBLOCK_SKILL_TAG_VERIFICATION_SWITCH, True)
def test_vertical_block_render_completed_filter_execution(self):
"""
Test the VerticalBlockRenderCompleted filter's execution.
Expand All @@ -427,6 +432,7 @@ def test_vertical_block_render_completed_filter_execution(self):
},
},
)
@override_waffle_switch(XBLOCK_SKILL_TAG_VERIFICATION_SWITCH, True)
def test_vertical_block_render_output_is_changed_on_filter_exception(self):
"""
Test VerticalBlockRenderCompleted filter can be used to prevent vertical block from rendering.
Expand Down
56 changes: 35 additions & 21 deletions xmodule/vertical_block.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,17 +9,19 @@
from functools import reduce

import pytz
from edx_toggles.toggles import WaffleSwitch
from lxml import etree
from openedx_filters.learning.filters import VerticalBlockChildRenderStarted, VerticalBlockRenderCompleted
from web_fragments.fragment import Fragment
from xblock.core import XBlock # lint-amnesty, pylint: disable=wrong-import-order
from xblock.fields import Boolean, Scope
from openedx_filters.learning.filters import VerticalBlockChildRenderStarted, VerticalBlockRenderCompleted

from xmodule.mako_block import MakoTemplateBlockBase
from xmodule.progress import Progress
from xmodule.seq_block import SequenceFields
from xmodule.studio_editable import StudioEditableBlock
from xmodule.util.misc import is_xblock_an_assignment
from xmodule.util.builtin_assets import add_webpack_js_to_fragment
from xmodule.util.misc import is_xblock_an_assignment
from xmodule.x_module import PUBLIC_VIEW, STUDENT_VIEW, XModuleFields
from xmodule.xml_block import XmlMixin

Expand All @@ -32,6 +34,17 @@
# HACK: This shouldn't be hard-coded to two types
# OBSOLETE: This obsoletes 'type'
CLASS_PRIORITY = ['video', 'problem']
WAFFLE_NAMESPACE = 'xblocks'
# .. toggle_name: xblocks.xblock_skill_tag_verification'
# .. toggle_implementation: WaffleSwitch
# .. toggle_default: False
# .. toggle_description: Set to True to get learner verification of the skills associated with the xblock.
# .. toggle_use_cases: open_edx
# .. toggle_creation_date: 2023-10-04
# .. toggle_target_removal_date: None
XBLOCK_SKILL_TAG_VERIFICATION_SWITCH = WaffleSwitch( # lint-amnesty, pylint: disable=toggle-missing-annotation
f'{WAFFLE_NAMESPACE}.xblock_skill_tag_verification', __name__
)


class VerticalFields:
Expand Down Expand Up @@ -116,16 +129,16 @@ def _student_or_public_view(self, context, view): # lint-amnesty, pylint: disab
child_block_context['wrap_xblock_data'] = {
'mark-completed-on-view-after-delay': complete_on_view_delay
}

try:
# .. filter_implemented_name: VerticalBlockChildRenderStarted
# .. filter_type: org.openedx.learning.vertical_block_child.render.started.v1
child, child_block_context = VerticalBlockChildRenderStarted.run_filter(
block=child, context=child_block_context
)
except VerticalBlockChildRenderStarted.PreventChildBlockRender as exc:
log.info("Skipping %s from vertical block. Reason: %s", child, exc.message)
continue
if XBLOCK_SKILL_TAG_VERIFICATION_SWITCH.is_enabled():

@ormsbee ormsbee Mar 7, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@irfanuddinahmad: Why is the execution of the VerticalBlockChildRenderStarted and VerticalBlockRenderCompleted filters now gated behind XBLOCK_SKILL_TAG_VERIFICATION_SWITCH? The filters are supposed to be a general extension mechanism for people to override unit rendering behavior, and is not specific to one feature.

@tecoholic: As the original author that added the filters, please let me know if I'm reading this wrong.

FYI @nsprenkle, since this is an extension point you may want to use.

@ormsbee ormsbee Mar 7, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also FYI to @mariajgrimaldi ^

@tecoholic tecoholic Mar 7, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@ormsbee Yes. VerticalBlockChildRenderStarted and VerticalBlockRenderCompleted are in fact meant to be generic filters. For eg., we use it in a course to automatically inject content like hyper-links to edit OLX source material stored in a Git repo like Gitlab/Github.

@ormsbee ormsbee Mar 8, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@irfanuddinahmad: Also, when you're making a change like this, please put the reason why this change was done in the commit message. I'm assuming this was in response to some bug that was found, but I can't access that JIRA ticket to find out what it is or why it broke.

@irfanuddinahmad irfanuddinahmad Mar 8, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@ormsbee ENT-7614 was in response to a downtime in Discovery and LMS due to delayed response originating from the XBlockSkillsAPI list endpoint that is responsible for gathering any skills linked to an xblock. This switch was added to quickly turn the feature on/off.

@ormsbee ormsbee Mar 8, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@irfanuddinahmad: Thank you for the quick response. I think I understand what happened now.

This needs to get reverted. If 2U needs to be able to toggle off a plugin to make things more operationally robust, then the toggle and the conditional code should go inside that plugin. We absolutely cannot have this switch disable the entire filter pipelines that the plugin uses, since that will break everyone else who uses these extension points. The fact that this regression is hidden because the tests for it were altered to run with that switch enabled is also not an acceptable solution, since most people won't be running their instances that way.

This change made it into the Quince release. Per the release schedule, the next patch to Quince will be released on 2024-04-09.

Please have these changes reverted in master and back-ported into quince.master by 2024-04-02.

I want to give you folks the time to make a smooth transition here, but this is a breaking bug in Quince, and we need to get it patched.

Please reach out to me if you have any questions or concerns.

FYI @georgebabey: I'm making a request of your team here ⬆️.

Thanks folks.

@irfanuddinahmad irfanuddinahmad Mar 11, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@ormsbee Here are the two PRs
#34348
#34347

try:
# .. filter_implemented_name: VerticalBlockChildRenderStarted
# .. filter_type: org.openedx.learning.vertical_block_child.render.started.v1
child, child_block_context = VerticalBlockChildRenderStarted.run_filter(
block=child, context=child_block_context
)
except VerticalBlockChildRenderStarted.PreventChildBlockRender as exc:
log.info("Skipping %s from vertical block. Reason: %s", child, exc.message)
continue

rendered_child = child.render(view, child_block_context)
fragment.add_fragment_resources(rendered_child)
Expand Down Expand Up @@ -167,15 +180,16 @@ def _student_or_public_view(self, context, view): # lint-amnesty, pylint: disab
add_webpack_js_to_fragment(fragment, 'VerticalStudentView')
fragment.initialize_js('VerticalStudentView')

try:
# .. filter_implemented_name: VerticalBlockRenderCompleted
# .. filter_type: org.openedx.learning.vertical_block.render.completed.v1
_, fragment, context, view = VerticalBlockRenderCompleted.run_filter(
block=self, fragment=fragment, context=context, view=view
)
except VerticalBlockRenderCompleted.PreventVerticalBlockRender as exc:
log.info("VerticalBlock rendering stopped. Reason: %s", exc.message)
fragment.content = exc.message
if XBLOCK_SKILL_TAG_VERIFICATION_SWITCH.is_enabled():
try:
# .. filter_implemented_name: VerticalBlockRenderCompleted
# .. filter_type: org.openedx.learning.vertical_block.render.completed.v1
_, fragment, context, view = VerticalBlockRenderCompleted.run_filter(
block=self, fragment=fragment, context=context, view=view
)
except VerticalBlockRenderCompleted.PreventVerticalBlockRender as exc:
log.info("VerticalBlock rendering stopped. Reason: %s", exc.message)
fragment.content = exc.message

return fragment

Expand Down