[FC-0009] Paste Components (OLX) into any Unit in Studio #31969

bradenmacdonald · 2023-03-20T18:03:20Z

Description

This continues the work from #31904 and implements ticket openedx/modular-learning#12

Affects: course authors [for now, only those who will be participating in the beta test]

Screenshot of the "Paste Component" button:

Demo video: coming

Supporting information

See tickets linked above.

Testing instructions

Go to (studio)/admin/waffle/flag/ and enable contentstore.enable_copy_paste_feature for Everyone, if you haven't already.
Open two browser tabs so you can see both at the same time. Ideally, they're in different courses.
Use the new Actions menu to select Copy. You should see an animation and the "Paste Component" button should instantly appear in any open browser tabs.
Try pasting into the same course and different course.
Refresh the page and ensure the "Paste Component" button still appears
Try various combinations of copying and pasting
- e.g. try copy, then delete the source block, then paste. It should still work!
- e.g. try within the same course, multiple pastes into the same unit (should get unique IDs), into different courses, etc.

Note: handling of static assets, intra-course links, pythonlib.zip, content groups, and anything along those lines is out of scope for now. Pasting within a course should always work fine. Pasting between courses may have issues if the component uses one or more of these features.

Deadline

"None" if there's no rush, or provide a specific date or event (and reason) if there is one.

Other information

Private ref: MNG-3605

openedx-webhooks · 2023-03-20T18:03:27Z

Thanks for the pull request, @bradenmacdonald!

As a core committer in this repo, you can merge this once the pull request is approved per the core committer reviewer requirements and according to the agreement with your edX Champion.

bradenmacdonald · 2023-04-19T23:34:42Z

@Agrendalath @ormsbee This is ready for review.

cms/djangoapps/contentstore/views/helpers.py

bradenmacdonald · 2023-04-20T18:53:12Z

setup.cfg

I am now using the new api.py import linter for this new content_staging django app; this work was part of my motivation for building that.

Agrendalath

@bradenmacdonald, ~~when I check out your branch, I get the following error in Studio logs. Do I need to do anything specific to get it working?~~

Exception in thread django-main-thread: Traceback (most recent call last): File "/usr/lib/python3.8/threading.py", line 932, in _bootstrap_inner self.run() File "/usr/lib/python3.8/threading.py", line 870, in run self._target(*self._args, **self._kwargs) File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/utils/autoreload.py", line 64, in wrapper fn(*args, **kwargs) File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/core/management/commands/runserver.py", line 118, in inner_run self.check(display_num_errors=True) File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/core/management/base.py", line 419, in check all_issues = checks.run_checks( File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/core/checks/registry.py", line 76, in run_checks new_errors = check(app_configs=app_configs, databases=databases) File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/core/checks/urls.py", line 13, in check_url_config return check_resolver(resolver) File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/core/checks/urls.py", line 23, in check_resolver return check_method() File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/urls/resolvers.py", line 416, in check for pattern in self.url_patterns: File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/utils/functional.py", line 48, in __get__ res = instance.__dict__[self.name] = self.func(instance) File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/urls/resolvers.py", line 602, in url_patterns patterns = getattr(self.urlconf_module, "urlpatterns", self.urlconf_module) File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/utils/functional.py", line 48, in __get__ res = instance.__dict__[self.name] = self.func(instance) File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/urls/resolvers.py", line 595, in urlconf_module return import_module(self.urlconf_name) File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/importlib/__init__.py", line 127, in import_module return _bootstrap._gcd_import(name[level:], package, level) File "<frozen importlib._bootstrap>", line 1014, in _gcd_import File "<frozen importlib._bootstrap>", line 991, in _find_and_load File "<frozen importlib._bootstrap>", line 975, in _find_and_load_unlocked File "<frozen importlib._bootstrap>", line 671, in _load_unlocked File "<frozen importlib._bootstrap_external>", line 848, in exec_module File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed File "/edx/app/edxapp/edx-platform/cms/urls.py", line 19, in <module> from cms.djangoapps.contentstore import views as contentstore_views File "/edx/app/edxapp/edx-platform/cms/djangoapps/contentstore/views/__init__.py", line 5, in <module> from .component import * File "/edx/app/edxapp/edx-platform/cms/djangoapps/contentstore/views/component.py", line 32, in <module> import openedx.core.djangoapps.content_staging.api as content_staging_api File "/edx/app/edxapp/edx-platform/openedx/core/djangoapps/content_staging/api.py", line 11, in <module> from .models import UserClipboard as _UserClipboard, StagedContent as _StagedContent File "/edx/app/edxapp/edx-platform/openedx/core/djangoapps/content_staging/models.py", line 20, in <module> class StagedContent(models.Model): File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/db/models/base.py", line 113, in __new__ raise RuntimeError( RuntimeError: Model class openedx.core.djangoapps.content_staging.models.StagedContent doesn't declare an explicit app_label and isn't in an application in INSTALLED_APPS.
Edit: nevermind, running `make studio-static` fixed this.

cms/djangoapps/contentstore/views/component.py

cms/djangoapps/contentstore/views/helpers.py

Agrendalath · 2023-04-24T14:05:05Z

cms/djangoapps/contentstore/views/tests/test_clipboard_paste.py

Nit: did you intentionally decide not to use def setUp(self): (to set self.course_key + self.client there)? I see you're using self.assertEqual instead of a standard assert, so it's a bit odd mix of unittest.TestCase and pytest styles.

I copied this from the other TestCase, where only some of the tests need this setup, which is why it's not in a setUp method. Here I could put it in a setUp method, if you think that's cleaner. Though if we add more tests here, some may not need the course.

As for mixing assert styles, is there a preferred style for the codebase now? I honestly don't know which I "should" be using.

I copied this from the other TestCase, where only some of the tests need this setup, which is why it's not in a setUp method. Here I could put it in a setUp method, if you think that's cleaner. Though if we add more tests here, some may not need the course.

No strong preference here. If you think we can have tests that won't need the course setup, then we can keep it as is.

As for mixing assert styles, is there a preferred style for the codebase now? I honestly don't know which I "should" be using.

I don't see any coding guideline for this, but there were 36 PRs like #26576 that replaced these asserts, so I believe that the pytest style is preferred.

OK, I converted this test suite to pytest style.

openedx/core/djangoapps/olx_rest_api/test_views.py

openedx/core/djangoapps/content_staging/api.py

cms/djangoapps/contentstore/views/helpers.py

Agrendalath

👍

I tested this: verified that pasting works in the devstack
I read through the code
I checked for accessibility issues: n/a
Includes documentation: n/a
I made sure any change in configuration variables is reflected in the corresponding client's configuration-secure repository: n/a

cms/lib/xblock/authoring_mixin.py

…ssues

ormsbee · 2023-04-26T14:59:38Z

Looking through this more now...

ormsbee

Looks great! Just a few minor requests.

ormsbee · 2023-04-26T15:01:23Z

openedx/core/djangoapps/content_staging/api.py

+class StagedContentData(NamedTuple):
+    """ Read-only data model for StagedContent """
+    id: int
+    user_id: int
+    created: datetime
+    purpose: str
+    status: StagedContentStatus
+    block_type: str
+    display_name: str
+
+
+class UserClipboardData(NamedTuple):
+    """ Read-only data model for StagedContent """
+    content: StagedContentData
+    source_usage_key: UsageKey


Per OEP-49, please make these attr classes in data.py unless you have a strong reason for preferring named tuples (and put comments explaining that decision).

I like NamedTuples since they're built-in and have all the functionality we need in this case, but it's no problem at all - I'll change this to use attrs and live in data.py for consistency.

I've moved them to data.py though I found some issues with the OEP-49 recommendation regarding enums - openedx/openedx-proposals#471 - so I'm sticking with Django enums for now.

ormsbee · 2023-04-26T15:08:33Z

openedx/core/djangoapps/content_staging/api.py

+from opaque_keys.edx.keys import UsageKey
+
+from .models import UserClipboard as _UserClipboard, StagedContent as _StagedContent
+from .serializers import UserClipboardSerializer as _UserClipboardSerializer


Interesting way to keep people from importing these from the wrong place. I'm not sure if I prefer it to importing the module and doing models.UserClipboard, but I'm fine with it, fwiw.

I actually thought this is what OEP-49 recommends? "When importing internal app code to be used in the api.py file, prefix it with an underscore so it’s clear it’s for internal use only." Or are you saying the convention is to use from . import models as _models?

You're right, it's there. I must have just forgotten about that part, because I haven't been writing my code like that. 😛 I'll try to do so going forward.

I actually meant:

from . import models

So that all references are to models.UserClipboard. Granted, it doesn't mean people can't re-import it, but it's less likely.

ormsbee · 2023-04-26T15:11:45Z

cms/djangoapps/contentstore/views/helpers.py

+    node = etree.fromstring(olx_str)
+    store = modulestore()
+    with store.bulk_operations(parent_key.course_key):
+        parent_descriptor = store.get_item(parent_key)


A very big descriptor -> block renaming refactor is dropping shortly. Please make check that your naming aligns:

[BD-13][BB-6927] Unify xblock naming descriptor #31492

Hmm, not sure. What I found was that this thing is not a proper XBlock, and it doesn't have a full XBlock runtime, just a CachingDescriptorSystem. So that's why I've called it a descriptor here, and then the line below uses _load_preview_block to convert it from a descriptor to a proper XBlock. I had thought we didn't have to worry about those things anymore but I guess we still do.

So in this case I think the language is clear and necessary but I am not sure it aligns with the changes in that PR. @Agrendalath can you advise?

@ormsbee, @bradenmacdonald, we still use the "descriptor" term in some places. In this case, we are retrieving an "unbound" XBlock from the Modulestore. _load_preview_block handles adding the services and binding student data. Therefore, it is fair to call it a "descriptor", as the only goal of this variable is to pass it to a function that initializes the full runtime.

Great, thanks for the quick reply. That aligns with my understanding here :)

openedx-webhooks · 2023-04-27T16:58:08Z

@bradenmacdonald 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

edx-pipeline-bot · 2023-04-27T18:06:52Z

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

edx-pipeline-bot · 2023-04-27T18:29:26Z

EdX Release Notice: This PR has been deployed to the production environment.

openedx-webhooks added open-source-contribution PR author is not from Axim or 2U core committer labels Mar 20, 2023

bradenmacdonald force-pushed the paste-olx branch 2 times, most recently from c1505b2 to 4aa324a Compare April 19, 2023 20:45

bradenmacdonald commented Apr 19, 2023

View reviewed changes

cms/djangoapps/contentstore/views/helpers.py Outdated Show resolved Hide resolved

bradenmacdonald force-pushed the paste-olx branch from 8415cd9 to fa6e990 Compare April 20, 2023 18:39

bradenmacdonald commented Apr 20, 2023

View reviewed changes

Agrendalath reviewed Apr 24, 2023

View reviewed changes

cms/djangoapps/contentstore/views/helpers.py Outdated Show resolved Hide resolved

Agrendalath reviewed Apr 24, 2023

View reviewed changes

cms/djangoapps/contentstore/views/helpers.py Outdated Show resolved Hide resolved

bradenmacdonald added 8 commits April 24, 2023 12:17

feat: Implement paste button

805a2a8

chore: improve docs and add tests for python API

6e11e05

fix: drive-by fix to use a better API for comparing XML

79dd95d

feat: track which XBlock something was copied from

0bccadf

feat: add tests

c85e189

feat: enable import linter so content_staging's public API is respected

fe539fe

fix: error seen when trying to paste drag-and-drop-v2 blocks

8ce5ea8

fix: use strip_text=True consistently for XML comparisons

2a9350c

bradenmacdonald force-pushed the paste-olx branch from fa6e990 to 2a9350c Compare April 24, 2023 20:33

refactor: rename get_user_clipboard_status to get_user_clipboard

a72fdd0

Agrendalath approved these changes Apr 25, 2023

View reviewed changes

ormsbee reviewed Apr 25, 2023

View reviewed changes

cms/lib/xblock/authoring_mixin.py Show resolved Hide resolved

bradenmacdonald added 6 commits April 25, 2023 10:44

feat: Better error reporting when pasting in Studio

db4eb97

chore: convert new test suite to pytest assertions

9f0dded

refactor: push READY status check into the API per review suggestion

1bf9f06

fix: use strip_text=True consistently for XML comparisons

4dd11cd

fix: store "copied_from_block" as a string to avoid Reference field i…

2bba746

…ssues

fix: minor lint error

d5429b1

bradenmacdonald mentioned this pull request Apr 25, 2023

[FC-0009] Show a preview of what will be pasted with new Paste Component Button #32132

Merged

2 tasks

ormsbee requested changes Apr 26, 2023

View reviewed changes

bradenmacdonald added 2 commits April 26, 2023 14:57

refactor: move data types to data.py per OEP-49

5e06a2c

chore: Update with latest master

c4129ce

bradenmacdonald force-pushed the paste-olx branch from d04ba9b to c4129ce Compare April 27, 2023 00:00

ormsbee approved these changes Apr 27, 2023

View reviewed changes

chore: Update with latest master

aaee378

bradenmacdonald merged commit 8ee1f66 into openedx:master Apr 27, 2023

bradenmacdonald deleted the paste-olx branch April 27, 2023 16:58

bradenmacdonald mentioned this pull request May 2, 2023

Create mixin XBlock field to track the usage key of the original Module it was copied from. openedx/modular-learning#17

Closed

kenclary mentioned this pull request Jun 29, 2023

feat: basic get/post endpoint for v2 xblocks. TNL-10873 #32600

Merged

kenclary mentioned this pull request Aug 4, 2023

fix: fix mako runtime bug with studio_view. TNL-10912 #32912

Merged

[FC-0009] Paste Components (OLX) into any Unit in Studio #31969

[FC-0009] Paste Components (OLX) into any Unit in Studio #31969

Uh oh!

Conversation

bradenmacdonald commented Mar 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Supporting information

Testing instructions

Deadline

Other information

Uh oh!

openedx-webhooks commented Mar 20, 2023

Uh oh!

bradenmacdonald commented Apr 19, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Agrendalath left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Agrendalath left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ormsbee commented Apr 26, 2023

Uh oh!

ormsbee left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bradenmacdonald Apr 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ormsbee Apr 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ormsbee Apr 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openedx-webhooks commented Apr 27, 2023

Uh oh!

edx-pipeline-bot commented Apr 27, 2023

Uh oh!

edx-pipeline-bot commented Apr 27, 2023

Uh oh!

Reviewers

bradenmacdonald commented Mar 20, 2023 •

edited

Loading

Agrendalath left a comment •

edited

Loading

bradenmacdonald Apr 26, 2023 •

edited

Loading

ormsbee Apr 26, 2023 •

edited

Loading

ormsbee Apr 26, 2023 •

edited

Loading