[FC-0009] New django app for copying and pasting OLX content in Studio by bradenmacdonald · Pull Request #31904 · openedx/openedx-platform

bradenmacdonald · 2023-03-10T01:50:18Z

Description

This implements openedx/modular-learning#51 and openedx/modular-learning#10

It is implemented as a more generic "staged content" area, where "staged content" is any content that isn't part of a particular course or library at the moment. Staged content is not editable; you have to paste it somewhere to edit it. We anticipate that "partial import" functionality may be another use case for staged content in addition to this clipboard functionality.

It's currently implemented as a django app plugin within edx-platform because it's highly coupled to platform internals and we want it tested as part of the test suite. However, if preferable we could move it to an external repo.

I am hoping to use #31903 to enforce that no other code imports the private API of this new app.

Supporting information

See GitHub issue linked above.

Analytics

When a user copies something to their clipboard, the CMS will log something like this:

2023-03-17 00:15:58,445 INFO 465 [openedx.core.djangoapps.content_staging.views] [user 4] [ip 172.18.0.1] views.py:142 - 
User 4 copied video component "block-v1:DemoX+D+EMO+type@video+block@56a60936c4c246ac861cfb4419cf235e" to their clipboard.

TODOs:

Add some test cases
Consolidate block_serializers to a single API - to do after, as Consolidate the OLX block_serializers APIs (backend/dev-only task) modular-learning#54
Use clipboard model and move source_context there.
Fix filename of downloaded OLX
Add logging for statistics/evaluation purposes

Testing instructions

Go to http://studio.local.overhang.io:8001/admin/waffle/flag/ (or devstack/sandbox equivalent)
Add a new flag, contentstore.enable_copy_paste_feature and enable it for Everyone
Go to a Unit page in Studio and use the new actions menu (see [FC-0009] New actions menu for editing components in Studio (behind waffle flag) #31853). The "Copy" action should now work.
You won't be able to paste, but you can go to http://studio.local.overhang.io:8001/api/content-staging/v1/clipboard/ to see the clipboard contents. That contains an olx_url that you can go to in turn to download the OLX as an .xml file.

How to test asynchronous copying

Copying is currently done synchronously, but the backend and frontend code do already support long-running asynchronous copy operations. To simulate this for testing:

Click to view instructions

First, make the following edit:

Then copy an XBlock. The copying process will continue until you retrieve the OLX. To retrieve the OLX, open the Network inspector and in one of the recent "clipboard/" responses, you'll see the URL - go to the highlighted URL:

You will see that the "Copying..." toast stays until you access that URL, which is how we signal the completion of our (faked) async operation.

Deadline

None

Other information

Private ref - MNG-3572

openedx-webhooks · 2023-03-10T01:50:26Z

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-03-10T01:50:56Z

@Agrendalath @ormsbee This isn't quite working yet but if you want to see how it's shaping up, feel free :)
Otherwise I'll let you know when it's all ready for review.

openedx/core/djangoapps/content_staging/models.py

bradenmacdonald · 2023-03-17T02:31:02Z

@Agrendalath @ormsbee This is ready for review now.

I would still like to consolidate the two similar XBlockSerializer implementations in olx_rest_api and this new content_staging, but I think I'll do that in a follow-up PR to make it easier to review. If you prefer I do that before merging, let me know.

Agrendalath

The Swagger is broken, but everything else works perfectly.

openedx/core/djangoapps/content_staging/views.py

openedx/core/djangoapps/content_staging/models.py

Agrendalath

👍

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

Agrendalath · 2023-03-20T15:27:22Z

openedx/core/djangoapps/content_staging/admin.py

+
+    def get_source_context_title(self, obj):
+        return obj.get_source_context_title()
+    get_source_context_title.short_description = 'Source Context Title'


Nit: should we mark this (and 'Content') as translatable?

I don't think it's worth translator's efforts to translate the Django admin, especially since the only use case I see here is for developers. I don't think operators would ever use this Django admin, and we may even remove it once the feature is stable.

Agrendalath · 2023-03-20T15:29:51Z

openedx/core/djangoapps/content_staging/admin.py

+    """ Admin config for UserClipboard """
+    list_display = ('user', 'content_link', 'source_usage_key', 'get_source_context_title')
+    search_fields = ('user__username', 'source_usage_key', 'content__display_name')
+    readonly_fields = ('source_context_key', 'get_source_context_title')


Nit: is it intended that User, Content, and Source usage key are editable in the admin detail view?

not really; I didn't worry too much about this. I can make everything read only before I merge this.

bradenmacdonald · 2023-03-28T14:51:41Z

@ormsbee Can you give this a final review?

ormsbee

Some immediate questions on deletion and longer term mulling over how much StagedContent has to understand.

ormsbee · 2023-04-04T04:57:25Z

openedx/core/djangoapps/content_staging/tasks.py

+
+
+@shared_task(base=LoggedTask)
+def delete_expired_staged_content():


We've had some painful operational experiences with select_for_update, to the point where I'm pretty skittish about code using it, even where skip_locked=True.

Do we really need this at the moment? The code already disconnects the StagedContent from the UserClipboard when there's another copy. Can't we spin off a task to delete that old entry at that time? (I guess we'd need to be careful about transactions to make sure there's no weird race condition, but still...)

It looks like the current philosophy of cleanup is to centralize it at the StagedContent model, and to cascade outwards to things that build on top of it like UserClipboard. But given the differing policies that might come up between things like UserClipboard and partial import, I think it might be easier to trigger the deletion from the other direction–i.e. "disconnect your extended thing (fast operation), queue task to delete StagedContent (potentially slow)"

OK, I'll refactor along those lines. Makes total sense.

@ormsbee OK, I've made that change. Can you give this another look?

ormsbee · 2023-04-04T05:03:05Z

openedx/core/djangoapps/content_staging/models.py

+    class Purpose(models.TextChoices):
+        """ The purpose of this staged content. """
+        CLIPBOARD = "clipboard", _("Clipboard")


Does StagedContent really need this? StagedContent is otherwise a lower-level model that UserClipboard makes use of. It seems odd for it to have this kind of knowledge.

I know, but otherwise it's hard to tell what the StagedContent was for. UserClipboard has a OneToOne relationship to User, which means that when a user copies something new, there is no "old" UserClipboard entry. So when a user copies something then copies something else, you end up with"stranded" StagedContent entries that aren't pointed to by UserClipboard and so without this field, there is no information about their purpose. It's impossible to tell from the database if they were used for clipboard or for some other thing (partial import, template, ... ?)

Now, if I make it so that UserClipboard always immediately deletes its old StagedContent rows, that would solve the problem, but that makes copying slower unless it's done asynchronously. And if it's done asynchronously, it might fail and then you'd get "stranded" StagedContent rows with no information about what they were used for. At least, that was my thought process. Let me know if you think that's not important.

Another option is to leave the purpose field but don't make it an enum, so that it can be flexibly used without knowing all the possible apps that will use it. Or I can just delete it any rely on the fact that for now all stagedcontent is clipboard content?

Another option is to leave the purpose field but don't make it an enum, so that it can be flexibly used without knowing all the possible apps that will use it. Or I can just delete it any rely on the fact that for now all stagedcontent is clipboard content?

I do think we should kick off the async deletion of the old StagedContent as soon as a new thing has started copying, but I take the point that deletion could always fail. I have a mild preference to just leave it a freeform field then (ditching the Enum as you suggest here). But I don't feel strongly about it.

ormsbee · 2023-04-04T05:10:57Z

openedx/core/djangoapps/content_staging/models.py

+    # The user that created and owns this staged content. Only this user can read it.
+    user = models.ForeignKey(User, null=False, on_delete=models.CASCADE)


Not something for this PR, but I'm starting to wonder if StagedContent might be better as a lower-level primitive that doesn't even necessarily have a REST API or user information–because it's not exposed directly and relies on higher level constructs for things like permissions. For instance, I can imagine things like Unit templates (e.g. "Case Study": html, video, question) that are StagedContent but don't belong to a particular user.

In that situation StagedContent isn't responsible for figuring how who owns content or how it should be used. But only storing a consistent little set of content + assets together.

ormsbee

Minor question and suggestion w.r.t. transaction handling.

ormsbee · 2023-04-11T16:38:23Z

openedx/core/djangoapps/content_staging/signal_handlers.py

@@ -0,0 +1,22 @@
+"""


I missed this the first time around, but please rename this to handlers.py. I personally prefer signal_handlers.py, but it handlers.py is the common convention in edx-platform and OEP-49.

Whoops, I don't actually want the signal handlers there anymore anyways since I'm now dispatching the cleanup for individual rows at copy time as you suggested. Fixed by deleting the whole file.

ormsbee · 2023-04-11T16:46:17Z

openedx/core/djangoapps/content_staging/models.py

+            e.g. "video" if a video is staged, or "vertical" for a unit.
+        """),
+    )
+    olx = models.TextField(null=False, blank=False)


No action required, but just for future consideration: Can we imagine using the clipboard to do a straight copy of course assets without any specific XBlock OLX?

Predicting future requirements is a risky business :p I can imagine it, sure, but I have no idea if we'll think it's a good idea at some point in the future or not. I can also imagine StagedContent being a mutable work area instead of a read-only staging area, but I'm also unsure if that will be a good idea or not.

ormsbee · 2023-04-11T16:57:16Z

openedx/core/djangoapps/content_staging/views.py

+    clipboard or to POST some content to the clipboard.
+    """
+
+    @atomic


Is anything actually being written in this request?

Whoops, no. Probably copy-pasta.

ormsbee · 2023-04-11T17:00:37Z

openedx/core/djangoapps/content_staging/views.py

+        })
+
+
+@view_auth_classes(is_authenticated=True)


You might want to decorate this to prevent Django from ever enclosing this in a view-wide implicit transaction (which could later cause concurrency issues with cleanup):

@method_decorator(transaction.non_atomic_requests, name='dispatch')

Thanks, done.

bradenmacdonald · 2023-04-14T01:09:04Z

@ormsbee @Agrendalath Is this good to merge?

Agrendalath

I didn't re-test it, but the code changes look good.

openedx-webhooks · 2023-04-14T18:41:46Z

@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-14T19:38:40Z

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

edx-pipeline-bot · 2023-04-14T20:00:21Z

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 10, 2023

feat: new django app for copying and pasting OLX content

ba2ae09

bradenmacdonald force-pushed the copy-olx-backend-app branch from fb9b58e to ba2ae09 Compare March 10, 2023 01:53

bradenmacdonald added FC Relates to an Axim Funded Contribution project and removed open-source-contribution PR author is not from Axim or 2U labels Mar 10, 2023

ormsbee reviewed Mar 10, 2023

View reviewed changes

openedx/core/djangoapps/content_staging/models.py Show resolved Hide resolved

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Mar 10, 2023

temp: get basic copying working

efd99b2

bradenmacdonald force-pushed the copy-olx-backend-app branch from 6649939 to 2a4c118 Compare March 14, 2023 22:13

bradenmacdonald added 3 commits March 16, 2023 15:36

temp: WIP - get OLX retrieval working

0afe43f

temp: WIP

5e8f320

feat: Use a nice filename when downloading OLX

abdac7b

bradenmacdonald force-pushed the copy-olx-backend-app branch from 2a4c118 to e2184e0 Compare March 17, 2023 00:02

bradenmacdonald added 2 commits March 16, 2023 17:18

refactor: Use a UserClipboard model that is a OneToOne to User

56145ab

feat: log an event when something is copied to the clipboard

0afd952

bradenmacdonald force-pushed the copy-olx-backend-app branch from e2184e0 to 0afd952 Compare March 17, 2023 00:19

bradenmacdonald added 2 commits March 16, 2023 17:27

chore: Fix lint errors

f3ec43b

feat: Add test cases

73ba7ab

bradenmacdonald force-pushed the copy-olx-backend-app branch from b01999f to 73ba7ab Compare March 17, 2023 02:17

fix: whoops, remove comment that's no longer true.

9817aa6

Agrendalath reviewed Mar 17, 2023

View reviewed changes

openedx/core/djangoapps/content_staging/views.py Outdated Show resolved Hide resolved

openedx/core/djangoapps/content_staging/models.py Show resolved Hide resolved

bradenmacdonald added 2 commits March 17, 2023 17:50

feat: Add a django admin view for developers and admins

83a0889

feat: Automatically clean up expired StagedContent rows in the DB

a42886d

bradenmacdonald force-pushed the copy-olx-backend-app branch from 9eed2ed to a42886d Compare March 18, 2023 00:50

fix: API docs were not generating due to a schema issue

c47af15

Agrendalath approved these changes Mar 20, 2023

View reviewed changes

bradenmacdonald mentioned this pull request Mar 20, 2023

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

Merged

bradenmacdonald mentioned this pull request Apr 3, 2023

Consolidate the OLX block_serializers APIs (backend/dev-only task) openedx/modular-learning#54

Closed

ormsbee requested changes Apr 4, 2023

View reviewed changes

bradenmacdonald added 3 commits April 10, 2023 18:24

refactor: Simplify cleanup of StagedContent

b813612

fix: lint issues

dc3fe03

fix: lint issue

c995b4e

ormsbee requested changes Apr 11, 2023

View reviewed changes

bradenmacdonald added 3 commits April 11, 2023 10:11

fix: remove unneeded signal handlers

30de843

fix: transaction handling cleanups

1fd56fc

fix: transaction management error in test case

1146856

ormsbee approved these changes Apr 14, 2023

View reviewed changes

Agrendalath approved these changes Apr 14, 2023

View reviewed changes

bradenmacdonald merged commit 2dc79bc into openedx:master Apr 14, 2023

bradenmacdonald deleted the copy-olx-backend-app branch April 14, 2023 18:41

bradenmacdonald mentioned this pull request Apr 21, 2023

Consolidate two different implementations of serializing an XBlock to OLX #32115

Merged

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



		@shared_task(base=LoggedTask)
		def delete_expired_staged_content():

		# The user that created and owns this staged content. Only this user can read it.
		user = models.ForeignKey(User, null=False, on_delete=models.CASCADE)

		})


		@view_auth_classes(is_authenticated=True)

Conversation

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

Uh oh!

Description

Supporting information

Analytics

TODOs:

Testing instructions

How to test asynchronous copying

Deadline

Other information

Uh oh!

openedx-webhooks commented Mar 10, 2023

Uh oh!

bradenmacdonald commented Mar 10, 2023

Uh oh!

Uh oh!

bradenmacdonald commented Mar 17, 2023

Uh oh!

Agrendalath left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Agrendalath 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 commented Mar 28, 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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

bradenmacdonald Apr 11, 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!

bradenmacdonald commented Apr 14, 2023

Uh oh!

Agrendalath left a comment

Choose a reason for hiding this comment

Uh oh!

bradenmacdonald commented Mar 10, 2023 •

edited

Loading

bradenmacdonald Apr 11, 2023 •

edited

Loading